| Message ID | 73e709cf0a87f3728de438e4a3b849462fd098ac.1587682041.git.rosbrookn@ainfosec.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | More wrappers for xenlight Go package | expand |
> 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
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
> 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
> > 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
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
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(-)