On 12/4/19 6:40 PM, George Dunlap wrote: > On 11/15/19 7:44 PM, Nick Rosbrook wrote: >> From: Nick Rosbrook <rosbro...@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