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

[Nick Rosbrook <rosbrookn@xxxxxxxxx>]

> 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?

That was my intention a while ago (one of the reasons I named the
return values in toC), but I forgot to make the change. Thanks.

> 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?

That makes sense to me. I'd like to avoid excessive copying, and
that's an easy change to make.

> 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.

Yes that's true, I guess it is mostly ineffectual then.

> 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.

Sounds good to me. It seems like it should be easy enough to generate
those helpers.

> 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?

Would the Go bindings not know about the field because it's something
internal? Or are you thinking a case where the bindings are out of
sync with libxl? In the former case, I'm not familiar enough with the
internal types to foresee potential issues. In the latter, I would
think the misalignment would be the larger issue. Given what you said
above, it doesn't feel all that necessary.

-NR

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