Re: [Qemu-block] [Qemu-devel] [PATCH v2 0/3] blockdev: Defer creation of implicit PCI devices for IF_VIRTIO drives

[Markus Armbruster]

Peter Maydell <address@hidden> writes:

> On 9 June 2015 at 18:48, Peter Maydell <address@hidden> wrote:
>> 1) explicit if=foo drive manually wired up to a device is an error
>>    (and always diagnosed as such, not indirectly via it being double-used)
>
> I had a look at getting this to be diagnosed properly, and the sketch
> of the patch is:
> 1. add "bool auto_claimed;" to struct DriveInfo
> 2. everywhere we grab a DriveInfo and auto-connect it to a device,
>    set dinfo->auto_claimed to true
> 3. in drive_check_orphaned() do this:
>
> +        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 drive 
> specified "
> +                    "(using if=%s or by not specifying if= at all) "
> +                    "but it was also connected manually (try using
> if=none ?): "
> +                    "id=%s,file=%s,if=%s,bus=%d,unit=%d\n",
> +                    if_name[dinfo->type],
> +                    blk_name(blk), blk_bs(blk)->filename, 
> if_name[dinfo->type],
> +                    dinfo->bus, dinfo->unit);
> +            rs = true;
> +        }
>
> (We could distinguish the "using if=" and "not specifying if="
> but we'd need another bool in the DriveInfo just to track that for
> the benefit of the warning.)
>
> This works, but part 2 is a bit awkward. I think it's the case that
> we do auto-claiming if and only if somebody calls blk_by_legacy_dinfo()
> on the dinfo, but it seems like a hack to add the "auto_claimed = true"
> inside that function... (OTOH, that function's called in about 50
> places, so it would be very handy not to need to change all of the
> callsites!)

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.