Message ID | 7dcbdc8169de6b0090aa1ba89e7426fb3cd0e6cf.1440171025.git.DirtY.iCE.hu@gmail.com |
---|---|
State | New |
Headers | show |
On Fri, Aug 21, 2015 at 05:36:59PM +0200, Kővágó, Zoltán wrote: > Signed-off-by: Kővágó, Zoltán <DirtY.iCE.hu@gmail.com> I don't understand QAPI enough to understand why exactly this is needed (so I would like to get feedback from somebody who actually understands QAPI unions), but I have one comment below. [...] > ## > +# @NumaDriver > +# > +# List of possible numa drivers. > +# > +# Since: 2.5 > +## > +{ 'enum': 'NumaDriver', > + 'data': [ 'node' ] } Why is the name "NumaDriver"? Below, the field is called "type", so why not something like "NumaOptionType"? > + > +## > +# @NumaCommonOptions > +# > +# Common set of numa options. > +# > +# @type: the numa driver to use > +# > +# Since: 2.5 > +## > +{ 'struct': 'NumaCommonOptions', > + 'data': { > + 'type': 'NumaDriver' } } > + > +## > +# @NumaOptions > +# > +# A discriminated record of NUMA options. (for OptsVisitor) > +# > +# Since 2.1 > +## > +{ 'union': 'NumaOptions', > + 'base': 'NumaCommonOptions', > + 'discriminator': 'type', > + 'data': { > + 'node': 'NumaNodeOptions' }} > + > +## > # @HostMemPolicy > # > # Host memory policy types > -- > 2.5.0 >
2015-08-22 01:13 keltezéssel, Eduardo Habkost írta: > On Fri, Aug 21, 2015 at 05:36:59PM +0200, Kővágó, Zoltán wrote: >> Signed-off-by: Kővágó, Zoltán <DirtY.iCE.hu@gmail.com> > > I don't understand QAPI enough to understand why exactly this is needed > (so I would like to get feedback from somebody who actually understands > QAPI unions), but I have one comment below. It's needed so the option visitor can support nested structures properly without flattening them (or breaking backward compatibility). There is some discussion here: http://lists.nongnu.org/archive/html/qemu-devel/2015-06/msg04438.html But basically the thing is that with new new opts visitor, unless we convert NumaOptions into a flat union, the user would have to type -numa node,node.nodeid=foo instead of -numa node,nodeid=foo which would break backward compatibility. > > [...] >> ## >> +# @NumaDriver >> +# >> +# List of possible numa drivers. >> +# >> +# Since: 2.5 >> +## >> +{ 'enum': 'NumaDriver', >> + 'data': [ 'node' ] } > > Why is the name "NumaDriver"? Below, the field is called "type", so why > not something like "NumaOptionType"? No particular reason other than the example in docs/qapi-code-gen.txt used driver. The field is called type because in the non-flat union the discriminator is called type. >> + >> +## >> +# @NumaCommonOptions >> +# >> +# Common set of numa options. >> +# >> +# @type: the numa driver to use >> +# >> +# Since: 2.5 >> +## >> +{ 'struct': 'NumaCommonOptions', >> + 'data': { >> + 'type': 'NumaDriver' } } >> + >> +## >> +# @NumaOptions >> +# >> +# A discriminated record of NUMA options. (for OptsVisitor) >> +# >> +# Since 2.1 >> +## >> +{ 'union': 'NumaOptions', >> + 'base': 'NumaCommonOptions', >> + 'discriminator': 'type', >> + 'data': { >> + 'node': 'NumaNodeOptions' }} >> + >> +## >> # @HostMemPolicy >> # >> # Host memory policy types >> -- >> 2.5.0 >> >
On Sat, Aug 22, 2015 at 05:56:40PM +0200, Kővágó Zoltán wrote: > 2015-08-22 01:13 keltezéssel, Eduardo Habkost írta: > >On Fri, Aug 21, 2015 at 05:36:59PM +0200, Kővágó, Zoltán wrote: > >>Signed-off-by: Kővágó, Zoltán <DirtY.iCE.hu@gmail.com> > > > >I don't understand QAPI enough to understand why exactly this is needed > >(so I would like to get feedback from somebody who actually understands > >QAPI unions), but I have one comment below. > > It's needed so the option visitor can support nested structures properly > without flattening them (or breaking backward compatibility). > > There is some discussion here: > http://lists.nongnu.org/archive/html/qemu-devel/2015-06/msg04438.html > > But basically the thing is that with new new opts visitor, unless we convert > NumaOptions into a flat union, the user would have to type -numa > node,node.nodeid=foo instead of -numa node,nodeid=foo which would break > backward compatibility. As long as somebody who understands QAPI says the changes make sense and should work, I am OK with them if the following is changed: > >[...] > >> ## > >>+# @NumaDriver > >>+# > >>+# List of possible numa drivers. > >>+# > >>+# Since: 2.5 > >>+## > >>+{ 'enum': 'NumaDriver', > >>+ 'data': [ 'node' ] } > > > >Why is the name "NumaDriver"? Below, the field is called "type", so why > >not something like "NumaOptionType"? > > No particular reason other than the example in docs/qapi-code-gen.txt used > driver. The field is called type because in the non-flat union the > discriminator is called type. In the docs/qapi-code-gen.txt example, the option is about an actual blockdev driver, and the discriminator is really called "driver". In this case, the field is called "type" and has nothing to do with drivers, so something like NumaOptionType makes more sense.
On 08/21/2015 09:36 AM, Kővágó, Zoltán wrote: > Signed-off-by: Kővágó, Zoltán <DirtY.iCE.hu@gmail.com> The subject line is a one-line "what", and the commit body should be the "why" - all but the most trivial "what" deserve a non-empty commit body. In particular, I think you NEED to mention in the commit body that the conversion does not strictly follow the QMP wire equivalency between simple and flat unions as spelled out in docs/qapi-code-gen.txt, but that this is okay because we do not have any QMP commands that use the NumaOptions type to begin with (so there is no observable change to any commands sent over QMP). Meanwhile, mention that the change is made to convert the generated C code into something that will ultimately be easier to work with. > --- > numa.c | 2 +- > qapi-schema.json | 47 ++++++++++++++++++++++++++++++++++++----------- > 2 files changed, 37 insertions(+), 12 deletions(-) > > > +++ b/numa.c > @@ -227,7 +227,7 @@ static int parse_numa(void *opaque, QemuOpts *opts, Error **errp) > } > > switch (object->kind) { > - case NUMA_OPTIONS_KIND_NODE: > + case NUMA_DRIVER_NODE: Now that commit 0f61af3 has landed, you'll have to rebase your patch to use 'switch (object->type) {' here. Basically, the conversion from simple to flat union now (temporarily) generates a different tag name for simple unions than for flat unions (although I have a pending patch [1] that solves that, but by using object->type everywhere). [1] https://lists.gnu.org/archive/html/qemu-devel/2015-08/msg02061.html > +++ b/qapi-schema.json > @@ -3536,17 +3536,6 @@ > 'data': { '*console':'int', 'events': [ 'InputEvent' ] } } > > ## > -# @NumaOptions > -# > -# A discriminated record of NUMA options. (for OptsVisitor) > -# > -# Since 2.1 > -## > -{ 'union': 'NumaOptions', > - 'data': { > - 'node': 'NumaNodeOptions' }} If we were following the equivalency documented in docs/qapi-code-gen.txt, then this part is okay (you are removing the simple union),... > ## > +# @NumaDriver > +# > +# List of possible numa drivers. > +# > +# Since: 2.5 > +## > +{ 'enum': 'NumaDriver', > + 'data': [ 'node' ] } ...this part is okay (you are creating a new enum, which covers all branches present in the former simple union),... > + > +## > +# @NumaCommonOptions > +# > +# Common set of numa options. > +# > +# @type: the numa driver to use > +# > +# Since: 2.5 > +## > +{ 'struct': 'NumaCommonOptions', > + 'data': { > + 'type': 'NumaDriver' } } ...this part is okay (you are creating a new base type, which consists of a single common member named 'type' of the new enum type),... [By the way, while the base type is currently required, I have a pending patch that would add syntax sugar to avoid it, so maybe you want to wait for my patch [2] to land? [2] https://lists.gnu.org/archive/html/qemu-devel/2015-08/msg02321.html so you could write 'base': { 'type': 'NumaDriver' } directly in the flat union, instead of having to declare NumaCommonOptions] > + > +## > +# @NumaOptions > +# > +# A discriminated record of NUMA options. (for OptsVisitor) > +# > +# Since 2.1 > +## > +{ 'union': 'NumaOptions', > + 'base': 'NumaCommonOptions', > + 'discriminator': 'type', > + 'data': { > + 'node': 'NumaNodeOptions' }} ...but this part is missing one piece of the conversion (to be strictly QMP compatible with the simple union, each branch of the flat union must be a NEW type that contains a single member 'data' of the old type). That is, if we were TRYING to preserve QMP compatibility, we'd need: { 'struct': 'NumaNodeOptionsWrapper', 'data': { 'data': 'NumaNodeOptions' } } { 'union': 'NumaOptions', 'base': 'NumaCommonOptions', 'discriminator': 'type', 'data': { 'node': 'NumaNodeOptionsWrapper' } } The difference on the wire is that the old simple union, or with my version of the flat union, would allow: { 'command': 'foo', 'arguments': { 'type': 'numa', 'data': { 'nodeid': 1 } } } while the new flat union as you wrote it reduces nesting: { 'command': 'foo', 'arguments': { 'type': 'numa', 'nodeid': 1 } } That said, there is another point of view to worry about, which is what C structures get generated. Here, I'm using output after Markus' v4 series is applied (since it is slightly more legible than current qemu.git mainline generated output) (and although I still have further pending patches that improve it further). The pre-patch simple union would generate: struct NumaOptions { NumaOptionsKind kind; union { /* union tag is @kind */ void *data; NumaNodeOptions *node; }; }; and your version would generate (notice the rename from 'kind' to 'type'): struct NumaOptions { /* Members inherited from NumaCommonOptions: */ NumaDriver type; /* Own members: */ union { /* union tag is @type */ void *data; NumaNodeOptions *node; }; }; but my proposal to keep QMP compatibility would generate an incompatible layer of boxing: struct NumaOptions { /* Members inherited from NumaCommonOptions: */ NumaDriver type; /* Own members: */ union { /* union tag is @type */ void *data; NumaNodeOptionsWrapper *node; }; }; So, in conclusion, since no existing QMP command used the old type, and your choice of changes preserved the generated struct layout, it means we have no change to command line parsing and no user-visible changes to worry about. And I'm fine with this patch, once you improve the commit message and rebase to mainline: Reviewed-by: Eric Blake <eblake@redhat.com>
On 08/26/2015 09:31 AM, Eduardo Habkost wrote: > As long as somebody who understands QAPI says the changes make sense and > should work, I am OK with them if the following is changed: That would be me, right? See my other mail for my tentative R-b; although if you start renaming things due to this discussion, maybe I should review v3 more closely after all. >>>> +## >>>> +{ 'enum': 'NumaDriver', >>>> + 'data': [ 'node' ] } >>> >>> Why is the name "NumaDriver"? Below, the field is called "type", so why >>> not something like "NumaOptionType"? >> >> No particular reason other than the example in docs/qapi-code-gen.txt used >> driver. The field is called type because in the non-flat union the >> discriminator is called type. > > In the docs/qapi-code-gen.txt example, the option is about an actual > blockdev driver, and the discriminator is really called "driver". > > In this case, the field is called "type" and has nothing to do with > drivers, so something like NumaOptionType makes more sense. The enum name is not ABI (we can name it whatever makes sense), but does show up in the C code that interacts with the generated type. In fact, if you could name the enum type 'NumaOptionsKind', then the C enum name would not change (staying at NUMA_OPTIONS_KIND_NODE instead of your change to NUMA_DRIVER_NODE). Except that right now, the qapi generator doesn't allow user-defined types ending in 'Kind'. :( I don't know if naming it NumaDriver makes sense; and so your suggestion of naming it NumaOptionsType may be better in the long run. It would make your C enum become NUMA_OPTIONS_TYPE_NODE. Or maybe you wait for me to do a followup patch to the qapi generator to allow user-defined types ending in 'Kind'.
diff --git a/numa.c b/numa.c index 402804b..376f990 100644 --- a/numa.c +++ b/numa.c @@ -227,7 +227,7 @@ static int parse_numa(void *opaque, QemuOpts *opts, Error **errp) } switch (object->kind) { - case NUMA_OPTIONS_KIND_NODE: + case NUMA_DRIVER_NODE: numa_node_parse(object->node, opts, &err); if (err) { goto error; diff --git a/qapi-schema.json b/qapi-schema.json index 4342a08..999faa3 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -3536,17 +3536,6 @@ 'data': { '*console':'int', 'events': [ 'InputEvent' ] } } ## -# @NumaOptions -# -# A discriminated record of NUMA options. (for OptsVisitor) -# -# Since 2.1 -## -{ 'union': 'NumaOptions', - 'data': { - 'node': 'NumaNodeOptions' }} - -## # @NumaNodeOptions # # Create a guest NUMA node. (for OptsVisitor) @@ -3573,6 +3562,42 @@ '*memdev': 'str' }} ## +# @NumaDriver +# +# List of possible numa drivers. +# +# Since: 2.5 +## +{ 'enum': 'NumaDriver', + 'data': [ 'node' ] } + +## +# @NumaCommonOptions +# +# Common set of numa options. +# +# @type: the numa driver to use +# +# Since: 2.5 +## +{ 'struct': 'NumaCommonOptions', + 'data': { + 'type': 'NumaDriver' } } + +## +# @NumaOptions +# +# A discriminated record of NUMA options. (for OptsVisitor) +# +# Since 2.1 +## +{ 'union': 'NumaOptions', + 'base': 'NumaCommonOptions', + 'discriminator': 'type', + 'data': { + 'node': 'NumaNodeOptions' }} + +## # @HostMemPolicy # # Host memory policy types
Signed-off-by: Kővágó, Zoltán <DirtY.iCE.hu@gmail.com> --- numa.c | 2 +- qapi-schema.json | 47 ++++++++++++++++++++++++++++++++++++----------- 2 files changed, 37 insertions(+), 12 deletions(-)