Re: [Qemu-devel] [PATCH 1/6] blockdev: Orphaned drive search

[John Snow <jsnow@redhat.com>]

On 09/24/2014 10:06 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> When users use command line options like -hda, -cdrom,
>> or even -drive if=ide, it is up to the board initialization
>> routines to pick up these drives and create backing
>> devices for them.
>>
>> Some boards, like Q35, have not been doing this.
>> However, there is no warning explaining why certain
>> drive specifications are just silently ignored,
>> so this function adds a check to print some warnings
>> to assist users in debugging these sorts of issues
>> in the future.
>>
>> This patch will warn about drives added with if_none,
>
> Judging from my testing, I suspect you mean "will not warn" ;)

A fun story: I did actually mean what I wrote, but in my own testing, 
found that this produced warnings for the test suite, so I decided to 
change the behavior to not warn for IF_NONE.

I figured it would make sense if it gave a little warning that amounted 
to "Don't forget to add a device for this drive!" but I didn't want to 
see any more output on the test suite, so I nixed it.

I amended my commit message for V2.

>> for which it is not possible to tell in advance if
>> the omission of a backing device is an issue.
>>
>> A warning in these cases is considered appropriate.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   blockdev.c                | 21 +++++++++++++++++++++
>>   include/sysemu/blockdev.h |  2 ++
>>   vl.c                      | 12 +++++++++++-
>>   3 files changed, 34 insertions(+), 1 deletion(-)
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index b361fbb..81398e7 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -166,6 +166,27 @@ DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit)
>>       return NULL;
>>   }
>>
>> +bool drive_check_orphaned(void)
>> +{
>> +    DriveInfo *dinfo;
>> +    bool rs = false;
>> +
>> +    QTAILQ_FOREACH(dinfo, &drives, next) {
>> +        /* If dinfo->bdrv->dev is NULL, it has no device attached. */
>> +        /* Unless this is a default drive, this may be an oversight. */
>> +        if (!dinfo->bdrv->dev && !dinfo->is_default &&
>> +            dinfo->type != IF_NONE) {
>> +            fprintf(stderr, "Warning: Orphaned drive without device: "
>> +                    "id=%s,file=%s,if=%s,bus=%d,unit=%d\n",
>> +                    dinfo->id, dinfo->bdrv->filename, if_name[dinfo->type],
>> +                    dinfo->bus, dinfo->unit);
>> +            rs = true;
>> +        }
>> +    }
>> +
>> +    return rs;
>> +}
>> +
>>   DriveInfo *drive_get_by_index(BlockInterfaceType type, int index)
>>   {
>>       return drive_get(type,
>> diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
>> index 23a5d10..80f768d 100644
>> --- a/include/sysemu/blockdev.h
>> +++ b/include/sysemu/blockdev.h
>> @@ -38,6 +38,7 @@ struct DriveInfo {
>>       int unit;
>>       int auto_del;               /* see blockdev_mark_auto_del() */
>>       bool enable_auto_del;       /* Only for legacy drive_new() */
>> +    bool is_default;            /* Added by default_drive() ?  */
>>       int media_cd;
>>       int cyls, heads, secs, trans;
>>       QemuOpts *opts;
>> @@ -46,6 +47,7 @@ struct DriveInfo {
>>   };
>>
>>   DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit);
>> +bool drive_check_orphaned(void);
>>   DriveInfo *drive_get_by_index(BlockInterfaceType type, int index);
>>   int drive_get_max_bus(BlockInterfaceType type);
>>   DriveInfo *drive_get_next(BlockInterfaceType type);
>> diff --git a/vl.c b/vl.c
>> index dc792fe..eaef240 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -1168,6 +1168,7 @@ static void default_drive(int enable, int snapshot, BlockInterfaceType type,
>>                             int index, const char *optstr)
>>   {
>>       QemuOpts *opts;
>> +    DriveInfo *dinfo;
>>
>>       if (!enable || drive_get_by_index(type, index)) {
>>           return;
>> @@ -1177,9 +1178,13 @@ static void default_drive(int enable, int snapshot, BlockInterfaceType type,
>>       if (snapshot) {
>>           drive_enable_snapshot(opts, NULL);
>>       }
>> -    if (!drive_new(opts, type)) {
>> +
>> +    dinfo = drive_new(opts, type);
>> +    if (!dinfo) {
>>           exit(1);
>>       }
>> +    dinfo->is_default = true;
>> +
>>   }
>>
>>   void qemu_register_boot_set(QEMUBootSetHandler *func, void *opaque)
>> @@ -4457,6 +4462,11 @@ int main(int argc, char **argv, char **envp)
>>       if (qemu_opts_foreach(qemu_find_opts("device"), device_init_func, NULL, 1) != 0)
>>           exit(1);
>>
>> +    /* anybody left over? */
>> +    if (drive_check_orphaned()) {
>> +        fprintf(stderr, "Warning: found drives without a backing device.\n");
>> +    }
>> +
>>       net_check_clients();
>>
>>       ds = init_displaystate();
>
> Terminology: the device model is the front end, the thing created by
> -drive is the back end.  I'd simply  drop this message.  If you want to
> keep it, rephrase it to avoid "backing device".
>

-- 
—js