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

[Markus Armbruster]

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 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.

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