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 <ehabk...@redhat.com>
> >>
> >> Reviewed-by: Halil Pasic <pa...@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
