[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH for-2.8] qdev: apply global properties in revers
From: |
Greg Kurz |
Subject: |
Re: [Qemu-devel] [PATCH for-2.8] qdev: apply global properties in reverse order |
Date: |
Tue, 6 Dec 2016 10:11:10 +0100 |
On Mon, 5 Dec 2016 19:14:40 +0100
Halil Pasic <address@hidden> 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 <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,
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
Re: [Qemu-devel] [PATCH for-2.8] qdev: apply global properties in reverse order, Greg Kurz, 2016/12/05
Re: [Qemu-devel] [PATCH for-2.8] qdev: apply global properties in reverse order, Daniel P. Berrange, 2016/12/05
Re: [Qemu-devel] [PATCH for-2.8] qdev: apply global properties in reverse order, Stefan Hajnoczi, 2016/12/06