Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Markus Armbruster]

Paolo Bonzini <address@hidden> writes:

> Il 16/09/2014 09:21, Markus Armbruster ha scritto:
>> The rebase onto QOM renamed name to legacy_name, to free name for use as
>> QOM type name (commit cafe5bd).
>
> Also, the QOM type name has strict rules:
>
> - either it is a QAPI type (primitive, enum or struct)
>
> - or it is link<qom-type-name>
>
> - or it is child<qom-type-name>
>
> The qdev type names had no rules.  We had uint8, hex8, on/off, drive, etc.
>
>> Human users do, however.  I'd object to a degradation of -device
>> FOO,help.  Changing it is fine, but it should remain at least as helpful
>> as it is now.
>
> The question is if it is really a degradation.
>
> Example 1:
>
> virtio-blk-pci.physical_block_size=blocksize
> virtio-blk-pci.logical_block_size=blocksize
>
> vs.
>
> virtio-blk-pci.physical_block_size=uint16
> virtio-blk-pci.logical_block_size=uint16
>
> What is a "blocksize"?  It is a power of two between 512 and 32768, but 
> how does the user know?  If the value is too small or invalid, the 
> error message is particularly helpful for QEMU standards:
>
>     qemu-system-x86_64: -device 
> virtio-blk-pci,physical_block_size=128,drive=hd:
>     Property .physical_block_size doesn't take value 128 (minimum:
> 512, maximum:
>     32768)
>
>     qemu-system-x86_64: -device
> virtio-blk-pci,physical_block_size=1023,drive=hd:
>     Property .physical_block_size doesn't take value '1023', it's not
> a power of 2
>
> I think uint16 is actually more informative than "blocksize".

Neither is particularly user-friendly, but I grant you uint16 is less
bad than blocksize in absence of definitions for these terms.

> Example 2:
>
>     virtio-blk-pci.drive=drive
>
> vs.
>
>     virtio-blk-pci.drive=str
>
> The fact that it points to a -drive is already guessable (for anyone who
> knows the relationship between -drive and -device) from the name of the
> property.
>
>     $ qemu-system-x86_64 -drive if=none,file=$HOME/test2.img,id=hd
> -device virtio-blk-pci
>     qemu-system-x86_64: -device virtio-blk-pci: drive property not set
>     qemu-system-x86_64: -device virtio-blk-pci: Device initialization failed.
>     qemu-system-x86_64: -device virtio-blk-pci: Device
> virtio-blk-pci' could not be
>     initialized
>
>     $ qemu-system-x86_64 -drive if=none,file=$HOME/test2.img,id=hd
>     -device virtio-blk-pci,drive=ff
>     qemu-system-x86_64: -device virtio-blk-pci,drive=ff: Property
>     'virtio-blk-device.drive' can't find value 'ff'
>
> If we QOMified BlockBackend, BTW, it would show up as
>
>     virtio-blk-pci.drive=link<block-backend>
>
> I think both "str" and "link<block-backend>" actually are a small degradation
> compared to "drive", and this is why I kept the legacy_name.  But overall I
> think it's not really worth the layering violation that patches 2 and 3 are;
> and it's definitely not stable material.

"str" is clearly a degradation for me.  I breaks usage like

    for i in `qemu -device help 2>&1 | sed -n 's/^name "\([^"]*\)".*/\1/p'`
    do qemu -device $i,help 2>&1
    done | grep =drive

Finds all block device models.  I've done such things many times.

Whether "link<block-backend>" is a degradation or an improvement is
debatable.

> I'd rather drop the legacy_name at all.  Here are the legacy_names currently
> in use:
>
>     hw/core/qdev-properties-system.c:    .legacy_name  = "drive",
>     hw/core/qdev-properties-system.c:    .legacy_name  = "chr",
>     hw/core/qdev-properties-system.c:    .legacy_name  = "netdev",
>     hw/core/qdev-properties-system.c:    .legacy_name  = "vlan",
>     hw/core/qdev-properties.c:    .legacy_name  = "on/off",
>     hw/core/qdev-properties.c:    .legacy_name  = "macaddr",
>     hw/core/qdev-properties.c:    .legacy_name = "bios-chs-trans",
>     hw/core/qdev-properties.c:    .legacy_name  = "pci-devfn",
>     hw/core/qdev-properties.c:    .legacy_name  = "blocksize",
>     hw/core/qdev-properties.c:    .legacy_name = "pci-host-devaddr",

You missed

    target-ppc/translate_init.c:8047:    .legacy_name = "powerpc-server-compat",

which is another enum.  Aside: it should really use the infrastructure
for enums.

>
> vlan is just a glorified int, not an id like the others.  chr should be
> named chardev.  blocksize, I already covered above.  bios-chs-trans is
> an enum (QAPI name BiosAtaTranslation) and is useless.  on/off vs.
> bool is just bikeshedding.  macaddr is obviously a string, whose format
> is clear from the property name.
>
> pci-host-devaddr and pci-devfn are the only ones that do not have an
> obvious property name (respectively "host" and "addr").

Agree on the uselessness of "on/off".

Agree on the uselessness of "blocksize" without a definition of the
term.

"chr" and "netdev" are like "drive", and replacing them by "str" is a
degradation in my book.

Help for enum-valued properties in the form of "prop=ENUM-NAME" is not
really helpful without a definition of ENUM-NAME.  It's still useful for
finding devices with this kind of property.

Same for structured strings like macaddr, pci-devfn, pci-host-devaddr.

legacy_name provides lousy help.  But it's better than nothing, and I'd
hate to see it dropped without a suitable replacement.