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

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

[Nicholas Rosbrook]

[-- Attachment #1.1: Type: text/plain, Size: 1197 bytes --]

Hello,

As a follow up to the presentation that Brendan Kerrigan and I gave at Xen
summit earlier this month, "Client Virtualization Toolstack in Go", I would like to open
a discussion around the development of Go bindings for libxl. George Dunlap,
Nicolas Belouin and I have had some discussion off-line already.

So far, these are the topics of discussion:

- Code generation: Should the Go bindings be generated from the IDL? Or should
  an existing cgo generator like c-for-go [1] be leveraged?

- What does the minimal viable Go package look like? IMO it should be able to create
  and destroy domains, attach and detach network and disk devices, list domains,
  and convert domid to name and vice versa. It is also important that the exported
  APIs reflect idiomatic Go.

- Challenges surrounding hypervisor versioning, go modules, etc.

ATM, the code generation piece is the primary concern.

George - could you explain your thoughts on the code generation topic?

The existing bindings that I have been working on are at [2].

Thanks,

- Nick Rosbrook

[1] https://github.com/xlab/c-for-go
[2] https://github.com/enr0n/xen/tree/libxl-go/tools/golibxl/libxl

[-- Attachment #1.2: Type: text/html, Size: 1990 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

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

[Tamas K Lengyel]

On Tue, Jul 30, 2019 at 7:32 AM Nicholas Rosbrook
<rosbrookn@ainfosec.com> wrote:
>
> Hello,
>
> As a follow up to the presentation that Brendan Kerrigan and I gave at Xen
> summit earlier this month, "Client Virtualization Toolstack in Go", I would like to open
> a discussion around the development of Go bindings for libxl. George Dunlap,
> Nicolas Belouin and I have had some discussion off-line already.
>
> So far, these are the topics of discussion:

Hi Nicholas,
to add to the list of topics I just want to mention that perhaps it
may be beneficial to consider parts of the go bindings not go to libxl
at all. I have been digging through libxl for the past couple months
and it's asynchronous callback system is damn near impossible to
follow and I just can't shake the feeling that it would be a lot
easier to follow if it was in go. Not to mention the performance
issues with the built-in garbage collector and fork/exec parts. I'm
also interested only in a very small subset of what libxl does today
but I want to be able to that as fast as possible - domain creation -
which has many steps that could be done in parallel to speed it up..
and that would just be a natural thing to do in go.

Tamas

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

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

[George Dunlap]

On 7/30/19 2:11 PM, Nicholas Rosbrook wrote:
> Hello,
> 
> As a follow up to the presentation that Brendan Kerrigan and I gave at Xen
> summit earlier this month, "Client Virtualization Toolstack in Go", I would like to open
> a discussion around the development of Go bindings for libxl. George Dunlap,
> Nicolas Belouin and I have had some discussion off-line already.
> 
> So far, these are the topics of discussion:
> 
> - Code generation: Should the Go bindings be generated from the IDL? Or should
>   an existing cgo generator like c-for-go [1] be leveraged?

Well a couple of general considerations:

* The IDL describes things at a more semantic level; it can be
arbitrarily extended with as much information as needed to allow the
generators to do their work.  And we have more control over the output:
in particular, we know we can enforce calling conventions such as
calling libxl_<type>_init() and libxl_<type>_dispose().

* AFAICT at the moment, the IDL is only used to generate C code, not for
any other languages; and only contains information about types, not
about the function signatures.  So using the IDL for "foreign" language
bindings is actually a new use case we haven't done before.

* Work enriching the IDL should have cross-over benefits into other
languages (for instance, ocaml, should XenServer ever decide to port
xapi to use libxl).  Such languages will either have no such
c-to-<language> translator, or will have a very different one.

* At the risk of falling into "NIH", adding any external dependency is
somewhat of a risk. While the c-for-go project seems reasonably stable,
it's not part of the core Go toolset, and doesn't seem to be backed by a
major corporation with a vested interest in keeping it going.  What
happens if the maintainer decides to move on in 4 years?  Making a
custom generator is a little bit of extra work, but saves us having to
potentially deal with abandoned upstream tooling down the line.

* FWIW we don't need to parse any C code to use the IDL, we can use
python's native parser.

All that said, the first question I think is, what the generated code
needs to look like.  Then, if c-for-go can be configured to do that,
then we can consider it; otherwise, making our own generator from the
IDL will be the only option.

Out of curiosity, have you looked at the existing in-tree bindings?  Any
particular opinions?

There are two major differences I note.

First, is that in your version, there seems to be two layers: libxl.go
is generated by c-for-go, and contains simple function calls; e.g.:
domainInfo(), which takes a *Ctx as an argument and calls
C.libxl_domain_info.  Then you have libxl_wrappers.go, which is written
manually, defining DomainInfo as a  method on Ctx, and calls domainInfo().

So you're writing the "idiomatic Go" part by hand anyway; I don't really
see why having a hand-written Go function call an automatically
generated Go function to call a C function is better than having a
hand-written Go function call a C function directly.

The other difference is in the handling of nested structures.  c-for-go
seems to generate a struct which has the core C struct inside it, as
well as a Go-like translation of that struct, and methods on that struct
which will copy things into and out of the C struct.

But rather than doing a "deep copy" for pointers within a struct, it
simply copies and casts the pointer from inside the struct to a pointer
outside the struct.

In fact, there's a Go-like clone of libxl_domain_config, but none for
the elements of it; DeviceDisk, for instance, is simply defined as
C.libxl_device_disk, and config->disks simply copied to the Disks
element of the struct.  That's just all wrong -- it's actually a C
array; Go can only access the first element of it.  How are you supposed
to create a domain with more than one disk?

Furthermore, these pointers are not re-set to `nil` after <type>.Free()
is called.  This just seems very dangerous: It would be way to easy to
introduce a use-after-free bug.

And keeping these C pointers around makes things very tricky, as far as
making sure they get freed.

The in-tree bindings generally only create C structures temporarily, and
do a full marshal and unmarshall into and out of Go structures.  This
means a lot of copying on every function call.  But it also means that
the callers can generally treat the Go structures like normal Go
structures -- they don't have to worry about keeping track of them and
freeing them or cleaning them up; they can let the GC deal with it, just
like they deal with everything else.

Which more or less brings me to the core design decision we have to
make: dealing with pointers to / in transient structures (as opposed to
long-lived structures like libxl_ctxt or xentoollogger).  It seems to me
we have a couple of options:

1. Keep separate structures, and do a full "deep copy", as the in-tree
bindings do.  Advantage: Callers can use GC like normal Go functions.
Structure elements are translated to go-native types. Disadvantage:
Copying overhead on every function call.

2. Use C types; do explicit allocate / free.  Advantage: No copying on
every function call.  Disadvantage: Needing to remember to clean up / no
GC; can't use Go-native types.

3. Nest one structure inside the other, and do a marshall only when one
of them changes.  Advantage: Copying only when one of the two sides
changes, rather than every function call; c-for-go already generates a
lot of the marshalling / unmashalling code.  Disadvantage:  Need to do a
full copy whenever one side changes (which in libxl's case will be
almost every function call); Needing to remember to treat pointers
carefully; complicated management of pointers; c-for-go implementation
probably not easily integrated with libxl_<type>_dispose() calling
discipline.

4. Attempt to use SetFinalizer() to automatically do frees / structure
clean-up [1].  Advantage: No / less copying on every function call, but
can still treat structures like they'll be GC'd.  Disadvantage: Requires
careful thinking; GC may not be as effective if C-allocated memory
greatly exceeds Go-allocated memory; can't use Go-native types for elements.

[1]
http://rabarar.github.io/blog/2015/09/29/cgo-and-destructors-for-managing-allocated-memory-in-go/

c-for-go seems to take the worst bits of #1 and #2: It requires explicit
allocate / free, but also actually does a full copy of each structure
whenever one "half" of it changes.

I think I'm coming more and more to the conclusion that I don't like
what c-for-go produces in libxl's case. :-)

On the whole, I still think #1 is the best option.  Thoughts?

 -George

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

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

[George Dunlap]

On 7/30/19 2:48 PM, Tamas K Lengyel wrote:
> On Tue, Jul 30, 2019 at 7:32 AM Nicholas Rosbrook
> <rosbrookn@ainfosec.com> wrote:
>>
>> Hello,
>>
>> As a follow up to the presentation that Brendan Kerrigan and I gave at Xen
>> summit earlier this month, "Client Virtualization Toolstack in Go", I would like to open
>> a discussion around the development of Go bindings for libxl. George Dunlap,
>> Nicolas Belouin and I have had some discussion off-line already.
>>
>> So far, these are the topics of discussion:
> 
> Hi Nicholas,
> to add to the list of topics I just want to mention that perhaps it
> may be beneficial to consider parts of the go bindings not go to libxl
> at all. I have been digging through libxl for the past couple months
> and it's asynchronous callback system is damn near impossible to
> follow and I just can't shake the feeling that it would be a lot
> easier to follow if it was in go.

So I don't think we're ever going to switch to golang being our primary
toolstack language, because calling it from other languages isn't really
an option.  That means that implementing things like domain creation in
Go mean duplicating functionality in two places, which is
extraordinarily expensive from a software-engineering perspective.

FWIW I think the asynchronous callback system just needs better
documentation.  It always takes me a little bit to get my bearings again
once I have to change that code, but once I do, everything is
consistent.  And as I understand it, the external interface was written
primarily with libvirt in mind, so it would probably be difficult to
change it while remaining compatible.

> Not to mention the performance
> issues with the built-in garbage collector

What performance issues were you seeing with libxl's garbage collector?
I thought it just kept a list of pointers and freed them at the very end.

> and fork/exec parts.

Since we only fork/exec when we need to do so, this part would probably
be the same no matter what language it was done in.

That said, very little of this has had much performance analysis -- if
this is important to you, I'm sure there's lots of low-hanging fruit in
terms of improvements we could make.

 -George

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

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

[George Dunlap]

On 7/30/19 2:11 PM, Nicholas Rosbrook wrote:
> Hello,
> 
> As a follow up to the presentation that Brendan Kerrigan and I gave at Xen
> summit earlier this month, "Client Virtualization Toolstack in Go", I would like to open
> a discussion around the development of Go bindings for libxl. George Dunlap,
> Nicolas Belouin and I have had some discussion off-line already.
> 
> So far, these are the topics of discussion:
> 
> - Code generation: Should the Go bindings be generated from the IDL? Or should
>   an existing cgo generator like c-for-go [1] be leveraged?
> 
> - What does the minimal viable Go package look like? IMO it should be able to create
>   and destroy domains, attach and detach network and disk devices, list domains,
>   and convert domid to name and vice versa. It is also important that the exported
>   APIs reflect idiomatic Go.

I'm not sure why "MVP" is important per se -- I expect everyone will
make sure to implement the bits they think are critical; and how much of
the rest gets implemented will depend on motivation, time, difficulty, &c.

> - Challenges surrounding hypervisor versioning, go modules, etc.

The main thing here is to make sure that we can avoid "breaking" changes
in the future, when we implement new functionality.  For instance, the
choice of package name -- `golang.xenproject.org/...`, was chosen so
that we could actually put something at golang.xenproject.org at some point.

Deciding how to do package versioning might be important too -- e.g.,
should we have the package version for a release be equivalent to the
underlying Xen version, or is there a better numbering scheme?

And although we don't need to do loadable plugins to begin with, it
would be good to take a look at what that might look like, so that (if
possible) we can make sure we can add them later transparently, without
any breaking changes.

 -George

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

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

[Tamas K Lengyel]

On Tue, Jul 30, 2019 at 9:49 AM George Dunlap <george.dunlap@citrix.com> wrote:
>
> On 7/30/19 2:48 PM, Tamas K Lengyel wrote:
> > On Tue, Jul 30, 2019 at 7:32 AM Nicholas Rosbrook
> > <rosbrookn@ainfosec.com> wrote:
> >>
> >> Hello,
> >>
> >> As a follow up to the presentation that Brendan Kerrigan and I gave at Xen
> >> summit earlier this month, "Client Virtualization Toolstack in Go", I would like to open
> >> a discussion around the development of Go bindings for libxl. George Dunlap,
> >> Nicolas Belouin and I have had some discussion off-line already.
> >>
> >> So far, these are the topics of discussion:
> >
> > Hi Nicholas,
> > to add to the list of topics I just want to mention that perhaps it
> > may be beneficial to consider parts of the go bindings not go to libxl
> > at all. I have been digging through libxl for the past couple months
> > and it's asynchronous callback system is damn near impossible to
> > follow and I just can't shake the feeling that it would be a lot
> > easier to follow if it was in go.
>
> So I don't think we're ever going to switch to golang being our primary
> toolstack language, because calling it from other languages isn't really
> an option.  That means that implementing things like domain creation in
> Go mean duplicating functionality in two places, which is
> extraordinarily expensive from a software-engineering perspective.
>
> FWIW I think the asynchronous callback system just needs better
> documentation.  It always takes me a little bit to get my bearings again
> once I have to change that code, but once I do, everything is
> consistent.  And as I understand it, the external interface was written
> primarily with libvirt in mind, so it would probably be difficult to
> change it while remaining compatible.
>
> > Not to mention the performance
> > issues with the built-in garbage collector
>
> What performance issues were you seeing with libxl's garbage collector?
> I thought it just kept a list of pointers and freed them at the very end.

I didn't investigate too closely but on some occasions a considerable
amount of the execution time was being spent in there according to
callgrind. After everything was finished in a domain creation xl would
just "hang" in there for a while before actually exiting. It was not
very consistent and recompiling libxl sometimes sped things up.
Haven't run into it since I've upgraded to debian buster and a newer
gcc.

>
> > and fork/exec parts.
>
> Since we only fork/exec when we need to do so, this part would probably
> be the same no matter what language it was done in.
>
> That said, very little of this has had much performance analysis -- if
> this is important to you, I'm sure there's lots of low-hanging fruit in
> terms of improvements we could make.
>

Right, it was my distinct impression that for the majority of libxl
tasks speed wasn't really a concern - after all, a second or two extra
for creating a domain would not be of concern for normal use-cases.
For the use-case I'm after, it is
(https://xenproject.atlassian.net/projects/XEN/board?issue-key=XEN-89).

Tamas

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

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

[Nicholas Rosbrook]

> All that said, the first question I think is, what the generated code
> needs to look like.  Then, if c-for-go can be configured to do that,
> then we can consider it; otherwise, making our own generator from the
> IDL will be the only option.

Writing a custom Go code generator means that all C symbols used need
to be known at generation time. E.g., the Go code generator needs to know
the signature of libxl_domain_create_new for C.libxl_domain_create_new(...)
to work. Currently, such knowledge is gained by parsing C code, which makes
sense given the nature of cgo. AFAICT, the IDL describes how to generate C types
and boiler-plate functions like libxl_<type>_dispose(). How would the IDL alone be able to 
generate valid Go code without significant expansion?

> Out of curiosity, have you looked at the existing in-tree bindings?  Any
> particular opinions?

Yes, my process started by using the existing bindings.

One thing is that they do not conform to basic go standards. For
example: naming conventions, naked returns are used everywhere, and I find
it strange that there is an exported Context variable. But, obviously those are
very minor things and easy to change. See [1] for general information on this.

I also thought it looked very tedious to do by hand, and would be hard to extend
in the future. Hence the search for a cgo generator.

> There are two major differences I note.
> 
> First, is that in your version, there seems to be two layers: libxl.go
> is generated by c-for-go, and contains simple function calls; e.g.:
> domainInfo(), which takes a *Ctx as an argument and calls
> C.libxl_domain_info.  Then you have libxl_wrappers.go, which is written
> manually, defining DomainInfo as a  method on Ctx, and calls domainInfo().
>
> So you're writing the "idiomatic Go" part by hand anyway; I don't really
> see why having a hand-written Go function call an automatically
> generated Go function to call a C function is better than having a
> hand-written Go function call a C function directly.

I'm sure you would agree that writing all of that cgo code by hand was a PITA.

This saved me a lot of time. There is an extra layer, but I didn't need to write
any of the cgo by hand. Adding a wrapper function for the generated Go functions,
e.g. domainInfo(), takes a couple minutes and you end up with easy-to-use Go.

But, I think we already agree on the necessity of code generation.

> In fact, there's a Go-like clone of libxl_domain_config, but none for
> the elements of it; DeviceDisk, for instance, is simply defined as
> C.libxl_device_disk, and config->disks simply copied to the Disks
> element of the struct.  That's just all wrong -- it's actually a C
> array; Go can only access the first element of it.  How are you supposed
> to create a domain with more than one disk?

This is simply because my fork is still WIP. If you look at [2], I'm telling c-for-go
to not generate a new Go type for libxl_device_disk, whereas at [3] I tell c-for-go
how to create libxl_domain_config. 

> Furthermore, these pointers are not re-set to `nil` after <type>.Free()
> is called.  This just seems very dangerous: It would be way to easy to
> introduce a use-after-free bug.

In [4], the C pointer is set to nil inside the call to DomInfo.Free().

> The in-tree bindings generally only create C structures temporarily, and
> do a full marshal and unmarshall into and out of Go structures.  This
> means a lot of copying on every function call.  But it also means that
> the callers can generally treat the Go structures like normal Go
> structures -- they don't have to worry about keeping track of them and
> freeing them or cleaning them up; they can let the GC deal with it, just
> like they deal with everything else.

AFAICT, the generated code provides the ability to do this. The wrappers
just need more discipline to have defer'd dispose/free calls. If the wrapper
hides the calls to freeing/disposing the C pointers, then the caller can still
rely on the garbage collector like normal.

> 1. Keep separate structures, and do a full "deep copy", as the in-tree
> bindings do.  Advantage: Callers can use GC like normal Go functions.
> Structure elements are translated to go-native types. Disadvantage:
> Copying overhead on every function call.

Personally, I'm not worried about optimizing just yet.

> 2. Use C types; do explicit allocate / free.  Advantage: No copying on
> every function call.  Disadvantage: Needing to remember to clean up / no
> GC; can't use Go-native types.

We definitely don't want to export the C types through the Go API.

> 4. Attempt to use SetFinalizer() to automatically do frees / structure
> clean-up [1].  Advantage: No / less copying on every function call, but
> can still treat structures like they'll be GC'd.  Disadvantage: Requires
> careful thinking; GC may not be as effective if C-allocated memory
> greatly exceeds Go-allocated memory; can't use Go-native types for elements.

If we start looking to use the Go runtime, we've gone in the wrong direction.

> c-for-go seems to take the worst bits of #1 and #2: It requires explicit
> allocate / free, but also actually does a full copy of each structure
> whenever one "half" of it changes.

Either way, we need to worry about the allocate/free. But that will at least be
hidden by the wrappers. Not sure how to get around that part in any case.

However, it is not immediately obvious to me how to tell c-for-go not to export
the functions in cgo_helpers.go, which is unfortunate.

> I think I'm coming more and more to the conclusion that I don't like
> what c-for-go produces in libxl's case. :-)
> 
> On the whole, I still think #1 is the best option.  Thoughts?

I agree that doing a full copy and allowing callers to simply rely on Go's 
garbage collector is the best approach. I'm just not entirely convinced that
cannot be accomplished using c-for-go.

-NR

[1] https://github.com/golang/go/wiki/CodeReviewComments
[2] https://github.com/enr0n/xen/blob/libxl-go/tools/golibxl/libxl.yml#L33
[3] https://github.com/enr0n/xen/blob/libxl-go/tools/golibxl/libxl.yml#L68
[4] https://github.com/enr0n/xen/blob/libxl-go/tools/golibxl/libxl/cgo_helpers.go#L422 
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[George Dunlap]

On 7/30/19 10:52 PM, Nicholas Rosbrook wrote:
>> All that said, the first question I think is, what the generated code
>> needs to look like.  Then, if c-for-go can be configured to do that,
>> then we can consider it; otherwise, making our own generator from the
>> IDL will be the only option.
> 
> Writing a custom Go code generator means that all C symbols used need
> to be known at generation time. E.g., the Go code generator needs to know
> the signature of libxl_domain_create_new for C.libxl_domain_create_new(...)
> to work. Currently, such knowledge is gained by parsing C code, which makes
> sense given the nature of cgo.

I return to the question I stated before.  At the moment, your bindings
have the following call chain:

* DomainInfo(), hand-crafted.  Calls domainInfo().
* domainInfo(), automaticall generated.  Calls C.libxl_domain_info().

The in-tree bindings have the following call chain:

* DomainInfo(), hand-crafted.  Calls C.libxl_domain_info().

Since DomainInfo() is hand-crafted in both cases, what's the advantage
of having domainInfo() at all?

> AFAICT, the IDL describes how to generate C types
> and boiler-plate functions like libxl_<type>_dispose(). How would the IDL alone be able to 
> generate valid Go code without significant expansion?

So just to clarify terminology: The IDL is the description language
itself, which at the moment only contains information about the libxl
structures.  We have generators for various C bits of libxl which read
the IDL and spit out boilerplate C.  The idea would be that we write a
new generator for Go which reads the IDL and spits out boilerplate Go.

If you look at gentypes.py, you'll see that it's not doing anything
fancy; it's very much just spitting out strings based on simple rules.
I think a large amount of the boilerplate Go code (the C <-> Go
marshalling code in particular) can be done in the same way.

>> Out of curiosity, have you looked at the existing in-tree bindings?  Any
>> particular opinions?
> 
> Yes, my process started by using the existing bindings.
> 
> One thing is that they do not conform to basic go standards. For
> example: naming conventions, naked returns are used everywhere, and I find
> it strange that there is an exported Context variable. But, obviously those are
> very minor things and easy to change. See [1] for general information on this.

I looked at the thing about naked returns, and didn't really understand
it; but anyway I'm happy to have things modified to be more Go-like.  I
definitely "speak" Go with a funny accent.

The exported Ctx variable is probably vestigial; I don't think I'd argue
against removing it.

Can I say -- I've been going open-source for so long, that I feel almost
unsafe when nobody reviews my stuff.  Most of this code was written by
me and reviewed by nobody (since I was the only person interested); it's
good to have someone else take a critical look at it.

> I also thought it looked very tedious to do by hand, and would be hard to extend
> in the future. Hence the search for a cgo generator.

Right; and the intention was always to begin to do it by hand to see how
we wanted it to look, and then write a generator to do the tedious work
by reading the IDL.

>> There are two major differences I note.
>>
>> First, is that in your version, there seems to be two layers: libxl.go
>> is generated by c-for-go, and contains simple function calls; e.g.:
>> domainInfo(), which takes a *Ctx as an argument and calls
>> C.libxl_domain_info.  Then you have libxl_wrappers.go, which is written
>> manually, defining DomainInfo as a  method on Ctx, and calls domainInfo().
>>
>> So you're writing the "idiomatic Go" part by hand anyway; I don't really
>> see why having a hand-written Go function call an automatically
>> generated Go function to call a C function is better than having a
>> hand-written Go function call a C function directly.
> 
> I'm sure you would agree that writing all of that cgo code by hand was a PITA.

Writing the .toC() and .toGo() methods is certainly a pain, and wants a
generator.  I didn't find writing:

func (Ctx *Context) DomainInfo(Id Domid) (di *Dominfo, err error) {
	err = Ctx.CheckOpen()
	if err != nil {
		return
	}

	var cdi C.libxl_dominfo
	C.libxl_dominfo_init(&cdi)
	defer C.libxl_dominfo_dispose(&cdi)

	ret := C.libxl_domain_info(Ctx.ctx, &cdi, C.uint32_t(Id))

	if ret != 0 {
		err = Error(-ret)
		return
	}

	di = cdi.toGo()

	return
}

to be terribly much more work than writing something like:

func (c *Context) DomainInfo(domid DomID) (*DomInfo, error) {
	di := DomInfo{}

	if ret := domainInfo(c.Ctx, &di, uint32(domid)); ret != 0 {
		return nil, fmt.Errorf("unable to retrieve domain info: %v", ret)
	}
	di.Deref()

	return &di, nil
}

If we "more discipline to have defer'd dispose/free calls", the code
will probably look largely the same.

And if we had a an IDL for the libxl functions, we could have it
generate the code above for the vast majority of cases.

>> In fact, there's a Go-like clone of libxl_domain_config, but none for
>> the elements of it; DeviceDisk, for instance, is simply defined as
>> C.libxl_device_disk, and config->disks simply copied to the Disks
>> element of the struct.  That's just all wrong -- it's actually a C
>> array; Go can only access the first element of it.  How are you supposed
>> to create a domain with more than one disk?
> 
> This is simply because my fork is still WIP. If you look at [2], I'm telling c-for-go
> to not generate a new Go type for libxl_device_disk, whereas at [3] I tell c-for-go
> how to create libxl_domain_config.
> 
>> Furthermore, these pointers are not re-set to `nil` after; but the biggest argument against it is not using Go-native types.
 <type>.Free()
>> is called.  This just seems very dangerous: It would be way to easy to
>> introduce a use-after-free bug.
> 
> In [4], the C pointer is set to nil inside the call to DomInfo.Free().

I meant something like DomainConfig.  DomainConfig.Deref() will set
DomainConfig.Disks (a pointer) equal to domain_config.disks.  But
DomainConfig.Free() doesn't set DomainConfig.Disks back to nil.

So suppose you had the following code:

  dc DomainConfig;

  [do something to fill dc.ref87e08e4d]

  dc.Deref();  // Sets dc.CInfo to non-nil.

  // Do something

  dc.Free(); // Sets dc.ref* to nil, but leaves dc.CInfo alone

  // forget you called Free

  if dc.Disks.blah { // ## Use-after-free
  }

I realize that deference is a clear bug either way; but if Disks was
nil, it would cause a program crash, which is much better than whatever
random corruption might happen otherwise.

And libxl_domain_info.disks, this brings be back to the point about the
IDL having more information.  libxl_domain_info.disks isn't a pointer to
an individual struct, it's a pointer to an array; and
libxl_domain_info.num_disks is the size of the array.

But the IDL doesn't actually have num_disks; it has
Array(libxl_device_disks, "disks"), and the C generator generates the
num_disks elemet from that.

If we wrote a generator from the IDL, we could make it smart enough to
use []Disks as the type there, and make the marshallers know how to use
num_disks to appropriately size the resulting slice and copy the right
number of values across.  To do that with c-for-go, we'd have to do a
lot of work teaching it what to do, if that's even possible.

>> The in-tree bindings generally only create C structures temporarily, and
>> do a full marshal and unmarshall into and out of Go structures.  This
>> means a lot of copying on every function call.  But it also means that
>> the callers can generally treat the Go structures like normal Go
>> structures -- they don't have to worry about keeping track of them and
>> freeing them or cleaning them up; they can let the GC deal with it, just
>> like they deal with everything else.
> 
> AFAICT, the generated code provides the ability to do this. The wrappers
> just need more discipline to have defer'd dispose/free calls. If the wrapper
> hides the calls to freeing/disposing the C pointers, then the caller can still
> rely on the garbage collector like normal.

So you mean, for example, after DomainInfo() calls DomInfo.Deref(), it
will then call libxl_dominfo_dispose() on the C struct?

>> 1. Keep separate structures, and do a full "deep copy", as the in-tree
>> bindings do.  Advantage: Callers can use GC like normal Go functions.
>> Structure elements are translated to go-native types. Disadvantage:
>> Copying overhead on every function call.
> 
> Personally, I'm not worried about optimizing just yet.
> 
>> 2. Use C types; do explicit allocate / free.  Advantage: No copying on
>> every function call.  Disadvantage: Needing to remember to clean up / no
>> GC; can't use Go-native types.
> 
> We definitely don't want to export the C types through the Go API.
> 
>> 4. Attempt to use SetFinalizer() to automatically do frees / structure
>> clean-up [1].  Advantage: No / less copying on every function call, but
>> can still treat structures like they'll be GC'd.  Disadvantage: Requires
>> careful thinking; GC may not be as effective if C-allocated memory
>> greatly exceeds Go-allocated memory; can't use Go-native types for elements.
> 
> If we start looking to use the Go runtime, we've gone in the wrong direction.

I'm mostly trying to be complete in my analysis.  I agree relying on
this sort of magic isn't very appealing; but the biggest argument
against it is not using Go-native types.  That makes it more or less a
non-starter.

>> c-for-go seems to take the worst bits of #1 and #2: It requires explicit
>> allocate / free, but also actually does a full copy of each structure
>> whenever one "half" of it changes.
> 
> Either way, we need to worry about the allocate/free. But that will at least be
> hidden by the wrappers. Not sure how to get around that part in any case.

Of course "we the library authors" have to worry about allocate / free;
but it's a much nicer interface for "them the library consumers" not to
have to do so.  At the moment you're both, but I'm hoping we'll have
lots of "library consumers" who are not "library authors". :-)

> I agree that doing a full copy and allowing callers to simply rely on Go's 
> garbage collector is the best approach. I'm just not entirely convinced that
> cannot be accomplished using c-for-go.

Right, and personally I'm not in principle opposed to using c-for-go, if
it can be made to generate code the way we like it.  My main fear is
that we'll spend a bunch of time tweaking c-for-go, and after going back
and forth and investing a lot of time in it, we'll end up either 1)
giving up and writing our own generator anyway, or 2) accepting
something sub-optimal because it's the best thing we could make c-for-go do.

 -George

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

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

[George Dunlap]

On 7/30/19 7:39 PM, Tamas K Lengyel wrote:
> On Tue, Jul 30, 2019 at 9:49 AM George Dunlap <george.dunlap@citrix.com> wrote:
>>
>> On 7/30/19 2:48 PM, Tamas K Lengyel wrote:
>>> On Tue, Jul 30, 2019 at 7:32 AM Nicholas Rosbrook
>>> <rosbrookn@ainfosec.com> wrote:
>>>>
>>>> Hello,
>>>>
>>>> As a follow up to the presentation that Brendan Kerrigan and I gave at Xen
>>>> summit earlier this month, "Client Virtualization Toolstack in Go", I would like to open
>>>> a discussion around the development of Go bindings for libxl. George Dunlap,
>>>> Nicolas Belouin and I have had some discussion off-line already.
>>>>
>>>> So far, these are the topics of discussion:
>>>
>>> Hi Nicholas,
>>> to add to the list of topics I just want to mention that perhaps it
>>> may be beneficial to consider parts of the go bindings not go to libxl
>>> at all. I have been digging through libxl for the past couple months
>>> and it's asynchronous callback system is damn near impossible to
>>> follow and I just can't shake the feeling that it would be a lot
>>> easier to follow if it was in go.
>>
>> So I don't think we're ever going to switch to golang being our primary
>> toolstack language, because calling it from other languages isn't really
>> an option.  That means that implementing things like domain creation in
>> Go mean duplicating functionality in two places, which is
>> extraordinarily expensive from a software-engineering perspective.
>>
>> FWIW I think the asynchronous callback system just needs better
>> documentation.  It always takes me a little bit to get my bearings again
>> once I have to change that code, but once I do, everything is
>> consistent.  And as I understand it, the external interface was written
>> primarily with libvirt in mind, so it would probably be difficult to
>> change it while remaining compatible.
>>
>>> Not to mention the performance
>>> issues with the built-in garbage collector
>>
>> What performance issues were you seeing with libxl's garbage collector?
>> I thought it just kept a list of pointers and freed them at the very end.
> 
> I didn't investigate too closely but on some occasions a considerable
> amount of the execution time was being spent in there according to
> callgrind. After everything was finished in a domain creation xl would
> just "hang" in there for a while before actually exiting. It was not
> very consistent and recompiling libxl sometimes sped things up.
> Haven't run into it since I've upgraded to debian buster and a newer
> gcc.

Is it possible this was actually doing the "async" parts of long-running
operations?  When no async callback is provided, the very last thing
that happens before a return is to  sleep and wait for the next thing to
happen, then call the next thing in the async chain.

 -George

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

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

[Nicholas Rosbrook]

> I return to the question I stated before.  At the moment, your bindings
> have the following call chain:
> 
> * DomainInfo(), hand-crafted.  Calls domainInfo().
> * domainInfo(), automaticall generated.  Calls C.libxl_domain_info().
> 
> The in-tree bindings have the following call chain:
> 
> * DomainInfo(), hand-crafted.  Calls C.libxl_domain_info().
> 
> Since DomainInfo() is hand-crafted in both cases, what's the advantage
> of having domainInfo() at all?

Point well taken.

> So just to clarify terminology: The IDL is the description language
> itself, which at the moment only contains information about the libxl
> structures.  We have generators for various C bits of libxl which read
> the IDL and spit out boilerplate C.  The idea would be that we write a
> new generator for Go which reads the IDL and spits out boilerplate Go.

Yes, I know. Sorry for the strange phrasing. I'll try again:

From what I understand, the IDL is only used to generate libxl types, and
the boiler-plate init, dispose, etc. functions. However, if we want to have a
generator that produces Go code that calls libxl functions, those function
signatures must be known during code generation. However, the description
of those functions is outside the scope of the IDL.

So, in order to write such a generator we would need to either:

1. Expand the IDL (significantly) so that function signatures could be described
in the general case.

2. Parse the C code.

With that said, what are your expectations for the generated Go code at this point?
Do you think we should try to generate the pieces that call into libxl? Or, do you think
the code generation should be limited to the structs and boiler-plate C <-> Go "type
marshaling?" 

> I looked at the thing about naked returns, and didn't really understand
> it; but anyway I'm happy to have things modified to be more Go-like.  I
> definitely "speak" Go with a funny accent.

TL;DR: Naked returns exist; don't use them (with the exception of defer'd closures if necessary).

> Can I say -- I've been going open-source for so long, that I feel almost
> unsafe when nobody reviews my stuff.  Most of this code was written by
> me and reviewed by nobody (since I was the only person interested); it's
> good to have someone else take a critical look at it.

Makes sense to me. Glad to be involved :)

> And if we had a an IDL for the libxl functions, we could have it
> generate the code above for the vast majority of cases.

I guess that answers my question above.

> If we wrote a generator from the IDL, we could make it smart enough to
> use []Disks as the type there, and make the marshallers know how to use
> num_disks to appropriately size the resulting slice and copy the right
> number of values across.  To do that with c-for-go, we'd have to do a
> lot of work teaching it what to do, if that's even possible.

Good point. AFAICT there isn't a way to provide such information to c-for-go.

> So you mean, for example, after DomainInfo() calls DomInfo.Deref(), it
> will then call libxl_dominfo_dispose() on the C struct?

Yes.

> Right, and personally I'm not in principle opposed to using c-for-go, if
> it can be made to generate code the way we like it.  My main fear is
> that we'll spend a bunch of time tweaking c-for-go, and after going back
> and forth and investing a lot of time in it, we'll end up either 1)
> giving up and writing our own generator anyway, or 2) accepting
> something sub-optimal because it's the best thing we could make c-for-go do.

I agree, those are legitimate concerns for using c-for-go. OTOH, one concern
I have for a custom generator is that it is indeed custom, and maybe wouldn't
be of much use outside of libxl. It would be great if the extra work in writing a new
generator meant that it could be used by other projects with similar needs. However,
I understand that concern may not be shared by others.

I think we have a decent enough idea for what a c-for-go version of this might look like. So,
what are the next steps in exploring the custom generator route?

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

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

[Nicholas Rosbrook]

> With that said, what are your expectations for the generated Go code at this point?
> Do you think we should try to generate the pieces that call into libxl? Or, do you think
> the code generation should be limited to the structs and boiler-plate C <-> Go "type
> marshaling?"

[...]

> I think we have a decent enough idea for what a c-for-go version of this might look like. So,
> what are the next steps in exploring the custom generator route?

Sorry to answer my own question, but I wanted to follow up with some thoughts I came up
with after I sent my last email.

I think maybe we should take the simpler approach. Meaning, the Go package would be
constructed as follows:

1. Generated code for Go types that are analogous to the C libxl types. The IDL should
already be able to provide enough information for a simple Go code generator (like gentypes.py).

2. Generated code to handle the marshaling between the pure-Go types and the C types. We
could tailor this piece to address the short-comings of c-for-go, e.g. the num_disks issue, preventing
use-after-free, etc.

3. Hand-written wrappers. As you stated before, there is not much difference between the in-tree
bindings calling C.libxl_domain_info, and my wrappers calling domainInfo() (besides the additional
layer in mine). And, we agree that writing those simple wrappers is pretty painless.

I think this is more or less what you have been suggesting, but I wanted to articulate the point that
I think if we go with a custom generator, we should not try do as much as c-for-go is trying do.

Thoughts on that?

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

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

[George Dunlap]

On 8/1/19 7:59 PM, Nicholas Rosbrook wrote:
>> With that said, what are your expectations for the generated Go code at this point?
>> Do you think we should try to generate the pieces that call into libxl? Or, do you think
>> the code generation should be limited to the structs and boiler-plate C <-> Go "type
>> marshaling?"
> 
> [...]
> 
>> I think we have a decent enough idea for what a c-for-go version of this might look like. So,
>> what are the next steps in exploring the custom generator route?
> 
> Sorry to answer my own question, but I wanted to follow up with some thoughts I came up
> with after I sent my last email.
> 
> I think maybe we should take the simpler approach. Meaning, the Go package would be
> constructed as follows:
> 
> 1. Generated code for Go types that are analogous to the C libxl types. The IDL should
> already be able to provide enough information for a simple Go code generator (like gentypes.py).
> 
> 2. Generated code to handle the marshaling between the pure-Go types and the C types. We
> could tailor this piece to address the short-comings of c-for-go, e.g. the num_disks issue, preventing
> use-after-free, etc.
> 
> 3. Hand-written wrappers. As you stated before, there is not much difference between the in-tree
> bindings calling C.libxl_domain_info, and my wrappers calling domainInfo() (besides the additional
> layer in mine). And, we agree that writing those simple wrappers is pretty painless.
> 
> I think this is more or less what you have been suggesting, but I wanted to articulate the point that
> I think if we go with a custom generator, we should not try do as much as c-for-go is trying do.

I was going to say, the idea of adding function signatures to the IDL
was just "exploring the state space" of approaches.  Parsing the
existing IDL to spit out Go structures and marshalling should be
reasonably straightforward.  Designing an IDL for the functions is a
fairly large design project, with all sorts of exciting
<strikethrough>bike sheds to paint</strikethrough> important design
decisions to make along the way: probably something to attempt if / when
we determine that it's worth the cost.

IOW, in response to GP, I was going to counter-suggest what you suggest
in this email. :-)

Are you up for taking a stab at something like `gengotypes.py`?

 -George

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

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

[George Dunlap]

On 7/31/19 10:22 PM, Nicholas Rosbrook wrote:
>> I looked at the thing about naked returns, and didn't really understand
>> it; but anyway I'm happy to have things modified to be more Go-like.  I
>> definitely "speak" Go with a funny accent.
> 
> TL;DR: Naked returns exist; don't use them (with the exception of defer'd closures if necessary).

Right, but that advice never made sense to me.  The main reason
deprecating them in the document you pointed to is that they cause
"GoDoc stuttering"; and that having an extra "allocation" line in your
function is worth it to make documentation clearer.  I agree with the
principle that clear documentation is important; but I wasn't clear what
the "stutter" looked like and why it was so bad.

The other reason listed is that you can accidentally mask your return
values; e.g. the following will return nil if Bar() returns an error,
which is probably not what the author intended:

func Foo() (err error) {
  if err := Bar(), err != nil {
    return;
  }
  ...
}

While the following avoids that issue:

func Foo() (error) {
  if err := Bar(), err != nil {
    return err;
  }
  ...
}

Normally I'm all for coding disciplines which add safety, but for some
reason this argument always seemed really weak to me.  I really like
naked returns as a feature, and would rather just be more careful to
avoid masking.

Anyway, I wouldn't necessarily nack a patch getting rid of naked returns
(particularly if a second person wanted it), but I'm not very keen.

 -George

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

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

[Nicholas Rosbrook]

> IOW, in response to GP, I was going to counter-suggest what you suggest
> in this email. :-)
>
> Are you up for taking a stab at something like `gengotypes.py`?

Yes, I think I can handle that.

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

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

[Nicholas Rosbrook]

George,

> Are you up for taking a stab at something like `gengotypes.py`?

I have was able to make a bit of progress on this over the weekend. I've started
`gengotypes.py` in my branch[1]; the portion which generates `xenlight_types.go`
(the counterpart to _libxl_types.h) is mostly working. 

The main exception is that I am not certain how the `KeyedUnion` type from the IDL
should be translated for Go. One option is to do something similar to the `oneof` field 
in gRPC's protobuf messages[2]. Essentially, we would define a separate struct for each
of the structs that belong to the union. Then, where a union would be used in C, we use
an interface type which the previously defined structs implement. E.g.

type isDomainTypeStruct interface {
        isDomainTypeStruct()
}

type domainTypeHVMStruct struct {
        ...
}

func (d domainTypeHVMStruct) isDomainTypeStruct() {}

type DomainBuildInfo struct {
        ...
        Type DomainType
        dts    isDomainTypeStruct
}

It is a bit ugly, but I think it's semantically the closest to 'KeyedUnion'. What are your thoughts?

-NR

[1] https://github.com/enr0n/xen/blob/gen-go-types/tools/golang/gengotypes.py
[2] https://developers.google.com/protocol-buffers/docs/proto3#oneof
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[George Dunlap]

On 9/4/19 1:36 AM, Nicholas Rosbrook wrote:
> George,
> 
>> Are you up for taking a stab at something like `gengotypes.py`?
> 
> I have was able to make a bit of progress on this over the weekend. I've started
> `gengotypes.py` in my branch[1]; the portion which generates `xenlight_types.go`
> (the counterpart to _libxl_types.h) is mostly working. 
> 
> The main exception is that I am not certain how the `KeyedUnion` type from the IDL
> should be translated for Go. One option is to do something similar to the `oneof` field 
> in gRPC's protobuf messages[2]. Essentially, we would define a separate struct for each
> of the structs that belong to the union. Then, where a union would be used in C, we use
> an interface type which the previously defined structs implement.

Yes, I think that's really the only option.  Poking around, it looks
like a lot of different people have recommended it; and the fact that
it's in use by gRPC means it can't be too terrible a solution.

The really annoying thing is that with the "interface-as-union", we
can't use anonymous types: we'll have to explicitly define the
{parent-struct} x {union-key} as a distinct type, and the is$TYPE()
method on each one.

> E.g.
> 
> type isDomainTypeStruct interface {
>         isDomainTypeStruct()
> }

The interface type itself will need to be exported, right?  (Obviously
we don't want to export the defining method.)

> type domainTypeHVMStruct struct {
>         ...
> }

So you've named the struct after the name of the key (libxl_domain_type)
and the key value (hvm); but I don't think that's sufficient.  Already
there are two different structures indexed by libxl_channel_connection:

typedef struct libxl_device_channel {
    libxl_domid backend_domid;
    char * backend_domname;
    libxl_devid devid;
    char * name;
    libxl_channel_connection connection;
    union {
        struct {
            char * path;
        } socket;
    } u;
} libxl_device_channel;

typedef struct libxl_channelinfo {
    char * backend;
    uint32_t backend_id;
    char * frontend;
    uint32_t frontend_id;
    libxl_devid devid;
    int state;
    int evtch;
    int rref;
    libxl_channel_connection connection;
    union {
        struct {
            char * path;
        } pty;
    } u;
} libxl_channelinfo;


(Note that in one, `socket` is defined, and in the other, `pty` is
defined.  I'm not sure that's not a bug, but anyway, that's what the IDL
allows.)

And there's no reason, theoretically, we couldn't have the following:

    ("u1", KeyedUnion(None, libxl_channel_connection, "connection",
           [/* One set of types */,
           ])),
    ("u2", KeyedUnion(None, libxl_channel_connection, "connection2",
           [/* A second set of types set of types */,
           ])),

So we need to include the element name as well.  But actually, I suppose
that means we don't actually need to include the type, since the element
name will be unique.

> func (d domainTypeHVMStruct) isDomainTypeStruct() {}
> 
> type DomainBuildInfo struct {
>         ...
>         Type DomainType
>         dts    isDomainTypeStruct
> }

...and then I'm afraid you'd need to have 'Dts' (should be exported,
right?) instead by the element specified by the IDL; so 'U' in all the
current cases.

This gives us:

type DomainBuildInfoU interface {
    isDomainBuildInfoU()
}

type DomainBuildInfoUHvmstruct {
  // ...
}

func (s DomainBuildInfoUHvm) isDomainBuildInfoU() { }

...

struct DomainBuildInfo {
    // ...
    Type DomainType
    U    DomainBuildInfoU
    // ...
}

Alternately, since the "key" element is also unique, we could use that
instead:

type DomainBuildInfoTypeUnion {
 // ...
}

struct DomainBuildInfo {
    // ...
    Type      DomainType
    TypeUnion DomainBuildInfoTypeUnion
    // ...
}

(And in the example given above where there are two keyed unions, one
would be "ConnectionUnion ParentTypeConnectionUnion" and
"Connection2Union ParentTypeConnection2Union".)

I think the second one looks prettier.  (Actually I think using 'u' as
the element name for the union was kind of a bad idea in the first
place.)  But that does mean we're 'overriding' the instructions of the
IDL (which prescribe both the key element name and the union element name).

What do you think?  If like me, you prefer the second one, I'll try to
ping Ian Jackson to make sure he doesn't have any objections to it.

 -George

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

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

[George Dunlap]

On 9/4/19 5:52 PM, George Dunlap wrote:
> On 9/4/19 1:36 AM, Nicholas Rosbrook wrote:
>> George,
>>
>>> Are you up for taking a stab at something like `gengotypes.py`?
>>
>> I have was able to make a bit of progress on this over the weekend. I've started
>> `gengotypes.py` in my branch[1]; the portion which generates `xenlight_types.go`
>> (the counterpart to _libxl_types.h) is mostly working. 
>>
>> The main exception is that I am not certain how the `KeyedUnion` type from the IDL
>> should be translated for Go. One option is to do something similar to the `oneof` field 
>> in gRPC's protobuf messages[2]. Essentially, we would define a separate struct for each
>> of the structs that belong to the union. Then, where a union would be used in C, we use
>> an interface type which the previously defined structs implement.
> 
> Yes, I think that's really the only option.  Poking around, it looks
> like a lot of different people have recommended it; and the fact that
> it's in use by gRPC means it can't be too terrible a solution.
> 
> The really annoying thing is that with the "interface-as-union", we
> can't use anonymous types: we'll have to explicitly define the
> {parent-struct} x {union-key} as a distinct type, and the is$TYPE()
> method on each one.
> 
>> E.g.
>>
>> type isDomainTypeStruct interface {
>>         isDomainTypeStruct()
>> }
> 
> The interface type itself will need to be exported, right?  (Obviously
> we don't want to export the defining method.)
> 
>> type domainTypeHVMStruct struct {
>>         ...
>> }
> 
> So you've named the struct after the name of the key (libxl_domain_type)
> and the key value (hvm); but I don't think that's sufficient.  Already
> there are two different structures indexed by libxl_channel_connection:
> 
> typedef struct libxl_device_channel {
>     libxl_domid backend_domid;
>     char * backend_domname;
>     libxl_devid devid;
>     char * name;
>     libxl_channel_connection connection;
>     union {
>         struct {
>             char * path;
>         } socket;
>     } u;
> } libxl_device_channel;
> 
> typedef struct libxl_channelinfo {
>     char * backend;
>     uint32_t backend_id;
>     char * frontend;
>     uint32_t frontend_id;
>     libxl_devid devid;
>     int state;
>     int evtch;
>     int rref;
>     libxl_channel_connection connection;
>     union {
>         struct {
>             char * path;
>         } pty;
>     } u;
> } libxl_channelinfo;
> 
> 
> (Note that in one, `socket` is defined, and in the other, `pty` is
> defined.  I'm not sure that's not a bug, but anyway, that's what the IDL
> allows.)
> 
> And there's no reason, theoretically, we couldn't have the following:
> 
>     ("u1", KeyedUnion(None, libxl_channel_connection, "connection",
>            [/* One set of types */,
>            ])),
>     ("u2", KeyedUnion(None, libxl_channel_connection, "connection2",
>            [/* A second set of types set of types */,
>            ])),
> 
> So we need to include the element name as well.  But actually, I suppose
> that means we don't actually need to include the type, since the element
> name will be unique.
> 
>> func (d domainTypeHVMStruct) isDomainTypeStruct() {}
>>
>> type DomainBuildInfo struct {
>>         ...
>>         Type DomainType
>>         dts    isDomainTypeStruct
>> }
> 
> ...and then I'm afraid you'd need to have 'Dts' (should be exported,
> right?) instead by the element specified by the IDL; so 'U' in all the
> current cases.
> 
> This gives us:
> 
> type DomainBuildInfoU interface {
>     isDomainBuildInfoU()
> }
> 
> type DomainBuildInfoUHvmstruct {
>   // ...
> }

Unfortunately this would mean the type assertion would be pretty long as
well:
  hvm := di.TypeUnion.(xenlight.DomainBuildInfoTypeUnionHvm)
  hvm.[element]

Compared to C:
  di.u.hvm.[element]

But unfortunately I don't think there's a way around that; that's just a
limitation of Go.

 -George

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

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

[Nicholas Rosbrook]

> Yes, I think that's really the only option.  Poking around, it looks
> like a lot of different people have recommended it; and the fact that
> it's in use by gRPC means it can't be too terrible a solution.

Yeah, having worked with generated gRPC code I don't think it's too bad.

> The interface type itself will need to be exported, right?  (Obviously
> we don't want to export the defining method.)

No actually, a struct field can be exported without its type being exported.
The code generated for gRPC does exactly that. It looks a little bit weird,
but it makes sense to do that in this scenario.

> So you've named the struct after the name of the key (libxl_domain_type)
> and the key value (hvm); but I don't think that's sufficient.  Already
> there are two different structures indexed by libxl_channel_connection:

Noted. I hadn't actually thought through the specifics of naming yet.

> ...and then I'm afraid you'd need to have 'Dts' (should be exported,
> right?) instead by the element specified by the IDL; so 'U' in all the
> current cases.

Yes, the field name needs to be exported unless we wanted to provide
getters/setters.

> I think the second one looks prettier.  (Actually I think using 'u' as
> the element name for the union was kind of a bad idea in the first
> place.)  But that does mean we're 'overriding' the instructions of the
> IDL (which prescribe both the key element name and the union element name).
> 
> What do you think?  If like me, you prefer the second one, I'll try to
> ping Ian Jackson to make sure he doesn't have any objections to it.

I agree, the second one looks better, except we won't export the interface type.

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

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

[Nicholas Rosbrook]

> Unfortunately this would mean the type assertion would be pretty long as
> well:
>   hvm := di.TypeUnion.(xenlight.DomainBuildInfoTypeUnionHvm)
>   hvm.[element]

Made worse by the fact that you really should check the type assertion first:

hvm, ok := di.TypeUnion.(xenlight.DomainBuildInfoTypeUnionHvm)
if !ok {
        //error
}

> But unfortunately I don't think there's a way around that; that's just a
> limitation of Go.

Right. If we wanted to make it easier on the users of the package, we *could*
add getters that hides the type assertion. But, that's still an extra step versus C.

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

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

[Nicholas Rosbrook]

Hi George,

I made some more progress on gengotypes.py [1].

First, I implemented the KeyedUnion translation that we talked about. 
You can see an example of the generated code in [2].

Second, I took a first pass at the C-to-Go type marshaling. I defined a
"marshaler" interface in [3], which allows the convenience function
`func fromC(m marshaler, ctype interface{}) (err error)`. My primary 
motivation for this interface is to allow the generated code to call panic
rather than checking for and handling errors. However, the previously
mentioned convenience function will recover from those panics and return
the appropriate error. So, each generated struct implements this interface.
See the generated code in [4].

You'll also notice in [4] that I defined C structs in the cgo preamble which
correspond to the Go KeyedUnion structs, e.g. DomainBuildInfoTypeUnionPv.
Since cgo treats C unions a byte slice, we need to do an unsafe.Pointer conversion
to some struct to be able to access the fields of a union. So, I thought it would
make the most sense to do the cast to a C type, and then convert those fields
to Go types accordingly. See [5] for example.

What are your thoughts on these implementations so far?

I was able to write a couple examples to demonstrate the generated code is 
working, but I had to make some small changes to the existing code WRT
libxl builtin types (not committed to my branch). So, I thought we should decide
how these builtin types will be defined in Go. This is what I was thinking so far:

Defbool (?)
Domid (already exists)
Devid => int
Uuid => [16]byte
Mac => [6]byte
Bitmap (already exists)
CpuidPolicyList (?)
StringList => [ ]string
KeyValueList => map[string]string
Hwcap (already exists, but should be re-factored to be like Bitmap to hide the C type)
MsVmGenid => [16]byte
EvLink (?)

Thoughts?

-NR

[1] https://github.com/enr0n/xen/blob/gen-go-types/tools/golang/gengotypes.py
[2] https://github.com/enr0n/xen/blob/gen-go-types/tools/golang/xenlight/xenlight_types.go#L511
[3] https://github.com/enr0n/xen/blob/gen-go-types/tools/golang/xenlight/xenlight_marshaler.go
[4] https://github.com/enr0n/xen/blob/gen-go-types/tools/golang/xenlight/xenlight_helpers.go
[5] https://github.com/enr0n/xen/blob/gen-go-types/tools/golang/xenlight/xenlight_helpers.go#L687
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[George Dunlap]

On 9/11/19 9:25 PM, Nicholas Rosbrook wrote:
> Hi George,
> 
> I made some more progress on gengotypes.py [1].
[snip]
> What are your thoughts on these implementations so far?

Great!  Overall it looks like it's really making progress, which is
exciting.

> First, I implemented the KeyedUnion translation that we talked about. 
> You can see an example of the generated code in [2].

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

I was chatting with Anthony today, and thought I'd just toss the idea
out there for discussion.

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.

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

type DomainInfo struct {
	/* etc */
	Type struct {
		Key int
		Hvm struct {
	   		Firmware string
			/* ... */
		}
		Pv struct {
	   		/* ... */
		}
		Pvh struct {
			/* ... */
		}
	}
}

This obviously means keeping a whole load of useless HVM and PV fields
around when you just want to run PVH, but it you can simply do this:

if ( di.Type.Key == libxl.DomainTypeHvm ) {
   /* ... */
   firmware := di.Type.Hvm.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.

> Second, I took a first pass at the C-to-Go type marshaling. I defined a
> "marshaler" interface in [3], which allows the convenience function
> `func fromC(m marshaler, ctype interface{}) (err error)`. My primary 
> motivation for this interface is to allow the generated code to call panic
> rather than checking for and handling errors. However, the previously
> mentioned convenience function will recover from those panics and return
> the appropriate error. So, each generated struct implements this interface.
> See the generated code in [4].

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.

I don't want to say I'd rule it out, but it doesn't seem to me like the
convenience is worth the cost (unless there's another advantage I'm
missing).

> You'll also notice in [4] that I defined C structs in the cgo preamble which
> correspond to the Go KeyedUnion structs, e.g. DomainBuildInfoTypeUnionPv.
> Since cgo treats C unions a byte slice, we need to do an unsafe.Pointer conversion
> to some struct to be able to access the fields of a union. So, I thought it would
> make the most sense to do the cast to a C type, and then convert those fields
> to Go types accordingly. See [5] for example.

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.

> I was able to write a couple examples to demonstrate the generated code is 
> working, but I had to make some small changes to the existing code WRT
> libxl builtin types (not committed to my branch). So, I thought we should decide
> how these builtin types will be defined in Go. This is what I was thinking so far:
> 
> Defbool (?)

Well this is really defined by the interface.  libxl.h has this defined as

struct {
   int val;
}


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

> Domid (already exists)
> Devid => int
> Uuid => [16]byte
> Mac => [6]byte
> Bitmap (already exists)

Yup

> CpuidPolicyList (?)

So this is an interesting one.  libxl__cpuid_policy is essentially a
type containing all non-exported fields.  (i.e., the actual elements are
defined inside libxl_internal.h, and the outside world only gets pointers).

And it's a list terminated by a specific value inside
libxl__cpuid_policy, which means the list itself is basically opaque
entirely.

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.

> StringList => [ ]string
> KeyValueList => map[string]string
> Hwcap (already exists, but should be re-factored to be like Bitmap to hide the C type)
> MsVmGenid => [16]byte

Should probably be C.LIBXL_MS_VM_GENID_LEN, but yes.

> EvLink (?)

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.

Thanks for all your work on this!

Peace,
 -George

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

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

[Nicholas Rosbrook]

> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[George Dunlap]

On 9/12/19 6:35 PM, Nicholas Rosbrook wrote:
> 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."

Fair enough.

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

Ah, right the main purpose was to have the single place at top to catch
 exceptions^W^W recover from panics, not so much because one form of the
function was nicer than the other one.  Still, I think static type
checking is a big thing to give up to make generated code cleaner.  Thanks.

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

Ack.

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

FWIW checked with Ian after I wrote this mail, and he confirmed that
that field (`link` in `libxl_event`) was only meant to be used
internally, and ideally we wouldn't even have that available in the Go
version of the struct (since it's not actually part of the public
interface).

Unfortunately we can't actually get rid of the element it without
special-casing it (which I don't think is a good idea), or adding a new
"PRIVATE()" annotation to the IDL or something (which would be nice to
have, but not something I expect anyone to have much time to do).  For
now, I think defining it as an empty struct will be good enough.

Great, thanks!  Look forward to the next iteration!

 -George

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

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

[Nicholas Rosbrook]

> FWIW checked with Ian after I wrote this mail, and he confirmed that
> that field (`link` in `libxl_event`) was only meant to be used
> internally, and ideally we wouldn't even have that available in the Go
> version of the struct (since it's not actually part of the public
> interface).
> 
> Unfortunately we can't actually get rid of the element it without
> special-casing it (which I don't think is a good idea), or adding a new
> "PRIVATE()" annotation to the IDL or something (which would be nice to
> have, but not something I expect anyone to have much time to do).  For
> now, I think defining it as an empty struct will be good enough.

Okay, good to know. I will do the empty struct definition for now. In the long run,
the addition to the IDL would probably be nice. My guess is that over time we will
identify other things we don't want to expose through the Go package for one
reason or another.

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

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

[Nicholas Rosbrook]

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.

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.

Best,
-NR

[1] https://github.com/enr0n/xen/blob/gen-go-types/tools/golang/xenlight/xenlight_helpers.go
[2] https://github.com/enr0n/xen/commit/70ec81acaf69df1c9446f94aeb4cebbe8983e6c5
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[George Dunlap]

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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Nicholas Rosbrook]

Hi George,

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

Okay that makes sense, thanks.

> 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?
>
> [...]
>
> What do you think?

No problem, I can do that. I'll send the patches along soon.

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