Re: [Xen-devel] [PATCH v2 16/22] golang/xenlight: implement keyed union C to Go marshaling

[George Dunlap <george.dunlap@citrix.com>]

On 11/15/19 7:44 PM, Nick Rosbrook wrote:
> From: Nick Rosbrook <rosbrookn@ainfosec.com>
> 
> Switch over union key to determine how to populate 'union' in Go struct.
> 
> Since the unions of C types cannot be directly accessed, add C structs in
> cgo preamble to assist in marshaling keyed unions. This allows the C
> type defined in the preamble to be populated first, and then accessed
> directly to populate the Go struct.

Blech. :-(

> +def xenlight_golang_union_fields_from_C(ty = None):
> +    s = ''
> +
> +    for f in ty.fields:
> +        gotypename = xenlight_golang_fmt_name(f.type.typename)
> +        ctypename  = f.type.typename
> +        gofname    = xenlight_golang_fmt_name(f.name)
> +        cfname     = f.name
> +
> +        is_castable = (f.type.json_parse_type == 'JSON_INTEGER' or
> +                       isinstance(f.type, idl.Enumeration) or
> +                       gotypename in go_builtin_types)
> +
> +        if not is_castable:
> +            s += 'if err := x.{}.fromC(&tmp.{});'.format(gofname,cfname)
> +            s += 'err != nil {\n return err \n}\n'
> +
> +        # We just did an unsafe.Pointer cast from []byte to the 'union' type
> +        # struct, so we need to make sure that any string fields are actually
> +        # converted properly.
> +        elif gotypename == 'string':
> +            s += 'x.{} = C.GoString(tmp.{})\n'.format(gofname,cfname)
> +
> +        else:
> +            s += 'x.{} = {}(tmp.{})\n'.format(gofname,gotypename,cfname)

It looks like this is duplicating (differently!) the field-copying code
from golang_define_from_C.  Is there any reason you couldn't have a
single function, `xenlight_golang_fields_from_C`, which would be used
for both?


> +typedef struct libxl_channelinfo_connection_union_pty {
> +	char * path;
> +} libxl_channelinfo_connection_union_pty;

It would be nice if there were some way we could verify that the
structures generated here matched the C unions generated.  It would
stink pretty badly if they drifted and nobody noticed until we started
getting weird errors.

We don't have to solve it now, but let's put it on the to-do list and
have a think about it.

Overall looks about as good as it can -- thanks again for working this out.

 -George


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