| Message ID | 20230904162544.2388037-1-berrange@redhat.com |
|---|---|
| Headers | show |
| Series | qom: fix setting of qdev array properties | expand |
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
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
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.
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
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. [...]
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
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
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.
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
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
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
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
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
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
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