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

[John Snow]

On 09/19/2014 04:28 AM, Markus Armbruster wrote:
> John Snow <address@hidden> writes:
>
> > Signed-off-by: John Snow <address@hidden>
> > ---
> >   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