Re: [Qemu-devel] [PATCH v3 04/23] block: Connect BlockBackend and DriveInfo

[Markus Armbruster]

Max Reitz <address@hidden> writes:

> On 16.09.2014 20:12, Markus Armbruster wrote:
>> Make the BlockBackend own the DriveInfo.  Change blockdev_init() to
>> return the BlockBackend instead of the DriveInfo.
>>
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>   block.c                   |  2 --
>>   block/block-backend.c     | 38 ++++++++++++++++++++++++
>>   blockdev.c | 73 ++++++++++++++++++++++++-----------------------
>>   include/sysemu/blockdev.h |  4 +++
>>   4 files changed, 79 insertions(+), 38 deletions(-)
>
> [snip]
>
>> diff --git a/blockdev.c b/blockdev.c
>> index 5da6028..722d083 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>
> [snip]
>
>> @@ -920,19 +922,18 @@ DriveInfo *drive_new(QemuOpts *all_opts, 
>> BlockInterfaceType block_default_type)
>>       }
>>         /* Actual block device init: Functionality shared with
>> blockdev-add */
>> -    dinfo = blockdev_init(filename, bs_opts, &local_err);
>> +    blk = blockdev_init(filename, bs_opts, &local_err);
>>       bs_opts = NULL;
>> -    if (dinfo == NULL) {
>> -        if (local_err) {
>> -            error_report("%s", error_get_pretty(local_err));
>> -            error_free(local_err);
>> -        }
>> +    if (!blk) {
>> +        error_report("%s", error_get_pretty(local_err));
>> +        error_free(local_err);
>>           goto fail;
>
> Not all of blockdev_init() sets errp on failure. Try
> "qemu-system-x86_64 -drive format=help" and you'll see a segfault with
> this patch applied.

Good catch!

The common contract is:

1. On success, return the new object and leave *errp alone.

2. On failure, return null and set *errp.

blockdev_init() adds:

3. On help, return null and leave *errp alone.

Fine, but it really needs a function comment.  More so since the unusual
case is buried in the middle of a 200+ line function.  Separate patch,
outside the scope of this series.

>                     So either you can make blockdev_init() do things
> right, which is, set errp for format=help instead of dumping the list
> to stdout (but I'm not even sure whether that's really correct,
> because it's not really an error), or you keep the "if" around
> error_report() and error_free() here.

Your doubts are justified: help is not an error.

How to ask for help with -drive is obvious enough:

    -drive format=help

Is the a way to ask for help with blockdev-add?  Hard to see, as its
arguments meander through QAPI-generated types, QDicts and QemuOpts.
When I try, I either get "QMP input object member 'format' is
unexpected", or crash in visitors; suspecting the bug fixed by "qapi:
fix crash in dealloc visitor for union types".

We'll need one Once we expose blockdev-add on the command line and in
the human monitor.  Since printing help to a QMP monitor isn't at all
helpful, we'll have to either catch and reject the attempt to ask for it
there, or print help with error_printf_unless_qmp().

I figure the cleanest solution would be to lift format help out of
blockdev_init() into drive_new() now and later on the command line code.

For this series, I'll simply refrain from breaking the existing logic.

> Looks good otherwise, though.

Thanks!