| Message ID | 1434115575-7214-2-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On 22 June 2015 at 10:59, Markus Armbruster <armbru@redhat.com> wrote: > 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(). So, the reason I didn't do this is that it breaks some options that currently work (the ones where the board doesn't pick up the device and so there's no conflict). I preferred to make those "continue to function, but warn", but if we're happy to make this a hard error we could go this way. > 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. How do we then tell the difference between parse_drive() being called for the auto-created virtio device, and it then later being called as part of connecting the drive to a manually created device? My grep (which was for 'qemu_find_opts.*device' -- is that right?) suggests that in fact the only case we need to care about is the one where we auto-create the virtio device (the other grep matches are not relevant). > In my opinion, a board that specifies a default interface type it > doesn't support is simply broken, and should be fixed. I'm inclined to agree, but I bet we have a lot. It doesn't help that the default if the machine doesn't specify anything is "IDE", not "you can't use a default interface"... > 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=. We have several of these boards, yes. (for example, hw/arm/cubieboard.c) thanks -- PMM
On 22 June 2015 at 10:59, Markus Armbruster <armbru@redhat.com> wrote: > 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. It looks like we missed a bunch of places that need changing, because we don't only go through parse_drive() for connections made by user -device creation and places where the code has added something to the "device" option group. For instance, if=scsi auto-plugin for x86 pc will end up calling qdev_prop_set_drive() from scsi_bus_legacy_add_drive(), which then goes through parse_drive(). There are 17 places that call qdev_prop_set_drive(); we could change them all, but that's the sort of invasive change I was hoping to avoid with my initial blk_by_legacy_dinfo change... (I think currently all callers of qdev_prop_set_drive{,_nofail}() are using it to connect legacy drives, except for hw/usb/dev-storage.c, which is calling drive_new() to create an if=none drive and then wiring it up to the device with qdev_prop_set_drive(). This feels like a fragile assumption for the future, though.) thanks -- PMM
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. */ 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]); + } 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); + 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;
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. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- block/block-backend.c | 4 ++++ blockdev.c | 39 +++++++++++++++++++++++++++++++++++++++ include/sysemu/blockdev.h | 2 ++ 3 files changed, 45 insertions(+)