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

[John Snow]

On 09/24/2014 10:06 AM, Markus Armbruster wrote:
> John Snow <address@hidden> 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 <address@hidden>
> > ---
> >   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