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

[Greg Kurz <groug@kaod.org>]

On Mon, 5 Dec 2016 19:14:40 +0100
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> 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 <ehabkost@redhat.com> 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 <groug@kaod.org> 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 <groug@kaod.org>
> >>>>> > > > > ---
> >>>>> > > > > 
> >>>>> > > > > 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,

With the current code, properties from the parent classes implicitly
prevail and this has nothing to do with command line order, or order
of appearance in HW_COMPAT_*.

From an OOP perspective, we usually expect child classes to override
parent classes behavior, not the contrary.

> 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.
> 

True, the documentation doesn't mention anything about the QEMU
object model. And the user cannot even think that virtio-pci
exists and is the parent of virtio-blk-pci...

> 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).
> 

True, so this should be the implicit command line order.

> 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.
> 

True, virtio-pci.disable-modern is an internal compat thing and
it silentyl overrides virtio-blk-pci.disable-modern which is a
legit option. Passing virtio-pci.disable-modern to the command
line is just messing with undocumented internals... and should
probably be disallowed.

> 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."
> 

I agree. So, as written several times in this thread, the only thing
we can do now is changing the virtio-pci compat props to be applied
to each subtype, so that they don't silently undo -global.

Cheers.

--
Greg