Re: [Xen-devel] [PATCH 14/24] golang/xenlight: generate structs from the IDL

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 10/7/19 4:13 PM, Nick Rosbrook wrote:
> From: Nick Rosbrook <rosbrookn@xxxxxxxxxxxx>
> 
> Add struct and keyed union generation to gengotypes.py. For keyed unions,
> use a method similar to gRPC's oneof to interpret C unions as Go types.
> Meaning, for a given struct with a union field, generate a struct for
> each sub-struct defined in the union. Then, define an interface of one
> method which is implemented by each of the defined sub-structs. For
> example:
> 
>   type domainBuildInfoTypeUnion interface {
>           isdomainBuildInfoTypeUnion()
>   }
> 
>   type DomainBuildInfoTypeUnionHvm struct {
>       // HVM-specific fields...
>   }
> 
>   func (x DomainBuildInfoTypeUnionHvm) isdomainBuildInfoTypeUnion() {}
> 
>   type DomainBuildInfoTypeUnionPv struct {
>       // PV-specific fields...
>   }
> 
>   func (x DomainBuildInfoTypeUnionPv) isdomainBuildInfoTypeUnion() {}
> 
>   type DomainBuildInfoTypeUnionPvh struct {
>       // PVH-specific fields...
>   }
> 
>   func (x DomainBuildInfoTypeUnionPvh) isdomainBuildInfoTypeUnion() {}
> 
> Then, remove existing struct definitions in xenlight.go that conflict
> with the generated types, and modify existing marshaling functions to
> align with the new type definitions. Notably, drop "time" package since
> fields of type time.Duration are now of type uint64.

BTW I was discussing with Ian Jackson, and I think at some point it
would be worth considering adding in an annotation or something to the
IDL such that the generator can use time.Duration for these things.
That opens up another can of works, like  the fact that Duration is
int64 rather than uint64.

But at any rate, that's something to look into *after* this series is
checked in.

The generated code here looks really good -- thanks for the work!  One
comment on the generator...

> 
> Signed-off-by: Nick Rosbrook <rosbrookn@xxxxxxxxxxxx>
> ---
> Cc: George Dunlap <george.dunlap@xxxxxxxxxx>
> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> Cc: Wei Liu <wl@xxxxxxx>
> 
>  tools/golang/xenlight/gengotypes.py     | 103 +++
>  tools/golang/xenlight/xenlight.go       | 123 +---
>  tools/golang/xenlight/xenlight_types.go | 834 ++++++++++++++++++++++++
>  3 files changed, 952 insertions(+), 108 deletions(-)
> 
> diff --git a/tools/golang/xenlight/gengotypes.py 
> b/tools/golang/xenlight/gengotypes.py
> index 59307492cb..c8513b79e0 100644
> --- a/tools/golang/xenlight/gengotypes.py
> +++ b/tools/golang/xenlight/gengotypes.py
> @@ -18,6 +18,10 @@ builtin_type_names = {
>      idl.uint64.typename: 'uint64',
>  }
>  
> +# List of strings that need to be written to a file
> +# after a struct definition.
> +type_extras = []
> +
>  def xenlight_golang_generate_types(path = None, types = None, comment = 
> None):
>      """
>      Generate a .go file (xenlight_types.go by default)
> @@ -35,6 +39,13 @@ def xenlight_golang_generate_types(path = None, types = 
> None, comment = None):
>              f.write(xenlight_golang_type_define(ty))
>              f.write('\n')
>  
> +            # Append extra types
> +            for extra in type_extras:
> +                f.write(extra)
> +                f.write('\n')
> +
> +            del type_extras[:]

So it looks like you've added a global variable, and have the various
functions adding to this global variable in the back-door, and then at
the toplevel "flush" the output and clear it?

This doesn't seem like great software engineering. :-)  Would it be
terribly difficult to refactor things to avoid this?  Perhaps by
returning tuples that are merged together or something?

 -George


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel