Re: [Qemu-devel] [PATCH for-2.8] qdev: apply global properties in reverse order

[Halil Pasic]

On 12/05/2016 06:41 PM, Eduardo Habkost wrote:
> On Mon, Dec 05, 2016 at 06:25:55PM +0100, Cornelia Huck wrote:
>> > On Mon, 5 Dec 2016 14:48:29 -0200
>> > Eduardo Habkost <address@hidden> wrote:
>> > 
>>> > > On Mon, Dec 05, 2016 at 04:42:00PM +0100, Cornelia Huck wrote:
>>>> > > > On Mon, 05 Dec 2016 16:21:22 +0100
>>>> > > > Greg Kurz <address@hidden> wrote:
>>>> > > > 
>>>>> > > > > The current code recursively applies global properties from child 
>>>>> > > > > up to
>>>>> > > > > parent. So, if you have:
>>>>> > > > > 
>>>>> > > > > -global virtio-pci.disable-modern=on
>>>>> > > > > -global virtio-blk-pci.disable-modern=off
>>>>> > > > > 
>>>>> > > > > Then the default value of disable-modern for a virtio-blk-pci 
>>>>> > > > > device is on,
>>>>> > > > > which looks wrong from an OOP perspective.
>>>>> > > > > 
>>>>> > > > > This patch reverses the logic, so that a child property always 
>>>>> > > > > prevail.
>>>> > > > 
>>>> > > > This sounds reasonable...
>>>> > > > 
>>>>> > > > > 
>>>>> > > > > This fixes a subtle bug that got introduced in 2.7 with commit 
>>>>> > > > > "9a4c0e220d8a
>>>>> > > > > hw/virtio-pci: fix virtio behaviour" for older (< 2.7) machine 
>>>>> > > > > types: the
>>>>> > > > > HW_COMPAT_2_6 macro contains global virtio-pci.disable-* 
>>>>> > > > > properties which
>>>>> > > > > would silently override global properties passed on the command 
>>>>> > > > > line for
>>>>> > > > > virtio subtypes.
>>>>> > > > > 
>>>>> > > > > Signed-off-by: Greg Kurz <address@hidden>
>>>>> > > > > ---
>>>>> > > > > 
>>>>> > > > > AFAIK, libvirt's XML doesn't know about modern/legacy modes for 
>>>>> > > > > virtio
>>>>> > > > > devices. Early adopters of virtio 1.0 had to rely on the 
>>>>> > > > > <qemu:commandline>
>>>>> > > > > tag to pass global properties to QEMU. This patch ensures that 
>>>>> > > > > XML files
>>>>> > > > > used with older machine types remain valid with newer versions of 
>>>>> > > > > QEMU.
>>>>> > > > > 
>>>>> > > > > FWIW I guess it could help to have this fix in 2.8, and also 
>>>>> > > > > probably in
>>>>> > > > > 2.7.1.
>>>> > > > 
>>>> > > > ...but I'm a bit worried about doing that change this late in the
>>>> > > > cycle, as we may introduce subtle changes for other configurations. 
>>>> > > > At
>>>> > > > the very least, we should look over the existing backwards compat
>>>> > > > properties (I'll look at those I'm familiar with).
>>> > > 
>>> > > This patch would change the behavior for:
>>> > >  -global virtio-blk-pci.disable-modern=on
>>> > >  -global virtio-pci.disable-modern=off
>>> > > 
>>> > > And I am not sure the new behavior would be correct. Shouldn't we
>>> > > apply the properties in the order specified in the command-line?
>> > 
>> > Probably; but how should this interact with compat props?
> compat props should be always applied in the order they appear.
> -global should always be applied after compat props.
> 
> So, it looks like we have two additional reasons to just follow
> the order the global properties were registered.
> 

How about a docs update? 

Given the current doc:
"""
-global driver.prop=value
-global driver=driver,property=property,value=value
    Set default value of driver's property prop to value, e.g.:

         qemu-system-i386 -global ide-drive.physical_block_size=4096 -drive
file=file,if=ide,index=0,media=disk

    In particular, you can use this to set driver properties for devices which
    are created automatically by the machine model. To create a device which is
    not created automatically and set properties on it, use -device.

    -global driver.prop=value is shorthand for -global 
driver=driver,property=prop,
value=value. The longhand syntax works even when
    driver contains a dot. 
"""
I think this OOP argument, which I do not completely understand,
is not the right direction.

Yet I do not think the current state of documentation gives a
definitive answer on what behavior should take place in the
scenarios described above.

Maybe it's my mistake, but I did not find a statement about
the order in which global properties are to be applied
(please point me to it if I've missed it).

Another problem with the doc (IMHO) is that it's not
really defined what a driver is. So I can't even tell if
-global virtio-pci.disable-modern=off is even legit on
the command line.

Regarding the order compat props. applied before command line it
makes a lot of sense to me with the documentation stating "In 
particular, you can use this to set driver properties for devices which
are created automatically by the machine model."