Re: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
Hi, On Wed, May 25, 2022 at 08:49:19AM -0500, Andrea Bolognani wrote: > On Wed, May 18, 2022 at 02:30:11PM +0200, Markus Armbruster wrote: > > Victor Toso writes: > > > IMHO, at this moment, qapi-go is targeting communicating with > > > QEMU and handling multiple QEMU versions seems reasonable to me. > > > > It's targeting communicating in *QMP*. QMP is designed to support > > communicating with a range of QEMU versions. Full compatibility is > > promised for a narrow range. Outside that range, graceful degradation. > > > > *If* you want to widen the full compatibility range, do it in *QMP*. Or > > do it on top of QEMU, e.g. in libvirt. > > > > > Perhaps libvirt can use qapi-go in the future or other generated > > > interface. That would be cool. > > > > "Would be cool" and a dollar buys you a cup of bad coffee. > > > > Is it a good use of our limited resources? > > > > How much will it delay delivery of Go bindings compared to less > > ambitious version? > > Yeah, this thread has basically branched to cover three topics: > > 1. what an MVP Go interface for QMP should look like; > 2. how to make sure said interface uses pretty names; > 3. how to make it work across multiple QEMU versions. > > All of these are important in the long run, but as far as I'm > concerned only 1. is an actual blocker to making progress. I agree although (1) and (3) are holding hands a bit. > If we get to the point where we can generate a reasonably > complete and well-typed Go interface that can be used to > communicate with a single version of QEMU, we should just > plaster EXPERIMENTAL all over it and get it merged. > > Basically get the MVP done and then iterate over it in-tree > rather than trying to get everything perfect from the start. > > Sounds reasonable? Yep. The whole discussion has been great as to clarify limitations and possible goals but not aiming to get it perfect all at once seems reasonable. Cheers, Victor signature.asc Description: PGP signature
Re: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
Andrea Bolognani writes: > On Wed, May 18, 2022 at 02:30:11PM +0200, Markus Armbruster wrote: >> Victor Toso writes: >> > IMHO, at this moment, qapi-go is targeting communicating with >> > QEMU and handling multiple QEMU versions seems reasonable to me. >> >> It's targeting communicating in *QMP*. QMP is designed to support >> communicating with a range of QEMU versions. Full compatibility is >> promised for a narrow range. Outside that range, graceful degradation. >> >> *If* you want to widen the full compatibility range, do it in *QMP*. Or >> do it on top of QEMU, e.g. in libvirt. >> >> > Perhaps libvirt can use qapi-go in the future or other generated >> > interface. That would be cool. >> >> "Would be cool" and a dollar buys you a cup of bad coffee. >> >> Is it a good use of our limited resources? >> >> How much will it delay delivery of Go bindings compared to less >> ambitious version? > > Yeah, this thread has basically branched to cover three topics: > > 1. what an MVP Go interface for QMP should look like; > 2. how to make sure said interface uses pretty names; > 3. how to make it work across multiple QEMU versions. > > All of these are important in the long run, but as far as I'm > concerned only 1. is an actual blocker to making progress. > > If we get to the point where we can generate a reasonably complete > and well-typed Go interface that can be used to communicate with a > single version of QEMU, we should just plaster EXPERIMENTAL all over > it and get it merged. > > Basically get the MVP done and then iterate over it in-tree rather > than trying to get everything perfect from the start. > > Sounds reasonable? Yes, with an undogmatic interpretation of "minimally viable". Doing more should be okay when it doesn't complicate things outside the Go-generating backend. Exploring how to generate Go bindings that make good use of static typing will be interesting enough. Aiming for wide compatibility in addition risks delay and/or failure. Exploding heads, too.
Re: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
On Wed, May 18, 2022 at 02:30:11PM +0200, Markus Armbruster wrote: > Victor Toso writes: > > IMHO, at this moment, qapi-go is targeting communicating with > > QEMU and handling multiple QEMU versions seems reasonable to me. > > It's targeting communicating in *QMP*. QMP is designed to support > communicating with a range of QEMU versions. Full compatibility is > promised for a narrow range. Outside that range, graceful degradation. > > *If* you want to widen the full compatibility range, do it in *QMP*. Or > do it on top of QEMU, e.g. in libvirt. > > > Perhaps libvirt can use qapi-go in the future or other generated > > interface. That would be cool. > > "Would be cool" and a dollar buys you a cup of bad coffee. > > Is it a good use of our limited resources? > > How much will it delay delivery of Go bindings compared to less > ambitious version? Yeah, this thread has basically branched to cover three topics: 1. what an MVP Go interface for QMP should look like; 2. how to make sure said interface uses pretty names; 3. how to make it work across multiple QEMU versions. All of these are important in the long run, but as far as I'm concerned only 1. is an actual blocker to making progress. If we get to the point where we can generate a reasonably complete and well-typed Go interface that can be used to communicate with a single version of QEMU, we should just plaster EXPERIMENTAL all over it and get it merged. Basically get the MVP done and then iterate over it in-tree rather than trying to get everything perfect from the start. Sounds reasonable? -- Andrea Bolognani / Red Hat / Virtualization
Re: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
Victor Toso writes:
> Hi,
>
> On Wed, May 11, 2022 at 04:17:35PM +0200, Markus Armbruster wrote:
>> Daniel P. Berrangé writes:
>> > Caller
>> >
>> > block_resize(device="dev0", size=1*GiB)
>> > block_resize(node_name="devnode0", size=1*GiB)
>> >
>> >
>> > In golang definition
>> >
>> >type BlockResizeArguments struct {
>> >Device string
>> >NodeName string
>> >Size int
>> >}
>> >
>> > Caller choice of
>> >
>> >cmd := {
>> >Device: "dev0",
>> >Size: 1 * GiB,
>> >}
>> >
>> >cmd := {
>> >NodeName: "devnode0",
>> >Size: 1 * GiB,
>> >}
>>
>> Note that the Go bindings you sketched effectively use (poor
>> man's) keyword arguments.
>>
>> > Neither case can easily prevent passing Device and NodeName
>> > at same time.
>>
>> That defect lies at the schema's feet.
>
> Right. The schema does not provide any metadata to explicit say
> that only @device or @node-name should be used, correct?
Correct.
The existing means to express either / or are tagged unions and
alternates.
Tagged unions require an explicit tag member.
Alternates select the alternative by the type of the value.
We don't have anything that selects by member name.
> This would be important to differentiate of a simple 'adding a
> new optional argument' plus 'making this other argument
> optional'.
We also don't have means to express "this integer must be a power of
two", or "this string must name a block backend", or any number of
semantic constraints.
We have to draw the line somewhere. Schema language complexity needs to
earn its keep.
>> >> * At some future date, the old way gets deprecated: argument @device
>> >> acquires feature @deprecated.
>> >
>> > Ok, no change needed to the APIs in either case. Possibly have
>> > code emit a warning if a deprecated field is set.
>> >
>> >> * Still later, the old way gets removed: @device is deleted, and
>> >> @node-name becomes mandatory.
>> >
>> > Again no change needed to APIs, but QEMU will throw back an
>> > error if the wrong one is used.
>> >
>> >> What is the proper version-spanning interface?
>> >>
>> >> I figure it's both arguments optional, must specify the right one for
>> >> the version of QEMU actually in use. This spans versions, but it fails
>> >> to abstract from them.
>> >
>> > Yep, I think that's inevitable in this scenario. THe plus side
>> > is that apps that want to span versions can do so. The downside
>> > is that apps that don't want smarts to span version, may loose
>> > compile time warnings about use of the now deleted field.
>>
>> The version-spanning interface will arguably be a bad interface for any
>> version.
>>
>> > I suggested the code generator have an option to say what level
>> > of compat to use for generated code, so that apps can request an
>> > API without compat, which will result in compile errors. This
>> > though assumes every consumer app is embedding their own
>> > generated copy of the code. Not neccessarily desirable.
>> >
>> > At the C level you can play games with __deprecated__ to get
>> > compile time warnings in some cases.
>> >
>> > #define GLIB_VERSION_MIN_REQUIRED GLIB_VERSION_2_56
>> >
>> > causes QEMU to get compile time warnings (or errors) if it
>> > attempts to use a API feature deprecated in 2.56, even if
>> > the API exists in the header & library.
>> >
>> >
>> >> Note that it's not enough to replace "delete member" by "mark member
>> >> deleted in ". You also have to keep full history for "is it
>> >> optional". And for types, because those can evolve compatibly, too,
>> >> e.g. from struct to flat union, or from string to alternate of string
>> >> and something else. What is the proper version-spanning interface in
>> >> all the possible cases?
>> >
>> > I've not thought through all possible scenarios, but there may end
>> > up being restrictions, such that changes that were previously possible
>> > may have to be forbidden.
>>
>> "There may be restrictions" is not exactly a confidence-inspring design
>> assumption. We need a reasonably dependable idea on what exactly we're
>> intending to sacrifice.
>
> I can't help much here but I guess we can evolve QAPI schema as
> we move forward. Adding metadata that helps document changes to
> the benefit of giving code generators tools to provide a way to
> work with those QAPI changes seems desirable, no?
QMP comes with a certain compatibility promise. Keeping the promise has
been expensive, but that's okay; a weaker promise would have been
differently expensive. It's a compromise that has emerged over a long
time.
I fail to see why QEMU-provided QMP bindings should come with a stronger
promise than QEMU-provided QMP.
>> > One example, in the past we could do deprecate a field 'foo', then
>> > delete 'foo' and then some time re-introduce 'foo' with a completely
>> > different type. That would not be possible if we wanted to maintain
>> > compat, but in this example that's
Re: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
HI,
On Wed, May 18, 2022 at 09:51:56AM +0100, Daniel P. Berrangé wrote:
> On Wed, May 18, 2022 at 10:10:48AM +0200, Victor Toso wrote:
> > Hi,
> >
> > On Wed, May 11, 2022 at 04:50:35PM +0100, Daniel P. Berrangé wrote:
> > > On Wed, May 11, 2022 at 08:38:04AM -0700, Andrea Bolognani wrote:
> > > > On Tue, May 10, 2022 at 01:51:05PM +0100, Daniel P. Berrangé wrote:
> > > > > In 7.0.0 we can now generate
> > > > >
> > > > >type BlockResizeArguments struct {
> > > > >V500 *BlockResizeArgumentsV500
> > > > >V520 *BlockResizeArgumentsV520
> > > > >V700 *BlockResizeArgumentsV700
> > > > >}
> > > > >
> > > > >type BlockResizeArgumentsV500 struct {
> > > > >Device string
> > > > >Size int
> > > > >}
> > > > >
> > > > >type BlockResizeArgumentsV520 struct {
> > > > >Device *string
> > > > >NodeName *string
> > > > >Size int
> > > > >}
> > > > >
> > > > >type BlockResizeArgumentsV700 struct {
> > > > >NodeName string
> > > > >Size int
> > > > >}
> > > > >
> > > > > App can use the same as before, or switch to
> > > > >
> > > > > node := "nodedev0"
> > > > > cmd := BlockResizeArguments{
> > > > > V700: {
> > > > > NodeName: node,
> > > > > Size: 1 * GiB
> > > > > }
> > > > > }
> > > >
> > > > This honestly looks pretty unwieldy.
> > >
> > > It isn't all that more verbose than without the versions - just
> > > a single struct wrapper.
> > >
> > > >
> > > > If the application already knows it's targeting a specific version of
> > > > the QEMU API, which for the above code to make any sense it will have
> > > > to, couldn't it do something like
> > > >
> > > > import qemu .../qemu/v700
> > > >
> > > > at the beginning of the file and then use regular old
> > > >
> > > > cmd := qemu.BlockResizeArguments{
> > > > NodeName: nodeName,
> > > > Size: size,
> > > > }
> > > >
> > > > instead?
> > >
> > > This would lead to a situation where every struct is duplicated
> > > for every version, even though 90% of the time they'll be identical
> > > across multiple versions. This is not very ammenable to the desire
> > > to be able to dynamically choose per-command which version you
> > > want based on which version of QEMU you're connected to.
> > >
> > > ie
> > >
> > >
> > > var cmd Command
> > > if qmp.HasVersion(qemu.Version(7, 0, 0)) {
> > > cmd = BlockResizeArguments{
> > >V700: {
> > > NodeName: node,
> > > Size: 1 * GiB
> > > }
> > > }
> > > } else {
> > > cmd = BlockResizeArguments{
> > >V520: {
> > > Device: dev,
> > > Size: 1 * GiB
> > > }
> > > }
> > > }
> > >
> > > And of course the HasVersion check is going to be different
> > > for each command that matters.
> > >
> > > Having said that, this perhaps shows the nested structs are
> > > overkill. We could have
> > >
> > >
> > > var cmd Command
> > > if qmp.HasVersion(qemu.Version(7, 0, 0)) {
> > > cmd = {
> > > NodeName: node,
> > > Size: 1 * GiB
> > > }
> > > } else {
> > > cmd = {
> > > Device: dev,
> > > Size: 1 * GiB
> > > }
> > > }
> >
> > The else block would be wrong in versions above 7.0.0 where
> > block_resize changed. There will be a need to know for a specific
> > Type if we are covered with latest qemu/qapi-go or not. Not yet
> > sure how to address that, likely we will need to keep the
> > information that something has been added/changed/removed per
> > version per Type in qapi-go...
>
> I put that in the "nice to have" category. No application can
> predict the future, and nor do they really need to try in
> general.
>
> If the application code was written when the newest QEMU was
> 7.1.0, and the above code is correct for QEMU <= 7.1.0, then
> that's good enough. If the BlockResizeArguments struct changed
> in a later QEMU version 8.0.0, that doesn't matter at the point
> the app code was written.
I'm not thinking at runtime, I'm thinking at compile time.
I update $myproject's qpai-go to 8.0.0 and get a warnings that
some types would not work with 8.0.0 (e.g: because there is a new
BlockResizeArguments800)
> Much of the time the changes are back compatible, ie just adding
> a new field, and so everything will still work fine if the app
> carries on using BlockResizeArguments700, despite a new
> BlockResizeArguments800 arriving with a new field.
>
> Only in the cases where a field was removed or changed in a
> non-compatible manner would an app have problems, and QEMU will
> happily report an error at runtime if the app sends something
> incompatible.
Yeah, runtime error is fine but if we can provide some extra
checks at the time someone wants to move qapi-go from 7.2.0 to
8.0.0, that would be great.
Re: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
Hi,
On Wed, May 11, 2022 at 04:17:35PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé writes:
> > Caller
> >
> > block_resize(device="dev0", size=1*GiB)
> > block_resize(node_name="devnode0", size=1*GiB)
> >
> >
> > In golang definition
> >
> >type BlockResizeArguments struct {
> >Device string
> >NodeName string
> >Size int
> >}
> >
> > Caller choice of
> >
> >cmd := {
> >Device: "dev0",
> >Size: 1 * GiB,
> >}
> >
> >cmd := {
> >NodeName: "devnode0",
> >Size: 1 * GiB,
> >}
>
> Note that the Go bindings you sketched effectively use (poor
> man's) keyword arguments.
>
> > Neither case can easily prevent passing Device and NodeName
> > at same time.
>
> That defect lies at the schema's feet.
Right. The schema does not provide any metadata to explicit say
that only @device or @node-name should be used, correct?
This would be important to differentiate of a simple 'adding a
new optional argument' plus 'making this other argument
optional'.
> >> * At some future date, the old way gets deprecated: argument @device
> >> acquires feature @deprecated.
> >
> > Ok, no change needed to the APIs in either case. Possibly have
> > code emit a warning if a deprecated field is set.
> >
> >> * Still later, the old way gets removed: @device is deleted, and
> >> @node-name becomes mandatory.
> >
> > Again no change needed to APIs, but QEMU will throw back an
> > error if the wrong one is used.
> >
> >> What is the proper version-spanning interface?
> >>
> >> I figure it's both arguments optional, must specify the right one for
> >> the version of QEMU actually in use. This spans versions, but it fails
> >> to abstract from them.
> >
> > Yep, I think that's inevitable in this scenario. THe plus side
> > is that apps that want to span versions can do so. The downside
> > is that apps that don't want smarts to span version, may loose
> > compile time warnings about use of the now deleted field.
>
> The version-spanning interface will arguably be a bad interface for any
> version.
>
> > I suggested the code generator have an option to say what level
> > of compat to use for generated code, so that apps can request an
> > API without compat, which will result in compile errors. This
> > though assumes every consumer app is embedding their own
> > generated copy of the code. Not neccessarily desirable.
> >
> > At the C level you can play games with __deprecated__ to get
> > compile time warnings in some cases.
> >
> > #define GLIB_VERSION_MIN_REQUIRED GLIB_VERSION_2_56
> >
> > causes QEMU to get compile time warnings (or errors) if it
> > attempts to use a API feature deprecated in 2.56, even if
> > the API exists in the header & library.
> >
> >
> >> Note that it's not enough to replace "delete member" by "mark member
> >> deleted in ". You also have to keep full history for "is it
> >> optional". And for types, because those can evolve compatibly, too,
> >> e.g. from struct to flat union, or from string to alternate of string
> >> and something else. What is the proper version-spanning interface in
> >> all the possible cases?
> >
> > I've not thought through all possible scenarios, but there may end
> > up being restrictions, such that changes that were previously possible
> > may have to be forbidden.
>
> "There may be restrictions" is not exactly a confidence-inspring design
> assumption. We need a reasonably dependable idea on what exactly we're
> intending to sacrifice.
I can't help much here but I guess we can evolve QAPI schema as
we move forward. Adding metadata that helps document changes to
the benefit of giving code generators tools to provide a way to
work with those QAPI changes seems desirable, no?
> > One example, in the past we could do deprecate a field 'foo', then
> > delete 'foo' and then some time re-introduce 'foo' with a completely
> > different type. That would not be possible if we wanted to maintain
> > compat, but in this example that's probably a good thing, as it'll
> > be super confusing to have the same field name change type like that
> > over time. Easier to just use a different name.
> >
> > So the question to me is not whether all our previous changes are
> > still possible, but whether enough of the typwes of change are
> > possible, such that we can cope with the ongoing maint in a
> > reasonable way. I don't think we've explored the possibility enough
> > to say one way or the other.
> >
> >> > Apps that wish to have version compat, would of course need to write
> >> > their code to be aware of which fields they need to seend for which
> >> > QEMU version.
> >>
> >> At which point we're reinventing libvirt.
IMHO, at this moment, qapi-go is targeting communicating with
QEMU and handling multiple QEMU versions seems reasonable to me.
Perhaps libvirt can use qapi-go in the future or other generated
interface. That would be cool.
> > The premise of the code generators is
Re: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
On Wed, May 18, 2022 at 10:10:48AM +0200, Victor Toso wrote:
> Hi,
>
> On Wed, May 11, 2022 at 04:50:35PM +0100, Daniel P. Berrangé wrote:
> > On Wed, May 11, 2022 at 08:38:04AM -0700, Andrea Bolognani wrote:
> > > On Tue, May 10, 2022 at 01:51:05PM +0100, Daniel P. Berrangé wrote:
> > > > In 7.0.0 we can now generate
> > > >
> > > >type BlockResizeArguments struct {
> > > >V500 *BlockResizeArgumentsV500
> > > >V520 *BlockResizeArgumentsV520
> > > >V700 *BlockResizeArgumentsV700
> > > >}
> > > >
> > > >type BlockResizeArgumentsV500 struct {
> > > >Device string
> > > >Size int
> > > >}
> > > >
> > > >type BlockResizeArgumentsV520 struct {
> > > >Device *string
> > > >NodeName *string
> > > >Size int
> > > >}
> > > >
> > > >type BlockResizeArgumentsV700 struct {
> > > >NodeName string
> > > >Size int
> > > >}
> > > >
> > > > App can use the same as before, or switch to
> > > >
> > > > node := "nodedev0"
> > > > cmd := BlockResizeArguments{
> > > > V700: {
> > > > NodeName: node,
> > > > Size: 1 * GiB
> > > > }
> > > > }
> > >
> > > This honestly looks pretty unwieldy.
> >
> > It isn't all that more verbose than without the versions - just
> > a single struct wrapper.
> >
> > >
> > > If the application already knows it's targeting a specific version of
> > > the QEMU API, which for the above code to make any sense it will have
> > > to, couldn't it do something like
> > >
> > > import qemu .../qemu/v700
> > >
> > > at the beginning of the file and then use regular old
> > >
> > > cmd := qemu.BlockResizeArguments{
> > > NodeName: nodeName,
> > > Size: size,
> > > }
> > >
> > > instead?
> >
> > This would lead to a situation where every struct is duplicated
> > for every version, even though 90% of the time they'll be identical
> > across multiple versions. This is not very ammenable to the desire
> > to be able to dynamically choose per-command which version you
> > want based on which version of QEMU you're connected to.
> >
> > ie
> >
> >
> > var cmd Command
> > if qmp.HasVersion(qemu.Version(7, 0, 0)) {
> > cmd = BlockResizeArguments{
> >V700: {
> > NodeName: node,
> > Size: 1 * GiB
> >}
> > }
> > } else {
> > cmd = BlockResizeArguments{
> >V520: {
> > Device: dev,
> > Size: 1 * GiB
> >}
> > }
> > }
> >
> > And of course the HasVersion check is going to be different
> > for each command that matters.
> >
> > Having said that, this perhaps shows the nested structs are
> > overkill. We could have
> >
> >
> > var cmd Command
> > if qmp.HasVersion(qemu.Version(7, 0, 0)) {
> > cmd = {
> > NodeName: node,
> > Size: 1 * GiB
> > }
> > } else {
> > cmd = {
> > Device: dev,
> > Size: 1 * GiB
> > }
> > }
>
> The else block would be wrong in versions above 7.0.0 where
> block_resize changed. There will be a need to know for a specific
> Type if we are covered with latest qemu/qapi-go or not. Not yet
> sure how to address that, likely we will need to keep the
> information that something has been added/changed/removed per
> version per Type in qapi-go...
I put that in the "nice to have" category. No application can
predict the future, and nor do they really need to try in
general.
If the application code was written when the newest QEMU was
7.1.0, and the above code is correct for QEMU <= 7.1.0, then
that's good enough. If the BlockResizeArguments struct changed
in a later QEMU version 8.0.0, that doesn't matter at the point
the app code was written.
Much of the time the changes are back compatible, ie just adding
a new field, and so everything will still work fine if the app
carries on using BlockResizeArguments700, despite a new
BlockResizeArguments800 arriving with a new field.
Only in the cases where a field was removed or changed in a
non-compatible manner would an app have problems, and QEMU will
happily report an error at runtime if the app sends something
incompatible.
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: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
Hi,
On Wed, May 11, 2022 at 04:50:35PM +0100, Daniel P. Berrangé wrote:
> On Wed, May 11, 2022 at 08:38:04AM -0700, Andrea Bolognani wrote:
> > On Tue, May 10, 2022 at 01:51:05PM +0100, Daniel P. Berrangé wrote:
> > > In 7.0.0 we can now generate
> > >
> > >type BlockResizeArguments struct {
> > >V500 *BlockResizeArgumentsV500
> > >V520 *BlockResizeArgumentsV520
> > >V700 *BlockResizeArgumentsV700
> > >}
> > >
> > >type BlockResizeArgumentsV500 struct {
> > >Device string
> > >Size int
> > >}
> > >
> > >type BlockResizeArgumentsV520 struct {
> > >Device *string
> > >NodeName *string
> > >Size int
> > >}
> > >
> > >type BlockResizeArgumentsV700 struct {
> > >NodeName string
> > >Size int
> > >}
> > >
> > > App can use the same as before, or switch to
> > >
> > > node := "nodedev0"
> > > cmd := BlockResizeArguments{
> > > V700: {
> > > NodeName: node,
> > > Size: 1 * GiB
> > > }
> > > }
> >
> > This honestly looks pretty unwieldy.
>
> It isn't all that more verbose than without the versions - just
> a single struct wrapper.
>
> >
> > If the application already knows it's targeting a specific version of
> > the QEMU API, which for the above code to make any sense it will have
> > to, couldn't it do something like
> >
> > import qemu .../qemu/v700
> >
> > at the beginning of the file and then use regular old
> >
> > cmd := qemu.BlockResizeArguments{
> > NodeName: nodeName,
> > Size: size,
> > }
> >
> > instead?
>
> This would lead to a situation where every struct is duplicated
> for every version, even though 90% of the time they'll be identical
> across multiple versions. This is not very ammenable to the desire
> to be able to dynamically choose per-command which version you
> want based on which version of QEMU you're connected to.
>
> ie
>
>
> var cmd Command
> if qmp.HasVersion(qemu.Version(7, 0, 0)) {
> cmd = BlockResizeArguments{
>V700: {
> NodeName: node,
> Size: 1 * GiB
> }
> }
> } else {
> cmd = BlockResizeArguments{
>V520: {
> Device: dev,
> Size: 1 * GiB
> }
> }
> }
>
> And of course the HasVersion check is going to be different
> for each command that matters.
>
> Having said that, this perhaps shows the nested structs are
> overkill. We could have
>
>
> var cmd Command
> if qmp.HasVersion(qemu.Version(7, 0, 0)) {
> cmd = {
> NodeName: node,
> Size: 1 * GiB
> }
> } else {
> cmd = {
> Device: dev,
> Size: 1 * GiB
> }
> }
The else block would be wrong in versions above 7.0.0 where
block_resize changed. There will be a need to know for a specific
Type if we are covered with latest qemu/qapi-go or not. Not yet
sure how to address that, likely we will need to keep the
information that something has been added/changed/removed per
version per Type in qapi-go...
Still, I think the above proposal is a good compromise to make..
> If there was some need for common handling of the different versioned
> variants, we could still have a 'BlockResizeArguments' that has a field
> per version, as an optional thing. Or have a BlockResizeArguments
> interface, implemented by each version
Cheers,
Victor
signature.asc
Description: PGP signature
Re: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
On Wed, May 11, 2022 at 09:22:36AM -0700, Andrea Bolognani wrote:
> On Wed, May 11, 2022 at 04:50:35PM +0100, Daniel P. Berrangé wrote:
> > This would lead to a situation where every struct is duplicated
> > for every version, even though 90% of the time they'll be identical
> > across multiple versions. This is not very ammenable to the desire
> > to be able to dynamically choose per-command which version you
> > want based on which version of QEMU you're connected to.
> >
> > ie
> >
> > var cmd Command
> > if qmp.HasVersion(qemu.Version(7, 0, 0)) {
> > cmd = BlockResizeArguments{
> >V700: {
> > NodeName: node,
> > Size: 1 * GiB
> >}
> > }
> > } else {
> > cmd = BlockResizeArguments{
> >V520: {
> > Device: dev,
> > Size: 1 * GiB
> >}
> > }
> > }
> >
> > And of course the HasVersion check is going to be different
> > for each command that matters.
> >
> > Having said that, this perhaps shows the nested structs are
> > overkill. We could have
> >
> > var cmd Command
> > if qmp.HasVersion(qemu.Version(7, 0, 0)) {
> > cmd = {
> > NodeName: node,
> > Size: 1 * GiB
> > }
> > } else {
> > cmd = {
> > Device: dev,
> > Size: 1 * GiB
> > }
> > }
>
> Right, making the decision at the import level would require adding a
> level of indirection and make this kind of dynamic logic awkward.
>
> We shouldn't force users to sprinkle version numbers everywhere
> though, especially since different commands will change at different
> points in time. It should be possible to do something like
>
> if !qmp.HasAPI(600) {
> panic("update QEMU")
> }
>
> cmd := { // really BlockResizeArguments520
> Device: device,
> Size: size,
> }
>
> or even
>
> if !qmp.HasAPI(qmp.API.Latest) {
> panic("update QEMU")
> }
>
> cmd := {
> NodeName: nodeName,
> Size: size,
> }
>
> Should be easy enough to achieve with type aliases.
I guess we would have a single package which does
typedef BlockResizeArguments520 BlockResizeArguments;
...for each type...
The interaction with API versioning will be tedious though. For the
versioned structs we'll be forever back compatible, due to the
append-only nature of the versioned struct approach. For the type
aliases, we'll be forever breaking compat as at least 1 struct
alias is likely to need changing every QEMU release.
Might suggest having 2 distinct go modules. A base module which
is versioned as a stable API with semver (forever v1.xxx), and
an add-on module that depends on base module, which is versioned
as an unstable API with semver (forever v0.xxx)
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: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
On Wed, May 11, 2022 at 04:50:35PM +0100, Daniel P. Berrangé wrote:
> This would lead to a situation where every struct is duplicated
> for every version, even though 90% of the time they'll be identical
> across multiple versions. This is not very ammenable to the desire
> to be able to dynamically choose per-command which version you
> want based on which version of QEMU you're connected to.
>
> ie
>
> var cmd Command
> if qmp.HasVersion(qemu.Version(7, 0, 0)) {
> cmd = BlockResizeArguments{
>V700: {
> NodeName: node,
> Size: 1 * GiB
> }
> }
> } else {
> cmd = BlockResizeArguments{
>V520: {
> Device: dev,
> Size: 1 * GiB
> }
> }
> }
>
> And of course the HasVersion check is going to be different
> for each command that matters.
>
> Having said that, this perhaps shows the nested structs are
> overkill. We could have
>
> var cmd Command
> if qmp.HasVersion(qemu.Version(7, 0, 0)) {
> cmd = {
> NodeName: node,
> Size: 1 * GiB
> }
> } else {
> cmd = {
> Device: dev,
> Size: 1 * GiB
> }
> }
Right, making the decision at the import level would require adding a
level of indirection and make this kind of dynamic logic awkward.
We shouldn't force users to sprinkle version numbers everywhere
though, especially since different commands will change at different
points in time. It should be possible to do something like
if !qmp.HasAPI(600) {
panic("update QEMU")
}
cmd := { // really BlockResizeArguments520
Device: device,
Size: size,
}
or even
if !qmp.HasAPI(qmp.API.Latest) {
panic("update QEMU")
}
cmd := {
NodeName: nodeName,
Size: size,
}
Should be easy enough to achieve with type aliases.
--
Andrea Bolognani / Red Hat / Virtualization
Re: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
On Wed, May 11, 2022 at 08:38:04AM -0700, Andrea Bolognani wrote:
> On Tue, May 10, 2022 at 01:51:05PM +0100, Daniel P. Berrangé wrote:
> > In 7.0.0 we can now generate
> >
> >type BlockResizeArguments struct {
> >V500 *BlockResizeArgumentsV500
> >V520 *BlockResizeArgumentsV520
> >V700 *BlockResizeArgumentsV700
> >}
> >
> >type BlockResizeArgumentsV500 struct {
> >Device string
> >Size int
> >}
> >
> >type BlockResizeArgumentsV520 struct {
> >Device *string
> >NodeName *string
> >Size int
> >}
> >
> >type BlockResizeArgumentsV700 struct {
> >NodeName string
> >Size int
> >}
> >
> > App can use the same as before, or switch to
> >
> > node := "nodedev0"
> > cmd := BlockResizeArguments{
> > V700: {
> > NodeName: node,
> > Size: 1 * GiB
> > }
> > }
>
> This honestly looks pretty unwieldy.
It isn't all that more verbose than without the versions - just
a single struct wrapper.
>
> If the application already knows it's targeting a specific version of
> the QEMU API, which for the above code to make any sense it will have
> to, couldn't it do something like
>
> import qemu .../qemu/v700
>
> at the beginning of the file and then use regular old
>
> cmd := qemu.BlockResizeArguments{
> NodeName: nodeName,
> Size: size,
> }
>
> instead?
This would lead to a situation where every struct is duplicated
for every version, even though 90% of the time they'll be identical
across multiple versions. This is not very ammenable to the desire
to be able to dynamically choose per-command which version you
want based on which version of QEMU you're connected to.
ie
var cmd Command
if qmp.HasVersion(qemu.Version(7, 0, 0)) {
cmd = BlockResizeArguments{
V700: {
NodeName: node,
Size: 1 * GiB
}
}
} else {
cmd = BlockResizeArguments{
V520: {
Device: dev,
Size: 1 * GiB
}
}
}
And of course the HasVersion check is going to be different
for each command that matters.
Having said that, this perhaps shows the nested structs are
overkill. We could have
var cmd Command
if qmp.HasVersion(qemu.Version(7, 0, 0)) {
cmd = {
NodeName: node,
Size: 1 * GiB
}
} else {
cmd = {
Device: dev,
Size: 1 * GiB
}
}
If there was some need for common handling of the different versioned
variants, we could still have a 'BlockResizeArguments' that has a field
per version, as an optional thing. Or have a BlockResizeArguments
interface, implemented by each version
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: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
On Tue, May 10, 2022 at 01:51:05PM +0100, Daniel P. Berrangé wrote:
> In 7.0.0 we can now generate
>
>type BlockResizeArguments struct {
>V500 *BlockResizeArgumentsV500
>V520 *BlockResizeArgumentsV520
>V700 *BlockResizeArgumentsV700
>}
>
>type BlockResizeArgumentsV500 struct {
>Device string
>Size int
>}
>
>type BlockResizeArgumentsV520 struct {
>Device *string
>NodeName *string
>Size int
>}
>
>type BlockResizeArgumentsV700 struct {
>NodeName string
>Size int
>}
>
> App can use the same as before, or switch to
>
> node := "nodedev0"
> cmd := BlockResizeArguments{
> V700: {
> NodeName: node,
> Size: 1 * GiB
> }
> }
This honestly looks pretty unwieldy.
If the application already knows it's targeting a specific version of
the QEMU API, which for the above code to make any sense it will have
to, couldn't it do something like
import qemu .../qemu/v700
at the beginning of the file and then use regular old
cmd := qemu.BlockResizeArguments{
NodeName: nodeName,
Size: size,
}
instead?
--
Andrea Bolognani / Red Hat / Virtualization
Re: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
On Wed, May 11, 2022 at 04:17:43PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé writes:
>
> > On Tue, May 10, 2022 at 01:34:03PM +0100, Daniel P. Berrangé wrote:
> > Having said that, a different way to approach the problem is to expose
> > the versioning directly in the generated code.
> >
> > Consider a QAPI with versioning info about the fields
> >
> > { 'command': 'block_resize',
> > 'since': '5.0.0',
> > 'data': { 'device': ['type': 'str', 'until': '5.2.0' ],
> > '*device': ['type': 'str', 'since': '5.2.0', 'until': '7.0.0'
> > ],
> > '*node-name': ['type': 'str', 'since': '5.2.0', 'until:
> > '7.0.0' ],
> > 'node-name': ['type': 'str', 'since': '7.0.0' ],
> > 'size': 'int' } }
> >
> > Meaning
> >
> > * Introduced in 5.0.0, with 'device' mandatory
> > * In 5.2.0, 'device' becomes optional, with optional 'node-name' as
> > alternative
> > * In 7.0.0, 'device' is deleted, and 'node-name' becomes mandatory
> >
> > Now consider the Go structs
> >
> > In 5.0.0 we can generate:
> >
> >type BlockResizeArguments struct {
> >V500 *BlockResizeArguments500
> >}
> >
> >type BlockResizeArgumentsV1 struct {
> > Device string
> > Size int
> > }
> >
> > app can use
> >
> > dev := "dev0"
> > cmd := BlockResizeArguments{
> >V500: {
> > Device: dev,
> > Size: 1 * GiB
> >}
> > }
> >
> >
> > In 5.2.0 we can now generate
> >
> >type BlockResizeArguments struct {
> >V500 *BlockResizeArgumentsV500
> >V520 *BlockResizeArgumentsV520
> >}
> >
> >type BlockResizeArgumentsV500 struct {
> > Device string
> > Size int
> > }
> >
> >type BlockResizeArgumentsV520 struct {
> > Device *string
> > NodeName *string
> > Size int
> > }
> >
> >
> > App can use the same as before, or switch to one of
> >
> > dev := "dev0"
> > cmd := BlockResizeArguments{
> >V520: {
> > Device: ,
> > Size: 1 * GiB
> >}
> > }
> >
> > or
> >
> > node := "nodedev0"
> > cmd := BlockResizeArguments{
> >V520: {
> > NodeName: ,
> > Size: 1 * GiB
> >}
> > }
> >
> >
> >
> > In 7.0.0 we can now generate
> >
> >
> >type BlockResizeArguments struct {
> >V500 *BlockResizeArgumentsV500
> >V520 *BlockResizeArgumentsV520
> >V700 *BlockResizeArgumentsV700
> >}
> >
> >type BlockResizeArgumentsV500 struct {
> > Device string
> > Size int
> >}
> >
> >type BlockResizeArgumentsV520 struct {
> > Device *string
> > NodeName *string
> > Size int
> >}
> >
> >type BlockResizeArgumentsV700 struct {
> > NodeName string
> > Size int
> >}
> >
> >
> >
> > App can use the same as before, or switch to
> >
> > node := "nodedev0"
> > cmd := BlockResizeArguments{
> >V700: {
> > NodeName: node,
> > Size: 1 * GiB
> >}
> > }
> >
> >
> > This kind of per-command/type versioning is not uncommon when defining API
> > protocols/interfaces.
>
> 1. At every release, put a copy of the QAPI schema in the freezer.
>
> 2. For every copy, generate Go types with a suitable name suffix.
>Collect all the name stems.
>
> 3. For each name stem, define a Go struct that contains all the suffixed
>Go types with that stem.
>
> Look Ma, no cluttering the QAPI schema with a full history! Also no
> complicating the schema language to provide the means for that.
That could indeed be a viable approach. Puts a little more work on the
code generator to match up the types, but probably isn't too hard.
Incidentally, we've intentionally not exposed type names in the QAPI
introspection in QMP. With code generators, when the generated code
type names driven from QAPI schema, there's likely going to be an
expectation that type names in QAPI have some kind of stability rules.
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: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
Daniel P. Berrangé writes:
> On Tue, May 10, 2022 at 02:02:56PM +0200, Markus Armbruster wrote:
>> Daniel P. Berrangé writes:
>>
>> > On Tue, Apr 26, 2022 at 01:14:28PM +0200, Markus Armbruster wrote:
>> >> We need to look at "following the QEMU releases" a bit more closely.
>> >>
>> >> Merging your patches gives us the capability to generate a Go interface
>> >> to HEAD's version of QMP.
>> >>
>> >> The obvious way for an out-of-tree Go program to use this generated Go
>> >> interface is to build with a specific version of it. It can then talk
>> >> QMP to any compatible QEMU version.
>> >>
>> >> Compatibility with older QEMUs is not assured: stuff added since is
>> >> present on the Go QMP client end, but not on the QEMU QMP server end.
>> >>
>> >> Compatibility with newer QEMUs is subject to our deprecation policy:
>> >>
>> >> In general features are intended to be supported indefinitely once
>> >> introduced into QEMU. In the event that a feature needs to be
>> >> removed, it will be listed in this section. The feature will remain
>> >> functional for the release in which it was deprecated and one
>> >> further release. After these two releases, the feature is liable to
>> >> be removed.
>> >>
>> >> So, if you stay away from deprecated stuff, you're good for two more
>> >> releases at least.
>> >>
>> >> Does this work for the projects you have in mind?
>> >
>> > It might work for some projects, but in the general case I find it pretty
>> > unappealing as a restriction. Mixing and matching new QEMU with old
>> > libvirt,
>> > or vica-verca has been an incredibly common thing todo when both developing
>> > and perhaps more importantly debugging problems. For example I have one
>> > libvirt build and I use it against any QEMU from Fedora / any RHEL-8.x
>> > update, which spans a great many QEMU releases.
>>
>> I'd like to propose that for compatibility with a wide range of QEMU
>> versions, you use or reinvent libvirt.
>
> Implicit in that statement though is that libvirt will not be able
> to make use of the QAPI code generator as proposed though. If we are
> designing something to make our application consumer's lives easier,
> but we exclude such a major application, is our solution actually
> a good one.
A solution for a narrow problem we can actually deliver and maintain is
better than a solution for a wider problem we can't.
>> > For a minimum viable use case, this doesn't feel all that difficult, as
>> > conceptually instead of deleting the field from QAPI, we just need to
>> > annotate it to say when it was deleted from the QEMU side. The QAPI
>> > generator for internal QEMU usage, can omit any fields annotated as
>> > deleted in QAPI schema. The QAPI generator for external app usage,
>> > can (optionally) be told to include deleted fields ranging back to
>> > a given version number. So apps can chooses what degree of compat
>> > they wish to retain.
>>
>> Consider this evolution of command block_resize
>
> To help us understand, I'll illustrate some possible interfaces
> in both Go and Python, since that covers dynamic and static
> languages
>
>> * Initially, it has a mandatory argument @device[*].
>
> Python definition:
>
>def block_resize(device, size)
>
> Caller:
>
> block_resize('dev0', 1*GiB)
>
>
> Golang definition
>
>type BlockResizeCommand struct {
>Device string
>Size int
>}
>
> Caller
>
>cmd := {
>Device: "dev0",
>Size: 1 * GiB,
>}
>
>> * An alternative way to specify the command's object emerges: new
>> argument @node-name. Both old @device and new @node-name become
>> optional, and exactly one of them must be specified. This is commit
>> 3b1dbd11a6 "qmp: Allow block_resize to manipulate bs graph nodes."
>
> Python definition. Tricky, as non-optional params must be before
> optional params, but size is naturally the last arg. One option
> is to pointlessly mark 'size' as optional
>
>def block_resize(device=None, node_name=None, size=None)
Who needs compile-time checking anyway.
Back to serious. Keyword arguments might be a better match for Python
bindings.
> Caller
>
> block_resize(device="dev0", size=1*GiB)
> block_resize(node_name="devnode0", size=1*GiB)
>
>
> In golang definition
>
>type BlockResizeArguments struct {
>Device string
>NodeName string
>Size int
>}
>
> Caller choice of
>
>cmd := {
>Device: "dev0",
>Size: 1 * GiB,
>}
>
>cmd := {
>NodeName: "devnode0",
>Size: 1 * GiB,
>}
Note that the Go bindings you sketched effectively use (poor man's)
keyword arguments.
> Neither case can easily prevent passing Device and NodeName
> at same time.
That defect lies at the schema's feet.
>> * At some future date, the old way gets deprecated: argument @device
>> acquires feature @deprecated.
>
> Ok, no change needed to the APIs in either case. Possibly have
> code emit a warning if
Re: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
Daniel P. Berrangé writes:
> On Tue, May 10, 2022 at 01:34:03PM +0100, Daniel P. Berrangé wrote:
>> On Tue, May 10, 2022 at 02:02:56PM +0200, Markus Armbruster wrote:
>> > > For a minimum viable use case, this doesn't feel all that difficult, as
>> > > conceptually instead of deleting the field from QAPI, we just need to
>> > > annotate it to say when it was deleted from the QEMU side. The QAPI
>> > > generator for internal QEMU usage, can omit any fields annotated as
>> > > deleted in QAPI schema. The QAPI generator for external app usage,
>> > > can (optionally) be told to include deleted fields ranging back to
>> > > a given version number. So apps can chooses what degree of compat
>> > > they wish to retain.
>> >
>> > Consider this evolution of command block_resize
>>
>> To help us understand, I'll illustrate some possible interfaces
>> in both Go and Python, since that covers dynamic and static
>> languages
>>
>> > * Initially, it has a mandatory argument @device[*].
>>
>> Python definition:
>>
>>def block_resize(device, size)
>>
>> Caller:
>>
>> block_resize('dev0', 1*GiB)
>>
>>
>> Golang definition
>>
>>type BlockResizeCommand struct {
>>Device string
>>Size int
>>}
>>
>> Caller
>>
>>cmd := {
>>Device: "dev0",
>>Size: 1 * GiB,
>>}
>>
>> > * An alternative way to specify the command's object emerges: new
>> > argument @node-name. Both old @device and new @node-name become
>> > optional, and exactly one of them must be specified. This is commit
>> > 3b1dbd11a6 "qmp: Allow block_resize to manipulate bs graph nodes."
>>
>> Python definition. Tricky, as non-optional params must be before
>> optional params, but size is naturally the last arg. One option
>> is to pointlessly mark 'size' as optional
>>
>>def block_resize(device=None, node_name=None, size=None)
>>
>> Caller
>>
>> block_resize(device="dev0", size=1*GiB)
>> block_resize(node_name="devnode0", size=1*GiB)
>>
>>
>> In golang definition
>>
>>type BlockResizeArguments struct {
>>Device string
>>NodeName string
>>Size int
>>}
>>
>> Caller choice of
>>
>>cmd := {
>>Device: "dev0",
>>Size: 1 * GiB,
>>}
>>
>>cmd := {
>>NodeName: "devnode0",
>>Size: 1 * GiB,
>>}
>>
>>
>> Neither case can easily prevent passing Device and NodeName
>> at same time.
>>
>> > * At some future date, the old way gets deprecated: argument @device
>> > acquires feature @deprecated.
>>
>> Ok, no change needed to the APIs in either case. Possibly have
>> code emit a warning if a deprecated field is set.
>>
>> > * Still later, the old way gets removed: @device is deleted, and
>> > @node-name becomes mandatory.
>>
>> Again no change needed to APIs, but QEMU will throw back an
>> error if the wrong one is used.
>>
>> > What is the proper version-spanning interface?
>> >
>> > I figure it's both arguments optional, must specify the right one for
>> > the version of QEMU actually in use. This spans versions, but it fails
>> > to abstract from them.
>>
>> Yep, I think that's inevitable in this scenario. THe plus side
>> is that apps that want to span versions can do so. The downside
>> is that apps that don't want smarts to span version, may loose
>> compile time warnings about use of the now deleted field.
>
> Having said that, a different way to approach the problem is to expose
> the versioning directly in the generated code.
>
> Consider a QAPI with versioning info about the fields
>
> { 'command': 'block_resize',
> 'since': '5.0.0',
> 'data': { 'device': ['type': 'str', 'until': '5.2.0' ],
> '*device': ['type': 'str', 'since': '5.2.0', 'until': '7.0.0' ],
> '*node-name': ['type': 'str', 'since': '5.2.0', 'until: '7.0.0'
> ],
> 'node-name': ['type': 'str', 'since': '7.0.0' ],
> 'size': 'int' } }
>
> Meaning
>
> * Introduced in 5.0.0, with 'device' mandatory
> * In 5.2.0, 'device' becomes optional, with optional 'node-name' as
> alternative
> * In 7.0.0, 'device' is deleted, and 'node-name' becomes mandatory
>
> Now consider the Go structs
>
> In 5.0.0 we can generate:
>
>type BlockResizeArguments struct {
>V500 *BlockResizeArguments500
>}
>
>type BlockResizeArgumentsV1 struct {
> Device string
> Size int
> }
>
> app can use
>
> dev := "dev0"
> cmd := BlockResizeArguments{
>V500: {
> Device: dev,
> Size: 1 * GiB
>}
> }
>
>
> In 5.2.0 we can now generate
>
>type BlockResizeArguments struct {
>V500 *BlockResizeArgumentsV500
>V520 *BlockResizeArgumentsV520
>}
>
>type BlockResizeArgumentsV500 struct {
> Device string
> Size int
> }
>
>type BlockResizeArgumentsV520 struct {
> Device *string
> NodeName *string
> Size int
> }
>
>
> App can use the same as before,
Re: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
Daniel P. Berrangé writes: > On Mon, May 02, 2022 at 10:01:41AM -0400, Andrea Bolognani wrote: >> > Times how many naming conventions? >> >> Yeah, I don't think requiring all possible permutations to be spelled >> out in the schema is the way to go. That's exactly why my proposal >> was to offer a way to inject the semantic information that the parser >> can't figure out itself. >> >> Once you have a way to inform the generator that "VNCProps" is made >> of the two words "vnc" and "props", and that "vnc" is an acronym, >> then it can generate an identifier appropriate for the target >> language without having to spell out anywhere that such an identifier >> would be VNCProps for Go and VncProps for Rust. >> >> By the way, while looking around I realized that we also have to take >> into account things like D-Bus: the QAPI type ChardevDBus, for >> example, would probably translate verbatim to Go but have to be >> changed to ChardevDbus for Rust. Fun :) For what it's worth, I prefer Rust's style, because I hate downcasing tails of abbreviations a lot less than I hate WORDSRUNTOGETHER. > The hardest one of all is probably > >QAuthZListPolicy > > which must be split 'QAuthZ' + 'List' + 'Policy' - the trailing > uppercase ruins all heuristics you can come up with, as there's no > viable way to distinguish that scenario from 'ChardevDBus' which > is 'Chardev' + 'DBus' not 'ChardevD' + 'Bus' > >> Revised proposal for the annotation: >> >> ns:word-WORD-WoRD-123Word > > Ugly, but we should only need this in the fairly niche scenarios, > so not too pain ful to add a handful of these: > > Essentially if have the schema use CamelCase with UPPERCASE > acronyms, and declare two rules: > > 1. Split on every boundary from lower to upper > 2. Acronym detection if there's a sequence of 3 uppercase > letters, then split before the last uppercase. > > then we'll do the right thing with the vast majority of cases: > > ChardevSocket: > Rule 1: Chardev + Socket > Rule 2: Chardev + Socket Rule 2 isn't used here. > > VNCProps: > Rule 1: VNCProps > Rule 2: VNC + Props Rule 2 is used here. For Rust-style VncProps, rule 2 is again unused. > > ChardevDBus Note: "DBus" is not an (upper-case) acronym. It's a proper name grown from an abbreviation of "desktop bus". > Rule 1: Chardev + DBus > Rule 2: Chardev + DBus Rule 2 isn't used here, either. > and fail on > > QAuthZListPolicy > > Rule 1: QAuth + ZList + Policy > Rule 2: QAuth + ZList + Policy > > so only the last case needs ns:QAuthZ-List-Policy annotation, whcih > doesn't feel like a big burden I think there are two interesting sub-problems within the larger problem of mapping QAPI names to "nice" target language identifiers. 1. Splitting words run together (mostly camels, but also mistakes like logappend). 2. Adjust case in words for the target language For 1., we can rely on word separators '-', '_' and change from lower to upper case. Cases where that doesn't suffice remain. Adopting Go-style camels for QAPI will add to them, but something like your rule 2 above should mitigate. What about this simple greedy algorithm: funny_words = [QAuthZ, ...] tail = name words = [] while tail != "": if tail starts with any of the words in funny_words: next_word = that member of funny_words else: next_word = tail up to next separator or change from lower to upper case append next_word to tail tail = remainder of tail with leading separator stripped For 2., adjusting to all lower case (like "vnc", "props", "dbus"), all upper case (like "VNC", "PROPS", "DBUS"), and capitalized (like "Vnc", "Props", "Dbus") is trivial. Adjusting to capitalized except for funny words (like "VNC", "Props", "DBus") is not. Need a list of funny words, I guess. Perhaps a single list of funnies would do for 1. and for 2.
Re: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
Andrea Bolognani writes:
> On Tue, May 03, 2022 at 09:57:27AM +0200, Markus Armbruster wrote:
>> Andrea Bolognani writes:
>> > I still feel that 1) users of a language SDK will ideally not need to
>> > look at the QAPI schema or wire chatter too often
>>
>> I think the most likely point of contact is the QEMU QMP Reference
>> Manual.
>
> Note that there isn't anything preventing us from including the
> original QAPI name in the documentation for the corresponding Go
> symbol, or even a link to the reference manual.
>
> So we could make jumping from the Go API documentation, which is what
> a Go programmer will be looking at most of the time, to the QMP
> documentation pretty much effortless.
>
>> My point is: a name override feature like the one you propose needs to
>> be used with discipline and restraint. Adds to reviewers' mental load.
>> Needs to be worth it. I'm not saying it isn't, I'm just pointing out a
>> cost.
>
> Yeah, I get that.
>
> Note that I'm not suggesting it should be possible for a name to be
> completely overridden - I just want to make it possible for a human
> to provide the name parsing algorithm solutions to those problems it
> can't figure out on its own.
>
> We could prevent that feature from being misused by verifying that
> the symbol the annotation is attached to can be derived from the list
> of words provided. That way, something like
>
> # SOMEName (completely-DIFFERENT-name)
>
> would be rejected and we would avoid misuse.
Possibly as simple as "down-case both names and drop the funny
characters, result must be the same".
>> Wild idea: assume all lower case, but keep a list of exceptions.
>
> That could actually work reasonably well for QEMU because we only
> need to handle correctly what's in the schema, not arbitrary input.
>
> There's always the risk of the list of exceptions getting out of sync
> with the needs of the schema, but there's similarly no guarantee that
> annotations are going to be introduced when they are necessary, so
> it's mostly a wash.
>
> The only slight advantage of the annotation approach would be that it
> might be easier to notice it being missing because it's close to the
> name it refers to, while the list of exceptions is tucked away in a
> script far away from it.
We'd put it in qapi/pragma.json, I guess.
>> The QAPI schema language uses three naming styles:
>>
>> * lower-case-with-hyphens for command and member names
>>
>> Many names use upper case and '_'. See pragma command-name-exceptions
>> and member-name-exceptions.
>
> Looking at the output generated by Victor's WIP script, it looks like
> these are already handled as nicely as those that don't fall under
> any exception.
>
>> Some (many?) names lack separators between words (example: logappend).
How many would be good to know.
Ad hoc hackery to find names, filter out camels (because word splitting
is too hard there), split into words, look up words in a word list:
$ for i in `/usr/bin/python3 /work/armbru/qemu/scripts/qapi-gen.py -o qapi
-b ../qapi/qapi-schema.json | sort -u | awk '/^### [a-z0-9-]+$/ { print "lc",
$2; next } /^### [a-z0-9_-]+$/ { print lu; next } /^### [A-Z0-9_]+$/ { print
"uc", $2; next } /^### ([A-Z][a-z]+)+/ { print "cc", $2; next } { print "mc",
$2 }' | sed '/^mc\|^cc/d;s/^.. //;s/[^A-Za-z0-9]/\n/g' | tr A-Z a-z | sort -u`;
do grep -q "^$i$" /usr/share/dict/words || echo $i; done
420 lines. How many arguably lack separators between words? Wild guess
based on glancing at the output sideways: some 50.
>> * UPPER_CASE_WITH_UNDERSCORE for event names
>>
>> * CamelCase for type names
>>
>> Capitalization of words is inconsistent in places (example: VncInfo
>> vs. DisplayReloadOptionsVNC).
>>
>> What style conversions will we need for Go? Any other conversions come
>> to mind?
>>
>> What problems do these conversions have?
>
> Go uses CamelCase for pretty much everything: types, methods,
> constants...
>
> There's one slight wrinkle, in that the case of the first letter
> decides whether it's going to be a PublicName or a privateName. We
> can't do anything about that, but it shouldn't really affect us
> that much because we'll want all QAPI names to be public.
>
> So the issues preventing us from producing a "perfect" Go API are
>
> 1. inconsistent capitalization in type names
>
>-> could be addressed by simply changing the schema, as type
> names do not travel on the wire
At the price of some churn in C code.
Perhaps more consistent capitalization could be regarded as a slight
improvement on its own. We need to see (a good sample of) the changes
to judge.
> 2. missing dashes in certain command/member names
>
>-> leads to Incorrectcamelcase.
Names with words run together are arguably no uglier in CamelCase (Go)
than in lower_case_with_underscores (C).
>Kevin's work is supposed to
> address this
Except it's stuck.
Perhaps Kevin and I can get it moving
Re: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
On Tue, May 10, 2022 at 01:37:50PM -0400, Andrea Bolognani wrote:
> On Mon, May 09, 2022 at 12:21:10PM +0200, Victor Toso wrote:
> > On Tue, Apr 19, 2022 at 11:12:28AM -0700, Andrea Bolognani wrote:
> > > Based on the example you have in the README and how commands are
> > > defined, invoking (a simplified version of) the trace-event-get-state
> > > command would look like
> > >
> > > cmd := Command{
> > > Name: "trace-event-get-state",
> > > Arg: TraceEventGetStateCommand{
> > > Name: "qemu_memalign",
> > > },
> > > }
Note there is clear redundancy here between 'Name' and the struct
type. IMHO the better approach would be
cmd := TraceEventGetStateCommand{
Name: 'qemu_memalign'
}
and have 'Command' simply as an interface that TraceEventGetStateCommand
implements. I don't think the interface would need more than a
Marshal and Demarshal method, which serialize the 'Arg' and then add
the Name field explicitly.
> > > qmp_input, _ := json.Marshal()
> > > // qmp_input now contains
> > > //
> > > {"execute":"trace-event-get-state","arguments":{"name":"qemu_memalign"}}
> > > // do something with it
> > >
> > > qmp_output :=
> > > ([]byte)(`{"return":{"name":"qemu_memalign","state":"disabled"}}`)
> > > ret := cmd.GetReturnType()
> > > _ = json.Unmarshal(qmp_output, )
> > > // ret is a CommandResult instance whose Value member can be cast
> > > // to a TraceEventInfo struct
> > >
> > > First of all, from an application's point of view there are way too
> > > many steps involved:
> >
> > It can actually get worse. I've used a lot of nested struct to
> > define a Base type for a given Type. In Go, If you try to
> > initialize a Type that has a nested Struct, you'll need to use
> > the nested struct Type as field name and this is too verbose.
> >
> > See https://github.com/golang/go/issues/29438 (merged with:
> > https://github.com/golang/go/issues/12854)
> >
> > The main reason that I kept it is because it maps very well with
> > the over-the-wire protocol.
>
> Right, I had not realized how bad things really were :)
>
> I've noticed the use of base types and while I didn't bring it up in
> my initial message because the other concerns seemed of much higher
> importance, I actually wondered whether we need to expose them to
> users of the Go SDK.
>
> I think we should flatten things. That's what happens with the C
> generator already, for example in
>
> struct InetSocketAddress {
> /* Members inherited from InetSocketAddressBase: */
> char *host;
> char *port;
> /* Own members: */
> bool has_numeric;
> bool numeric;
> /* ... */
> };
>
> This representation mirrors the wire protocol perfectly, so I see no
> reason not to adopt it. Am I missing something?
The main reason not to flatten is if you have scenarios where it is
useful to work against the base type directly. I'm not sure that we
have such a need though.
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: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
On Mon, May 09, 2022 at 12:21:10PM +0200, Victor Toso wrote:
> On Tue, Apr 19, 2022 at 11:12:28AM -0700, Andrea Bolognani wrote:
> > Based on the example you have in the README and how commands are
> > defined, invoking (a simplified version of) the trace-event-get-state
> > command would look like
> >
> > cmd := Command{
> > Name: "trace-event-get-state",
> > Arg: TraceEventGetStateCommand{
> > Name: "qemu_memalign",
> > },
> > }
> > qmp_input, _ := json.Marshal()
> > // qmp_input now contains
> > //
> > {"execute":"trace-event-get-state","arguments":{"name":"qemu_memalign"}}
> > // do something with it
> >
> > qmp_output :=
> > ([]byte)(`{"return":{"name":"qemu_memalign","state":"disabled"}}`)
> > ret := cmd.GetReturnType()
> > _ = json.Unmarshal(qmp_output, )
> > // ret is a CommandResult instance whose Value member can be cast
> > // to a TraceEventInfo struct
> >
> > First of all, from an application's point of view there are way too
> > many steps involved:
>
> It can actually get worse. I've used a lot of nested struct to
> define a Base type for a given Type. In Go, If you try to
> initialize a Type that has a nested Struct, you'll need to use
> the nested struct Type as field name and this is too verbose.
>
> See https://github.com/golang/go/issues/29438 (merged with:
> https://github.com/golang/go/issues/12854)
>
> The main reason that I kept it is because it maps very well with
> the over-the-wire protocol.
Right, I had not realized how bad things really were :)
I've noticed the use of base types and while I didn't bring it up in
my initial message because the other concerns seemed of much higher
importance, I actually wondered whether we need to expose them to
users of the Go SDK.
I think we should flatten things. That's what happens with the C
generator already, for example in
struct InetSocketAddress {
/* Members inherited from InetSocketAddressBase: */
char *host;
char *port;
/* Own members: */
bool has_numeric;
bool numeric;
/* ... */
};
This representation mirrors the wire protocol perfectly, so I see no
reason not to adopt it. Am I missing something?
> > performing this operation should really be as
> > simple as
> >
> > ret, _ := qmp.TraceEventGetState("qemu_memalign")
> > // ret is a TraceEventInfo instance
> >
> > That's the end state we should be working towards.
> >
> > Of course that assumes that the "qmp" object knows where the
> > QMP socket is, knows how to talk the QMP protocol,
> > transparently deals with serializing and deserializing data...
> > Plus, in some case you might want to deal with the wire
> > transfer yourself in an application-specific manner. So it
> > makes sense to have the basic building blocks available and
> > then build the more ergonomic SDK on top of that - with only
> > the first part being in scope for this series.
>
> Right. Indeed, I thought a bit about what I want to fit into the
> code generator that will reside in QEMU and what we might want to
> develop on top of that.
>
> The goal for this series really is generating the data types that
> can be converted to/from QMP messages.
That's perfectly fine, and in fact I believe that splitting the whole
endeavor into three parts - QMP protocol implementation, QAPI types
serialization/deserialization, and a high-level SDK that gives easy
access to the previous two - is the best approach.
> > qmp_output :=
> > ([]byte)(`{"return":{"name":"qemu_memalign","state":"disabled"}}`)
> > ret := TraceEventInfo{}
> > _ = json.Unmarshal(qmp_output, )
> > // ret is a TraceEventInfo instance
> >
> > The advantages over the current implementation is that the compiler
> > will prevent you from doing something silly like passing the wrong
> > set of arguments to a commmand, and that the application has to
> > explicitly spell out what kind of object it expects to get as output.
>
> I think that, if we know all types that we can have at QAPI spec,
> the process of marshalling and unmarshalling should verify it.
> So, even if we don't change the expected interface as suggested,
> that work needs to be done. For some types, I've already did it,
> like for Unions and Alternate types.
>
> Example:
> https://gitlab.com/victortoso/qapi-go/-/blob/main/pkg/qapi/unions.go#L28
>
> This union type can have 4 values for the Any interface type. The
> code generator documents it to help user's out.
>
> | type SocketAddressLegacy struct {
> | // Base type for this struct
> | SocketAddressLegacyBase
> | // Value based on @type, possible types:
> | // * InetSocketAddressWrapper
> | // * UnixSocketAddressWrapper
> | // * VsockSocketAddressWrapper
> | // * StringWrapper
> | Value Any
> | }
>
> On the Marshal function, I used Sprintf as a way to fetch Value's
> type. There are other alternatives but to the cost of adding
> other deps.
>
> | func (s SocketAddressLegacy)
Re: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
On Tue, May 10, 2022 at 10:52:34AM +0100, Daniel P. Berrangé wrote:
> On Mon, May 02, 2022 at 10:01:41AM -0400, Andrea Bolognani wrote:
> > Revised proposal for the annotation:
> >
> > ns:word-WORD-WoRD-123Word
>
> Ugly, but we should only need this in the fairly niche scenarios,
> so not too pain ful to add a handful of these:
>
> Essentially if have the schema use CamelCase with UPPERCASE
> acronyms, and declare two rules:
>
> 1. Split on every boundary from lower to upper
> 2. Acronym detection if there's a sequence of 3 uppercase
> letters, then split before the last uppercase.
That should cover most of the type names, but we're still going to
need to help the parser out when it comes to detecting acronyms in
other contexts, such as all instances of the word "VNC" below:
{ 'enum': 'DisplayProtocol',
'data': [ 'vnc', ... ] }
{ 'command': 'query-vnc-servers', ... }
{ 'event': 'VNC_DISCONNECTED', ... }
> QAuthZListPolicy
>
> Rule 1: QAuth + ZList + Policy
> Rule 2: QAuth + ZList + Policy
>
> so only the last case needs ns:QAuthZ-List-Policy annotation, whcih
> doesn't feel like a big burden
Note that in my proposal the ns: part would be used exactly for cases
like this one to separate the namespace part which, as you said in
your other reply, needs to be preserved when generating C code but
can be safely dropped when targeting a language that has actual
namespace support. So the annotation would look like
Q:AuthZ-List-Policy
in this specific case. The ns: part would be optional, as a namespace
is not embedded in most of the names.
It's also interesting to see how "AuthZ" is capitalized in various Go
modules that implement the concept:
https://pkg.go.dev/search?limit=50=symbol=authz
Most use "Authz" rather than "AuthZ". If we get to the point where
the only bit of weirdness is how we spell this specific word, I think
I'll be able to live with it :)
--
Andrea Bolognani / Red Hat / Virtualization
Re: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
On Tue, May 10, 2022 at 01:34:03PM +0100, Daniel P. Berrangé wrote:
> On Tue, May 10, 2022 at 02:02:56PM +0200, Markus Armbruster wrote:
> > > For a minimum viable use case, this doesn't feel all that difficult, as
> > > conceptually instead of deleting the field from QAPI, we just need to
> > > annotate it to say when it was deleted from the QEMU side. The QAPI
> > > generator for internal QEMU usage, can omit any fields annotated as
> > > deleted in QAPI schema. The QAPI generator for external app usage,
> > > can (optionally) be told to include deleted fields ranging back to
> > > a given version number. So apps can chooses what degree of compat
> > > they wish to retain.
> >
> > Consider this evolution of command block_resize
>
> To help us understand, I'll illustrate some possible interfaces
> in both Go and Python, since that covers dynamic and static
> languages
>
> > * Initially, it has a mandatory argument @device[*].
>
> Python definition:
>
>def block_resize(device, size)
>
> Caller:
>
> block_resize('dev0', 1*GiB)
>
>
> Golang definition
>
>type BlockResizeCommand struct {
>Device string
>Size int
>}
>
> Caller
>
>cmd := {
>Device: "dev0",
>Size: 1 * GiB,
>}
>
> > * An alternative way to specify the command's object emerges: new
> > argument @node-name. Both old @device and new @node-name become
> > optional, and exactly one of them must be specified. This is commit
> > 3b1dbd11a6 "qmp: Allow block_resize to manipulate bs graph nodes."
>
> Python definition. Tricky, as non-optional params must be before
> optional params, but size is naturally the last arg. One option
> is to pointlessly mark 'size' as optional
>
>def block_resize(device=None, node_name=None, size=None)
>
> Caller
>
> block_resize(device="dev0", size=1*GiB)
> block_resize(node_name="devnode0", size=1*GiB)
>
>
> In golang definition
>
>type BlockResizeArguments struct {
>Device string
>NodeName string
>Size int
>}
>
> Caller choice of
>
>cmd := {
>Device: "dev0",
>Size: 1 * GiB,
>}
>
>cmd := {
>NodeName: "devnode0",
>Size: 1 * GiB,
>}
>
>
> Neither case can easily prevent passing Device and NodeName
> at same time.
>
> > * At some future date, the old way gets deprecated: argument @device
> > acquires feature @deprecated.
>
> Ok, no change needed to the APIs in either case. Possibly have
> code emit a warning if a deprecated field is set.
>
> > * Still later, the old way gets removed: @device is deleted, and
> > @node-name becomes mandatory.
>
> Again no change needed to APIs, but QEMU will throw back an
> error if the wrong one is used.
>
> > What is the proper version-spanning interface?
> >
> > I figure it's both arguments optional, must specify the right one for
> > the version of QEMU actually in use. This spans versions, but it fails
> > to abstract from them.
>
> Yep, I think that's inevitable in this scenario. THe plus side
> is that apps that want to span versions can do so. The downside
> is that apps that don't want smarts to span version, may loose
> compile time warnings about use of the now deleted field.
Having said that, a different way to approach the problem is to expose
the versioning directly in the generated code.
Consider a QAPI with versioning info about the fields
{ 'command': 'block_resize',
'since': '5.0.0',
'data': { 'device': ['type': 'str', 'until': '5.2.0' ],
'*device': ['type': 'str', 'since': '5.2.0', 'until': '7.0.0' ],
'*node-name': ['type': 'str', 'since': '5.2.0', 'until: '7.0.0' ],
'node-name': ['type': 'str', 'since': '7.0.0' ],
'size': 'int' } }
Meaning
* Introduced in 5.0.0, with 'device' mandatory
* In 5.2.0, 'device' becomes optional, with optional 'node-name' as
alternative
* In 7.0.0, 'device' is deleted, and 'node-name' becomes mandatory
Now consider the Go structs
In 5.0.0 we can generate:
type BlockResizeArguments struct {
V500 *BlockResizeArguments500
}
type BlockResizeArgumentsV1 struct {
Device string
Size int
}
app can use
dev := "dev0"
cmd := BlockResizeArguments{
V500: {
Device: dev,
Size: 1 * GiB
}
}
In 5.2.0 we can now generate
type BlockResizeArguments struct {
V500 *BlockResizeArgumentsV500
V520 *BlockResizeArgumentsV520
}
type BlockResizeArgumentsV500 struct {
Device string
Size int
}
type BlockResizeArgumentsV520 struct {
Device *string
NodeName *string
Size int
}
App can use the same as before, or switch to one of
dev := "dev0"
cmd := BlockResizeArguments{
V520: {
Device: ,
Size: 1 * GiB
}
}
or
node := "nodedev0"
cmd := BlockResizeArguments{
V520: {
NodeName: ,
Re: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
On Tue, May 10, 2022 at 02:02:56PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé writes:
>
> > On Tue, Apr 26, 2022 at 01:14:28PM +0200, Markus Armbruster wrote:
> >> We need to look at "following the QEMU releases" a bit more closely.
> >>
> >> Merging your patches gives us the capability to generate a Go interface
> >> to HEAD's version of QMP.
> >>
> >> The obvious way for an out-of-tree Go program to use this generated Go
> >> interface is to build with a specific version of it. It can then talk
> >> QMP to any compatible QEMU version.
> >>
> >> Compatibility with older QEMUs is not assured: stuff added since is
> >> present on the Go QMP client end, but not on the QEMU QMP server end.
> >>
> >> Compatibility with newer QEMUs is subject to our deprecation policy:
> >>
> >> In general features are intended to be supported indefinitely once
> >> introduced into QEMU. In the event that a feature needs to be
> >> removed, it will be listed in this section. The feature will remain
> >> functional for the release in which it was deprecated and one
> >> further release. After these two releases, the feature is liable to
> >> be removed.
> >>
> >> So, if you stay away from deprecated stuff, you're good for two more
> >> releases at least.
> >>
> >> Does this work for the projects you have in mind?
> >
> > It might work for some projects, but in the general case I find it pretty
> > unappealing as a restriction. Mixing and matching new QEMU with old libvirt,
> > or vica-verca has been an incredibly common thing todo when both developing
> > and perhaps more importantly debugging problems. For example I have one
> > libvirt build and I use it against any QEMU from Fedora / any RHEL-8.x
> > update, which spans a great many QEMU releases.
>
> I'd like to propose that for compatibility with a wide range of QEMU
> versions, you use or reinvent libvirt.
Implicit in that statement though is that libvirt will not be able
to make use of the QAPI code generator as proposed though. If we are
designing something to make our application consumer's lives easier,
but we exclude such a major application, is our solution actually
a good one.
> > For a minimum viable use case, this doesn't feel all that difficult, as
> > conceptually instead of deleting the field from QAPI, we just need to
> > annotate it to say when it was deleted from the QEMU side. The QAPI
> > generator for internal QEMU usage, can omit any fields annotated as
> > deleted in QAPI schema. The QAPI generator for external app usage,
> > can (optionally) be told to include deleted fields ranging back to
> > a given version number. So apps can chooses what degree of compat
> > they wish to retain.
>
> Consider this evolution of command block_resize
To help us understand, I'll illustrate some possible interfaces
in both Go and Python, since that covers dynamic and static
languages
> * Initially, it has a mandatory argument @device[*].
Python definition:
def block_resize(device, size)
Caller:
block_resize('dev0', 1*GiB)
Golang definition
type BlockResizeCommand struct {
Device string
Size int
}
Caller
cmd := {
Device: "dev0",
Size: 1 * GiB,
}
> * An alternative way to specify the command's object emerges: new
> argument @node-name. Both old @device and new @node-name become
> optional, and exactly one of them must be specified. This is commit
> 3b1dbd11a6 "qmp: Allow block_resize to manipulate bs graph nodes."
Python definition. Tricky, as non-optional params must be before
optional params, but size is naturally the last arg. One option
is to pointlessly mark 'size' as optional
def block_resize(device=None, node_name=None, size=None)
Caller
block_resize(device="dev0", size=1*GiB)
block_resize(node_name="devnode0", size=1*GiB)
In golang definition
type BlockResizeArguments struct {
Device string
NodeName string
Size int
}
Caller choice of
cmd := {
Device: "dev0",
Size: 1 * GiB,
}
cmd := {
NodeName: "devnode0",
Size: 1 * GiB,
}
Neither case can easily prevent passing Device and NodeName
at same time.
> * At some future date, the old way gets deprecated: argument @device
> acquires feature @deprecated.
Ok, no change needed to the APIs in either case. Possibly have
code emit a warning if a deprecated field is set.
> * Still later, the old way gets removed: @device is deleted, and
> @node-name becomes mandatory.
Again no change needed to APIs, but QEMU will throw back an
error if the wrong one is used.
> What is the proper version-spanning interface?
>
> I figure it's both arguments optional, must specify the right one for
> the version of QEMU actually in use. This spans versions, but it fails
> to abstract from them.
Yep, I think that's inevitable in this scenario. THe plus side
is that apps that want to span versions can do so. The downside
is that
Re: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
Daniel P. Berrangé writes: > On Tue, Apr 26, 2022 at 01:14:28PM +0200, Markus Armbruster wrote: >> Victor Toso writes: >> >> > Hi, >> > >> > Happy 1st April. Not a joke :) /* ugh, took me too long to send */ >> > >> > This series is about adding a generator in scripts/qapi to produce >> > Go data structures that can be used to communicate with QEMU over >> > QMP. >> > >> > >> > * Why Go? >> > >> > There are quite a few Go projects that interact with QEMU over QMP >> > and they endup using a mix of different libraries with their own >> > code. >> > >> > >> > ** Which projects? >> > >> > The ones I've found so far: >> > >> > - podman machine >> > https://github.com/containers/podman/tree/main/pkg/machine/qemu >> > >> > - kata-containers (govmm) >> > >> > https://github.com/kata-containers/kata-containers/tree/main/src/runtime/pkg/govmm >> > >> > - lxd >> > https://github.com/lxc/lxd/tree/master/lxd/instance/drivers >> > >> > - kubevirt (plain json strings) >> > https://github.com/kubevirt/kubevirt >> > >> > (let me know if you know others) >> > >> > >> > * But Why? >> > >> > I'm particularly interested in 3 out of 4 of the projects above and >> > only Kubevirt uses libvirt to handle QEMU. That means that every >> > QEMU releases where a QMP command, event or other data struct is >> > added, removed or changed, those projects need to check what changed >> > in QEMU and then address those changes in their projects, if needed. >> > >> > The idea behind generating Go data structures is that we can keep a >> > Go module which can have releases that follow QEMU releases. >> >> We need to look at "following the QEMU releases" a bit more closely. >> >> Merging your patches gives us the capability to generate a Go interface >> to HEAD's version of QMP. >> >> The obvious way for an out-of-tree Go program to use this generated Go >> interface is to build with a specific version of it. It can then talk >> QMP to any compatible QEMU version. >> >> Compatibility with older QEMUs is not assured: stuff added since is >> present on the Go QMP client end, but not on the QEMU QMP server end. >> >> Compatibility with newer QEMUs is subject to our deprecation policy: >> >> In general features are intended to be supported indefinitely once >> introduced into QEMU. In the event that a feature needs to be >> removed, it will be listed in this section. The feature will remain >> functional for the release in which it was deprecated and one >> further release. After these two releases, the feature is liable to >> be removed. >> >> So, if you stay away from deprecated stuff, you're good for two more >> releases at least. >> >> Does this work for the projects you have in mind? > > It might work for some projects, but in the general case I find it pretty > unappealing as a restriction. Mixing and matching new QEMU with old libvirt, > or vica-verca has been an incredibly common thing todo when both developing > and perhaps more importantly debugging problems. For example I have one > libvirt build and I use it against any QEMU from Fedora / any RHEL-8.x > update, which spans a great many QEMU releases. I'd like to propose that for compatibility with a wide range of QEMU versions, you use or reinvent libvirt. > I like the idea of auto-generating clients from the QAPI schema, and > would like it if we were able to use this kind of approach on the libvirt > side, but for that we need to be more flexible in version matching. > > Our current approach to deprecation features and subsequently removing > them from the QAPI schema works fine when the QAPI schema is only used > internally by QEMU, not when we we expand usage of QAPI to external > applications. > > I think we need to figure out a way to make the QAPI schema itself be > append only, while still allowing QEMU to deprecation & remove features. This is going to get complicated fast. > For a minimum viable use case, this doesn't feel all that difficult, as > conceptually instead of deleting the field from QAPI, we just need to > annotate it to say when it was deleted from the QEMU side. The QAPI > generator for internal QEMU usage, can omit any fields annotated as > deleted in QAPI schema. The QAPI generator for external app usage, > can (optionally) be told to include deleted fields ranging back to > a given version number. So apps can chooses what degree of compat > they wish to retain. Consider this evolution of command block_resize * Initially, it has a mandatory argument @device[*]. * An alternative way to specify the command's object emerges: new argument @node-name. Both old @device and new @node-name become optional, and exactly one of them must be specified. This is commit 3b1dbd11a6 "qmp: Allow block_resize to manipulate bs graph nodes." * At some future date, the old way gets deprecated: argument @device acquires feature @deprecated. * Still later, the old way gets removed: @device is deleted, and
Re: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
Hi,
On Tue, May 10, 2022 at 10:06:34AM +0200, Markus Armbruster wrote:
> Victor Toso writes:
> >> That's true, but at least to me the trade-off feels reasonable.
> >
> > I don't quite get the argument why it gets harder to find. We can
> > simply provide the actual name as reference in the generated
> > documentation, no?
>
> Predictable name transformation can save me detours through
> documentation. Being able to guess the Go bindings just from the QMP
> Reference Manual can be nice: it lets me write Go code with just the QMP
> Reference manual in view. Being able to guess the name in the QAPI
> schema from the name in the Go bindings can also be nice: it lets me
> look it up in the QMP Reference manual without jumping through the
> bindings documentation.
>
> Or do you envisage a Go bindings manual that fully replaces the
> QMP Reference Manual for developers writing Go?
When the language specifies a way of documenting the code and
provides tools to generate documentation (e.g: html format) from
the source code, it is common that IDEs would use this to provide
documentation from a given library in a simple way.
Links break that. But we might not have a choice anyway due the
fact that copying GPLv2+ could potentially be a problem to
license the Go module as something else.
> > As Kevin already pointed out that he is not planning to work on
> > the Alias (reference below), any other idea besides the Andrea's
> > annotation suggestion? While this is not a blocker, I agree it
> > would be nice to be consistent.
> >
> > https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg04703.html
>
> Let's first try to get a handle on how many schema names are
> problematic.
Thanks for this. I'll reply again when I went through it (if no
one else beats me to it).
Cheers,
Victor
> $ git-diff
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index 5a1782b57e..804b8ab455 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -94,6 +94,7 @@ def check_name_str(name: str, info: QAPISourceInfo, source:
> str) -> str:
> """
> # Reserve the entire 'q_' namespace for c_name(), and for 'q_empty'
> # and 'q_obj_*' implicit type names.
> +print("###", name)
> match = valid_name.match(name)
> if not match or c_name(name, False).startswith('q_'):
> raise QAPISemError(info, "%s has an invalid name" % source)
> $ cd bld
> $ python3 ../scripts/qapi-gen.py -o qapi -b ../qapi/qapi-schema.json | sort
> -u | awk '/^### [a-z0-9-]+$/ { print "lc", $2; next } /^### [A-Z0-9_]+$/ {
> print "uc", $2; next } /^### ([A-Z][a-z]+)+/ { print "cc", $2; next } { print
> "mc", $2 }' | sort
> cc Abort
> cc AbortWrapper
> cc AcpiTableOptions
> cc ActionCompletionMode
> cc AddfdInfo
> cc AnnounceParameters
> cc AudioFormat
> cc Audiodev
> cc AudiodevAlsaOptions
> cc AudiodevAlsaPerDirectionOptions
> cc AudiodevCoreaudioOptions
> cc AudiodevCoreaudioPerDirectionOptions
> cc AudiodevDriver
> cc AudiodevDsoundOptions
> cc AudiodevGenericOptions
> cc AudiodevJackOptions
> cc AudiodevJackPerDirectionOptions
> cc AudiodevOssOptions
> cc AudiodevOssPerDirectionOptions
> cc AudiodevPaOptions
> cc AudiodevPaPerDirectionOptions
> cc AudiodevPerDirectionOptions
> cc AudiodevSdlOptions
> cc AudiodevSdlPerDirectionOptions
> cc AudiodevWavOptions
> cc AuthZListFileProperties
> cc AuthZListProperties
> cc AuthZPAMProperties
> cc AuthZSimpleProperties
> cc BackupCommon
> cc BackupPerf
> cc BalloonInfo
> cc BiosAtaTranslation
> cc BitmapMigrationBitmapAlias
> cc BitmapMigrationBitmapAliasTransform
> cc BitmapMigrationNodeAlias
> cc BitmapSyncMode
> cc BlkdebugEvent
> cc BlkdebugIOType
> cc BlkdebugInjectErrorOptions
> cc BlkdebugSetStateOptions
> cc BlockDeviceInfo
> cc BlockDeviceIoStatus
> cc BlockDeviceStats
> cc BlockDeviceTimedStats
> cc BlockDirtyBitmap
> cc BlockDirtyBitmapAdd
> cc BlockDirtyBitmapAddWrapper
> cc BlockDirtyBitmapMerge
> cc BlockDirtyBitmapMergeWrapper
> cc BlockDirtyBitmapOrStr
> cc BlockDirtyBitmapSha256
> cc BlockDirtyBitmapWrapper
> cc BlockDirtyInfo
> cc BlockErrorAction
> cc BlockExportInfo
> cc BlockExportOptions
> cc BlockExportOptionsFuse
> cc BlockExportOptionsNbd
> cc BlockExportOptionsNbdBase
> cc BlockExportOptionsVhostUserBlk
> cc BlockExportRemoveMode
> cc BlockExportType
> cc BlockIOThrottle
> cc BlockInfo
> cc BlockJobInfo
> cc BlockLatencyHistogramInfo
> cc BlockMeasureInfo
> cc BlockPermission
> cc BlockStats
> cc BlockStatsSpecific
> cc BlockStatsSpecificFile
> cc BlockStatsSpecificNvme
> cc BlockdevAioOptions
> cc BlockdevAmendOptions
> cc BlockdevAmendOptionsLUKS
> cc BlockdevAmendOptionsQcow2
> cc BlockdevBackup
> cc BlockdevBackupWrapper
> cc BlockdevCacheInfo
> cc BlockdevCacheOptions
> cc BlockdevChangeReadOnlyMode
> cc BlockdevCreateOptions
> cc BlockdevCreateOptionsFile
> cc BlockdevCreateOptionsGluster
> cc BlockdevCreateOptionsLUKS
> cc BlockdevCreateOptionsNfs
> cc BlockdevCreateOptionsParallels
> cc BlockdevCreateOptionsQcow
> cc
Re: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
On Tue, May 10, 2022 at 12:50:47PM +0200, Victor Toso wrote: > Hi, > > On Tue, May 10, 2022 at 10:32:49AM +0100, Daniel P. Berrangé wrote: > > On Tue, May 10, 2022 at 10:18:15AM +0100, Daniel P. Berrangé wrote: > > > On Tue, May 10, 2022 at 11:06:39AM +0200, Victor Toso wrote: > > > > > > > generated code needs to be compatible with other Golang projects, > > > > > > > such as the ones mentioned above. My intention is to keep a Go > > > > > > > module with a MIT license. > > > > > > > > > > > > Meh. Can't be helped, I guess. > > > > > > > > > > This does make me wonder though whether the license of the QAPI > > > > > input files has a bearing on the Go (or other $LANGUAGE) ouput > > > > > files. eg is the Go code to be considered a derived work of the > > > > > QAPI JSON files. > > > > > > > > GPL does not enforce that the generated code to be GPL [0] unless > > > > the generator copies GPL code to the output. My only concern has > > > > been the fact that I am copying the documentation of QAPI > > > > specification to the Go package in order to have data structures, > > > > commands, etc. with the information provided by the > > > > specification. > > > > > > > > [0] https://www.gnu.org/licenses/old-licenses/gpl-2.0-faq.html#GPLOutput > > > > > > > > > I'm not finding a clearly articulated POV on this question so > > > > > far. > > > > > > > > I don't find it trivial either but I've accepted that the Go data > > > > structures are fine based on [0] and the documentation can be > > > > split from the Go module (sadly!) if someone finds it to be a > > > > legal issue. > > > > > > Ah well that link above is actually reasonably clear: > > > > > > "More generally, when a program translates its input into > > >some other form, the copyright status of the output inherits > > >that of the input it was generated from. " > > > > > > In our case the input is the QAPI JSON, and we're translating that > > > into Golang. That could be read as meaning our Golang code has to > > > be GPLv2+ licensed just as with the QAPI, not merely the docs. > > > > Oh but I'm forgetting that QAPI JSON can be said to be our > > public API interface, and so while the docs text would be > > GPLv2+, we can claim fair use for the interface definition in > > the generator and pick an arbitrary output license. > > Still, it explicit says in the section "In what cases is the > output of a GPL program covered by the GPL too?" is " Only when > the program copies part of itself into the output". > > > https://www.gnu.org/licenses/old-licenses/gpl-2.0-faq.html#WhatCaseIsOutputGPL > > So, to my understating, even if we are consuming a GPLv2+ spec > with a GPLv2+ generator, the output does not need to be GPLv2+ > too, unless we are *copying* parts of the input/generator into > the output - which is the case for the documentation only. > > I'll raise this again with the my company's legal team to be > sure. > > > We could likely deal with the docs problem by not copying the > > docs directly into the generated code, instead link to the > > published docs on qemu.org. This would require us to improve > > our current docs generated anchor generation. ie currently docs > > link for say the struct 'NumaNodeOptions' potentially changes > > every time we generate it > > > > https://www.qemu.org/docs/master/interop/qemu-qmp-ref.html#qapidoc-2416 > > > > we would need to that be something stable, ie > > > > https://www.qemu.org/docs/master/interop/qemu-qmp-ref.html#struct-NumaNodeOptions > > > > Then the generated Go can just do > > > >// See QAPI docs at > > https://www.qemu.org/docs/master/interop/qemu-qmp-ref.html#struct-NumaNodeOptions > > > > thus avoiding any copyright complication > > Yes, but it would be quite sad solution. Documentation in Go is > bounded to the module source code and we would be making people > jump thorough links here. > > I mean, if that's what we need to do, okay. It isn't the end of the world IMHO, as people are typically browsing docs from the online docs site, so full details are only a click away. This is what I did in libvirt Go APIs for example https://pkg.go.dev/libvirt.org/go/libvirt#Domain.SnapshotLookupByName I feel the biggest impact for developers is actually the quality of the docs that exist. Time invested in better QAPI docs will have more impact on developers, than trying to eliminate the need to follow one extra hyperlink. 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: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
Hi, On Tue, May 10, 2022 at 10:32:49AM +0100, Daniel P. Berrangé wrote: > On Tue, May 10, 2022 at 10:18:15AM +0100, Daniel P. Berrangé wrote: > > On Tue, May 10, 2022 at 11:06:39AM +0200, Victor Toso wrote: > > > > > > generated code needs to be compatible with other Golang projects, > > > > > > such as the ones mentioned above. My intention is to keep a Go > > > > > > module with a MIT license. > > > > > > > > > > Meh. Can't be helped, I guess. > > > > > > > > This does make me wonder though whether the license of the QAPI > > > > input files has a bearing on the Go (or other $LANGUAGE) ouput > > > > files. eg is the Go code to be considered a derived work of the > > > > QAPI JSON files. > > > > > > GPL does not enforce that the generated code to be GPL [0] unless > > > the generator copies GPL code to the output. My only concern has > > > been the fact that I am copying the documentation of QAPI > > > specification to the Go package in order to have data structures, > > > commands, etc. with the information provided by the > > > specification. > > > > > > [0] https://www.gnu.org/licenses/old-licenses/gpl-2.0-faq.html#GPLOutput > > > > > > > I'm not finding a clearly articulated POV on this question so > > > > far. > > > > > > I don't find it trivial either but I've accepted that the Go data > > > structures are fine based on [0] and the documentation can be > > > split from the Go module (sadly!) if someone finds it to be a > > > legal issue. > > > > Ah well that link above is actually reasonably clear: > > > > "More generally, when a program translates its input into > >some other form, the copyright status of the output inherits > >that of the input it was generated from. " > > > > In our case the input is the QAPI JSON, and we're translating that > > into Golang. That could be read as meaning our Golang code has to > > be GPLv2+ licensed just as with the QAPI, not merely the docs. > > Oh but I'm forgetting that QAPI JSON can be said to be our > public API interface, and so while the docs text would be > GPLv2+, we can claim fair use for the interface definition in > the generator and pick an arbitrary output license. Still, it explicit says in the section "In what cases is the output of a GPL program covered by the GPL too?" is " Only when the program copies part of itself into the output". https://www.gnu.org/licenses/old-licenses/gpl-2.0-faq.html#WhatCaseIsOutputGPL So, to my understating, even if we are consuming a GPLv2+ spec with a GPLv2+ generator, the output does not need to be GPLv2+ too, unless we are *copying* parts of the input/generator into the output - which is the case for the documentation only. I'll raise this again with the my company's legal team to be sure. > We could likely deal with the docs problem by not copying the > docs directly into the generated code, instead link to the > published docs on qemu.org. This would require us to improve > our current docs generated anchor generation. ie currently docs > link for say the struct 'NumaNodeOptions' potentially changes > every time we generate it > > https://www.qemu.org/docs/master/interop/qemu-qmp-ref.html#qapidoc-2416 > > we would need to that be something stable, ie > > https://www.qemu.org/docs/master/interop/qemu-qmp-ref.html#struct-NumaNodeOptions > > Then the generated Go can just do > >// See QAPI docs at > https://www.qemu.org/docs/master/interop/qemu-qmp-ref.html#struct-NumaNodeOptions > > thus avoiding any copyright complication Yes, but it would be quite sad solution. Documentation in Go is bounded to the module source code and we would be making people jump thorough links here. I mean, if that's what we need to do, okay. I'll keep thinking about alternatives. Cheers, Victor signature.asc Description: PGP signature
Re: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
On Sat, Apr 02, 2022 at 12:40:56AM +0200, Victor Toso wrote: > * Status > > There are a few rough edges to work on but this is usable. The major > thing I forgot to add is handling Error from Commands. It'll be the > first thing I'll work on next week. > > If you want to start using this Today you can fetch it in at > > https://gitlab.com/victortoso/qapi-go/ Looking at this my main concern is that there is way too much use of the 'Any' type. The valid Golang types that can be used with any of the 'Any' fields are merely expresssed as comments in the code. I think this needs changing so that the Golang types are directly expressed in code. I think there are perhaps only 2-3 cases where the 'Any' type is genuinely neccessary due to the QAPI schema having an unbounded set of possible types - SchemaInfoObjectMember, ObjectPropertyInfo and QomSetCommand 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: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
On Tue, May 10, 2022 at 10:18:15AM +0100, Daniel P. Berrangé wrote: > On Tue, May 10, 2022 at 11:06:39AM +0200, Victor Toso wrote: > > Hi, > > > > On Tue, May 10, 2022 at 09:53:05AM +0100, Daniel P. Berrangé wrote: > > > > > * License > > > > > > > > > > While the generator (golang.py in this series) is GPL v2, the > > > > > > > > I'd make it v2+, just to express my displeasure with the decision to > > > > make the initial QAPI generator v2 only for no good reason at all. > > > > > > Our policy is that all new code should be v2+ anyway, unless it > > > was clearly derived from some pre-existing v2-only code. Upto > > > Victor to say whether the golang.py is considered clean, or was > > > copy+paste in any parts from existin v2-only code > > > > Should be fine to consider it v2+, the generator. > > > > > > > generated code needs to be compatible with other Golang projects, > > > > > such as the ones mentioned above. My intention is to keep a Go > > > > > module with a MIT license. > > > > > > > > Meh. Can't be helped, I guess. > > > > > > This does make me wonder though whether the license of the QAPI > > > input files has a bearing on the Go (or other $LANGUAGE) ouput > > > files. eg is the Go code to be considered a derived work of the > > > QAPI JSON files. > > > > GPL does not enforce that the generated code to be GPL [0] unless > > the generator copies GPL code to the output. My only concern has > > been the fact that I am copying the documentation of QAPI > > specification to the Go package in order to have data structures, > > commands, etc. with the information provided by the > > specification. > > > > [0] https://www.gnu.org/licenses/old-licenses/gpl-2.0-faq.html#GPLOutput > > > > > I'm not finding a clearly articulated POV on this question so > > > far. > > > > I don't find it trivial either but I've accepted that the Go data > > structures are fine based on [0] and the documentation can be > > split from the Go module (sadly!) if someone finds it to be a > > legal issue. > > Ah well that link above is actually reasonably clear: > > "More generally, when a program translates its input into >some other form, the copyright status of the output inherits >that of the input it was generated from. " > > In our case the input is the QAPI JSON, and we're translating that > into Golang. That could be read as meaning our Golang code has to > be GPLv2+ licensed just as with the QAPI, not merely the docs. Oh but I'm forgetting that QAPI JSON can be said to be our public API interface, and so while the docs text would be GPLv2+, we can claim fair use for the interface definition in the generator and pick an arbitrary output license. We could likely deal with the docs problem by not copying the docs directly into the generated code, instead link to the published docs on qemu.org. This would require us to improve our current docs generated anchor generation. ie currently docs link for say the struct 'NumaNodeOptions' potentially changes every time we generate it https://www.qemu.org/docs/master/interop/qemu-qmp-ref.html#qapidoc-2416 we would need to that be something stable, ie https://www.qemu.org/docs/master/interop/qemu-qmp-ref.html#struct-NumaNodeOptions Then the generated Go can just do // See QAPI docs at https://www.qemu.org/docs/master/interop/qemu-qmp-ref.html#struct-NumaNodeOptions thus avoiding any copyright complication 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: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
On Tue, May 03, 2022 at 02:40:14AM -0700, Andrea Bolognani wrote: > On Tue, May 03, 2022 at 09:57:27AM +0200, Markus Armbruster wrote: > > Andrea Bolognani writes: > > > I still feel that 1) users of a language SDK will ideally not need to > > > look at the QAPI schema or wire chatter too often > > > > I think the most likely point of contact is the QEMU QMP Reference > > Manual. > > Note that there isn't anything preventing us from including the > original QAPI name in the documentation for the corresponding Go > symbol, or even a link to the reference manual. > > So we could make jumping from the Go API documentation, which is what > a Go programmer will be looking at most of the time, to the QMP > documentation pretty much effortless. > > > My point is: a name override feature like the one you propose needs to > > be used with discipline and restraint. Adds to reviewers' mental load. > > Needs to be worth it. I'm not saying it isn't, I'm just pointing out a > > cost. > > Yeah, I get that. > > Note that I'm not suggesting it should be possible for a name to be > completely overridden - I just want to make it possible for a human > to provide the name parsing algorithm solutions to those problems it > can't figure out on its own. > > We could prevent that feature from being misused by verifying that > the symbol the annotation is attached to can be derived from the list > of words provided. That way, something like > > # SOMEName (completely-DIFFERENT-name) > > would be rejected and we would avoid misuse. > > > Wild idea: assume all lower case, but keep a list of exceptions. > > That could actually work reasonably well for QEMU because we only > need to handle correctly what's in the schema, not arbitrary input. > > There's always the risk of the list of exceptions getting out of sync > with the needs of the schema, but there's similarly no guarantee that > annotations are going to be introduced when they are necessary, so > it's mostly a wash. > > The only slight advantage of the annotation approach would be that it > might be easier to notice it being missing because it's close to the > name it refers to, while the list of exceptions is tucked away in a > script far away from it. > > > The QAPI schema language uses three naming styles: > > > > * lower-case-with-hyphens for command and member names > > > > Many names use upper case and '_'. See pragma command-name-exceptions > > and member-name-exceptions. > > Looking at the output generated by Victor's WIP script, it looks like > these are already handled as nicely as those that don't fall under > any exception. > > > Some (many?) names lack separators between words (example: logappend). > > > > * UPPER_CASE_WITH_UNDERSCORE for event names > > > > * CamelCase for type names > > > > Capitalization of words is inconsistent in places (example: VncInfo > > vs. DisplayReloadOptionsVNC). > > > > What style conversions will we need for Go? Any other conversions come > > to mind? > > > > What problems do these conversions have? > > Go uses CamelCase for pretty much everything: types, methods, > constants... > > There's one slight wrinkle, in that the case of the first letter > decides whether it's going to be a PublicName or a privateName. We > can't do anything about that, but it shouldn't really affect us > that much because we'll want all QAPI names to be public. > > So the issues preventing us from producing a "perfect" Go API are > > 1. inconsistent capitalization in type names > >-> could be addressed by simply changing the schema, as type > names do not travel on the wire > > 2. missing dashes in certain command/member names > >-> leads to Incorrectcamelcase. Kevin's work is supposed to > address this > > 3. inability to know which parts of a lower-case-name or > UPPER_CASE_NAME are acronyms or are otherwise supposed to be > capitalized in a specific way > >-> leads to WeirdVncAndDbusCapitalization. There's currently no > way, either implemented or planned, to avoid this > > In addition to these I'm also thinking that QKeyCode and all the > QCrypto stuff should probably lose their prefixes. At the C level, those prefixes are pretty important to avoid clashing with symbols defined by system headers and/or the external crypto library headers, as the unprefixed names are too generic. All non-C languages of course have proper namespacing > > > Revised proposal for the annotation: > > > > > > ns:word-WORD-WoRD-123Word > > > > > > Words are always separated by dashes; "regular" words are entirely > > > lowercase, while the presence of even a single uppercase letter in a > > > word denotes the fact that its case should be preserved when the > > > naming conventions of the target language allow that. > > > > Is a word always capitalized the same for a single target language? Or > > could capitalization depend on context? > > I'm not aware of any language that
Re: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
On Mon, May 02, 2022 at 10:01:41AM -0400, Andrea Bolognani wrote: > > Times how many naming conventions? > > Yeah, I don't think requiring all possible permutations to be spelled > out in the schema is the way to go. That's exactly why my proposal > was to offer a way to inject the semantic information that the parser > can't figure out itself. > > Once you have a way to inform the generator that "VNCProps" is made > of the two words "vnc" and "props", and that "vnc" is an acronym, > then it can generate an identifier appropriate for the target > language without having to spell out anywhere that such an identifier > would be VNCProps for Go and VncProps for Rust. > > By the way, while looking around I realized that we also have to take > into account things like D-Bus: the QAPI type ChardevDBus, for > example, would probably translate verbatim to Go but have to be > changed to ChardevDbus for Rust. Fun :) The hardest one of all is probably QAuthZListPolicy which must be split 'QAuthZ' + 'List' + 'Policy' - the trailing uppercase ruins all heuristics you can come up with, as there's no viable way to distinguish that scenario from 'ChardevDBus' which is 'Chardev' + 'DBus' not 'ChardevD' + 'Bus' > Revised proposal for the annotation: > > ns:word-WORD-WoRD-123Word Ugly, but we should only need this in the fairly niche scenarios, so not too pain ful to add a handful of these: Essentially if have the schema use CamelCase with UPPERCASE acronyms, and declare two rules: 1. Split on every boundary from lower to upper 2. Acronym detection if there's a sequence of 3 uppercase letters, then split before the last uppercase. then we'll do the right thing with the vast majority of cases: ChardevSocket: Rule 1: Chardev + Socket Rule 2: Chardev + Socket VNCProps: Rule 1: VNCProps Rule 2: VNC + Props ChardevDBus Rule 1: Chardev + DBus Rule 2: Chardev + DBus and fail on QAuthZListPolicy Rule 1: QAuth + ZList + Policy Rule 2: QAuth + ZList + Policy so only the last case needs ns:QAuthZ-List-Policy annotation, whcih doesn't feel like a big burden 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: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
On Tue, May 10, 2022 at 11:06:39AM +0200, Victor Toso wrote: > Hi, > > On Tue, May 10, 2022 at 09:53:05AM +0100, Daniel P. Berrangé wrote: > > > > * License > > > > > > > > While the generator (golang.py in this series) is GPL v2, the > > > > > > I'd make it v2+, just to express my displeasure with the decision to > > > make the initial QAPI generator v2 only for no good reason at all. > > > > Our policy is that all new code should be v2+ anyway, unless it > > was clearly derived from some pre-existing v2-only code. Upto > > Victor to say whether the golang.py is considered clean, or was > > copy+paste in any parts from existin v2-only code > > Should be fine to consider it v2+, the generator. > > > > > generated code needs to be compatible with other Golang projects, > > > > such as the ones mentioned above. My intention is to keep a Go > > > > module with a MIT license. > > > > > > Meh. Can't be helped, I guess. > > > > This does make me wonder though whether the license of the QAPI > > input files has a bearing on the Go (or other $LANGUAGE) ouput > > files. eg is the Go code to be considered a derived work of the > > QAPI JSON files. > > GPL does not enforce that the generated code to be GPL [0] unless > the generator copies GPL code to the output. My only concern has > been the fact that I am copying the documentation of QAPI > specification to the Go package in order to have data structures, > commands, etc. with the information provided by the > specification. > > [0] https://www.gnu.org/licenses/old-licenses/gpl-2.0-faq.html#GPLOutput > > > I'm not finding a clearly articulated POV on this question so > > far. > > I don't find it trivial either but I've accepted that the Go data > structures are fine based on [0] and the documentation can be > split from the Go module (sadly!) if someone finds it to be a > legal issue. Ah well that link above is actually reasonably clear: "More generally, when a program translates its input into some other form, the copyright status of the output inherits that of the input it was generated from. " In our case the input is the QAPI JSON, and we're translating that into Golang. That could be read as meaning our Golang code has to be GPLv2+ licensed just as with the QAPI, not merely the docs. 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: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
Hi, On Tue, May 10, 2022 at 09:53:05AM +0100, Daniel P. Berrangé wrote: > > > * License > > > > > > While the generator (golang.py in this series) is GPL v2, the > > > > I'd make it v2+, just to express my displeasure with the decision to > > make the initial QAPI generator v2 only for no good reason at all. > > Our policy is that all new code should be v2+ anyway, unless it > was clearly derived from some pre-existing v2-only code. Upto > Victor to say whether the golang.py is considered clean, or was > copy+paste in any parts from existin v2-only code Should be fine to consider it v2+, the generator. > > > generated code needs to be compatible with other Golang projects, > > > such as the ones mentioned above. My intention is to keep a Go > > > module with a MIT license. > > > > Meh. Can't be helped, I guess. > > This does make me wonder though whether the license of the QAPI > input files has a bearing on the Go (or other $LANGUAGE) ouput > files. eg is the Go code to be considered a derived work of the > QAPI JSON files. GPL does not enforce that the generated code to be GPL [0] unless the generator copies GPL code to the output. My only concern has been the fact that I am copying the documentation of QAPI specification to the Go package in order to have data structures, commands, etc. with the information provided by the specification. [0] https://www.gnu.org/licenses/old-licenses/gpl-2.0-faq.html#GPLOutput > I'm not finding a clearly articulated POV on this question so > far. I don't find it trivial either but I've accepted that the Go data structures are fine based on [0] and the documentation can be split from the Go module (sadly!) if someone finds it to be a legal issue. Cheers, Victor signature.asc Description: PGP signature
Re: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
On Tue, Apr 26, 2022 at 01:14:28PM +0200, Markus Armbruster wrote: > Victor Toso writes: > > > Hi, > > > > Happy 1st April. Not a joke :) /* ugh, took me too long to send */ > > > > This series is about adding a generator in scripts/qapi to produce > > Go data structures that can be used to communicate with QEMU over > > QMP. > > > > > > * Why Go? > > > > There are quite a few Go projects that interact with QEMU over QMP > > and they endup using a mix of different libraries with their own > > code. > > > > > > ** Which projects? > > > > The ones I've found so far: > > > > - podman machine > > https://github.com/containers/podman/tree/main/pkg/machine/qemu > > > > - kata-containers (govmm) > > > > https://github.com/kata-containers/kata-containers/tree/main/src/runtime/pkg/govmm > > > > - lxd > > https://github.com/lxc/lxd/tree/master/lxd/instance/drivers > > > > - kubevirt (plain json strings) > > https://github.com/kubevirt/kubevirt > > > > (let me know if you know others) > > > > > > * But Why? > > > > I'm particularly interested in 3 out of 4 of the projects above and > > only Kubevirt uses libvirt to handle QEMU. That means that every > > QEMU releases where a QMP command, event or other data struct is > > added, removed or changed, those projects need to check what changed > > in QEMU and then address those changes in their projects, if needed. > > > > The idea behind generating Go data structures is that we can keep a > > Go module which can have releases that follow QEMU releases. > > We need to look at "following the QEMU releases" a bit more closely. > > Merging your patches gives us the capability to generate a Go interface > to HEAD's version of QMP. > > The obvious way for an out-of-tree Go program to use this generated Go > interface is to build with a specific version of it. It can then talk > QMP to any compatible QEMU version. > > Compatibility with older QEMUs is not assured: stuff added since is > present on the Go QMP client end, but not on the QEMU QMP server end. > > Compatibility with newer QEMUs is subject to our deprecation policy: > > In general features are intended to be supported indefinitely once > introduced into QEMU. In the event that a feature needs to be > removed, it will be listed in this section. The feature will remain > functional for the release in which it was deprecated and one > further release. After these two releases, the feature is liable to > be removed. > > So, if you stay away from deprecated stuff, you're good for two more > releases at least. > > Does this work for the projects you have in mind? It might work for some projects, but in the general case I find it pretty unappealing as a restriction. Mixing and matching new QEMU with old libvirt, or vica-verca has been an incredibly common thing todo when both developing and perhaps more importantly debugging problems. For example I have one libvirt build and I use it against any QEMU from Fedora / any RHEL-8.x update, which spans a great many QEMU releases. I like the idea of auto-generating clients from the QAPI schema, and would like it if we were able to use this kind of approach on the libvirt side, but for that we need to be more flexible in version matching. Our current approach to deprecation features and subsequently removing them from the QAPI schema works fine when the QAPI schema is only used internally by QEMU, not when we we expand usage of QAPI to external applications. I think we need to figure out a way to make the QAPI schema itself be append only, while still allowing QEMU to deprecation & remove features. For a minimum viable use case, this doesn't feel all that difficult, as conceptually instead of deleting the field from QAPI, we just need to annotate it to say when it was deleted from the QEMU side. The QAPI generator for internal QEMU usage, can omit any fields annotated as deleted in QAPI schema. The QAPI generator for external app usage, can (optionally) be told to include deleted fields ranging back to a given version number. So apps can chooses what degree of compat they wish to retain. Apps that wish to have version compat, would of course need to write their code to be aware of which fields they need to seend for which QEMU version. > > * Status > > > > There are a few rough edges to work on but this is usable. The major > > thing I forgot to add is handling Error from Commands. It'll be the > > first thing I'll work on next week. > > > > If you want to start using this Today you can fetch it in at > > > > https://gitlab.com/victortoso/qapi-go/ > > > > There are quite a few tests that I took from the examples in the > > qapi schema. Coverage using go's cover tool is giving `28.6% of > > statements` > > > > I've uploaded the a static generated godoc output of the above Go > > module here: > > > > > > https://fedorapeople.org/~victortoso/qapi-go/rfc/victortoso.com/qapi-go/pkg/qapi/ > > > > > > * License > > > >
Re: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
Victor Toso writes: > Hi, > > On Mon, May 02, 2022 at 10:01:41AM -0400, Andrea Bolognani wrote: >> On Mon, May 02, 2022 at 01:46:23PM +0200, Markus Armbruster wrote: >> > Andrea Bolognani writes: >> > >> > The wire protocol would still retain the unappealing >> > >> > name, but at least client libraries could hide the >> > >> > uglyness from users. >> > >> >> > >> At the price of mild inconsistency between the library >> > >> interface and QMP. >> > > >> > > That's fine, and in fact it already happens all the time >> > > when QAPI names (log-append) are translated to C >> > > identifiers (log_append). >> > >> > There's a difference between trivial translations like >> > "replace '-' by '_'" and arbitrary replacement like the one >> > for enumeration constants involving 'prefix'. >> >> Fair enough. >> >> I still feel that 1) users of a language SDK will ideally not >> need to look at the QAPI schema or wire chatter too often and > > That should be the preference, yes. > >> 2) even when that ends up being necessary, figuring out that >> LogAppend and logappend are the same thing is not going to be >> an unreasonable hurdle, especially when the status quo would be >> to work with Logappend instead. > > If user really needs to leave their ecosystem in order to check > an alias, I hope we are already considering this a corner case. > Still, if we are thinking about multiple languages communicating > with QEMU, it is reasonable to consider the necessity of some > docs page where they can easily grep/search for the types, alias, > examples, etc. IMHO, this should be a long term goal and not a > blocker... I can volunteer on working on that later. > >> > > The point is that, if we want to provide a language >> > > interface that feels natural, we need a way to mark parts >> > > of a QAPI symbol's name in a way that makes it possible for >> > > the generator to know they're acronyms and treat them in an >> > > appropriate, language-specific manner. >> > >> > It's not just acronyms. Consider IAmALittleTeapot. If you >> > can assume that only beginning of words are capitalized, even >> > for acronyms, you can split this into words without trouble. >> > You can't recover correct case, though: "i am a little >> > teapot" is wrong. >> >> Is there any scenario in which we would care though? We're in >> the business of translating identifiers from one machine >> representation to another, so once it has been split up >> correctly into the words that compose it (which in your example >> above it has) then we don't really care about anything else >> unless acronyms are involved. >> >> In other words, we can obtain the list of words "i am a little >> teapot" programmatically both from IAmALittleTeapot and >> i-am-a-little-teapot, and in both cases we can then generate >> IAmALittleTeapot or I_AM_A_LITTLE_TEAPOT or >> i_am_a_little_teapot or whatever is appropriate for the context >> and target language, but the fact that in a proper English >> sentence "I" would have to be capitalized doesn't really enter >> the picture. >> >> > "Split before capital letter" falls apart when you have >> > characters that cannot be capitalized: Point3d. >> > >> > Camel case is hopeless. >> >> I would argue that it works quite well for most scenarios, but >> there are some corner cases where it's clearly not good enough. >> If we can define a way to clue in the generator about "Point3d" >> having to be interpreted as "point 3d" and "VNCProps" as "vnc >> props", then we are golden. That wouldn't be necessary for >> simple cases that are already handled correctly. >> >> A more radical idea would be to start using dash-notation for >> types too. That'd remove the word splitting issue altogether, >> at the cost of the schema being (possibly) harder to read and >> more distanced from the generated code. >> >> You'd still only be able to generate VncProps from vnc-props >> though. >> >> > > The obvious way to implement this would be with an >> > > annotation along the lines of the one I proposed. Other >> > > ideas? >> > >> > I'm afraid having the schema spell out names in multiple >> > naming conventions could be onerous. How many names will >> > need it? >> >> I don't have hard data on this. I could try extracting it, but >> that might end up being a bigger job than I had anticipated. > > The only way to know is by checking /o\ > I'll give it a shot. I append a quick hack to find names in a QAPI schema, and sort them into buckets "lc" (lower-with-hyphen), "uc" (UPPER_WITH_UNDERSCORE), "cc" (CamelCase where words are obvious), and "mc" (everything else, mostly CamelCase where words aren't obvious). Note that this ignores naming rule violations such as upper case enum member EAX. >> My guess is that the number of cases where the naive algorithm >> can't split words correctly is relatively small compared to the >> size of the entire QAPI schema. Fair warning: I have made >> incorrect guesses in the past ;) >> >> > Times how many
Re: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
Hi, On Mon, May 02, 2022 at 10:01:41AM -0400, Andrea Bolognani wrote: > On Mon, May 02, 2022 at 01:46:23PM +0200, Markus Armbruster wrote: > > Andrea Bolognani writes: > > >> > The wire protocol would still retain the unappealing > > >> > name, but at least client libraries could hide the > > >> > uglyness from users. > > >> > > >> At the price of mild inconsistency between the library > > >> interface and QMP. > > > > > > That's fine, and in fact it already happens all the time > > > when QAPI names (log-append) are translated to C > > > identifiers (log_append). > > > > There's a difference between trivial translations like > > "replace '-' by '_'" and arbitrary replacement like the one > > for enumeration constants involving 'prefix'. > > Fair enough. > > I still feel that 1) users of a language SDK will ideally not > need to look at the QAPI schema or wire chatter too often and That should be the preference, yes. > 2) even when that ends up being necessary, figuring out that > LogAppend and logappend are the same thing is not going to be > an unreasonable hurdle, especially when the status quo would be > to work with Logappend instead. If user really needs to leave their ecosystem in order to check an alias, I hope we are already considering this a corner case. Still, if we are thinking about multiple languages communicating with QEMU, it is reasonable to consider the necessity of some docs page where they can easily grep/search for the types, alias, examples, etc. IMHO, this should be a long term goal and not a blocker... I can volunteer on working on that later. > > > The point is that, if we want to provide a language > > > interface that feels natural, we need a way to mark parts > > > of a QAPI symbol's name in a way that makes it possible for > > > the generator to know they're acronyms and treat them in an > > > appropriate, language-specific manner. > > > > It's not just acronyms. Consider IAmALittleTeapot. If you > > can assume that only beginning of words are capitalized, even > > for acronyms, you can split this into words without trouble. > > You can't recover correct case, though: "i am a little > > teapot" is wrong. > > Is there any scenario in which we would care though? We're in > the business of translating identifiers from one machine > representation to another, so once it has been split up > correctly into the words that compose it (which in your example > above it has) then we don't really care about anything else > unless acronyms are involved. > > In other words, we can obtain the list of words "i am a little > teapot" programmatically both from IAmALittleTeapot and > i-am-a-little-teapot, and in both cases we can then generate > IAmALittleTeapot or I_AM_A_LITTLE_TEAPOT or > i_am_a_little_teapot or whatever is appropriate for the context > and target language, but the fact that in a proper English > sentence "I" would have to be capitalized doesn't really enter > the picture. > > > "Split before capital letter" falls apart when you have > > characters that cannot be capitalized: Point3d. > > > > Camel case is hopeless. > > I would argue that it works quite well for most scenarios, but > there are some corner cases where it's clearly not good enough. > If we can define a way to clue in the generator about "Point3d" > having to be interpreted as "point 3d" and "VNCProps" as "vnc > props", then we are golden. That wouldn't be necessary for > simple cases that are already handled correctly. > > A more radical idea would be to start using dash-notation for > types too. That'd remove the word splitting issue altogether, > at the cost of the schema being (possibly) harder to read and > more distanced from the generated code. > > You'd still only be able to generate VncProps from vnc-props > though. > > > > The obvious way to implement this would be with an > > > annotation along the lines of the one I proposed. Other > > > ideas? > > > > I'm afraid having the schema spell out names in multiple > > naming conventions could be onerous. How many names will > > need it? > > I don't have hard data on this. I could try extracting it, but > that might end up being a bigger job than I had anticipated. The only way to know is by checking /o\ I'll give it a shot. > My guess is that the number of cases where the naive algorithm > can't split words correctly is relatively small compared to the > size of the entire QAPI schema. Fair warning: I have made > incorrect guesses in the past ;) > > > Times how many naming conventions? > > Yeah, I don't think requiring all possible permutations to be > spelled out in the schema is the way to go. That's exactly why > my proposal was to offer a way to inject the semantic > information that the parser can't figure out itself. > > Once you have a way to inform the generator that "VNCProps" is > made of the two words "vnc" and "props", and that "vnc" is an > acronym, then it can generate an identifier appropriate for the > target
Re: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
Hi, Thanks for the quick review Markus. Sorry for taking quite a bit of time to get back to you. On Tue, Apr 26, 2022 at 01:14:28PM +0200, Markus Armbruster wrote: > Victor Toso writes: > > > Hi, > > > > Happy 1st April. Not a joke :) /* ugh, took me too long to send */ > > > > This series is about adding a generator in scripts/qapi to produce > > Go data structures that can be used to communicate with QEMU over > > QMP. > > > > > > * Why Go? > > > > There are quite a few Go projects that interact with QEMU over QMP > > and they endup using a mix of different libraries with their own > > code. > > > > > > ** Which projects? > > > > The ones I've found so far: > > > > - podman machine > > https://github.com/containers/podman/tree/main/pkg/machine/qemu > > > > - kata-containers (govmm) > > > > https://github.com/kata-containers/kata-containers/tree/main/src/runtime/pkg/govmm > > > > - lxd > > https://github.com/lxc/lxd/tree/master/lxd/instance/drivers > > > > - kubevirt (plain json strings) > > https://github.com/kubevirt/kubevirt > > > > (let me know if you know others) > > > > > > * But Why? > > > > I'm particularly interested in 3 out of 4 of the projects above and > > only Kubevirt uses libvirt to handle QEMU. That means that every > > QEMU releases where a QMP command, event or other data struct is > > added, removed or changed, those projects need to check what changed > > in QEMU and then address those changes in their projects, if needed. > > > > The idea behind generating Go data structures is that we can keep a > > Go module which can have releases that follow QEMU releases. > > We need to look at "following the QEMU releases" a bit more > closely. > > Merging your patches gives us the capability to generate a Go > interface to HEAD's version of QMP. Right, just to put it up here, it should be expected that the qapi-go project is to have releases that match QEMU's release. > The obvious way for an out-of-tree Go program to use this > generated Go interface is to build with a specific version of > it. It can then talk QMP to any compatible QEMU version. > > Compatibility with older QEMUs is not assured: stuff added > since is present on the Go QMP client end, but not on the QEMU > QMP server end. > > Compatibility with newer QEMUs is subject to our deprecation > policy: > > In general features are intended to be supported > indefinitely once introduced into QEMU. In the event that > a feature needs to be removed, it will be listed in this > section. The feature will remain functional for the > release in which it was deprecated and one further release. > After these two releases, the feature is liable to be > removed. > > So, if you stay away from deprecated stuff, you're good for two > more releases at least. > > Does this work for the projects you have in mind? It depends on how the project will be using qapi-go, so I can't say for them all. There are projects that will be targeting specific QEMU version (e.g: Kubevirt, Kata containers) and for those, I think they don't mind only bumping qapi-go when they plan to change the target QEMU version or perhaps to keep separated binary versions for a limited amount of time (just my guess). Some other projects like Podman, will likely be talking with the running version of QEMU they have in that host. The possibilities are quite broad here. > Aside: graceful degradation in case of incompatibility seems > desirable. I agree. I haven't thought much on how to handle those scenarios yet and suggestions are more than welcomed. I know that, those projects are already hardcoding commands and expected return types by hand. My first goal is to provide well defined types, QAPI/QMP compliant, with easy to reach documentation as provided by QAPI docs. I expect that, step by step, we can improve things all around but I don't expect it to be done all at once. > > The project that uses this Go module, only need to bump the > > module version and it shall receive all the changes in their > > own vendored code base. > > Ideally, incompatible changes that affect the Go program show > up as compile errors. Do they? It depends. A field/type that they were using but is removed, for sure a compile-time error. What about a new mandatory field? If you run the unit tests in qapi-go, there will be failures but it wouldn't be compile time error. If user doesn't define a mandatory field in a Go struct, the default value is used for the over-the-wire message. Perhaps some tooling can be developed to help users check that something they are using has changed. I'll look into it. > > * Status > > > > There are a few rough edges to work on but this is usable. The major > > thing I forgot to add is handling Error from Commands. It'll be the > > first thing I'll work on next week. > > > > If you want to start using this Today you can fetch it in at > > > > https://gitlab.com/victortoso/qapi-go/ > > > > There are quite
Re: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
Hi!
Sorry for taking some time to reply.
On Tue, Apr 19, 2022 at 11:12:28AM -0700, Andrea Bolognani wrote:
> On Sat, Apr 02, 2022 at 12:40:56AM +0200, Victor Toso wrote:
> > Thanks for taking a look, let me know if you have questions, ideas
> > or suggestions.
>
> Full disclosure: I have only given the actual implementation a very
> cursory look so far, and I've focused on the generated Go API
> instead.
>
> Overall things look pretty good.
Glad to hear.
> One concern that I have is about naming struct members: things like
> SpiceInfo.MouseMode and most others are translated from the QAPI
> schema exactly the way you'd expect them, but for example
> ChardevCommon.Logappend doesn't look quite right. Of course there's
> no way to programmatically figure out what to capitalize, but maybe
> there's room for adding this kind of information in the form of
> additional annotations or something like that? Same for the various
> structs or members that have unexpectedly-capitalized "Tls" or "Vnc"
> in them.
>
> To be clear, I don't think the above is a blocker - just something to
> be aware of, and think about.
There was a good discussion around this with Markus so I don't
want to break it in another thread.
I'm happy that you have found those inconsistencies. I'll reply
on the other thread about it but I don't mind working towards
fixing it, either at code generator level or at QAPI level.
> My biggest concern is about the interface offered for commands.
>
> Based on the example you have in the README and how commands are
> defined, invoking (a simplified version of) the trace-event-get-state
> command would look like
>
> cmd := Command{
> Name: "trace-event-get-state",
> Arg: TraceEventGetStateCommand{
> Name: "qemu_memalign",
> },
> }
> qmp_input, _ := json.Marshal()
> // qmp_input now contains
> //
> {"execute":"trace-event-get-state","arguments":{"name":"qemu_memalign"}}
> // do something with it
>
> qmp_output :=
> ([]byte)(`{"return":{"name":"qemu_memalign","state":"disabled"}}`)
> ret := cmd.GetReturnType()
> _ = json.Unmarshal(qmp_output, )
> // ret is a CommandResult instance whose Value member can be cast
> // to a TraceEventInfo struct
>
> First of all, from an application's point of view there are way too
> many steps involved:
It can actually get worse. I've used a lot of nested struct to
define a Base type for a given Type. In Go, If you try to
initialize a Type that has a nested Struct, you'll need to use
the nested struct Type as field name and this is too verbose.
See https://github.com/golang/go/issues/29438 (merged with:
https://github.com/golang/go/issues/12854)
The main reason that I kept it is because it maps very well with
the over-the-wire protocol.
> performing this operation should really be as
> simple as
>
> ret, _ := qmp.TraceEventGetState("qemu_memalign")
> // ret is a TraceEventInfo instance
>
> That's the end state we should be working towards.
>
> Of course that assumes that the "qmp" object knows where the
> QMP socket is, knows how to talk the QMP protocol,
> transparently deals with serializing and deserializing data...
> Plus, in some case you might want to deal with the wire
> transfer yourself in an application-specific manner. So it
> makes sense to have the basic building blocks available and
> then build the more ergonomic SDK on top of that - with only
> the first part being in scope for this series.
Right. Indeed, I thought a bit about what I want to fit into the
code generator that will reside in QEMU and what we might want to
develop on top of that.
The goal for this series really is generating the data types that
can be converted to/from QMP messages.
I completely agree with the message below: Type validation is
important at this stage.
> Even with that in mind, the current interface is IMO
> problematic because of its almost complete lack of type safety.
> Both Command.Arg and CommandResult.Value are of type Any and
> CommandBase.Name, which is used to drive the JSON unmarshal
> logic as well as ending up on the wire when executing a
> command, is just a plain string.
>
> I think the low-level interface should look more like
>
> cmd := TraceEventGetStateCommand{
> Name: "qemu_memalign",
> }
> qmp_input, _ := json.Marshal()
> // qmp_input looks the same as before
That isn't too hard to implement and I've started with this
design at first. Each QAPI Command can implement a method Name()
which returns the over-the-wire name for that Command.
I'm not yet sure if this is preferable over some other syntactic
sugar function that might be generated (this series) or the next
layer that will be on top of this.
But I agree with you that it should be improved before reaching
actual Applications.
> qmp_output :=
> ([]byte)(`{"return":{"name":"qemu_memalign","state":"disabled"}}`)
> ret := TraceEventInfo{}
> _ = json.Unmarshal(qmp_output, )
> // ret is a TraceEventInfo
Re: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
Am 03.05.2022 um 11:40 hat Andrea Bolognani geschrieben: > So the issues preventing us from producing a "perfect" Go API are > > 1. inconsistent capitalization in type names > >-> could be addressed by simply changing the schema, as type > names do not travel on the wire > > 2. missing dashes in certain command/member names > >-> leads to Incorrectcamelcase. Kevin's work is supposed to > address this I am surprised that Markus pointed you to my aliases work because the conclusion after reviewing my last attempt was that he doesn't want the feature in the proposed form and he wasn't sure if he wants it at all. To be honest, I considered this one dead. Even if magically a solution appeared that everyone agreed to, don't hold your breath, I'm working on different things now and not planning to get back to QAPI stuff before 7.1 (and preferably not before meeting Markus in person and discussing the design before I spend the time to produce another thing that will never be merged). I am hoping to get back to QAPI later, but I was mostly planning with continuing the QOM integration, not aliases. Kevin
Re: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
On Tue, May 03, 2022 at 09:57:27AM +0200, Markus Armbruster wrote: > Andrea Bolognani writes: > > I still feel that 1) users of a language SDK will ideally not need to > > look at the QAPI schema or wire chatter too often > > I think the most likely point of contact is the QEMU QMP Reference > Manual. Note that there isn't anything preventing us from including the original QAPI name in the documentation for the corresponding Go symbol, or even a link to the reference manual. So we could make jumping from the Go API documentation, which is what a Go programmer will be looking at most of the time, to the QMP documentation pretty much effortless. > My point is: a name override feature like the one you propose needs to > be used with discipline and restraint. Adds to reviewers' mental load. > Needs to be worth it. I'm not saying it isn't, I'm just pointing out a > cost. Yeah, I get that. Note that I'm not suggesting it should be possible for a name to be completely overridden - I just want to make it possible for a human to provide the name parsing algorithm solutions to those problems it can't figure out on its own. We could prevent that feature from being misused by verifying that the symbol the annotation is attached to can be derived from the list of words provided. That way, something like # SOMEName (completely-DIFFERENT-name) would be rejected and we would avoid misuse. > Wild idea: assume all lower case, but keep a list of exceptions. That could actually work reasonably well for QEMU because we only need to handle correctly what's in the schema, not arbitrary input. There's always the risk of the list of exceptions getting out of sync with the needs of the schema, but there's similarly no guarantee that annotations are going to be introduced when they are necessary, so it's mostly a wash. The only slight advantage of the annotation approach would be that it might be easier to notice it being missing because it's close to the name it refers to, while the list of exceptions is tucked away in a script far away from it. > The QAPI schema language uses three naming styles: > > * lower-case-with-hyphens for command and member names > > Many names use upper case and '_'. See pragma command-name-exceptions > and member-name-exceptions. Looking at the output generated by Victor's WIP script, it looks like these are already handled as nicely as those that don't fall under any exception. > Some (many?) names lack separators between words (example: logappend). > > * UPPER_CASE_WITH_UNDERSCORE for event names > > * CamelCase for type names > > Capitalization of words is inconsistent in places (example: VncInfo > vs. DisplayReloadOptionsVNC). > > What style conversions will we need for Go? Any other conversions come > to mind? > > What problems do these conversions have? Go uses CamelCase for pretty much everything: types, methods, constants... There's one slight wrinkle, in that the case of the first letter decides whether it's going to be a PublicName or a privateName. We can't do anything about that, but it shouldn't really affect us that much because we'll want all QAPI names to be public. So the issues preventing us from producing a "perfect" Go API are 1. inconsistent capitalization in type names -> could be addressed by simply changing the schema, as type names do not travel on the wire 2. missing dashes in certain command/member names -> leads to Incorrectcamelcase. Kevin's work is supposed to address this 3. inability to know which parts of a lower-case-name or UPPER_CASE_NAME are acronyms or are otherwise supposed to be capitalized in a specific way -> leads to WeirdVncAndDbusCapitalization. There's currently no way, either implemented or planned, to avoid this In addition to these I'm also thinking that QKeyCode and all the QCrypto stuff should probably lose their prefixes. Note that 3 shouldn't be an issue for Rust and addressing 1 would actually make things worse for that language, because at the moment at least *some* of the types follow its expected naming rules :) > > Revised proposal for the annotation: > > > > ns:word-WORD-WoRD-123Word > > > > Words are always separated by dashes; "regular" words are entirely > > lowercase, while the presence of even a single uppercase letter in a > > word denotes the fact that its case should be preserved when the > > naming conventions of the target language allow that. > > Is a word always capitalized the same for a single target language? Or > could capitalization depend on context? I'm not aware of any language that would adopt more than a single style of capitalization, outside of course the obvious lower_case_name or UPPER_CASE_NAME scenarios where the original capitalization stops being relevant. -- Andrea Bolognani / Red Hat / Virtualization
Re: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
Andrea Bolognani writes: > On Mon, May 02, 2022 at 01:46:23PM +0200, Markus Armbruster wrote: >> Andrea Bolognani writes: >> >> > The wire protocol would still retain the unappealing name, but at >> >> > least client libraries could hide the uglyness from users. >> >> >> >> At the price of mild inconsistency between the library interface and >> >> QMP. >> > >> > That's fine, and in fact it already happens all the time when QAPI >> > names (log-append) are translated to C identifiers (log_append). >> >> There's a difference between trivial translations like "replace '-' by >> '_'" and arbitrary replacement like the one for enumeration constants >> involving 'prefix'. > > Fair enough. > > I still feel that 1) users of a language SDK will ideally not need to > look at the QAPI schema or wire chatter too often I think the most likely point of contact is the QEMU QMP Reference Manual. > even when > that ends up being necessary, figuring out that LogAppend and > logappend are the same thing is not going to be an unreasonable > hurdle, especially when the status quo would be to work with > Logappend instead. Yes, it's "mild inconsistency", hardly an unreasonable hurdle. I think it gets in the way mostly when searching documentation. Differences in case are mostly harmless, just use case-insensitive search. Use of '_' vs '-' would also be harmless (just do the replacement), if the use of '-' in the schema was consistent. Sadly, it's not, and that's already a perennial low-level annoyance. My point is: a name override feature like the one you propose needs to be used with discipline and restraint. Adds to reviewers' mental load. Needs to be worth it. I'm not saying it isn't, I'm just pointing out a cost. >> > The point is that, if we want to provide a language interface that >> > feels natural, we need a way to mark parts of a QAPI symbol's name in >> > a way that makes it possible for the generator to know they're >> > acronyms and treat them in an appropriate, language-specific manner. >> >> It's not just acronyms. Consider IAmALittleTeapot. If you can assume >> that only beginning of words are capitalized, even for acronyms, you can >> split this into words without trouble. You can't recover correct case, >> though: "i am a little teapot" is wrong. > > Is there any scenario in which we would care though? We're in the > business of translating identifiers from one machine representation > to another, so once it has been split up correctly into the words > that compose it (which in your example above it has) then we don't > really care about anything else unless acronyms are involved. > > In other words, we can obtain the list of words "i am a little > teapot" programmatically both from IAmALittleTeapot and > i-am-a-little-teapot, and in both cases we can then generate > IAmALittleTeapot or I_AM_A_LITTLE_TEAPOT or i_am_a_little_teapot or > whatever is appropriate for the context and target language, but the > fact that in a proper English sentence "I" would have to be > capitalized doesn't really enter the picture. My point is that conversion from CamelCase has two sub-problems: splitting words and recovering case. Splitting words is easy when exactly the beginning of words is capitalized. Recovering case is guesswork. Most English words are all lower case, but some start with a capital letter, and acronyms are all caps. Wild idea: assume all lower case, but keep a list of exceptions. >> "Split before capital letter" falls apart when you have characters that >> cannot be capitalized: Point3d. >> >> Camel case is hopeless. > > I would argue that it works quite well for most scenarios, but there > are some corner cases where it's clearly not good enough. If we can > define a way to clue in the generator about "Point3d" having to be > interpreted as "point 3d" and "VNCProps" as "vnc props", then we are > golden. That wouldn't be necessary for simple cases that are already > handled correctly. Hyphenization rules? *Cough* *cough* > A more radical idea would be to start using dash-notation for types > too. That'd remove the word splitting issue altogether, at the cost > of the schema being (possibly) harder to read and more distanced from > the generated code. Yes. > You'd still only be able to generate VncProps from vnc-props though. > >> > The obvious way to implement this would be with an annotation along >> > the lines of the one I proposed. Other ideas? >> >> I'm afraid having the schema spell out names in multiple naming >> conventions could be onerous. How many names will need it? > > I don't have hard data on this. I could try extracting it, but that > might end up being a bigger job than I had anticipated. I figure extracting is easier for me than for you. But let's have a closer look at the job at hand first. The QAPI schema language uses three naming styles: * lower-case-with-hyphens for command and member names
Re: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
On Mon, May 02, 2022 at 01:46:23PM +0200, Markus Armbruster wrote: > Andrea Bolognani writes: > >> > The wire protocol would still retain the unappealing name, but at > >> > least client libraries could hide the uglyness from users. > >> > >> At the price of mild inconsistency between the library interface and > >> QMP. > > > > That's fine, and in fact it already happens all the time when QAPI > > names (log-append) are translated to C identifiers (log_append). > > There's a difference between trivial translations like "replace '-' by > '_'" and arbitrary replacement like the one for enumeration constants > involving 'prefix'. Fair enough. I still feel that 1) users of a language SDK will ideally not need to look at the QAPI schema or wire chatter too often and 2) even when that ends up being necessary, figuring out that LogAppend and logappend are the same thing is not going to be an unreasonable hurdle, especially when the status quo would be to work with Logappend instead. > > The point is that, if we want to provide a language interface that > > feels natural, we need a way to mark parts of a QAPI symbol's name in > > a way that makes it possible for the generator to know they're > > acronyms and treat them in an appropriate, language-specific manner. > > It's not just acronyms. Consider IAmALittleTeapot. If you can assume > that only beginning of words are capitalized, even for acronyms, you can > split this into words without trouble. You can't recover correct case, > though: "i am a little teapot" is wrong. Is there any scenario in which we would care though? We're in the business of translating identifiers from one machine representation to another, so once it has been split up correctly into the words that compose it (which in your example above it has) then we don't really care about anything else unless acronyms are involved. In other words, we can obtain the list of words "i am a little teapot" programmatically both from IAmALittleTeapot and i-am-a-little-teapot, and in both cases we can then generate IAmALittleTeapot or I_AM_A_LITTLE_TEAPOT or i_am_a_little_teapot or whatever is appropriate for the context and target language, but the fact that in a proper English sentence "I" would have to be capitalized doesn't really enter the picture. > "Split before capital letter" falls apart when you have characters that > cannot be capitalized: Point3d. > > Camel case is hopeless. I would argue that it works quite well for most scenarios, but there are some corner cases where it's clearly not good enough. If we can define a way to clue in the generator about "Point3d" having to be interpreted as "point 3d" and "VNCProps" as "vnc props", then we are golden. That wouldn't be necessary for simple cases that are already handled correctly. A more radical idea would be to start using dash-notation for types too. That'd remove the word splitting issue altogether, at the cost of the schema being (possibly) harder to read and more distanced from the generated code. You'd still only be able to generate VncProps from vnc-props though. > > The obvious way to implement this would be with an annotation along > > the lines of the one I proposed. Other ideas? > > I'm afraid having the schema spell out names in multiple naming > conventions could be onerous. How many names will need it? I don't have hard data on this. I could try extracting it, but that might end up being a bigger job than I had anticipated. My guess is that the number of cases where the naive algorithm can't split words correctly is relatively small compared to the size of the entire QAPI schema. Fair warning: I have made incorrect guesses in the past ;) > Times how many naming conventions? Yeah, I don't think requiring all possible permutations to be spelled out in the schema is the way to go. That's exactly why my proposal was to offer a way to inject the semantic information that the parser can't figure out itself. Once you have a way to inform the generator that "VNCProps" is made of the two words "vnc" and "props", and that "vnc" is an acronym, then it can generate an identifier appropriate for the target language without having to spell out anywhere that such an identifier would be VNCProps for Go and VncProps for Rust. By the way, while looking around I realized that we also have to take into account things like D-Bus: the QAPI type ChardevDBus, for example, would probably translate verbatim to Go but have to be changed to ChardevDbus for Rust. Fun :) Revised proposal for the annotation: ns:word-WORD-WoRD-123Word Words are always separated by dashes; "regular" words are entirely lowercase, while the presence of even a single uppercase letter in a word denotes the fact that its case should be preserved when the naming conventions of the target language allow that. > Another issue: the fancier the translation from schema name to > language-specific name gets, the harder it becomes to find one from the > other. That's
Re: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
Andrea Bolognani writes: > On Mon, May 02, 2022 at 09:21:36AM +0200, Markus Armbruster wrote: >> Andrea Bolognani writes: >> > The wire protocol would still retain the unappealing name, but at >> > least client libraries could hide the uglyness from users. >> >> At the price of mild inconsistency between the library interface and >> QMP. > > That's fine, and in fact it already happens all the time when QAPI > names (log-append) are translated to C identifiers (log_append). There's a difference between trivial translations like "replace '-' by '_'" and arbitrary replacement like the one for enumeration constants involving 'prefix'. >> We could clean up QMP if we care, keeping around the old names for >> compatibility. See also Kevin's work on QAPI aliases. Which is much >> more ambitious, though. > > I wasn't aware of that effort. Personally I'm always in favor of > cleaning up inconsistencies, so I am automatically a fan :) > > That said, the idea of exposing a sub-par Go API until such massive > undertaking can be completed is not terribly appealing. Point. > And it would > not address every facet of the issue (see below). > >> > Capitalization of these acronyms is inconsistent across the schema, >> >> Common issue with camel-cased compounds containing acronyms, because >> either way is ugly. > > Agreed :) But consistent ugliness is still preferable to inconsistent > ugliness. True. >> > with one of the two forms disagreeing with Go naming expectations. >> >> Pardon my ignorance: What are Go's expectations? > > Acronyms are usually all upper case: > > https://pkg.go.dev/net/http#ParseHTTPVersion > https://pkg.go.dev/net/http#ProxyURL > https://pkg.go.dev/crypto/tls#NewLRUClientSessionCache > > The same seems to be true of Python: > > https://docs.python.org/3/library/http.html#http.HTTPStatus > https://docs.python.org/3/library/urllib.error.html#urllib.error.URLError > > https://docs.python.org/3/library/xmlrpc.server.html#xmlrpc.server.SimpleXMLRPCServer > > Rust, on the other hand, seems to prefer only capitalizing the first > letter of a word, no matter if it's an acronym: > > https://doc.rust-lang.org/std/net/struct.TcpStream.html > https://doc.rust-lang.org/std/net/struct.Ipv4Addr.html > https://doc.rust-lang.org/std/ffi/struct.OsString.html Another strange game where the only winning move is not to play. > Whether different naming conventions are used for types, functions > and struct members is also language-dependent. Yes. >> > In this case we might be able to just change the schema without >> > introducing backwards compatibility issues, though? Type names are >> > not actually transmitted on the wire IIUC. >> >> Correct! > > That's great, but even if we decided to change all type names so that > the schema is internally consistent and follows a naming convention > that's reasonable for C, Go and Python, we'd still leave the Rust > interface looking weird... There's no one-size-fits-all name, > unfortunately. C will remain the primary customer for quite a while. It doesn't come with universally accepted naming conventions, so we made up our own. I think we have some wiggle room there. We could, for instance, decide to clean up the current inconsistent capitalization of acronyms in the QAPI schema to either style, TCPStream or TcpStream. Your point remains: some names will still look "weird" in some possible target languages. >> > Of course such annotations would only be necessary to deal with >> > identifiers that are not already following the expected naming >> > conventions and when MLAs are involved. >> >> Pardon my ignorance some more: what are MLAs? > > Multi Letter Acronyms. Which are actually just called "acronyms" I > guess? O:-) Well played! > The point is that, if we want to provide a language interface that > feels natural, we need a way to mark parts of a QAPI symbol's name in > a way that makes it possible for the generator to know they're > acronyms and treat them in an appropriate, language-specific manner. It's not just acronyms. Consider IAmALittleTeapot. If you can assume that only beginning of words are capitalized, even for acronyms, you can split this into words without trouble. You can't recover correct case, though: "i am a little teapot" is wrong. "Split before capital letter" falls apart when you have characters that cannot be capitalized: Point3d. Camel case is hopeless. > The obvious way to implement this would be with an annotation along > the lines of the one I proposed. Other ideas? I'm afraid having the schema spell out names in multiple naming conventions could be onerous. How many names will need it? Times how many naming conventions? Another issue: the fancier the translation from schema name to language-specific name gets, the harder it becomes to find one from the other.
Re: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
On Mon, May 02, 2022 at 09:21:36AM +0200, Markus Armbruster wrote: > Andrea Bolognani writes: > > The wire protocol would still retain the unappealing name, but at > > least client libraries could hide the uglyness from users. > > At the price of mild inconsistency between the library interface and > QMP. That's fine, and in fact it already happens all the time when QAPI names (log-append) are translated to C identifiers (log_append). > We could clean up QMP if we care, keeping around the old names for > compatibility. See also Kevin's work on QAPI aliases. Which is much > more ambitious, though. I wasn't aware of that effort. Personally I'm always in favor of cleaning up inconsistencies, so I am automatically a fan :) That said, the idea of exposing a sub-par Go API until such massive undertaking can be completed is not terribly appealing. And it would not address every facet of the issue (see below). > > Capitalization of these acronyms is inconsistent across the schema, > > Common issue with camel-cased compounds containing acronyms, because > either way is ugly. Agreed :) But consistent ugliness is still preferable to inconsistent ugliness. > > with one of the two forms disagreeing with Go naming expectations. > > Pardon my ignorance: What are Go's expectations? Acronyms are usually all upper case: https://pkg.go.dev/net/http#ParseHTTPVersion https://pkg.go.dev/net/http#ProxyURL https://pkg.go.dev/crypto/tls#NewLRUClientSessionCache The same seems to be true of Python: https://docs.python.org/3/library/http.html#http.HTTPStatus https://docs.python.org/3/library/urllib.error.html#urllib.error.URLError https://docs.python.org/3/library/xmlrpc.server.html#xmlrpc.server.SimpleXMLRPCServer Rust, on the other hand, seems to prefer only capitalizing the first letter of a word, no matter if it's an acronym: https://doc.rust-lang.org/std/net/struct.TcpStream.html https://doc.rust-lang.org/std/net/struct.Ipv4Addr.html https://doc.rust-lang.org/std/ffi/struct.OsString.html Whether different naming conventions are used for types, functions and struct members is also language-dependent. > > In this case we might be able to just change the schema without > > introducing backwards compatibility issues, though? Type names are > > not actually transmitted on the wire IIUC. > > Correct! That's great, but even if we decided to change all type names so that the schema is internally consistent and follows a naming convention that's reasonable for C, Go and Python, we'd still leave the Rust interface looking weird... There's no one-size-fits-all name, unfortunately. > > Of course such annotations would only be necessary to deal with > > identifiers that are not already following the expected naming > > conventions and when MLAs are involved. > > Pardon my ignorance some more: what are MLAs? Multi Letter Acronyms. Which are actually just called "acronyms" I guess? O:-) The point is that, if we want to provide a language interface that feels natural, we need a way to mark parts of a QAPI symbol's name in a way that makes it possible for the generator to know they're acronyms and treat them in an appropriate, language-specific manner. The obvious way to implement this would be with an annotation along the lines of the one I proposed. Other ideas? -- Andrea Bolognani / Red Hat / Virtualization
Re: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
Andrea Bolognani writes:
> On Thu, Apr 28, 2022 at 03:50:55PM +0200, Markus Armbruster wrote:
>> Andrea Bolognani writes:
>> > One concern that I have is about naming struct members: things like
>> > SpiceInfo.MouseMode and most others are translated from the QAPI
>> > schema exactly the way you'd expect them, but for example
>> > ChardevCommon.Logappend doesn't look quite right.
>>
>> It doesn't look quite right in the QAPI schema, either: @logappend. If
>> it was @log-append, as it should, then it would get translated to
>> LogAppend, I guess.
>>
>> Fixing up style isn't a code generator's job.
>
> I agree that the generator shouldn't take too many liberties when
> translating names, and specifically should never attempt to figure
> out that @logappend should have been @log-append instead.
>
> What I was thinking of was more along the lines of, can we change the
> schema so that the proper name is available to the generator without
> breaking the wire protocol? Maybe something like
>
> ##
> # ChardevCommon:
> #
> # @logappend (rename @log-append): ...
> ##
>
> That way the generator would have access to both information, and
> would thus be able to generate
>
> type ChardevCommon struct {
> LogAppend *bool `json:"logappend,omitempty"`
> }
>
> The wire protocol would still retain the unappealing name, but at
> least client libraries could hide the uglyness from users.
At the price of mild inconsistency between the library interface and
QMP.
We could clean up QMP if we care, keeping around the old names for
compatibility. See also Kevin's work on QAPI aliases. Which is much
more ambitious, though.
>> > Same for the various
>> > structs or members that have unexpectedly-capitalized "Tls" or "Vnc"
>> > in them.
>>
>> Examples?
>
> A perfect one is TlsCredsProperties, whose endpoint member is of type
> QCryptoTLSCredsEndpoint.
>
> On the VNC front, we have SetPasswordOptionsVnc but also
> DisplayReloadOptionsVNC.
>
> There's plenty more, but this should be illustrative enough already.
> Capitalization of these acronyms is inconsistent across the schema,
Common issue with camel-cased compounds containing acronyms, because
either way is ugly.
> with one of the two forms disagreeing with Go naming expectations.
Pardon my ignorance: What are Go's expectations?
> In this case we might be able to just change the schema without
> introducing backwards compatibility issues, though? Type names are
> not actually transmitted on the wire IIUC.
Correct!
>> > but maybe
>> > there's room for adding this kind of information in the form of
>> > additional annotations or something like that?
>>
>> We did for enumeration types: 'prefix' overrides the TYPE_NAME prefix.
>> I fear this was a mistake.
>
> This might be an oversimplification, but I think we might be able to
> solve all of these issues with a single annotation in the form
>
> namespace:word-MLA-other-words
>
> So for example QCryptoTLSCredsEndpoint would be annotated with
>
> q:crypto-TLS-creds-endpoint
>
> and each generator would have enough information to produce
> identifiers that fit into the corresponding language, such as
>
> qcrypto_tls_creds_endpoint
> CryptoTlsCredsEndpoint
>
> or whatever else.
>
> Of course such annotations would only be necessary to deal with
> identifiers that are not already following the expected naming
> conventions and when MLAs are involved.
Pardon my ignorance some more: what are MLAs?
Re: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
On Thu, Apr 28, 2022 at 03:50:55PM +0200, Markus Armbruster wrote:
> Andrea Bolognani writes:
> > One concern that I have is about naming struct members: things like
> > SpiceInfo.MouseMode and most others are translated from the QAPI
> > schema exactly the way you'd expect them, but for example
> > ChardevCommon.Logappend doesn't look quite right.
>
> It doesn't look quite right in the QAPI schema, either: @logappend. If
> it was @log-append, as it should, then it would get translated to
> LogAppend, I guess.
>
> Fixing up style isn't a code generator's job.
I agree that the generator shouldn't take too many liberties when
translating names, and specifically should never attempt to figure
out that @logappend should have been @log-append instead.
What I was thinking of was more along the lines of, can we change the
schema so that the proper name is available to the generator without
breaking the wire protocol? Maybe something like
##
# ChardevCommon:
#
# @logappend (rename @log-append): ...
##
That way the generator would have access to both information, and
would thus be able to generate
type ChardevCommon struct {
LogAppend *bool `json:"logappend,omitempty"`
}
The wire protocol would still retain the unappealing name, but at
least client libraries could hide the uglyness from users.
> > Same for the various
> > structs or members that have unexpectedly-capitalized "Tls" or "Vnc"
> > in them.
>
> Examples?
A perfect one is TlsCredsProperties, whose endpoint member is of type
QCryptoTLSCredsEndpoint.
On the VNC front, we have SetPasswordOptionsVnc but also
DisplayReloadOptionsVNC.
There's plenty more, but this should be illustrative enough already.
Capitalization of these acronyms is inconsistent across the schema,
with one of the two forms disagreeing with Go naming expectations.
In this case we might be able to just change the schema without
introducing backwards compatibility issues, though? Type names are
not actually transmitted on the wire IIUC.
> > but maybe
> > there's room for adding this kind of information in the form of
> > additional annotations or something like that?
>
> We did for enumeration types: 'prefix' overrides the TYPE_NAME prefix.
> I fear this was a mistake.
This might be an oversimplification, but I think we might be able to
solve all of these issues with a single annotation in the form
namespace:word-MLA-other-words
So for example QCryptoTLSCredsEndpoint would be annotated with
q:crypto-TLS-creds-endpoint
and each generator would have enough information to produce
identifiers that fit into the corresponding language, such as
qcrypto_tls_creds_endpoint
CryptoTlsCredsEndpoint
or whatever else.
Of course such annotations would only be necessary to deal with
identifiers that are not already following the expected naming
conventions and when MLAs are involved.
--
Andrea Bolognani / Red Hat / Virtualization
Re: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
Andrea Bolognani writes: > On Sat, Apr 02, 2022 at 12:40:56AM +0200, Victor Toso wrote: >> Thanks for taking a look, let me know if you have questions, ideas >> or suggestions. > > Full disclosure: I have only given the actual implementation a very > cursory look so far, and I've focused on the generated Go API > instead. > > Overall things look pretty good. > > One concern that I have is about naming struct members: things like > SpiceInfo.MouseMode and most others are translated from the QAPI > schema exactly the way you'd expect them, but for example > ChardevCommon.Logappend doesn't look quite right. It doesn't look quite right in the QAPI schema, either: @logappend. If it was @log-append, as it should, then it would get translated to LogAppend, I guess. Fixing up style isn't a code generator's job. > Of course there's > no way to programmatically figure out what to capitalize, Some case conversions are straightforward enough. For instance, the C code generator generates qapi_event_send_some_event() for event SOME_EVENT, and inclusion guard macro FILE_NAME_H for module file name file-name.json. No magic involved. Conversion from lower-case-with-dashes to CamelCase doesn't have to be magic, either. You just have to accept garbage-in (like missing dashes) will give you garbage-out. I wouldn't care too much; CamelCaseIsAnIllegibleMessAnyway. Conversion from CamelCase is always trouble, though. There is one instance so far: generating C enumeration constants. We want TYPE_NAME + '_' + MEMBER_NAME, where TYPE_NAME is the enumeration type's name converted from CamelCase to UPPER_CASE_WITH_UNDERSCORES, and MEMBER_NAME is the member name converted from lower-case-with-dashes to UPPER_CASE_WITH_UNDERSCORES. camel_to_upper() tries, but the result is often unappealing, surprising, or both. > but maybe > there's room for adding this kind of information in the form of > additional annotations or something like that? We did for enumeration types: 'prefix' overrides the TYPE_NAME prefix. I fear this was a mistake. >Same for the various > structs or members that have unexpectedly-capitalized "Tls" or "Vnc" > in them. Examples? > To be clear, I don't think the above is a blocker - just something to > be aware of, and think about. Yup. [...]
Re: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
Victor Toso writes: > Hi, > > Happy 1st April. Not a joke :) /* ugh, took me too long to send */ > > This series is about adding a generator in scripts/qapi to produce > Go data structures that can be used to communicate with QEMU over > QMP. > > > * Why Go? > > There are quite a few Go projects that interact with QEMU over QMP > and they endup using a mix of different libraries with their own > code. > > > ** Which projects? > > The ones I've found so far: > > - podman machine > https://github.com/containers/podman/tree/main/pkg/machine/qemu > > - kata-containers (govmm) > > https://github.com/kata-containers/kata-containers/tree/main/src/runtime/pkg/govmm > > - lxd > https://github.com/lxc/lxd/tree/master/lxd/instance/drivers > > - kubevirt (plain json strings) > https://github.com/kubevirt/kubevirt > > (let me know if you know others) > > > * But Why? > > I'm particularly interested in 3 out of 4 of the projects above and > only Kubevirt uses libvirt to handle QEMU. That means that every > QEMU releases where a QMP command, event or other data struct is > added, removed or changed, those projects need to check what changed > in QEMU and then address those changes in their projects, if needed. > > The idea behind generating Go data structures is that we can keep a > Go module which can have releases that follow QEMU releases. We need to look at "following the QEMU releases" a bit more closely. Merging your patches gives us the capability to generate a Go interface to HEAD's version of QMP. The obvious way for an out-of-tree Go program to use this generated Go interface is to build with a specific version of it. It can then talk QMP to any compatible QEMU version. Compatibility with older QEMUs is not assured: stuff added since is present on the Go QMP client end, but not on the QEMU QMP server end. Compatibility with newer QEMUs is subject to our deprecation policy: In general features are intended to be supported indefinitely once introduced into QEMU. In the event that a feature needs to be removed, it will be listed in this section. The feature will remain functional for the release in which it was deprecated and one further release. After these two releases, the feature is liable to be removed. So, if you stay away from deprecated stuff, you're good for two more releases at least. Does this work for the projects you have in mind? Aside: graceful degradation in case of incompatibility seems desirable. > The project that uses this Go module, only need to bump the module > version and it shall receive all the changes in their own vendored > code base. Ideally, incompatible changes that affect the Go program show up as compile errors. Do they? > * Status > > There are a few rough edges to work on but this is usable. The major > thing I forgot to add is handling Error from Commands. It'll be the > first thing I'll work on next week. > > If you want to start using this Today you can fetch it in at > > https://gitlab.com/victortoso/qapi-go/ > > There are quite a few tests that I took from the examples in the > qapi schema. Coverage using go's cover tool is giving `28.6% of > statements` > > I've uploaded the a static generated godoc output of the above Go > module here: > > > https://fedorapeople.org/~victortoso/qapi-go/rfc/victortoso.com/qapi-go/pkg/qapi/ > > > * License > > While the generator (golang.py in this series) is GPL v2, the I'd make it v2+, just to express my displeasure with the decision to make the initial QAPI generator v2 only for no good reason at all. > generated code needs to be compatible with other Golang projects, > such as the ones mentioned above. My intention is to keep a Go > module with a MIT license. Meh. Can't be helped, I guess. > * Disclaimer to reviewers > > This is my first serious python project so there'll be lots of > suggetions that I'll be happy to take and learn from. > > > Thanks for taking a look, let me know if you have questions, ideas > or suggestions. > > Cheers, > Victor
Re: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
On Tue, Apr 19, 2022 at 11:12:28AM -0700, Andrea Bolognani wrote:
> Dealing with errors and commands that don't have a return value might
> require us to have generic CommandResult wrapper after all, but we
> should really try as hard as we can to stick to type safe interfaces.
On second thought, this wouldn't actually need to be generic: we
could have something like
type TraceEventGetStateResult struct {
Result TraceEventInfo `json:"return"`
Error *Error `json:"error"`
}
and the caller would check that res.Error is nil before accessing
res.Result.
Commands for which a return value is not expected would just have the
Error part in their corresponding Result struct, and those that can
either return an object or nothing (are there actually any like
that?) could have a pointer as the Result member.
--
Andrea Bolognani / Red Hat / Virtualization
Re: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
On Sat, Apr 02, 2022 at 12:40:56AM +0200, Victor Toso wrote:
> Thanks for taking a look, let me know if you have questions, ideas
> or suggestions.
Full disclosure: I have only given the actual implementation a very
cursory look so far, and I've focused on the generated Go API
instead.
Overall things look pretty good.
One concern that I have is about naming struct members: things like
SpiceInfo.MouseMode and most others are translated from the QAPI
schema exactly the way you'd expect them, but for example
ChardevCommon.Logappend doesn't look quite right. Of course there's
no way to programmatically figure out what to capitalize, but maybe
there's room for adding this kind of information in the form of
additional annotations or something like that? Same for the various
structs or members that have unexpectedly-capitalized "Tls" or "Vnc"
in them.
To be clear, I don't think the above is a blocker - just something to
be aware of, and think about.
My biggest concern is about the interface offered for commands.
Based on the example you have in the README and how commands are
defined, invoking (a simplified version of) the trace-event-get-state
command would look like
cmd := Command{
Name: "trace-event-get-state",
Arg: TraceEventGetStateCommand{
Name: "qemu_memalign",
},
}
qmp_input, _ := json.Marshal()
// qmp_input now contains
// {"execute":"trace-event-get-state","arguments":{"name":"qemu_memalign"}}
// do something with it
qmp_output :=
([]byte)(`{"return":{"name":"qemu_memalign","state":"disabled"}}`)
ret := cmd.GetReturnType()
_ = json.Unmarshal(qmp_output, )
// ret is a CommandResult instance whose Value member can be cast
// to a TraceEventInfo struct
First of all, from an application's point of view there are way too
many steps involved: performing this operation should really be as
simple as
ret, _ := qmp.TraceEventGetState("qemu_memalign")
// ret is a TraceEventInfo instance
That's the end state we should be working towards.
Of course that assumes that the "qmp" object knows where the QMP
socket is, knows how to talk the QMP protocol, transparently deals
with serializing and deserializing data... Plus, in some case you
might want to deal with the wire transfer yourself in an
application-specific manner. So it makes sense to have the basic
building blocks available and then build the more ergonomic SDK on
top of that - with only the first part being in scope for this
series.
Even with that in mind, the current interface is IMO problematic
because of its almost complete lack of type safety. Both Command.Arg
and CommandResult.Value are of type Any and CommandBase.Name, which
is used to drive the JSON unmarshal logic as well as ending up on the
wire when executing a command, is just a plain string.
I think the low-level interface should look more like
cmd := TraceEventGetStateCommand{
Name: "qemu_memalign",
}
qmp_input, _ := json.Marshal()
// qmp_input looks the same as before
qmp_output :=
([]byte)(`{"return":{"name":"qemu_memalign","state":"disabled"}}`)
ret := TraceEventInfo{}
_ = json.Unmarshal(qmp_output, )
// ret is a TraceEventInfo instance
The advantages over the current implementation is that the compiler
will prevent you from doing something silly like passing the wrong
set of arguments to a commmand, and that the application has to
explicitly spell out what kind of object it expects to get as output.
I'm attaching an incomplete implementation that I used for playing
around. It's obviously too simplistic, but hopefully it will help
illustrate my point.
Dealing with errors and commands that don't have a return value might
require us to have generic CommandResult wrapper after all, but we
should really try as hard as we can to stick to type safe interfaces.
--
Andrea Bolognani / Red Hat / Virtualization
package main
import (
"encoding/json"
"fmt"
)
type TraceEventGetStateCommand struct {
Name string `json:"name"`
}
func (self *TraceEventGetStateCommand) MarshalJSON() ([]byte, error) {
type Arguments TraceEventGetStateCommand
return json.Marshal( {
Execute string `json:"execute"`
Arguments *Arguments `json:"arguments"`
}{
Execute: "trace-event-get-state",
Arguments: (*Arguments)(self),
})
}
type TraceEventInfo struct {
Name string `json:"name"`
State string `json:"state"`
}
func (self *TraceEventInfo) UnmarshalJSON(data []byte) error {
type Return TraceEventInfo
ret := struct {
Return Return `json:"return"`
}{}
err := json.Unmarshal(data, )
if err != nil {
return err
}
self.Name = ret.Return.Name
self.State = ret.Return.State
return nil
}
func main() {
var err error
var qmp_input []byte
var qmp_output []byte
cmd :=
