| Message ID | 1434115575-7214-2-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
Peter Maydell <peter.maydell@linaro.org> 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 <peter.maydell@linaro.org> > --- > 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;
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
Peter Maydell <peter.maydell@linaro.org> writes: > 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. I think we could make it a warning in parse_drive(), too: blk = blk_by_name(str); if (!blk) { // no such backend, bail out } if (blk_attach_dev(blk, dev) < 0) { // backend already attached, bail out } dinfo = blk_legacy_dinfo(blk); if (dinfo && dinfo->type != IF_NONE) { // warn, but carry on } >> 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? Good point. We need a way to determine whether we're running on behalf of the QemuOpts created for this DriveInfo. So make the DriveInfo mark a QemuOpts *device_opts, non-null only when drive_new() creates device options. Now parse_drive() has to check whether dinfo->device_opts matches the qdev_device_add()'s opts argument. Fortunately, qdev_device_add() stores it in dev->opts, so this could do: dinfo = blk_legacy_dinfo(blk); if (dinfo && dinfo->type != IF_NONE && dinfo->device_opts != dev->opts) { // warn, but carry on } > 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). I'm not aware of another one. >> 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"... Easy enough to change: typedef enum { IF_DEFAULT = -1, /* for use with drive_add() only */ /* * IF_NONE must be zero, because we want QEMUMachine member * block_default_type to default-initialize to IF_NONE */ IF_NONE, IF_IDE, IF_SCSI, IF_FLOPPY, IF_PFLASH, IF_MTD, IF_SD, IF_VIRTIO, IF_XEN, IF_COUNT } BlockInterfaceType; Then special-case IF_NONE in drive_new(): value = qemu_opt_get(legacy_opts, "if"); if (value) { ... } else if (block_default_type == IF_NONE) { error_report("bla, bla"); goto fail; } else { type = block_default_type; } If you want to support .block_default_type = IF_NONE, you need to create a new special member for this purpose instead. Naturally, we then have to find all boards that actually claim their IDE drives and fix them to .block_default_type = IF_IDE. Shouldn't be hard. >> 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(+)