[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v4 2/6] golang/xenlight: begin Go to C type marshaling
> 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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |