Peter Maydell <peter.mayd...@linaro.org> writes:
> On 9 June 2015 at 18:48, Peter Maydell <peter.mayd...@linaro.org> 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.
