| Message ID | 1374584606-5615-3-git-send-email-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 07/23/2013 07:03 AM, Kevin Wolf wrote: > The new 'base' key in a union definition refers to a struct type, which > is inlined into the union definition and can represent fields common to > all kinds. > > For example the following schema definition... > > { 'type': 'BlockOptionsBase', 'data': { 'read-only': 'bool' } } > > { 'union': 'BlockOptions', > 'base': 'BlockOptionsBase', > 'data': { > 'raw': 'BlockOptionsRaw' > 'qcow2': 'BlockOptionsQcow2' > } } > > ...would result in this generated C struct: > > struct BlockOptions > { > BlockOptionsKind kind; > union { > void *data; > BlockOptionsRaw * raw; > BlockOptionsQcow2 * qcow2; > }; > bool read_only; > }; > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > scripts/qapi-types.py | 16 ++++++++++++++-- > 1 file changed, 14 insertions(+), 2 deletions(-) Reviewed-by: Eric Blake <eblake@redhat.com>
On Tue, Jul 23, 2013 at 03:03:10PM +0200, Kevin Wolf wrote: > The new 'base' key in a union definition refers to a struct type, which > is inlined into the union definition and can represent fields common to > all kinds. > > For example the following schema definition... > > { 'type': 'BlockOptionsBase', 'data': { 'read-only': 'bool' } } > > { 'union': 'BlockOptions', > 'base': 'BlockOptionsBase', > 'data': { > 'raw': 'BlockOptionsRaw' > 'qcow2': 'BlockOptionsQcow2' > } } > > ...would result in this generated C struct: > > struct BlockOptions > { > BlockOptionsKind kind; > union { > void *data; > BlockOptionsRaw * raw; > BlockOptionsQcow2 * qcow2; > }; > bool read_only; > }; > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > scripts/qapi-types.py | 16 ++++++++++++++-- > 1 file changed, 14 insertions(+), 2 deletions(-) > > diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py > index e1239e1..9882b95 100644 > --- a/scripts/qapi-types.py > +++ b/scripts/qapi-types.py > @@ -157,7 +157,12 @@ typedef enum %(name)s > > return lookup_decl + enum_decl > > -def generate_union(name, typeinfo): > +def generate_union(expr): > + > + name = expr['union'] > + typeinfo = expr['data'] > + base = expr.get('base') > + > ret = mcgen(''' > struct %(name)s > { > @@ -176,6 +181,13 @@ struct %(name)s > > ret += mcgen(''' > }; > +''') > + > + if base: > + struct = find_struct(base) > + ret += generate_struct_fields(struct['data']) generate_struct_fields() doesn't exist in upstream. [qemu-upstream]$ grep generate_struct_fields -r * scripts/qapi-types.py: ret += generate_struct_fields(struct['data']) [qemu-upstream]$ > + > + ret += mcgen(''' > }; > ''') > > @@ -359,7 +371,7 @@ for expr in exprs: > ret += generate_type_cleanup_decl(expr['type']) > fdef.write(generate_type_cleanup(expr['type']) + "\n") > elif expr.has_key('union'): > - ret += generate_union(expr['union'], expr['data']) > + ret += generate_union(expr) > ret += generate_type_cleanup_decl(expr['union'] + "List") > fdef.write(generate_type_cleanup(expr['union'] + "List") + "\n") > ret += generate_type_cleanup_decl(expr['union']) > -- > 1.8.1.4 >
Am 21.08.2013 um 05:38 hat Amos Kong geschrieben: > On Tue, Jul 23, 2013 at 03:03:10PM +0200, Kevin Wolf wrote: > > The new 'base' key in a union definition refers to a struct type, which > > is inlined into the union definition and can represent fields common to > > all kinds. > > > > For example the following schema definition... > > > > { 'type': 'BlockOptionsBase', 'data': { 'read-only': 'bool' } } > > > > { 'union': 'BlockOptions', > > 'base': 'BlockOptionsBase', > > 'data': { > > 'raw': 'BlockOptionsRaw' > > 'qcow2': 'BlockOptionsQcow2' > > } } > > > > ...would result in this generated C struct: > > > > struct BlockOptions > > { > > BlockOptionsKind kind; > > union { > > void *data; > > BlockOptionsRaw * raw; > > BlockOptionsQcow2 * qcow2; > > }; > > bool read_only; > > }; > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > --- > > scripts/qapi-types.py | 16 ++++++++++++++-- > > 1 file changed, 14 insertions(+), 2 deletions(-) > > > > diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py > > index e1239e1..9882b95 100644 > > --- a/scripts/qapi-types.py > > +++ b/scripts/qapi-types.py > > @@ -157,7 +157,12 @@ typedef enum %(name)s > > > > return lookup_decl + enum_decl > > > > -def generate_union(name, typeinfo): > > +def generate_union(expr): > > + > > + name = expr['union'] > > + typeinfo = expr['data'] > > + base = expr.get('base') > > + > > ret = mcgen(''' > > struct %(name)s > > { > > @@ -176,6 +181,13 @@ struct %(name)s > > > > ret += mcgen(''' > > }; > > +''') > > + > > + if base: > > + struct = find_struct(base) > > + ret += generate_struct_fields(struct['data']) > > > generate_struct_fields() doesn't exist in upstream. > > [qemu-upstream]$ grep generate_struct_fields -r * > scripts/qapi-types.py: ret += generate_struct_fields(struct['data']) > [qemu-upstream]$ Yup, something went wrong while applying the series, that patch was simply dropped (and interestingly it didn't result in any conflicts or compile errors). I'll include it in my next pull request. If you need it before that, you can apply the patch manually from the mailing list, its subject line is: qapi-types.py: Split off generate_struct_fields() Kevin
On Tue, 27 Aug 2013 17:58:59 +0200 Kevin Wolf <kwolf@redhat.com> wrote: > Am 21.08.2013 um 05:38 hat Amos Kong geschrieben: > > On Tue, Jul 23, 2013 at 03:03:10PM +0200, Kevin Wolf wrote: > > > The new 'base' key in a union definition refers to a struct type, which > > > is inlined into the union definition and can represent fields common to > > > all kinds. > > > > > > For example the following schema definition... > > > > > > { 'type': 'BlockOptionsBase', 'data': { 'read-only': 'bool' } } > > > > > > { 'union': 'BlockOptions', > > > 'base': 'BlockOptionsBase', > > > 'data': { > > > 'raw': 'BlockOptionsRaw' > > > 'qcow2': 'BlockOptionsQcow2' > > > } } > > > > > > ...would result in this generated C struct: > > > > > > struct BlockOptions > > > { > > > BlockOptionsKind kind; > > > union { > > > void *data; > > > BlockOptionsRaw * raw; > > > BlockOptionsQcow2 * qcow2; > > > }; > > > bool read_only; > > > }; > > > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > > --- > > > scripts/qapi-types.py | 16 ++++++++++++++-- > > > 1 file changed, 14 insertions(+), 2 deletions(-) > > > > > > diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py > > > index e1239e1..9882b95 100644 > > > --- a/scripts/qapi-types.py > > > +++ b/scripts/qapi-types.py > > > @@ -157,7 +157,12 @@ typedef enum %(name)s > > > > > > return lookup_decl + enum_decl > > > > > > -def generate_union(name, typeinfo): > > > +def generate_union(expr): > > > + > > > + name = expr['union'] > > > + typeinfo = expr['data'] > > > + base = expr.get('base') > > > + > > > ret = mcgen(''' > > > struct %(name)s > > > { > > > @@ -176,6 +181,13 @@ struct %(name)s > > > > > > ret += mcgen(''' > > > }; > > > +''') > > > + > > > + if base: > > > + struct = find_struct(base) > > > + ret += generate_struct_fields(struct['data']) > > > > > > generate_struct_fields() doesn't exist in upstream. > > > > [qemu-upstream]$ grep generate_struct_fields -r * > > scripts/qapi-types.py: ret += generate_struct_fields(struct['data']) > > [qemu-upstream]$ > > Yup, something went wrong while applying the series, that patch was > simply dropped (and interestingly it didn't result in any conflicts or > compile errors). I'll include it in my next pull request. Strange, it appears on your pull request... But anyway, your series made it into 1.6.0, so I think we'll need the missing patch in 1.6.1 too?
Am 29.08.2013 um 15:52 hat Luiz Capitulino geschrieben: > On Tue, 27 Aug 2013 17:58:59 +0200 > Kevin Wolf <kwolf@redhat.com> wrote: > > > Am 21.08.2013 um 05:38 hat Amos Kong geschrieben: > > > On Tue, Jul 23, 2013 at 03:03:10PM +0200, Kevin Wolf wrote: > > > > The new 'base' key in a union definition refers to a struct type, which > > > > is inlined into the union definition and can represent fields common to > > > > all kinds. > > > > > > > > For example the following schema definition... > > > > > > > > { 'type': 'BlockOptionsBase', 'data': { 'read-only': 'bool' } } > > > > > > > > { 'union': 'BlockOptions', > > > > 'base': 'BlockOptionsBase', > > > > 'data': { > > > > 'raw': 'BlockOptionsRaw' > > > > 'qcow2': 'BlockOptionsQcow2' > > > > } } > > > > > > > > ...would result in this generated C struct: > > > > > > > > struct BlockOptions > > > > { > > > > BlockOptionsKind kind; > > > > union { > > > > void *data; > > > > BlockOptionsRaw * raw; > > > > BlockOptionsQcow2 * qcow2; > > > > }; > > > > bool read_only; > > > > }; > > > > > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > > > --- > > > > scripts/qapi-types.py | 16 ++++++++++++++-- > > > > 1 file changed, 14 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py > > > > index e1239e1..9882b95 100644 > > > > --- a/scripts/qapi-types.py > > > > +++ b/scripts/qapi-types.py > > > > @@ -157,7 +157,12 @@ typedef enum %(name)s > > > > > > > > return lookup_decl + enum_decl > > > > > > > > -def generate_union(name, typeinfo): > > > > +def generate_union(expr): > > > > + > > > > + name = expr['union'] > > > > + typeinfo = expr['data'] > > > > + base = expr.get('base') > > > > + > > > > ret = mcgen(''' > > > > struct %(name)s > > > > { > > > > @@ -176,6 +181,13 @@ struct %(name)s > > > > > > > > ret += mcgen(''' > > > > }; > > > > +''') > > > > + > > > > + if base: > > > > + struct = find_struct(base) > > > > + ret += generate_struct_fields(struct['data']) > > > > > > > > > generate_struct_fields() doesn't exist in upstream. > > > > > > [qemu-upstream]$ grep generate_struct_fields -r * > > > scripts/qapi-types.py: ret += generate_struct_fields(struct['data']) > > > [qemu-upstream]$ > > > > Yup, something went wrong while applying the series, that patch was > > simply dropped (and interestingly it didn't result in any conflicts or > > compile errors). I'll include it in my next pull request. > > Strange, it appears on your pull request... But anyway, your series > made it into 1.6.0, so I think we'll need the missing patch in 1.6.1 too? There's no user in 1.6 (or would we have a build failure) because I didn't merge blockdev-add, so I guess it doesn't matter. Kevin
On Thu, 29 Aug 2013 18:06:50 +0200 Kevin Wolf <kwolf@redhat.com> wrote: > Am 29.08.2013 um 15:52 hat Luiz Capitulino geschrieben: > > On Tue, 27 Aug 2013 17:58:59 +0200 > > Kevin Wolf <kwolf@redhat.com> wrote: > > > > > Am 21.08.2013 um 05:38 hat Amos Kong geschrieben: > > > > On Tue, Jul 23, 2013 at 03:03:10PM +0200, Kevin Wolf wrote: > > > > > The new 'base' key in a union definition refers to a struct type, which > > > > > is inlined into the union definition and can represent fields common to > > > > > all kinds. > > > > > > > > > > For example the following schema definition... > > > > > > > > > > { 'type': 'BlockOptionsBase', 'data': { 'read-only': 'bool' } } > > > > > > > > > > { 'union': 'BlockOptions', > > > > > 'base': 'BlockOptionsBase', > > > > > 'data': { > > > > > 'raw': 'BlockOptionsRaw' > > > > > 'qcow2': 'BlockOptionsQcow2' > > > > > } } > > > > > > > > > > ...would result in this generated C struct: > > > > > > > > > > struct BlockOptions > > > > > { > > > > > BlockOptionsKind kind; > > > > > union { > > > > > void *data; > > > > > BlockOptionsRaw * raw; > > > > > BlockOptionsQcow2 * qcow2; > > > > > }; > > > > > bool read_only; > > > > > }; > > > > > > > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > > > > --- > > > > > scripts/qapi-types.py | 16 ++++++++++++++-- > > > > > 1 file changed, 14 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py > > > > > index e1239e1..9882b95 100644 > > > > > --- a/scripts/qapi-types.py > > > > > +++ b/scripts/qapi-types.py > > > > > @@ -157,7 +157,12 @@ typedef enum %(name)s > > > > > > > > > > return lookup_decl + enum_decl > > > > > > > > > > -def generate_union(name, typeinfo): > > > > > +def generate_union(expr): > > > > > + > > > > > + name = expr['union'] > > > > > + typeinfo = expr['data'] > > > > > + base = expr.get('base') > > > > > + > > > > > ret = mcgen(''' > > > > > struct %(name)s > > > > > { > > > > > @@ -176,6 +181,13 @@ struct %(name)s > > > > > > > > > > ret += mcgen(''' > > > > > }; > > > > > +''') > > > > > + > > > > > + if base: > > > > > + struct = find_struct(base) > > > > > + ret += generate_struct_fields(struct['data']) > > > > > > > > > > > > generate_struct_fields() doesn't exist in upstream. > > > > > > > > [qemu-upstream]$ grep generate_struct_fields -r * > > > > scripts/qapi-types.py: ret += generate_struct_fields(struct['data']) > > > > [qemu-upstream]$ > > > > > > Yup, something went wrong while applying the series, that patch was > > > simply dropped (and interestingly it didn't result in any conflicts or > > > compile errors). I'll include it in my next pull request. > > > > Strange, it appears on your pull request... But anyway, your series > > made it into 1.6.0, so I think we'll need the missing patch in 1.6.1 too? > > There's no user in 1.6 (or would we have a build failure) because I > didn't merge blockdev-add, so I guess it doesn't matter. I won't say it's a huge deal, but any downstreamers basing on 1.6 will have a hard time if they backport blockdev-add or any future command that my depend on this.
Am 29.08.2013 um 18:33 hat Luiz Capitulino geschrieben: > On Thu, 29 Aug 2013 18:06:50 +0200 > Kevin Wolf <kwolf@redhat.com> wrote: > > > Am 29.08.2013 um 15:52 hat Luiz Capitulino geschrieben: > > > On Tue, 27 Aug 2013 17:58:59 +0200 > > > Kevin Wolf <kwolf@redhat.com> wrote: > > > > > > > Am 21.08.2013 um 05:38 hat Amos Kong geschrieben: > > > > > On Tue, Jul 23, 2013 at 03:03:10PM +0200, Kevin Wolf wrote: > > > > > > The new 'base' key in a union definition refers to a struct type, which > > > > > > is inlined into the union definition and can represent fields common to > > > > > > all kinds. > > > > > > > > > > > > For example the following schema definition... > > > > > > > > > > > > { 'type': 'BlockOptionsBase', 'data': { 'read-only': 'bool' } } > > > > > > > > > > > > { 'union': 'BlockOptions', > > > > > > 'base': 'BlockOptionsBase', > > > > > > 'data': { > > > > > > 'raw': 'BlockOptionsRaw' > > > > > > 'qcow2': 'BlockOptionsQcow2' > > > > > > } } > > > > > > > > > > > > ...would result in this generated C struct: > > > > > > > > > > > > struct BlockOptions > > > > > > { > > > > > > BlockOptionsKind kind; > > > > > > union { > > > > > > void *data; > > > > > > BlockOptionsRaw * raw; > > > > > > BlockOptionsQcow2 * qcow2; > > > > > > }; > > > > > > bool read_only; > > > > > > }; > > > > > > > > > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > > > > > @@ -176,6 +181,13 @@ struct %(name)s > > > > > > > > > > > > ret += mcgen(''' > > > > > > }; > > > > > > +''') > > > > > > + > > > > > > + if base: > > > > > > + struct = find_struct(base) > > > > > > + ret += generate_struct_fields(struct['data']) > > > > > > > > > > > > > > > generate_struct_fields() doesn't exist in upstream. > > > > > > > > > > [qemu-upstream]$ grep generate_struct_fields -r * > > > > > scripts/qapi-types.py: ret += generate_struct_fields(struct['data']) > > > > > [qemu-upstream]$ > > > > > > > > Yup, something went wrong while applying the series, that patch was > > > > simply dropped (and interestingly it didn't result in any conflicts or > > > > compile errors). I'll include it in my next pull request. > > > > > > Strange, it appears on your pull request... But anyway, your series > > > made it into 1.6.0, so I think we'll need the missing patch in 1.6.1 too? > > > > There's no user in 1.6 (or would we have a build failure) because I > > didn't merge blockdev-add, so I guess it doesn't matter. > > I won't say it's a huge deal, but any downstreamers basing on 1.6 will > have a hard time if they backport blockdev-add or any future command > that my depend on this. About as hard as with any other dependency. But I won't oppose any request to include it in a stable release, adding a missing function that is called is clearly a bug fix. Kevin
On 08/29/2013 10:33 AM, Luiz Capitulino wrote: >>> >>> Strange, it appears on your pull request... But anyway, your series >>> made it into 1.6.0, so I think we'll need the missing patch in 1.6.1 too? >> >> There's no user in 1.6 (or would we have a build failure) because I >> didn't merge blockdev-add, so I guess it doesn't matter. > > I won't say it's a huge deal, but any downstreamers basing on 1.6 will > have a hard time if they backport blockdev-add or any future command > that my depend on this. Any downstreamers that plans to backport blockdev-add would also backport this as part of their efforts. I don't see that as any different from any other backport effort that includes requiring multiple non-contiguous pre-req patches. We don't need it on the 1.6 stable tree, and downstream is no worse for the wear.
On Thu, 29 Aug 2013 11:02:26 -0600 Eric Blake <eblake@redhat.com> wrote: > On 08/29/2013 10:33 AM, Luiz Capitulino wrote: > > >>> > >>> Strange, it appears on your pull request... But anyway, your series > >>> made it into 1.6.0, so I think we'll need the missing patch in 1.6.1 too? > >> > >> There's no user in 1.6 (or would we have a build failure) because I > >> didn't merge blockdev-add, so I guess it doesn't matter. > > > > I won't say it's a huge deal, but any downstreamers basing on 1.6 will > > have a hard time if they backport blockdev-add or any future command > > that my depend on this. > > Any downstreamers that plans to backport blockdev-add would also > backport this as part of their efforts. I don't see that as any > different from any other backport effort that includes requiring > multiple non-contiguous pre-req patches. Backport work can be hard. I'd praise people for making it easier for me and would curse people for making it harder for no reason. > We don't need it on the 1.6 > stable tree, and downstream is no worse for the wear. Why is the feature there in the first place then?
δΊ 2013-8-30 2:36, Luiz Capitulino ει: > On Thu, 29 Aug 2013 11:02:26 -0600 > Eric Blake <eblake@redhat.com> wrote: > >> On 08/29/2013 10:33 AM, Luiz Capitulino wrote: >> >>>>> >>>>> Strange, it appears on your pull request... But anyway, your series >>>>> made it into 1.6.0, so I think we'll need the missing patch in 1.6.1 too? >>>> >>>> There's no user in 1.6 (or would we have a build failure) because I >>>> didn't merge blockdev-add, so I guess it doesn't matter. >>> >>> I won't say it's a huge deal, but any downstreamers basing on 1.6 will >>> have a hard time if they backport blockdev-add or any future command >>> that my depend on this. >> >> Any downstreamers that plans to backport blockdev-add would also >> backport this as part of their efforts. I don't see that as any >> different from any other backport effort that includes requiring >> multiple non-contiguous pre-req patches. > > Backport work can be hard. I'd praise people for making it easier > for me and would curse people for making it harder for no reason. > >> We don't need it on the 1.6 >> stable tree, and downstream is no worse for the wear. > > Why is the feature there in the first place then? > Guess it is for a better way to enable Fam Zheng's point in time block snapshot, which need to create a temporary BDS. This series provide a more general way to manage the BDS life cycle? I just noticed this series, it seems an example as a block layer interface design, looking forward to V2.
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py index e1239e1..9882b95 100644 --- a/scripts/qapi-types.py +++ b/scripts/qapi-types.py @@ -157,7 +157,12 @@ typedef enum %(name)s return lookup_decl + enum_decl -def generate_union(name, typeinfo): +def generate_union(expr): + + name = expr['union'] + typeinfo = expr['data'] + base = expr.get('base') + ret = mcgen(''' struct %(name)s { @@ -176,6 +181,13 @@ struct %(name)s ret += mcgen(''' }; +''') + + if base: + struct = find_struct(base) + ret += generate_struct_fields(struct['data']) + + ret += mcgen(''' }; ''') @@ -359,7 +371,7 @@ for expr in exprs: ret += generate_type_cleanup_decl(expr['type']) fdef.write(generate_type_cleanup(expr['type']) + "\n") elif expr.has_key('union'): - ret += generate_union(expr['union'], expr['data']) + ret += generate_union(expr) ret += generate_type_cleanup_decl(expr['union'] + "List") fdef.write(generate_type_cleanup(expr['union'] + "List") + "\n") ret += generate_type_cleanup_decl(expr['union'])
The new 'base' key in a union definition refers to a struct type, which is inlined into the union definition and can represent fields common to all kinds. For example the following schema definition... { 'type': 'BlockOptionsBase', 'data': { 'read-only': 'bool' } } { 'union': 'BlockOptions', 'base': 'BlockOptionsBase', 'data': { 'raw': 'BlockOptionsRaw' 'qcow2': 'BlockOptionsQcow2' } } ...would result in this generated C struct: struct BlockOptions { BlockOptionsKind kind; union { void *data; BlockOptionsRaw * raw; BlockOptionsQcow2 * qcow2; }; bool read_only; }; Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- scripts/qapi-types.py | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)