[PATCH 0/1] qom: fix setting of qdev array properties

[PATCH 0/1] qom: fix setting of qdev array properties

[Daniel P. Berrangé]

By the time of the 8.2.0 release, it will have been 2 years and 6
releases since we accidentally broke setting of array properties
for user creatable devices:

  https://gitlab.com/qemu-project/qemu/-/issues/1090

Some context:

* Initial identification / report on the mailing list

   https://lists.gnu.org/archive/html/qemu-devel/2022-07/msg00111.html

* Sub-thread of that exploring the background on need/use of array
  properties:

   https://lists.gnu.org/archive/html/qemu-devel/2022-07/msg01531.html

* Markus' initial PoC for an order preserving QDict impl

   https://lists.gnu.org/archive/html/qemu-devel/2022-07/msg00758.html

* A later (unrelated?) patch for order preserving QDict impl

   https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg03229.html

* A re-posting of the new patch

   https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg00292.html

Personally I'm not a fan of the introducing the order preserving QDict
impl, because I feel that the need to preserve QDict ordering is a
design bug. Not that I think the current ordering when iterating over
QDict is in any way special. I just rather see the ordering left as
"undefined" and any callers that need a specific ordering should apply
what they need.

Since setting device array properties requires that 'len-XXX' be
processed first, so the following patch does exactly that. We iterate
over the properties twice, first setting the 'len-XXX' properties,
then setting everything else.

I still think for user creatable devices we'd be better off just
mandating the use of JSON syntax for -device and thus leveraging
the native JSON array type. This patch was the quick fix for the
existing array property syntax though.

Daniel P. Berrangé (1):
  qom: fix setting of array properties

 qom/object_interfaces.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

-- 
2.41.0

[PATCH 1/1] qom: fix setting of array properties

[Daniel P. Berrangé]

DEFINE_PROP_ARRAY() creates a property 'len-$ARRAY-PROP-NAME'
which, when set, will create a sequence of '$ARRAY-PROP-NAME[N]'
properties.

This only works if the 'len-$ARRAY-PROP-NAME' property is
set first, and the array elements afterwards. Historically
this required the user to set correct ordering and QemuOpts
traversal would preserve that ordering. With QemuOpts now
converted to QDict, iteration ordering is undefined. Thus
to keep array properties working, we iterate over the QDict
twice.

Doing this in QOM is a bit of a layering violation since
DEFINE_PROP_ARRAY is part of QDev, but it is the simplest
option to preserve backwards compatibility, without ripple
effects across any other part of QEMU.

Fixes: https://gitlab.com/qemu-project/qemu/-/issues/1090
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 qom/object_interfaces.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index 7d31589b04..6aaaf42ffc 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -51,7 +51,37 @@ static void object_set_properties_from_qdict(Object *obj, const QDict *qdict,
     if (!visit_start_struct(v, NULL, NULL, 0, errp)) {
         return;
     }
+
+    /* Layering violation here...
+     *
+     * DEFINE_PROP_ARRAY() creates a property 'len-$ARRAY-PROP-NAME'
+     * which, when set, will create a sequence of '$ARRAY-PROP-NAME[N]'
+     * properties.
+     *
+     * This only works if the 'len-$ARRAY-PROP-NAME' property is
+     * set first, and the array elements afterwards. Historically
+     * this required the user to get correct ordering and QemuOpts
+     * traversal would preserve that ordering. With QemuOpts now
+     * converted to QDict, iteration ordering is undefined. Thus
+     * to keep array properties working, we iterate over the QDict
+     * twice.
+     */
+
+    /* First the props that control array property length */
     for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) {
+        if (!g_str_has_prefix(e->key, "len-")) {
+            continue;
+        }
+        if (!object_property_set(obj, e->key, v, errp)) {
+            goto out;
+        }
+    }
+
+    /* Then any other normal properties */
+    for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) {
+        if (g_str_has_prefix(e->key, "len-")) {
+            continue;
+        }
         if (!object_property_set(obj, e->key, v, errp)) {
             goto out;
         }
-- 
2.41.0

Re: [PATCH 0/1] qom: fix setting of qdev array properties

[Kevin Wolf]

Am 04.09.2023 um 18:25 hat Daniel P. Berrangé geschrieben:
> By the time of the 8.2.0 release, it will have been 2 years and 6
> releases since we accidentally broke setting of array properties
> for user creatable devices:
> 
>   https://gitlab.com/qemu-project/qemu/-/issues/1090

Oh, nice!

Well, maybe that sounds a bit wrong, but the syntax that was broken was
problematic and more of a hack, and after two years there is clearly no
need to bring the exact same syntax back now.

So I'd suggest we bring the funcionality back, but with proper QAPI
lists instead of len-foo/foo[*].

If we ever want to continue with command line QAPIfication, this change
would already solve one of the compatibility concerns we've had in the
past.

> I still think for user creatable devices we'd be better off just
> mandating the use of JSON syntax for -device and thus leveraging
> the native JSON array type. This patch was the quick fix for the
> existing array property syntax though.

I agree, let's not apply this one. It puts another ugly hack in the
common QOM code path just to bring back the old ugly hack in qdev.

Kevin

Re: [PATCH 0/1] qom: fix setting of qdev array properties

[Peter Maydell]

On Tue, 5 Sept 2023 at 09:59, Kevin Wolf <kwolf@redhat.com> wrote:
>
> Am 04.09.2023 um 18:25 hat Daniel P. Berrangé geschrieben:
> > By the time of the 8.2.0 release, it will have been 2 years and 6
> > releases since we accidentally broke setting of array properties
> > for user creatable devices:
> >
> >   https://gitlab.com/qemu-project/qemu/-/issues/1090
>
> Oh, nice!
>
> Well, maybe that sounds a bit wrong, but the syntax that was broken was
> problematic and more of a hack, and after two years there is clearly no
> need to bring the exact same syntax back now.
>
> So I'd suggest we bring the funcionality back, but with proper QAPI
> lists instead of len-foo/foo[*].

I don't object, as long as somebody is proposing to actually
do this work (eg, in this release cycle), rather than merely
suggest the idea as a reason why we should continue to leave
this device's configurability broken...

thanks
-- PMM

Re: [PATCH 0/1] qom: fix setting of qdev array properties

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 04.09.2023 um 18:25 hat Daniel P. Berrangé geschrieben:
>> By the time of the 8.2.0 release, it will have been 2 years and 6
>> releases since we accidentally broke setting of array properties
>> for user creatable devices:
>> 
>>   https://gitlab.com/qemu-project/qemu/-/issues/1090
>
> Oh, nice!

Nice?  *Awesome*!

> Well, maybe that sounds a bit wrong, but the syntax that was broken was
> problematic and more of a hack,

A monstrosity, in my opinion.  I tried to strangle it in the crib, but
its guardians wouldn't let me.  Can dig up references for the morbidly
curious.

>                                 and after two years there is clearly no
> need to bring the exact same syntax back now.

Exactly.

> So I'd suggest we bring the funcionality back, but with proper QAPI
> lists instead of len-foo/foo[*].
>
> If we ever want to continue with command line QAPIfication, this change
> would already solve one of the compatibility concerns we've had in the
> past.
>
>> I still think for user creatable devices we'd be better off just
>> mandating the use of JSON syntax for -device and thus leveraging
>> the native JSON array type. This patch was the quick fix for the
>> existing array property syntax though.
>
> I agree, let's not apply this one. It puts another ugly hack in the
> common QOM code path just to bring back the old ugly hack in qdev.

Since -device supports both JSON and dotted keys, we'd still offer a
(differently ugly) solution for users averse to JSON.

Re: [PATCH 0/1] qom: fix setting of qdev array properties

[Peter Maydell]

On Thu, 7 Sept 2023 at 10:33, Markus Armbruster <armbru@redhat.com> wrote:
>
> Kevin Wolf <kwolf@redhat.com> writes:
>
> > Am 04.09.2023 um 18:25 hat Daniel P. Berrangé geschrieben:
> >> By the time of the 8.2.0 release, it will have been 2 years and 6
> >> releases since we accidentally broke setting of array properties
> >> for user creatable devices:
> >>
> >>   https://gitlab.com/qemu-project/qemu/-/issues/1090
> >
> > Oh, nice!
>
> Nice?  *Awesome*!
>
> > Well, maybe that sounds a bit wrong, but the syntax that was broken was
> > problematic and more of a hack,
>
> A monstrosity, in my opinion.  I tried to strangle it in the crib, but
> its guardians wouldn't let me.  Can dig up references for the morbidly
> curious.

I don't care about the syntax on the command line much (AFAIK that's
just the rocker device). But the actual feature is used more widely
within QEMU itself for devices created in C code, which is what it
was intended for. If you want to get rid of it you need to provide
an adequate replacement.

-- PMM

Re: [PATCH 0/1] qom: fix setting of qdev array properties

[Daniel P. Berrangé]

On Thu, Sep 07, 2023 at 10:35:22AM +0100, Peter Maydell wrote:
> On Thu, 7 Sept 2023 at 10:33, Markus Armbruster <armbru@redhat.com> wrote:
> >
> > Kevin Wolf <kwolf@redhat.com> writes:
> >
> > > Am 04.09.2023 um 18:25 hat Daniel P. Berrangé geschrieben:
> > >> By the time of the 8.2.0 release, it will have been 2 years and 6
> > >> releases since we accidentally broke setting of array properties
> > >> for user creatable devices:
> > >>
> > >>   https://gitlab.com/qemu-project/qemu/-/issues/1090
> > >
> > > Oh, nice!
> >
> > Nice?  *Awesome*!
> >
> > > Well, maybe that sounds a bit wrong, but the syntax that was broken was
> > > problematic and more of a hack,
> >
> > A monstrosity, in my opinion.  I tried to strangle it in the crib, but
> > its guardians wouldn't let me.  Can dig up references for the morbidly
> > curious.
> 
> I don't care about the syntax on the command line much (AFAIK that's
> just the rocker device). But the actual feature is used more widely
> within QEMU itself for devices created in C code, which is what it
> was intended for. If you want to get rid of it you need to provide
> an adequate replacement.

I wonder if we can poison  DEFINE_PROP_ARRAY somewhere such that
it fails if DeviceClass user_creatable == true.

That would let internal code carry on using it, while ensuring
anyone creating a new user creatable device will quickly fnid
out it doesn't allow these arrays. Rocker would need fixing
of course.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 0/1] qom: fix setting of qdev array properties

[Kevin Wolf]

Am 07.09.2023 um 11:35 hat Peter Maydell geschrieben:
> On Thu, 7 Sept 2023 at 10:33, Markus Armbruster <armbru@redhat.com> wrote:
> >
> > Kevin Wolf <kwolf@redhat.com> writes:
> >
> > > Am 04.09.2023 um 18:25 hat Daniel P. Berrangé geschrieben:
> > >> By the time of the 8.2.0 release, it will have been 2 years and 6
> > >> releases since we accidentally broke setting of array properties
> > >> for user creatable devices:
> > >>
> > >>   https://gitlab.com/qemu-project/qemu/-/issues/1090
> > >
> > > Oh, nice!
> >
> > Nice?  *Awesome*!
> >
> > > Well, maybe that sounds a bit wrong, but the syntax that was broken was
> > > problematic and more of a hack,
> >
> > A monstrosity, in my opinion.  I tried to strangle it in the crib, but
> > its guardians wouldn't let me.  Can dig up references for the morbidly
> > curious.
> 
> I don't care about the syntax on the command line much (AFAIK that's
> just the rocker device). But the actual feature is used more widely
> within QEMU itself for devices created in C code, which is what it
> was intended for. If you want to get rid of it you need to provide
> an adequate replacement.

I have a patch to use QList (i.e. JSON lists) that seems to work for the
rocker case. Now I need to find and update all of those internal
callers. Should grepping for '"len-' find all instances that need to be
changed or are you aware of other ways to access the feature?

Kevin

Re: [PATCH 0/1] qom: fix setting of qdev array properties

[Daniel P. Berrangé]

On Fri, Sep 08, 2023 at 11:25:54AM +0200, Kevin Wolf wrote:
> Am 07.09.2023 um 11:35 hat Peter Maydell geschrieben:
> > On Thu, 7 Sept 2023 at 10:33, Markus Armbruster <armbru@redhat.com> wrote:
> > >
> > > Kevin Wolf <kwolf@redhat.com> writes:
> > >
> > > > Am 04.09.2023 um 18:25 hat Daniel P. Berrangé geschrieben:
> > > >> By the time of the 8.2.0 release, it will have been 2 years and 6
> > > >> releases since we accidentally broke setting of array properties
> > > >> for user creatable devices:
> > > >>
> > > >>   https://gitlab.com/qemu-project/qemu/-/issues/1090
> > > >
> > > > Oh, nice!
> > >
> > > Nice?  *Awesome*!
> > >
> > > > Well, maybe that sounds a bit wrong, but the syntax that was broken was
> > > > problematic and more of a hack,
> > >
> > > A monstrosity, in my opinion.  I tried to strangle it in the crib, but
> > > its guardians wouldn't let me.  Can dig up references for the morbidly
> > > curious.
> > 
> > I don't care about the syntax on the command line much (AFAIK that's
> > just the rocker device). But the actual feature is used more widely
> > within QEMU itself for devices created in C code, which is what it
> > was intended for. If you want to get rid of it you need to provide
> > an adequate replacement.
> 
> I have a patch to use QList (i.e. JSON lists) that seems to work for the
> rocker case. Now I need to find and update all of those internal
> callers. Should grepping for '"len-' find all instances that need to be
> changed or are you aware of other ways to access the feature?

IMHO we can just leave the internal only code callers unchanged. I was
about to send this patch to prevent usage leaking into user creatable
devices:

diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 357b8761b5..2d295411ef 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -584,6 +584,12 @@ static void set_prop_arraylen(Object *obj, Visitor *v, const char *name,
     void *eltptr;
     const char *arrayname;
     int i;
+    DeviceClass *devc = DEVICE_CLASS(object_get_class(obj));
+
+    if (devc->user_creatable) {
+        error_setg(errp, "array property not permitted for user creatable devices");
+        return;
+    }
 
     if (*alenptr) {
         error_setg(errp, "array size property %s may not be set more than once",



With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 0/1] qom: fix setting of qdev array properties

[Kevin Wolf]

Am 08.09.2023 um 11:27 hat Daniel P. Berrangé geschrieben:
> On Fri, Sep 08, 2023 at 11:25:54AM +0200, Kevin Wolf wrote:
> > Am 07.09.2023 um 11:35 hat Peter Maydell geschrieben:
> > > On Thu, 7 Sept 2023 at 10:33, Markus Armbruster <armbru@redhat.com> wrote:
> > > >
> > > > Kevin Wolf <kwolf@redhat.com> writes:
> > > >
> > > > > Am 04.09.2023 um 18:25 hat Daniel P. Berrangé geschrieben:
> > > > >> By the time of the 8.2.0 release, it will have been 2 years and 6
> > > > >> releases since we accidentally broke setting of array properties
> > > > >> for user creatable devices:
> > > > >>
> > > > >>   https://gitlab.com/qemu-project/qemu/-/issues/1090
> > > > >
> > > > > Oh, nice!
> > > >
> > > > Nice?  *Awesome*!
> > > >
> > > > > Well, maybe that sounds a bit wrong, but the syntax that was broken was
> > > > > problematic and more of a hack,
> > > >
> > > > A monstrosity, in my opinion.  I tried to strangle it in the crib, but
> > > > its guardians wouldn't let me.  Can dig up references for the morbidly
> > > > curious.
> > > 
> > > I don't care about the syntax on the command line much (AFAIK that's
> > > just the rocker device). But the actual feature is used more widely
> > > within QEMU itself for devices created in C code, which is what it
> > > was intended for. If you want to get rid of it you need to provide
> > > an adequate replacement.
> > 
> > I have a patch to use QList (i.e. JSON lists) that seems to work for the
> > rocker case. Now I need to find and update all of those internal
> > callers. Should grepping for '"len-' find all instances that need to be
> > changed or are you aware of other ways to access the feature?
> 
> IMHO we can just leave the internal only code callers unchanged. I was
> about to send this patch to prevent usage leaking into user creatable
> devices:
> [...]

The bug report is about a user creatable device (rocker) that doesn't
work any more, so forbidding the case entirely doesn't really improve
the situation.

Kevin

Re: [PATCH 0/1] qom: fix setting of qdev array properties

[Daniel P. Berrangé]

On Fri, Sep 08, 2023 at 02:16:35PM +0200, Kevin Wolf wrote:
> Am 08.09.2023 um 11:27 hat Daniel P. Berrangé geschrieben:
> > On Fri, Sep 08, 2023 at 11:25:54AM +0200, Kevin Wolf wrote:
> > > Am 07.09.2023 um 11:35 hat Peter Maydell geschrieben:
> > > > On Thu, 7 Sept 2023 at 10:33, Markus Armbruster <armbru@redhat.com> wrote:
> > > > >
> > > > > Kevin Wolf <kwolf@redhat.com> writes:
> > > > >
> > > > > > Am 04.09.2023 um 18:25 hat Daniel P. Berrangé geschrieben:
> > > > > >> By the time of the 8.2.0 release, it will have been 2 years and 6
> > > > > >> releases since we accidentally broke setting of array properties
> > > > > >> for user creatable devices:
> > > > > >>
> > > > > >>   https://gitlab.com/qemu-project/qemu/-/issues/1090
> > > > > >
> > > > > > Oh, nice!
> > > > >
> > > > > Nice?  *Awesome*!
> > > > >
> > > > > > Well, maybe that sounds a bit wrong, but the syntax that was broken was
> > > > > > problematic and more of a hack,
> > > > >
> > > > > A monstrosity, in my opinion.  I tried to strangle it in the crib, but
> > > > > its guardians wouldn't let me.  Can dig up references for the morbidly
> > > > > curious.
> > > > 
> > > > I don't care about the syntax on the command line much (AFAIK that's
> > > > just the rocker device). But the actual feature is used more widely
> > > > within QEMU itself for devices created in C code, which is what it
> > > > was intended for. If you want to get rid of it you need to provide
> > > > an adequate replacement.
> > > 
> > > I have a patch to use QList (i.e. JSON lists) that seems to work for the
> > > rocker case. Now I need to find and update all of those internal
> > > callers. Should grepping for '"len-' find all instances that need to be
> > > changed or are you aware of other ways to access the feature?
> > 
> > IMHO we can just leave the internal only code callers unchanged. I was
> > about to send this patch to prevent usage leaking into user creatable
> > devices:
> > [...]
> 
> The bug report is about a user creatable device (rocker) that doesn't
> work any more, so forbidding the case entirely doesn't really improve
> the situation.

Oh, sure we still need to patch rocker.c to make it use a QAPI friendly
way of accepting the list of devices.

The forbidding patch was just to proactively prevent the bad design
practice recurring in the future.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 0/1] qom: fix setting of qdev array properties

[Peter Maydell]

On Fri, 8 Sept 2023 at 10:26, Kevin Wolf <kwolf@redhat.com> wrote:
>
> Am 07.09.2023 um 11:35 hat Peter Maydell geschrieben:
> > On Thu, 7 Sept 2023 at 10:33, Markus Armbruster <armbru@redhat.com> wrote:
> > >
> > > Kevin Wolf <kwolf@redhat.com> writes:
> > >
> > > > Am 04.09.2023 um 18:25 hat Daniel P. Berrangé geschrieben:
> > > >> By the time of the 8.2.0 release, it will have been 2 years and 6
> > > >> releases since we accidentally broke setting of array properties
> > > >> for user creatable devices:
> > > >>
> > > >>   https://gitlab.com/qemu-project/qemu/-/issues/1090
> > > >
> > > > Oh, nice!
> > >
> > > Nice?  *Awesome*!
> > >
> > > > Well, maybe that sounds a bit wrong, but the syntax that was broken was
> > > > problematic and more of a hack,
> > >
> > > A monstrosity, in my opinion.  I tried to strangle it in the crib, but
> > > its guardians wouldn't let me.  Can dig up references for the morbidly
> > > curious.
> >
> > I don't care about the syntax on the command line much (AFAIK that's
> > just the rocker device). But the actual feature is used more widely
> > within QEMU itself for devices created in C code, which is what it
> > was intended for. If you want to get rid of it you need to provide
> > an adequate replacement.
>
> I have a patch to use QList (i.e. JSON lists) that seems to work for the
> rocker case. Now I need to find and update all of those internal
> callers. Should grepping for '"len-' find all instances that need to be
> changed or are you aware of other ways to access the feature?

AFAIK the only way to use the feature is to set the len-foo and
then foo[0], foo[1], ... properties, using any of the usual APIs.
So git grep '\<len-[^-]' should find them all.

If you want a cross-check, the devices that use it are easy
to find (search for DEFINE_PROP_ARRAY), and almost all of them
picked property names that are easy to grep for.

But as Daniel says, if you haven't changed the behaviour for
"code sets the properties in the right order" they may not
need updating.

(I would be happy to see the rather hacky implementation replaced
with true support for list properties at the qom/qdev level.
But the hack is there because it was simpler :-))

thanks
-- PMM

Re: [PATCH 0/1] qom: fix setting of qdev array properties

[Kevin Wolf]

Am 08.09.2023 um 11:53 hat Peter Maydell geschrieben:
> On Fri, 8 Sept 2023 at 10:26, Kevin Wolf <kwolf@redhat.com> wrote:
> >
> > Am 07.09.2023 um 11:35 hat Peter Maydell geschrieben:
> > > On Thu, 7 Sept 2023 at 10:33, Markus Armbruster <armbru@redhat.com> wrote:
> > > >
> > > > Kevin Wolf <kwolf@redhat.com> writes:
> > > >
> > > > > Am 04.09.2023 um 18:25 hat Daniel P. Berrangé geschrieben:
> > > > >> By the time of the 8.2.0 release, it will have been 2 years and 6
> > > > >> releases since we accidentally broke setting of array properties
> > > > >> for user creatable devices:
> > > > >>
> > > > >>   https://gitlab.com/qemu-project/qemu/-/issues/1090
> > > > >
> > > > > Oh, nice!
> > > >
> > > > Nice?  *Awesome*!
> > > >
> > > > > Well, maybe that sounds a bit wrong, but the syntax that was broken was
> > > > > problematic and more of a hack,
> > > >
> > > > A monstrosity, in my opinion.  I tried to strangle it in the crib, but
> > > > its guardians wouldn't let me.  Can dig up references for the morbidly
> > > > curious.
> > >
> > > I don't care about the syntax on the command line much (AFAIK that's
> > > just the rocker device). But the actual feature is used more widely
> > > within QEMU itself for devices created in C code, which is what it
> > > was intended for. If you want to get rid of it you need to provide
> > > an adequate replacement.
> >
> > I have a patch to use QList (i.e. JSON lists) that seems to work for the
> > rocker case. Now I need to find and update all of those internal
> > callers. Should grepping for '"len-' find all instances that need to be
> > changed or are you aware of other ways to access the feature?
> 
> AFAIK the only way to use the feature is to set the len-foo and
> then foo[0], foo[1], ... properties, using any of the usual APIs.
> So git grep '\<len-[^-]' should find them all.
> 
> If you want a cross-check, the devices that use it are easy
> to find (search for DEFINE_PROP_ARRAY), and almost all of them
> picked property names that are easy to grep for.
> 
> But as Daniel says, if you haven't changed the behaviour for
> "code sets the properties in the right order" they may not
> need updating.

I'm replacing the 'len-foo' and 'foo[0]' etc. properties with a single
'foo' property that takes a list, so the callers need to be adjusted.
The devices can stay unchanged, though.

> (I would be happy to see the rather hacky implementation replaced
> with true support for list properties at the qom/qdev level.
> But the hack is there because it was simpler :-))

It's not really that hard, but it would probably have been even easier
if we had just used the list support in the visitor interface from the
beginning instead of adding this hack. (Though obviously that wouldn't
have worked for user creatable devices before we added JSON support to
-device.)

Kevin

Re: [PATCH 0/1] qom: fix setting of qdev array properties

[Peter Maydell]

On Fri, 8 Sept 2023 at 13:22, Kevin Wolf <kwolf@redhat.com> wrote:
>
> Am 08.09.2023 um 11:53 hat Peter Maydell geschrieben:
> > On Fri, 8 Sept 2023 at 10:26, Kevin Wolf <kwolf@redhat.com> wrote:
> > > I have a patch to use QList (i.e. JSON lists) that seems to work for the
> > > rocker case. Now I need to find and update all of those internal
> > > callers. Should grepping for '"len-' find all instances that need to be
> > > changed or are you aware of other ways to access the feature?
> >
> > AFAIK the only way to use the feature is to set the len-foo and
> > then foo[0], foo[1], ... properties, using any of the usual APIs.
> > So git grep '\<len-[^-]' should find them all.
> >
> > If you want a cross-check, the devices that use it are easy
> > to find (search for DEFINE_PROP_ARRAY), and almost all of them
> > picked property names that are easy to grep for.
> >
> > But as Daniel says, if you haven't changed the behaviour for
> > "code sets the properties in the right order" they may not
> > need updating.
>
> I'm replacing the 'len-foo' and 'foo[0]' etc. properties with a single
> 'foo' property that takes a list, so the callers need to be adjusted.
> The devices can stay unchanged, though.
>
> > (I would be happy to see the rather hacky implementation replaced
> > with true support for list properties at the qom/qdev level.
> > But the hack is there because it was simpler :-))
>
> It's not really that hard, but it would probably have been even easier
> if we had just used the list support in the visitor interface from the
> beginning instead of adding this hack. (Though obviously that wouldn't
> have worked for user creatable devices before we added JSON support to
> -device.)

If you'd suggested it back in 2013 I'd have been happy to do
array properties a different way :-)

thanks
-- PMM

Re: [PATCH 0/1] qom: fix setting of qdev array properties

[Kevin Wolf]

Am 07.09.2023 um 11:33 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> > Am 04.09.2023 um 18:25 hat Daniel P. Berrangé geschrieben:
> >> I still think for user creatable devices we'd be better off just
> >> mandating the use of JSON syntax for -device and thus leveraging
> >> the native JSON array type. This patch was the quick fix for the
> >> existing array property syntax though.
> >
> > I agree, let's not apply this one. It puts another ugly hack in the
> > common QOM code path just to bring back the old ugly hack in qdev.
> 
> Since -device supports both JSON and dotted keys, we'd still offer a
> (differently ugly) solution for users averse to JSON.

I'm afraid this is not true until we actually QAPIfy -device and change
the non-JSON path to the keyval parser. At the moment, it still uses
qemu_opts_parse_noisily(), so no dotted key syntax.

Kevin

Re: [PATCH 0/1] qom: fix setting of qdev array properties

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 07.09.2023 um 11:33 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> > Am 04.09.2023 um 18:25 hat Daniel P. Berrangé geschrieben:
>> >> I still think for user creatable devices we'd be better off just
>> >> mandating the use of JSON syntax for -device and thus leveraging
>> >> the native JSON array type. This patch was the quick fix for the
>> >> existing array property syntax though.
>> >
>> > I agree, let's not apply this one. It puts another ugly hack in the
>> > common QOM code path just to bring back the old ugly hack in qdev.
>> 
>> Since -device supports both JSON and dotted keys, we'd still offer a
>> (differently ugly) solution for users averse to JSON.
>
> I'm afraid this is not true until we actually QAPIfy -device and change
> the non-JSON path to the keyval parser. At the moment, it still uses
> qemu_opts_parse_noisily(), so no dotted key syntax.

You're right; we chickened out of switching the parser, and I suppressed
the memory.

Re: [PATCH 0/1] qom: fix setting of qdev array properties

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> By the time of the 8.2.0 release, it will have been 2 years and 6
> releases since we accidentally broke setting of array properties
> for user creatable devices:
>
>   https://gitlab.com/qemu-project/qemu/-/issues/1090
>
> Some context:
>
> * Initial identification / report on the mailing list
>
>    https://lists.gnu.org/archive/html/qemu-devel/2022-07/msg00111.html
>
> * Sub-thread of that exploring the background on need/use of array
>   properties:
>
>    https://lists.gnu.org/archive/html/qemu-devel/2022-07/msg01531.html
>
> * Markus' initial PoC for an order preserving QDict impl
>
>    https://lists.gnu.org/archive/html/qemu-devel/2022-07/msg00758.html
>
> * A later (unrelated?) patch for order preserving QDict impl
>
>    https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg03229.html
>
> * A re-posting of the new patch
>
>    https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg00292.html
>
> Personally I'm not a fan of the introducing the order preserving QDict
> impl, because I feel that the need to preserve QDict ordering is a
> design bug. Not that I think the current ordering when iterating over
> QDict is in any way special. I just rather see the ordering left as
> "undefined" and any callers that need a specific ordering should apply
> what they need.

QDict preserving order was never intended to be part of the interface.
But then Hyrum's Law kicked in.

Since it's been broken for so long, we now have a chance to kick it back
out.

However, if we want an order-preserving hash table (stress on *if*!), be
it for QDict or other uses: do it the elegant way it's done in Python.
Fun little project, but I couldn't justify the expense of doing it.

[...]