Re: [Xen-devel] [PATCH v4 2/6] golang/xenlight: begin Go to C type marshaling

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 12/23/19 3:17 PM, Nick Rosbrook wrote:
> @@ -92,6 +131,43 @@ func (x *SpiceInfo) fromC(xc *C.libxl_spice_info) error {
>       return nil
>  }
>  
> +func (x *SpiceInfo) toC() (xc C.libxl_spice_info, err error) {
> +     C.libxl_spice_info_init(&xc)
> +     xc.enable, err = x.Enable.toC()
> +     if err != nil {
> +             C.libxl_spice_info_dispose(&xc)
> +             return xc, err
> +     }
> +     xc.port = C.int(x.Port)
> +     xc.tls_port = C.int(x.TlsPort)
> +     xc.host = C.CString(x.Host)
> +     xc.disable_ticketing, err = x.DisableTicketing.toC()
> +     if err != nil {
> +             C.libxl_spice_info_dispose(&xc)

I realize this is all generated code, but there's still a massive amount
of duplication here, which will at very least cause code bloat.  I think
it should be possible to do this all at once with a defer, like this:

func (x *SpiceInfo) toC() (xc C.libxl_spice_info, err error) {
    C.libxl_spice_info_init(&xc)
    defer func () {
        if err != nil {
            libxl_spice_info_dispose(&xc);
        }
    }()

What do you think?

The other comment I have right now is about the return-by-value rather
than by reference.  It does mean you can do things like:

    cfoo, err := gfoo.toC()

rather than

    var cfoo C.libxl_foo
    err := gfoo.toC(&cfoo)

But it means there's an inordinate amount of copying.  Every structure
at depth N will be copied N times; and some of these substructures are
quite large.  Because we're returning errors from toC(), we're never
going to be able to do something like

    C.libxl_bar(Ctx.ctx, gfoo.toC())

anyway.  If we switched to passing in pointers (and used the defer trick
above), a lot of these could end up looking like this:

    if err = x.DisableTicketing.toC(&xc.disable_ticketing); err != nil {
        return err
    }

Avoiding the copy.  What do you think?

The final thing is actually whether we want to do the "_init" at the
beginning of the function at all.  The _init functions have two purposes:

1. To zero out the structures
2. To set things to default values.

In Go, #1 isn't necessary; structures are defined as zeroed.  In .toC(),
#2 won't be effective either in most cases, since we'll be writing over
values with the values found in the Go struct.

To get the default values, I think we'll probably need to write helpers
like this:

func NewDomainConfig(t DomainType) (*DomainConfig, error) {
        var cconfig C.libxl_domain_config

        C.libxl_domain_config_init(&cconfig)
        C.libxl_domain_build_info_init_type(&cconfig.b_info,
C.libxl_domain_type(t))

        gconfig := &DomainConfig{}
        err := gconfig.fromC(&cconfig)
        if err != nil {
                return nil, err
        }

        return gconfig, nil
}

This makes sure that not only DomainConfig has the right default values,
but that DomainConfig.BuildInfo.TypeUnion has the right default values
for HVM guests.

So the only time #2 would have any effect in .toC() would be if there
was a field in a structure that the Go bindings didn't know about.  I
kind of go back and forth as to how necessary that is.

Any thoughts?

 -George

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