Re: [Qemu-devel] [RFC v2 1/3] blockdev: Add function to search for orphaned drives

[John Snow <jsnow@redhat.com>]

On 09/19/2014 04:28 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   blockdev.c                | 19 +++++++++++++++++++
>>   include/sysemu/blockdev.h |  1 +
>>   vl.c                      |  5 +++++
>>   3 files changed, 25 insertions(+)
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index b361fbb..5e7c93a 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -166,6 +166,25 @@ DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit)
>>       return NULL;
>>   }
>>
>> +DriveInfo *drive_check_orphaned(void)
>> +{
>> +    DriveInfo *dinfo;
>> +    DriveInfo *ret = NULL;
>> +
>> +    QTAILQ_FOREACH(dinfo, &drives, next) {
>> +        /* If dev is NULL, it has no device attached.
>> +         * If drv is non-NULL, it has a file attached.
>> +         * If both conditions are true, it is possibly an oversight. */
>
> Suggest to spell out dinfo->bdrv->dev and dinfo->bdrv->drv.
>
> "File attached" is imprecise.  BDS member drv is non-null betwen
> bdrv_open() and bdrv_close().  A BDS with null drv means "empty", in the
> sense of "no medium".
>
>
>> +        if ((dinfo->bdrv->dev == NULL) && (dinfo->bdrv->drv != NULL)) {
>> +            fprintf(stderr, "Orphaned drive: id=%s,if=%s,file=%s\n",
>> +                    dinfo->id, if_name[dinfo->type], dinfo->bdrv->filename);
>> +            ret = dinfo;
>> +        }
>> +    }
>
> Please prefix "Warning:" to make the nature of this message more
> explicit.
>
> "Orphaned drive" might not be obvious to all users, but it's concise,
> and no worse than the "has no peer" we use for NICs.
>
> You warn when a non-empty drive is not used by a device model.
>
> This warns when you create one with -drive if=none for future use in the
> monitor.  I guess that's fine.
>
> It doesn't warn for empty drives.  I doubt "empty" should make a
> difference.

I didn't want it to warn about things like the SD drive and the floppy 
drive, created by default, but you're right. Your suggestion below fixes 
this problem.

> I think the condition to check is "has the board failed to pick up a
> drive that is meant to be picked up by the board":
>
>      dinfo->type != IF_NONE && !dinfo->bdrv->dev
>
> I guess this can warn about default drives, because we blindly add them
> whether the boards wants them or not.  Stupidest solution that could
> possibly work: add a flag to DriveInfo to suppress the warning for them.

Hm. Actually, the default drives are added by their specific interfaces. 
IF_IDE, IF_FLOPPY and IF_SD. This is a good improvement.
(Well, cdrom is actually added via block_default_type which is usually 
unset, and happens to coincide with IF_IDE.)

> Better solution: don't add them unless the board wants them.  I tried
> that before, but my solution[*] went nowhere.  If you're interested in
> trying again, let me know, and I'll explain.
>
>> +
>> +    return ret;
>> +}
>> +
>>   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..25d52d2 100644
>> --- a/include/sysemu/blockdev.h
>> +++ b/include/sysemu/blockdev.h
>> @@ -46,6 +46,7 @@ struct DriveInfo {
>>   };
>>
>>   DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit);
>> +DriveInfo *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 5db0d08..e095bcd 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -4457,6 +4457,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();
>
>
> [*] https://lists.nongnu.org/archive/html/qemu-devel/2012-08/msg02993.html
>