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

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

On 12/4/19 6:40 PM, George Dunlap wrote:
> 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.

Actually, it turns out we don't strictly need to duplicate this at all,
if we use the `typeof` operator, like this:

---
typedef typeof(((struct libxl_channelinfo *)NULL)->u.pty)
libxl_channelinfo_connection_union_pty;

typedef typeof(((struct libxl_domain_build_info *)NULL)->u.hvm)
libxl_domain_build_info_type_union_hvm;

typedef typeof(((struct libxl_domain_build_info *)NULL)->u.pv)
libxl_domain_build_info_type_union_pv;

typedef typeof(((struct libxl_domain_build_info *)NULL)->u.pvh)
libxl_domain_build_info_type_union_pvh;

typedef typeof(((struct libxl_device_usbdev *)NULL)->u.hostdev)
libxl_device_usbdev_type_union_hostdev;

typedef typeof(((struct libxl_device_channel *)NULL)->u.socket)
libxl_device_channel_connection_union_socket;
---

This guarantees we'll have the correct layout for the resulting type.

I talked to Ian Jackson, and he agreed that long-term it would be good
for the C generator to generate named types for these union elements
(likke you have here).  If you felt really motivated you could do that
now; but I think using the `typeof` trick would be suitable to get this
patch in.

 -George

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