[PATCH v2 0/1] More wrappers for xenlight Go package

[PATCH v2 0/1] More wrappers for xenlight Go package

[Nick Rosbrook]

This series adds wrappers to the xenlight package for various libxl
functions, which are now trivial to add with the generated types and 
marshaling helpers. In particular, these are functions that would allow
redctl to begin making the transition to using the xenlight package. For 
reference, I have started an experimental branch where I am using these
functions in redctl [1].

[1] https://gitlab.com/enr0n/redctl/-/blob/1bdf7b515654cc030e095f3a630a24530f930c00/internal/server/xenlight_xen_driver.go

Changes in v2:
 - Define constant DomidInvalid in first patch for use in NameToDomid
 - Add more detail to function comments

Nick Rosbrook (1):
  golang/xenlight: add NameToDomid and DomidToName util functions

 tools/golang/xenlight/xenlight.go | 38 ++++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

-- 
2.17.1

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

[Nick Rosbrook]

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@ainfosec.com>
---
 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)
+)
+
 func (e Error) Error() string {
 	if s, ok := libxlErrors[e]; ok {
 		return s
@@ -190,6 +194,38 @@ func (ctx *Context) Close() error {
 
 type Domid uint32
 
+// NameToDomid returns the Domid for a domain, given its name, if it exists.
+//
+// NameToDomid does not guarantee that the domid associated with name at
+// the time NameToDomid is called is the same as the domid associated with
+// name at the time NameToDomid returns.
+func (Ctx *Context) NameToDomid(name string) (Domid, error) {
+	var domid C.uint32_t
+
+	cname := C.CString(name)
+	defer C.free(unsafe.Pointer(cname))
+
+	if ret := C.libxl_name_to_domid(Ctx.ctx, cname, &domid); ret != 0 {
+		return DomidInvalid, Error(ret)
+	}
+
+	return Domid(domid), nil
+}
+
+// DomidToName returns the name for a domain, given its domid. If there
+// is no domain with the given domid, DomidToName will return the empty
+// string.
+//
+// DomidToName does not guarantee that the name (if any) associated with domid
+// at the time DomidToName is called is the same as the name (if any) associated
+// with domid at the time DomidToName returns.
+func (Ctx *Context) DomidToName(domid Domid) string {
+	cname := C.libxl_domid_to_name(Ctx.ctx, C.uint32_t(domid))
+	defer C.free(unsafe.Pointer(cname))
+
+	return C.GoString(cname)
+}
+
 // Devid is a device ID.
 type Devid int
 
-- 
2.17.1

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

[George Dunlap]

> On Apr 24, 2020, at 4:02 AM, Nick Rosbrook <rosbrookn@gmail.com> 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@ainfosec.com>
> ---
> 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

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

[Nick Rosbrook]

On Mon, Apr 27, 2020 at 8:59 AM George Dunlap <George.Dunlap@citrix.com> wrote:
>
>
>
> > On Apr 24, 2020, at 4:02 AM, Nick Rosbrook <rosbrookn@gmail.com> 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@ainfosec.com>
> > ---
> > 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

I just tested this way, and it does not work as expected. Since
C.INVALID_DOMID is #define'd, cgo does not know it is intended to be
used as uint32_t, and decides to declare C.INVALID_DOMID as int. So,
C.INVALID_DOMID = ^int(0) = -1, which overflows Domid.

I tried a few things in the cgo preamble without any luck.
Essentially, one cannot define a const uint32_t in C space, and use
that to define a const in Go space.

Any ideas?

-NR

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

[George Dunlap]

[-- Attachment #1: Type: text/plain, Size: 2874 bytes --]



> On Apr 27, 2020, at 7:51 PM, Nick Rosbrook <rosbrookn@gmail.com> wrote:
> 
> On Mon, Apr 27, 2020 at 8:59 AM George Dunlap <George.Dunlap@citrix.com> wrote:
>> 
>> 
>> 
>>> On Apr 24, 2020, at 4:02 AM, Nick Rosbrook <rosbrookn@gmail.com> 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@ainfosec.com>
>>> ---
>>> 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
> 
> I just tested this way, and it does not work as expected. Since
> C.INVALID_DOMID is #define'd, cgo does not know it is intended to be
> used as uint32_t, and decides to declare C.INVALID_DOMID as int. So,
> C.INVALID_DOMID = ^int(0) = -1, which overflows Domid.
> 
> I tried a few things in the cgo preamble without any luck.
> Essentially, one cannot define a const uint32_t in C space, and use
> that to define a const in Go space.
> 
> Any ideas?

The following seems to work for me.  In the C section:

// #define INVALID_DOMID_TYPED ((uint32_t)INVALID_DOMID)

Then:

    DomidInvalid Domid = C.INVALID_DOMID_TYPED

Attached is a PoC.  What do you think?

 -George


[-- Attachment #2: invalid-domid-test.go --]
[-- Type: application/octet-stream, Size: 315 bytes --]

package main

import (
	"fmt"
)

// #include <stdint.h>
// #define INVALID_DOMID ~0
//
// #define INVALID_DOMID_VALUE ((uint32_t)INVALID_DOMID)
import "C"

type Domid uint32

const DomidInvalid Domid = C.INVALID_DOMID_VALUE

func main() {
	fmt.Printf("INVALID_DOMID value: %v\n", DomidInvalid)
}

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

[Nick Rosbrook]

> > I just tested this way, and it does not work as expected. Since
> > C.INVALID_DOMID is #define'd, cgo does not know it is intended to be
> > used as uint32_t, and decides to declare C.INVALID_DOMID as int. So,
> > C.INVALID_DOMID = ^int(0) = -1, which overflows Domid.
> >
> > I tried a few things in the cgo preamble without any luck.
> > Essentially, one cannot define a const uint32_t in C space, and use
> > that to define a const in Go space.
> >
> > Any ideas?
>
> The following seems to work for me.  In the C section:
>
> // #define INVALID_DOMID_TYPED ((uint32_t)INVALID_DOMID)
>
> Then:
>
>     DomidInvalid Domid = C.INVALID_DOMID_TYPED
>
> Attached is a PoC.  What do you think?

Yes, that looks good.

Thanks,
-NR