Re: [Qemu-devel] [PATCH 2/3] test-qdev-global-props: Test global property ordering

[Eduardo Habkost <ehabkost@redhat.com>]

On Sun, Jul 16, 2017 at 02:35:49PM +0200, Halil Pasic wrote:
> On 07/12/2017 08:48 PM, Eduardo Habkost wrote:
> > On Wed, Jul 12, 2017 at 08:06:14PM +0200, Halil Pasic wrote:
> >>
> >>
> >> On 07/11/2017 02:43 AM, Eduardo Habkost wrote:
> >>> Test case to detect the bug fixed by commit
> >>> "qdev: fix the order compat and global properties are applied".
> >>>
> >>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> >>
> >> Reviewed-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> >>
> >> Please see below nevertheless.
> >>
> >>> ---
> >>>  tests/test-qdev-global-props.c | 33 +++++++++++++++++++++++++++++++++
> >>>  1 file changed, 33 insertions(+)
> >>>
> >>> diff --git a/tests/test-qdev-global-props.c b/tests/test-qdev-global-props.c
> >>> index 48e5b73..ef2951f 100644
> >>> --- a/tests/test-qdev-global-props.c
> >>> +++ b/tests/test-qdev-global-props.c
> >>> @@ -33,6 +33,8 @@
> >>>  #define STATIC_TYPE(obj) \
> >>>      OBJECT_CHECK(MyType, (obj), TYPE_STATIC_PROPS)
> >>>
> >>> +#define TYPE_SUBCLASS "static_prop_subtype"
> >>> +
> >>>  #define PROP_DEFAULT 100
> >>>
> >>>  typedef struct MyType {
> >>> @@ -63,6 +65,11 @@ static const TypeInfo static_prop_type = {
> >>>      .class_init = static_prop_class_init,
> >>>  };
> >>>
> >>> +static const TypeInfo subclass_type = {
> >>> +    .name = TYPE_SUBCLASS,
> >>> +    .parent = TYPE_STATIC_PROPS,
> >>> +};
> >>> +
> >>>  /* Test simple static property setting to default value */
> >>>  static void test_static_prop_subprocess(void)
> >>>  {
> >>> @@ -279,12 +286,35 @@ static void test_dynamic_globalprop_nouser(void)
> >>>      g_test_trap_assert_stdout("");
> >>>  }
> >>>
> >>> +/* Test if global props affecting subclasses are applied in the right order */
> >>
> >> Now that I think about it this is affecting an  external and
> >> (end-)user facing interface. You say this is the right order.
> >> Based on what is this the right order? Do we need to update some
> >> documentation too?
> > 
> > -global documentation is already very poor, as you have noticed:
> > 
> >>
> >> I've checked out the user manual. There we talk about 'driver' but
> >> I could not find a spot where 'driver' is explained.
> > 
> > Yes.  For that reason, it looks like there's nothing to be
> > updated on the existing (poor) documentation.  I will re-read it
> > to be sure.
> > 
> 
> I don't agree. IMHO poor is reason enough to update.

I agree we should update it.  I just meant I don't see any
existing documentation that will become incorrect/outdated when
we apply this patch.

> 
> >>
> >> If I would have to translate between user manual terminology and
> >> code terminology, I would say a driver is a not abstract class
> >> inheriting from device. If that's right I wonder if it should
> >> be allowed for a non-abstract class inheriting from device to
> >> inherit form another non-abstract class.
> > 
> > We could, but this wouldn't save us from having to decide what to
> > do if people are already using those non-abstract superclasses
> > with -global.  e.g.: "-global spapr-pci-host-bridge.FOO=BAR"
> > (spapr-pci-host-bridge is the parent of spapr-pci-vfio-host-bridge).
> > 
> > This means a new rule like this would necessarily break
> > command-line compatibility.  Probably not a blocker in the
> > spapr-pci-host-bridge case, but to me it looks like the rule
> > would cause more problems than it would solve.
> 
> We are already breaking the command line compatibility with this
> series aren't we?
> 
> You may be right about the cause more problems than it solves though.
> I may end up thinking some more about this (or forgetting about it).

Yeah.  Breaking compatibility is not a blocker, but one
additional obstacle for changing the rules.

> > 
> >>
> >> Another question is -global with an abstract class seems to be
> >> accepted on the command line. Is that an undocumented feature?
> > 
> > Considering that we never documented that "driver" could be a
> > superclass, that's true: it is an undocumented feature.
> > 
> > However, it is a feature people are likely to be relying upon, to
> > configure a feature on all devices of a given type.
> > 
> 
> Nod. I do however see a difference between abstract and non-abstract.

I think I see your point.  The semantics of "-global" would be
clearer if all non-leaf classes were abstract.

-- 
Eduardo