[PATCH 0/4] More wrappers for xenlight Go package

[PATCH 0/4] 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

Nick Rosbrook (4):
  golang/xenlight: add NameToDomid and DomidToName util functions
  golang/xenlight: add DeviceNicAdd/Remove wrappers
  golang/xenlight: add DevicePciAdd/Remove wrappers
  golang/xenlight: add DeviceUsbdevAdd/Remove wrappers

 tools/golang/xenlight/xenlight.go | 125 ++++++++++++++++++++++++++++++
 1 file changed, 125 insertions(+)

-- 
2.17.1

[PATCH 1/4] 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 | 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)
+	}
+
+	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
 
-- 
2.17.1

[PATCH 2/4] golang/xenlight: add DeviceNicAdd/Remove wrappers

[Nick Rosbrook]

Add DeviceNicAdd and DeviceNicRemove as wrappers for
libxl_device_nic_add and libxl_device_nic_remove.

Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com>
---
 tools/golang/xenlight/xenlight.go | 34 +++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/tools/golang/xenlight/xenlight.go b/tools/golang/xenlight/xenlight.go
index 8492bcec4e..a56f913b81 100644
--- a/tools/golang/xenlight/xenlight.go
+++ b/tools/golang/xenlight/xenlight.go
@@ -1068,3 +1068,37 @@ func (Ctx *Context) PrimaryConsoleGetTty(domid uint32) (path string, err error)
 	path = C.GoString(cpath)
 	return
 }
+
+// DeviceNicAdd adds a nic to a domain.
+func (Ctx *Context) DeviceNicAdd(domid Domid, nic *DeviceNic) error {
+	var cnic C.libxl_device_nic
+
+	if err := nic.toC(&cnic); err != nil {
+		return err
+	}
+	defer C.libxl_device_nic_dispose(&cnic)
+
+	ret := C.libxl_device_nic_add(Ctx.ctx, C.uint32_t(domid), &cnic, nil)
+	if ret != 0 {
+		return Error(ret)
+	}
+
+	return nil
+}
+
+// DeviceNicRemove removes a nic from a domain.
+func (Ctx *Context) DeviceNicRemove(domid Domid, nic *DeviceNic) error {
+	var cnic C.libxl_device_nic
+
+	if err := nic.toC(&cnic); err != nil {
+		return err
+	}
+	defer C.libxl_device_nic_dispose(&cnic)
+
+	ret := C.libxl_device_nic_remove(Ctx.ctx, C.uint32_t(domid), &cnic, nil)
+	if ret != 0 {
+		return Error(ret)
+	}
+
+	return nil
+}
-- 
2.17.1

[PATCH 3/4] golang/xenlight: add DevicePciAdd/Remove wrappers

[Nick Rosbrook]

Add DevicePciAdd and DevicePciRemove as wrappers for
libxl_device_pci_add and libxl_device_pci remove.

Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com>
---
 tools/golang/xenlight/xenlight.go | 34 +++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/tools/golang/xenlight/xenlight.go b/tools/golang/xenlight/xenlight.go
index a56f913b81..c94b046667 100644
--- a/tools/golang/xenlight/xenlight.go
+++ b/tools/golang/xenlight/xenlight.go
@@ -1102,3 +1102,37 @@ func (Ctx *Context) DeviceNicRemove(domid Domid, nic *DeviceNic) error {
 
 	return nil
 }
+
+// DevicePciAdd is used to passthrough a PCI device to a domain.
+func (Ctx *Context) DevicePciAdd(domid Domid, pci *DevicePci) error {
+	var cpci C.libxl_device_pci
+
+	if err := pci.toC(&cpci); err != nil {
+		return err
+	}
+	defer C.libxl_device_pci_dispose(&cpci)
+
+	ret := C.libxl_device_pci_add(Ctx.ctx, C.uint32_t(domid), &cpci, nil)
+	if ret != 0 {
+		return Error(ret)
+	}
+
+	return nil
+}
+
+// DevicePciRemove removes a PCI device from a domain.
+func (Ctx *Context) DevicePciRemove(domid Domid, pci *DevicePci) error {
+	var cpci C.libxl_device_pci
+
+	if err := pci.toC(&cpci); err != nil {
+		return err
+	}
+	defer C.libxl_device_pci_dispose(&cpci)
+
+	ret := C.libxl_device_pci_remove(Ctx.ctx, C.uint32_t(domid), &cpci, nil)
+	if ret != 0 {
+		return Error(ret)
+	}
+
+	return nil
+}
-- 
2.17.1

[PATCH 4/4] golang/xenlight: add DeviceUsbdevAdd/Remove wrappers

[Nick Rosbrook]

Add DeviceUsbdevAdd and DeviceUsbdevRemove as wrappers for
libxl_device_usbdev_add and libxl_device_usbdev_remove.

Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com>
---
 tools/golang/xenlight/xenlight.go | 34 +++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/tools/golang/xenlight/xenlight.go b/tools/golang/xenlight/xenlight.go
index c94b046667..2d45d2bd48 100644
--- a/tools/golang/xenlight/xenlight.go
+++ b/tools/golang/xenlight/xenlight.go
@@ -1136,3 +1136,37 @@ func (Ctx *Context) DevicePciRemove(domid Domid, pci *DevicePci) error {
 
 	return nil
 }
+
+// DeviceUsbdevAdd adds a USB device to a domain.
+func (Ctx *Context) DeviceUsbdevAdd(domid Domid, usbdev *DeviceUsbdev) error {
+	var cusbdev C.libxl_device_usbdev
+
+	if err := usbdev.toC(&cusbdev); err != nil {
+		return err
+	}
+	defer C.libxl_device_usbdev_dispose(&cusbdev)
+
+	ret := C.libxl_device_usbdev_add(Ctx.ctx, C.uint32_t(domid), &cusbdev, nil)
+	if ret != 0 {
+		return Error(ret)
+	}
+
+	return nil
+}
+
+// DeviceUsbdevRemove removes a USB device from a domain.
+func (Ctx *Context) DeviceUsbdevRemove(domid Domid, usbdev *DeviceUsbdev) error {
+	var cusbdev C.libxl_device_usbdev
+
+	if err := usbdev.toC(&cusbdev); err != nil {
+		return err
+	}
+	defer C.libxl_device_usbdev_dispose(&cusbdev)
+
+	ret := C.libxl_device_usbdev_remove(Ctx.ctx, C.uint32_t(domid), &cusbdev, nil)
+	if ret != 0 {
+		return Error(ret)
+	}
+
+	return nil
+}
-- 
2.17.1

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

[George Dunlap]

> 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

Re: [PATCH 2/4] golang/xenlight: add DeviceNicAdd/Remove wrappers

[George Dunlap]

> On Apr 12, 2020, at 11:02 PM, Nick Rosbrook <rosbrookn@gmail.com> wrote:
> 
> Add DeviceNicAdd and DeviceNicRemove as wrappers for
> libxl_device_nic_add and libxl_device_nic_remove.
> 
> Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com>
> ---
> tools/golang/xenlight/xenlight.go | 34 +++++++++++++++++++++++++++++++
> 1 file changed, 34 insertions(+)
> 
> diff --git a/tools/golang/xenlight/xenlight.go b/tools/golang/xenlight/xenlight.go
> index 8492bcec4e..a56f913b81 100644
> --- a/tools/golang/xenlight/xenlight.go
> +++ b/tools/golang/xenlight/xenlight.go
> @@ -1068,3 +1068,37 @@ func (Ctx *Context) PrimaryConsoleGetTty(domid uint32) (path string, err error)
> 	path = C.GoString(cpath)
> 	return
> }
> +
> +// DeviceNicAdd adds a nic to a domain.
> +func (Ctx *Context) DeviceNicAdd(domid Domid, nic *DeviceNic) error {
> +	var cnic C.libxl_device_nic
> +
> +	if err := nic.toC(&cnic); err != nil {
> +		return err
> +	}
> +	defer C.libxl_device_nic_dispose(&cnic)
> +
> +	ret := C.libxl_device_nic_add(Ctx.ctx, C.uint32_t(domid), &cnic, nil)
> +	if ret != 0 {
> +		return Error(ret)
> +	}
> +
> +	return nil
> +}
> +
> +// DeviceNicRemove removes a nic from a domain.

I feel like I want to say here what it is you actually have to fill in to remove the nic; but after 10 minutes of poking around the code, I’m not actually sure myself. :-)  (I think it *might* be just Devid and BackendDomid.)

So I’ll give this for now:

Reviewed-by: George Dunlap <george.dunlap@citrix.com>

And if I find it before I finish reviewing the end of the series, we can check it in and look at improving the documentation later.

 -George

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

[Nick Rosbrook]

> 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

Re: [PATCH 2/4] golang/xenlight: add DeviceNicAdd/Remove wrappers

[Nick Rosbrook]

> I feel like I want to say here what it is you actually have to fill in to remove the nic; but after 10 minutes of poking around the code, I’m not actually sure myself. :-)  (I think it *might* be just Devid and BackendDomid.)

IIRC, you can use just the MAC or devid. I was using just the MAC when
I tested it out.

> So I’ll give this for now:
>
> Reviewed-by: George Dunlap <george.dunlap@citrix.com>
>
> And if I find it before I finish reviewing the end of the series, we can check it in and look at improving the documentation later.

Sounds good, thanks!

-NR

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

[George Dunlap]

> 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

Re: [PATCH 3/4] golang/xenlight: add DevicePciAdd/Remove wrappers

[George Dunlap]

> On Apr 12, 2020, at 11:02 PM, Nick Rosbrook <rosbrookn@gmail.com> wrote:
> 
> Add DevicePciAdd and DevicePciRemove as wrappers for
> libxl_device_pci_add and libxl_device_pci remove.
> 
> Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com>

For 4.14, we should really look at adding functions to the IDL so that all this can be auto-generated.

Reviewed-by: George Dunlap <george.dunlap@citrix.com>

Re: [PATCH 4/4] golang/xenlight: add DeviceUsbdevAdd/Remove wrappers

[George Dunlap]

> On Apr 12, 2020, at 11:02 PM, Nick Rosbrook <rosbrookn@gmail.com> wrote:
> 
> Add DeviceUsbdevAdd and DeviceUsbdevRemove as wrappers for
> libxl_device_usbdev_add and libxl_device_usbdev_remove.
> 
> Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com>

Reviewed-by: George Dunlap <george.dunlap@citrix.com>

Re: [PATCH 0/4] More wrappers for xenlight Go package

[George Dunlap]

> On Apr 12, 2020, at 11:02 PM, Nick Rosbrook <rosbrookn@gmail.com> wrote:
> 
> 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
> 
> Nick Rosbrook (4):
>  golang/xenlight: add NameToDomid and DomidToName util functions
>  golang/xenlight: add DeviceNicAdd/Remove wrappers
>  golang/xenlight: add DevicePciAdd/Remove wrappers
>  golang/xenlight: add DeviceUsbdevAdd/Remove wrappers

FYI I’ve pushed the last three to staging.

 -George

Re: [PATCH 3/4] golang/xenlight: add DevicePciAdd/Remove wrappers

[Nick Rosbrook]

On Thu, Apr 23, 2020 at 6:22 AM George Dunlap <George.Dunlap@citrix.com> wrote:
>
>
>
> > On Apr 12, 2020, at 11:02 PM, Nick Rosbrook <rosbrookn@gmail.com> wrote:
> >
> > Add DevicePciAdd and DevicePciRemove as wrappers for
> > libxl_device_pci_add and libxl_device_pci remove.
> >
> > Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com>
>
> For 4.14, we should really look at adding functions to the IDL so that all this can be auto-generated.

Agreed, there is obvious repetition here.

> Reviewed-by: George Dunlap <george.dunlap@citrix.com>

Thanks!

-NR