Peter Maydell <peter.mayd...@linaro.org> writes: > On 22 June 2015 at 10:59, Markus Armbruster <arm...@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