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

[Victor Toso]

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

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 </hand-wavey>

Cheers,
Victor

signature.asc
Description: PGP signature