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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 9/24/19 1:33 AM, Nicholas Rosbrook wrote:
> Hi George,
> 
> I made the changes that we discussed WRT C to Go type marshaling. See [1] for 
> generated code.
> 
> In addition, I took a pass at implementing Go to C type marshaling. The 
> generated toC functions are also in [1].
> 
> Finally, I made the necessary changes[2] to the existing xenlight.go so that 
> it uses the new generated code. To summarize, from my commit message:
> 
>     * Define missing libxl builtin types
>     * Remove types that are now defined from gengotypes.py
>     * Define fromC/toC for all builtin types
>     * Add CreateDomain to demonstrate functioning generated code
>     * Update any existing code so that compilation succeeds
> 
> I've so far kept the changes to xenlight.go within the scope of the code 
> generation effort. I figured the rest of the API development would be our 
> next step.

That makes sense.

Just going through in detail, I notice one thing about your
implementation of Defbool: you simply copy over the value of
libxl_defbool.  The header says of libxl_defbool:

 * Users should treat this struct as opaque and use the following
 * defined macros and accessor functions.

The meaning of 'val' is unlikely to change, but in theory it could.  So
I think the fromC method should do something like:

if ( C.libxl_defbool_is_default(c) ) {
    // Set d.val to 'default'
} else if ( C.libxl_defbool_val(c) ) {
    // Set d.val to 'true'
} else {
    // Set d.val to 'false'
}

And of course, Defbool will need similar methods for external callers.

But we're going to have to find a better way to review the changes
you're making.  Would it be too much to ask you to break the series down
into individual chunks that each made one logical change, and sending
the results to the list?

e.g.:

* golang/libxl: Revise implementaiton of Uuid builtin
 - Changes definition to [16]byte
 - Implements .toC(), .fromC(), and .String()

* golang/libxl: Implement Defbool builtin
 ...

[other builtins]

* golang/libxl: Generate specific integer types from the IDL
 - Remove Memkb 'builtin' implementation

* golang/libxl: Generate enumerations from the IDL
 - Adds the code to generate enumerations from the idl
 - Has in the commit message a sample of what the generated code looks like
 - Remove enumeration definitions from xenlight.go
 - Rename `errors` to `libxlErrors` and adds the `-`
   &c

* golang/libxl: Generate structs from the IDL

And so on.  I realize this is some extra work for you, particularly if
you're not familiar with the idea.  But it has some distinct advantages:

* I (and anyone else following along) can see more clearly each
individual design decision that's being made.  Right now things are all
sort of lost in a big jumble.

* Rather than C&P'ing code I want to comment on, I can reply in-line.

* As the series progresses and we "nail down" individual decisions, we
don't need to go back over them.  For instance, if the Defbool
implementation is in its own patch, then once I give my Reviewed-by,
then I know I don't need to look at Defbool anymore.  But when
everything is in one big change, each iteration I have to scan Defbool
again and remember whether I need to review it or not.

What do you think?

> Besides that, the last thing I need is to know how I should integrate this 
> into the build. Of note, gengotypes.py needs to import tools/libxl/idl.py. 
> Maybe that part is easier to discuss on IRC.

I'm not super-familiar with Python's importing; could you do something
like the following?

xenlight_types.go [...]: $(XEN_ROOT)/tools/libxl/libxl_types.idl [...]
    PYTHONPATH=$(XEN_ROOT)/tools/libxl $(PYTHON) gentypes.py [...]

(`gentypes.go` should be inside of the xenlight directory.)

 -George

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