Re: [Qemu-block] [Qemu-devel] [PATCH 1/4] block: Warn if an if=<something> drive was also connected manually

[Markus Armbruster]

Peter Maydell <address@hidden> writes:

> On 22 June 2015 at 10:59, Markus Armbruster <address@hidden> 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