| Message ID | e2d8d6c1293c8251be881cd471265aa8188ae2ae.1586727061.git.rosbrookn@ainfosec.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | More wrappers for xenlight Go package | expand |
> On Apr 12, 2020, at 11:02 PM, 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 | 23 +++++++++++++++++++++++ > 1 file changed, 23 insertions(+) > > diff --git a/tools/golang/xenlight/xenlight.go b/tools/golang/xenlight/xenlight.go > index 3f1b0baa0c..8492bcec4e 100644 > --- a/tools/golang/xenlight/xenlight.go > +++ b/tools/golang/xenlight/xenlight.go > @@ -20,6 +20,7 @@ package xenlight > #cgo LDFLAGS: -lxenlight -lyajl -lxentoollog > #include <stdlib.h> > #include <libxl.h> > +#include <libxl_utils.h> > */ > import "C" > > @@ -124,6 +125,28 @@ func (ctx *Context) Close() error { > > type Domid uint32 > > +// NameToDomid returns the Domid for a domain, given its name, if it exists. > +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 ^Domid(0), Error(ret) libxl.h defines INVALID_DOMID — do we want to define an exported constant with the same name and use that here? (Although part of me wonders if DOMID_INVALID would be a better option.) > + } > + > + return Domid(domid), nil > +} > + > +// DomidToName returns the name for a domain, given its domid. > +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) > +} It looks to me like if the domid doesn’t exist, libxl_domid_to_name() will return NULL; and then DomidToName will return “”. Is that what we want? If so, it should probably be documented. One thing that might be worth pointing out is that both of these functions are actually racy: There’s no guarantee that by the time libxl_domid_to_name() returns that the domain with that name has died, and another domain with a different name has re-used the same domid. That’s partly why Xen has the whole “domain reaper” system, like for Unix processes (which so far isn’t implementable yet with the golang wrappers). I think when we make our “vm” library, we’re going to want to try to come up with something like an object that makes it easy to avoid this sort of race. But that’s a discussion for another day. :-) Everything else looks good, thanks. -George
> libxl.h defines INVALID_DOMID — do we want to define an exported constant with the same name and use that here? (Although part of me wonders if DOMID_INVALID would be a better option.) Yeah, that makes sense. I'll add that. > > + } > > + > > + return Domid(domid), nil > > +} > > + > > +// DomidToName returns the name for a domain, given its domid. > > +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) > > +} > > It looks to me like if the domid doesn’t exist, libxl_domid_to_name() will return NULL; and then DomidToName will return “”. Is that what we want? > > If so, it should probably be documented. I considered returning an error if C.GoString(cname) == "". But, with these functions (as well as the others in these series), I opted to keep the signatures aligned with their libxl counterparts since we're keeping the package API mostly one-to-one with libxl. I can add a second return value if you prefer, otherwise I'll just add a note in the comment. > One thing that might be worth pointing out is that both of these functions are actually racy: There’s no guarantee that by the time libxl_domid_to_name() returns that the domain with that name has died, and another domain with a different name has re-used the same domid. That’s partly why Xen has the whole “domain reaper” system, like for Unix processes (which so far isn’t implementable yet with the golang wrappers). I think when we make our “vm” library, we’re going to want to try to come up with something like an object that makes it easy to avoid this sort of race. That's good to know, thanks. I'll add that to the comments as well. > But that’s a discussion for another day. :-) Everything else looks good, thanks. Thanks! -NR
> On Apr 22, 2020, at 8:46 PM, Nick Rosbrook <rosbrookn@gmail.com> wrote: > >> libxl.h defines INVALID_DOMID — do we want to define an exported constant with the same name and use that here? (Although part of me wonders if DOMID_INVALID would be a better option.) > > Yeah, that makes sense. I'll add that. > >>> + } >>> + >>> + return Domid(domid), nil >>> +} >>> + >>> +// DomidToName returns the name for a domain, given its domid. >>> +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) >>> +} >> >> It looks to me like if the domid doesn’t exist, libxl_domid_to_name() will return NULL; and then DomidToName will return “”. Is that what we want? >> >> If so, it should probably be documented. > > I considered returning an error if C.GoString(cname) == "". But, with > these functions (as well as the others in these series), I opted to > keep the signatures aligned with their libxl counterparts since we're > keeping the package API mostly one-to-one with libxl. I can add a > second return value if you prefer, otherwise I'll just add a note in > the comment. OK — adding a note in the comment is fine. I mainly wanted to make sure the question had actually been considered (although of course documenting that behavior is also important). Thanks, -George
diff --git a/tools/golang/xenlight/xenlight.go b/tools/golang/xenlight/xenlight.go index 3f1b0baa0c..8492bcec4e 100644 --- a/tools/golang/xenlight/xenlight.go +++ b/tools/golang/xenlight/xenlight.go @@ -20,6 +20,7 @@ package xenlight #cgo LDFLAGS: -lxenlight -lyajl -lxentoollog #include <stdlib.h> #include <libxl.h> +#include <libxl_utils.h> */ import "C" @@ -124,6 +125,28 @@ func (ctx *Context) Close() error { type Domid uint32 +// NameToDomid returns the Domid for a domain, given its name, if it exists. +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 ^Domid(0), Error(ret) + } + + return Domid(domid), nil +} + +// DomidToName returns the name for a domain, given its domid. +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 | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)