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

[Victor Toso]

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: &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.

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.

Cheers,
Victor

signature.asc
Description: PGP signature