Re: [Qemu-devel] [PATCH 1/4] block: Warn if an if=<something> drive was also connected manually

[Markus Armbruster]

Peter Maydell <address@hidden> writes:

> Improve the diagnosis of command line errors where the user requested
> an automatic connection of a drive (via if=<something>, or by not
> setting if= and using the board-default-if). We already fail this
> case if the board actually handles if=<something>, but if the board
> did not auto-connect the drive we should at least warn about the
> problem, since the most likely reason is a forgotten if=none, and
> this command line might become an error if the board starts handling
> this if= type in future.
>
> To do this we need to identify when a drive is automatically
> connected by the board; we do this by assuming that all calls
> to blk_by_legacy_dinfo() imply that we're about to assign the
> drive to a device. This is a slightly ugly place to make the
> test, but simpler than trying to locate and change every place
> in the code that does automatic drive handling, and the worst
> case is that we might print out a spurious warning.

Copying what I wrote on an earlier iteration of this idea:

Adding it to blk_by_legacy_dinfo() is sound when it's called exactly for
the block backends the board claims.  We need to show:

(A) It's called for all block backends the board claims

    Plausible enough, because:

    * Boards claim only drives defined with interface types other than
      IF_NONE.

    * Boards find these drives with drive_get() or its wrappers.  They
      all return DriveInfo.

    * To actually connect a frontend, they need to find the DriveInfo's
      BlockBackend, with blk_by_legacy_dinfo().

(B) It's not called for any block backend the board doesn't claim

    Counter-example: hmp_drive_add().  However, that can only run later,
    so we can ignore it.  Still, not exactly inspiring confidence.

What about this instead:

1. When -device creation connects a qdev_prop_drive property to a
backend, fail when the backend has a DriveInfo and the DriveInfo has
type != IF_NONE.  Note: the connection is made in parse_drive().

2. This breaks -drive if=virtio and possibly more.  That's because for
if=virtio, we create input for -device creation that asks to connect
this IF_VIRTIO drive.  To unbreak it, mark the DriveInfo when we create
such input, and make parse_drive() accept backends so marked.  To find
the places that need to mark, examine users of option group "device".  A
quick, sloppy grep shows a bit over a dozen candidates.  Not too bad.

> Signed-off-by: Peter Maydell <address@hidden>
> ---
>  block/block-backend.c     |  4 ++++
>  blockdev.c                | 39 +++++++++++++++++++++++++++++++++++++++
>  include/sysemu/blockdev.h |  2 ++
>  3 files changed, 45 insertions(+)
>
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 93e46f3..a45c207 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -260,6 +260,9 @@ DriveInfo *blk_set_legacy_dinfo(BlockBackend *blk, 
> DriveInfo *dinfo)
>  /*
>   * Return the BlockBackend with DriveInfo @dinfo.
>   * It must exist.
> + * For the purposes of providing helpful error messages, we assume
> + * that any call to this function implies that the drive is going
> + * to be automatically claimed by the board model.

As explained above, this is a problematic assumption.  If we decice to
rely on it anyway, we need a scarier comment here, and a /* This
violates the assumption ..., but that's okay, because ... */ next to
calls that violate the assumption, like hmp_drive_add() does.

Can we find a way to check for not-okay violations with assert()?

>   */
>  BlockBackend *blk_by_legacy_dinfo(DriveInfo *dinfo)
>  {
> @@ -267,6 +270,7 @@ BlockBackend *blk_by_legacy_dinfo(DriveInfo *dinfo)
>  
>      QTAILQ_FOREACH(blk, &blk_backends, link) {
>          if (blk->legacy_dinfo == dinfo) {
> +            dinfo->auto_claimed = true;
>              return blk;
>          }
>      }
> diff --git a/blockdev.c b/blockdev.c
> index de94a8b..97a56b9 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -230,6 +230,32 @@ bool drive_check_orphaned(void)
>                      dinfo->bus, dinfo->unit);
>              rs = true;
>          }
> +        if (blk_get_attached_dev(blk) && dinfo->type != IF_NONE &&
> +            !dinfo->auto_claimed) {
> +            /* This drive is attached to something, but it was specified
> +             * with if=<something> and it's not attached because it was
> +             * automatically claimed by the board code because of the if=
> +             * specification. The user probably forgot an if=none.
> +             */
> +            fprintf(stderr,
> +                    "Warning: automatic connection of this drive requested 
> ");
> +            if (dinfo->type_is_board_default) {
> +                fprintf(stderr, "(because if= was not specified and this "
> +                        "machine defaults to if=%s) ",
> +                        if_name[dinfo->type]);

In my opinion, a board that specifies a default interface type it
doesn't support is simply broken, and should be fixed.

Even if we fix that, we could still reach this case when the board
claims only *some* of the drives for its default type.  Example:

    $ qemu-system-x86_64 -nodefaults -S -display none -drive 
if=floppy,file=tmp.qcow2,index=99
    Warning: Orphaned drive without device: 
id=floppy99,file=tmp.qcow2,if=floppy,bus=0,unit=99

Compare:

    $ qemu-system-x86_64 -nodefaults -S -display none -drive 
if=ide,file=tmp.qcow2,index=99
    qemu-system-x86_64: Too many IDE buses defined (50 > 2)

Crap shot.

If we have boards that don't claim *any* interface type, we need to give
them a .block_default_type that rejects -drive without an explicit if=.

> +            } else {
> +                fprintf(stderr, "(because if=%s was specified) ",
> +                        if_name[dinfo->type]);
> +            }
> +            fprintf(stderr,
> +                    "but it was also connected manually to a device: "
> +                    "id=%s,file=%s,if=%s,bus=%d,unit=%d\n"
> +                    "(If you don't want this drive auto-connected, "
> +                    "use if=none.)\n",
> +                    blk_name(blk), blk_bs(blk)->filename, 
> if_name[dinfo->type],
> +                    dinfo->bus, dinfo->unit);

Doesn't point the user to the offending -device.  If we detected the
problem right when we connect -device drive=, in parse_drive(), we'd get
that for free.

> +            rs = true;
> +        }
>      }
>  
>      return rs;
> @@ -683,6 +709,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, 
> BlockInterfaceType block_default_type)
>      const char *werror, *rerror;
>      bool read_only = false;
>      bool copy_on_read;
> +    bool type_is_board_default = false;
>      const char *serial;
>      const char *filename;
>      Error *local_err = NULL;
> @@ -808,6 +835,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, 
> BlockInterfaceType block_default_type)
>          }
>      } else {
>          type = block_default_type;
> +        type_is_board_default = true;
>      }
>  
>      /* Geometry */
> @@ -994,6 +1022,17 @@ DriveInfo *drive_new(QemuOpts *all_opts, 
> BlockInterfaceType block_default_type)
>      dinfo->devaddr = devaddr;
>      dinfo->serial = g_strdup(serial);
>  
> +    if (type == IF_VIRTIO) {
> +        /* We created the automatic device earlier. For other types this
> +         * will be set to true at the point where the drive is claimed
> +         * by the IDE/SCSI/etc bus, when that code calls 
> blk_by_legacy_dinfo()
> +         * to find the block backend from the drive.
> +         */
> +        dinfo->auto_claimed = true;
> +    }
> +
> +    dinfo->type_is_board_default = type_is_board_default;
> +
>      blk_set_legacy_dinfo(blk, dinfo);
>  
>      switch(type) {
> diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
> index 3104150..f9c44e2 100644
> --- a/include/sysemu/blockdev.h
> +++ b/include/sysemu/blockdev.h
> @@ -36,6 +36,8 @@ struct DriveInfo {
>      int unit;
>      int auto_del;               /* see blockdev_mark_auto_del() */
>      bool is_default;            /* Added by default_drive() ?  */
> +    bool auto_claimed;          /* Automatically claimed by board model? */
> +    bool type_is_board_default; /* type is from board default, not user 
> 'if=' */
>      int media_cd;
>      int cyls, heads, secs, trans;
>      QemuOpts *opts;