Re: [PATCH v2 1/1] golang/xenlight: add NameToDomid and DomidToName util functions

[George Dunlap <George.Dunlap@xxxxxxxxxx>]

> On Apr 24, 2020, at 4:02 AM, Nick Rosbrook <rosbrookn@xxxxxxxxx> wrote:
> 
> Many exported functions in xenlight require a domid as an argument. Make
> it easier for package users to use these functions by adding wrappers
> for the libxl utility functions libxl_name_to_domid and
> libxl_domid_to_name.
> 
> Signed-off-by: Nick Rosbrook <rosbrookn@xxxxxxxxxxxx>
> ---
> tools/golang/xenlight/xenlight.go | 38 ++++++++++++++++++++++++++++++-
> 1 file changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/golang/xenlight/xenlight.go 
> b/tools/golang/xenlight/xenlight.go
> index 6b4f492550..d1d30b63e1 100644
> --- a/tools/golang/xenlight/xenlight.go
> +++ b/tools/golang/xenlight/xenlight.go
> @@ -21,13 +21,13 @@ package xenlight
> #cgo LDFLAGS: -lxenlight -lyajl -lxentoollog
> #include <stdlib.h>
> #include <libxl.h>
> +#include <libxl_utils.h>
> 
> static const libxl_childproc_hooks childproc_hooks = { .chldowner = 
> libxl_sigchld_owner_mainloop };
> 
> void xenlight_set_chldproc(libxl_ctx *ctx) {
>       libxl_childproc_setmode(ctx, &childproc_hooks, NULL);
> }
> -
> */
> import "C"
> 
> @@ -75,6 +75,10 @@ var libxlErrors = map[Error]string{
>       ErrorFeatureRemoved:               "Feature removed",
> }
> 
> +const (
> +     DomidInvalid Domid = ^Domid(0)

Not to be a stickler, but are we positive that this will always result in the 
same value as C.INVALID_DOMID?

There are some functions that will return INVALID_DOMID, or accept 
INVALID_DOMID as an argument.  Users of the `xenlight` package will presumably 
need to compare against this value and/or pass it in.

It seems like we could:

1. Rely on language lawyering to justify our assumption that ^Domid(0) will 
always be the same as C “~0”

2. On marshalling into / out of C, convert C.INVALID_DOMID to DomidInvalid

3. Set Domid = C.INVALID_DOMID

If you’re confident in your language lawyering skills, I could accept #1; but 
I’d prefer a comment with some sort of reference.  But if it were me I’d just 
go with #3; particularly given that this value could theoretically change 
(libxl has a stable API, not a stable ABI).

Everything else looks good.

If you want I could s/~Domid(0)/C.INVALID_DOMID/; myself, add my R-b and check 
it in.

 -George