Re: [RFC PATCH v1 0/8] qapi: add generator for Golang interface

[Daniel P . Berrangé]

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: &BlockResizeArguments700{
> > > >             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: &BlockResizeArguments700{
> >              NodeName: node,
> >              Size: 1 * GiB
> >        }
> >          }
> >      } else {
> >         cmd = BlockResizeArguments{
> >            V520: &BlockResizeArguments520{
> >              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 = &BlockResizeArguments700{
> >              NodeName: node,
> >              Size: 1 * GiB
> >          }
> >      } else {
> >         cmd = &BlockResizeArguments520{
> >              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 :|