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

[Daniel P . Berrangé]

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

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