Max Reitz <mre...@redhat.com> 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 <arm...@redhat.com>
>> ---
>> 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!
