Peter Maydell <peter.mayd...@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.mayd...@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;