Re: [Xen-devel] [RFC] Generating Go bindings for libxl

[Nicholas Rosbrook <rosbrookn@xxxxxxxxxxxx>]

> That looks about like we expected -- tolerable and functional, to be
> certain, but LotsOfReallyLongTypeNames.

Yeah that's definitely a down side...

> The only purpose of unions in these structures is to save space (as
> opposed to other kinds of unions are specifically designed to allow
> different "views" of the same underlying data).  We're replacing the
> unions with structures which will be 1) allocated separately, and 2)
> require casting and type assertions to handle properly.  This will save
> *some* space, but at the cost of a certain amount of complexity, and
> run-time overhead.

WRT runtime overhead, type assertions are actually quite fast. You can
try this [1] benchmark that I found referenced in several threads asking
about the performance of type assertions. On my machine, each case
was ~1.5 ns/op.

> What we just defined three separate elements in the struct?  E.g.:

[...]

> if ( di.Type.Key == libxl.DomainTypeHvm ) {
>    /* ... */
>    firmware := di.Type.Hvm.Firmware;
> }

I see your point here, but as type assertions are so common in Go, I don't
think we need to worry about making this part "easier to write." If someone
really wanted to have easier access to DomainBuildInfoTypeUnionHvm, they
could write a getter that hides the type assertion. Then they could have:

hvminfo := di.getHVMBuildInfo()
if hvminfo != nil {
        firmware := hvminfo.Firmware
}

> This also mean you could make a mistake and access the HVM fields for a
> PV domain, and you'd get neither a compile-time nor a run-time error.
>
> Anyway, like I said, just tossing it out there.  If we decide we don't
> want duplicate structs, I think your implementation looks about as good
> as it can be.

I'm not strongly opposed to the struct duplication, but I do prefer the ability
to perform type assertions as a way to determine which field is "valid."

> So the advantage of this is that you can just call:
>
>     fromC(&di, &cdi)
>
> Rather than:
>
>     di.fromC(&cdi)
>
> ?
> 
> But the cost for this is that we're switching from static type-checking
> to dynamic type-checking.  If in the first example, cdi is the wrong
> type (for instance, if we forget the & at the front), everything will
> compile, and we won't notice unless the function actually gets called.
> In the second example, if we're not trying to implement a generic
> "marshaler" method, we can define the function signature to specify
> exactly what pointer we need.

The advantage is it simplifies the generated code's error handling. However,
I was re-thinking this portion as well, because giving up the static type
checking is not worth "cleaner" generated code. I'll make that change.

> Right -- that looks like just about the only option?  Anyway, it's a
> good option; no point spending a lot of time looking for ways to improve
> something that's only really going to live inside one generated file.

Agreed.

> We could basically do the same thing; but make `val` non-exported.

Okay.

> I'd be tempted to say just do something like:
> 
> type CpuidPolicyList struct {
>     val C.libxl_cpuid_policy_list
> };
>
> A part of me thinks even something like this wouldn't be terrible:
> 
> type CpuidPolicyList C.libxl_cpuid_policy_list
> 
> It "leaks" the internals out to the callers, but it also means you don't
> have to do all this faff of marshalling / unmarshalling what's
> essentially just a pointer.

I don't think it's a good idea to expose the C type. Besides the fact that [2]
explicitly states not to do this, exporting this type gives the false idea that
this type is somehow portable.

> Should probably be C.LIBXL_MS_VM_GENID_LEN, but yes.

Sounds good.

> It sort of looks like this is an entirely internal thing that libxl
> uses.  I think to begin with we can just declare this as an empty
> struct, and figure out what to put in it once it becomes more clear how
> it needs to be used.

Okay, will do.

> Thanks for all your work on this!

No problem, I'm glad to be working on it.

-NR

[1] https://play.golang.org/p/E9H_4K2J9-
[2] https://golang.org/cmd/cgo/#hdr-Go_references_to_C
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel