[RESEND PATCH 00/12] golang/xenlight: domain life cycle support

[RESEND PATCH 00/12] golang/xenlight: domain life cycle support

[Nick Rosbrook]

The primary goal of this patch series is to allow users of the xenlight
package to manage a full domain life cycle. In particular, it provides
support for receiving domain death events so that domain shutdown,
reboot, destroy, etc. can be handled. And, it addresses issues found
when using the package to boot domains with various configurations.

These patches address several things (e.g. bug fixes, code style,
conveniences, new wrapper functions), but are all work towards the final
goal of allowing a package user to manage a full domain life cycle.

Nick Rosbrook (12):
  golang/xenlight: update generated code
  golang/xenlight: fix StringList toC conversion
  golang/xenlight: fix string conversion in generated toC functions
  golang/xenlight: export keyed union interface types
  golang/xenlight: use struct pointers in keyed union fields
  golang/xenlight: rename Ctx receivers to ctx
  golang/xenlight: add logging conveniences for within xenlight
  golang/xenlight: add functional options to configure Context
  golang/xenlight: add DomainDestroy wrapper
  golang/xenlight: add SendTrigger wrapper
  golang/xenlight: do not negate ret when converting to Error
  golang/xenlight: add NotifyDomainDeath method to Context

 tools/golang/xenlight/gengotypes.py  |  11 +-
 tools/golang/xenlight/helpers.gen.go | 210 ++++++++++++--
 tools/golang/xenlight/types.gen.go   |  63 +++--
 tools/golang/xenlight/xenlight.go    | 398 ++++++++++++++++++++-------
 4 files changed, 521 insertions(+), 161 deletions(-)

-- 
2.17.1

[RESEND PATCH 01/12] golang/xenlight: update generated code

[Nick Rosbrook]

Re-generate code to reflect changes to libxl_types.idl from the
following commits:

0570d7f276 x86/msr: introduce an option for compatible MSR behavior selection
7e5cffcd1e viridian: allow vCPU hotplug for Windows VMs
9835246710 viridian: remove implicit limit of 64 VPs per partition

Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com>
---
 tools/golang/xenlight/helpers.gen.go | 6 ++++++
 tools/golang/xenlight/types.gen.go   | 5 +++++
 2 files changed, 11 insertions(+)

diff --git a/tools/golang/xenlight/helpers.gen.go b/tools/golang/xenlight/helpers.gen.go
index 4c60d27a9c..b454b12d52 100644
--- a/tools/golang/xenlight/helpers.gen.go
+++ b/tools/golang/xenlight/helpers.gen.go
@@ -1113,6 +1113,9 @@ default:
 return fmt.Errorf("invalid union key '%v'", x.Type)}
 x.ArchArm.GicVersion = GicVersion(xc.arch_arm.gic_version)
 x.ArchArm.Vuart = VuartType(xc.arch_arm.vuart)
+if err := x.ArchX86.MsrRelaxed.fromC(&xc.arch_x86.msr_relaxed);err != nil {
+return fmt.Errorf("converting field ArchX86.MsrRelaxed: %v", err)
+}
 x.Altp2M = Altp2MMode(xc.altp2m)
 x.VmtraceBufKb = int(xc.vmtrace_buf_kb)
 
@@ -1589,6 +1592,9 @@ default:
 return fmt.Errorf("invalid union key '%v'", x.Type)}
 xc.arch_arm.gic_version = C.libxl_gic_version(x.ArchArm.GicVersion)
 xc.arch_arm.vuart = C.libxl_vuart_type(x.ArchArm.Vuart)
+if err := x.ArchX86.MsrRelaxed.toC(&xc.arch_x86.msr_relaxed); err != nil {
+return fmt.Errorf("converting field ArchX86.MsrRelaxed: %v", err)
+}
 xc.altp2m = C.libxl_altp2m_mode(x.Altp2M)
 xc.vmtrace_buf_kb = C.int(x.VmtraceBufKb)
 
diff --git a/tools/golang/xenlight/types.gen.go b/tools/golang/xenlight/types.gen.go
index cb13002fdb..f2ceceb61c 100644
--- a/tools/golang/xenlight/types.gen.go
+++ b/tools/golang/xenlight/types.gen.go
@@ -211,6 +211,8 @@ ViridianEnlightenmentSynic ViridianEnlightenment = 7
 ViridianEnlightenmentStimer ViridianEnlightenment = 8
 ViridianEnlightenmentHcallIpi ViridianEnlightenment = 9
 ViridianEnlightenmentExProcessorMasks ViridianEnlightenment = 10
+ViridianEnlightenmentNoVpLimit ViridianEnlightenment = 11
+ViridianEnlightenmentCpuHotplug ViridianEnlightenment = 12
 )
 
 type Hdtype int
@@ -513,6 +515,9 @@ ArchArm struct {
 GicVersion GicVersion
 Vuart VuartType
 }
+ArchX86 struct {
+MsrRelaxed Defbool
+}
 Altp2M Altp2MMode
 VmtraceBufKb int
 }
-- 
2.17.1

Re: [RESEND PATCH 01/12] golang/xenlight: update generated code

[George Dunlap]

> On May 24, 2021, at 9:36 PM, Nick Rosbrook <rosbrookn@gmail.com> wrote:
> 
> Re-generate code to reflect changes to libxl_types.idl from the
> following commits:
> 
> 0570d7f276 x86/msr: introduce an option for compatible MSR behavior selection
> 7e5cffcd1e viridian: allow vCPU hotplug for Windows VMs
> 9835246710 viridian: remove implicit limit of 64 VPs per partition
> 
> Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com>

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

[RESEND PATCH 02/12] golang/xenlight: fix StringList toC conversion

[Nick Rosbrook]

The current implementation of StringList.toC does not correctly account
for how libxl_string_list is expected to be laid out in C, which is clear
when one looks at libxl_string_list_length in libxl.c. In particular,
StringList.toC does not account for the extra memory that should be
allocated for the "sentinel" entry. And, when using the "slice trick" to
create a slice that can address C memory, the unsafe.Pointer conversion
should be on a C.libxl_string_list, not *C.libxl_string_list.

Fix these problems by (1) allocating an extra slot in the slice used to
address the C memory, and explicity set the last entry to nil so the C
memory will be zeroed out, and (2) dereferencing csl in the
unsafe.Pointer conversion.

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

diff --git a/tools/golang/xenlight/xenlight.go b/tools/golang/xenlight/xenlight.go
index b9189dec5c..13171d0ad1 100644
--- a/tools/golang/xenlight/xenlight.go
+++ b/tools/golang/xenlight/xenlight.go
@@ -491,13 +491,14 @@ func (sl *StringList) fromC(csl *C.libxl_string_list) error {
 
 func (sl StringList) toC(csl *C.libxl_string_list) error {
 	var char *C.char
-	size := len(sl)
+	size := len(sl) + 1
 	*csl = (C.libxl_string_list)(C.malloc(C.ulong(size) * C.ulong(unsafe.Sizeof(char))))
-	clist := (*[1 << 30]*C.char)(unsafe.Pointer(csl))[:size:size]
+	clist := (*[1 << 30]*C.char)(unsafe.Pointer(*csl))[:size:size]
 
 	for i, v := range sl {
 		clist[i] = C.CString(v)
 	}
+	clist[len(clist)-1] = nil
 
 	return nil
 }
-- 
2.17.1

Re: [RESEND PATCH 02/12] golang/xenlight: fix StringList toC conversion

[George Dunlap]

> On May 24, 2021, at 9:36 PM, Nick Rosbrook <rosbrookn@gmail.com> wrote:
> 
> The current implementation of StringList.toC does not correctly account
> for how libxl_string_list is expected to be laid out in C, which is clear
> when one looks at libxl_string_list_length in libxl.c. In particular,
> StringList.toC does not account for the extra memory that should be
> allocated for the "sentinel" entry. And, when using the "slice trick" to
> create a slice that can address C memory, the unsafe.Pointer conversion
> should be on a C.libxl_string_list, not *C.libxl_string_list.
> 
> Fix these problems by (1) allocating an extra slot in the slice used to
> address the C memory, and explicity set the last entry to nil so the C
> memory will be zeroed out, and (2) dereferencing csl in the
> unsafe.Pointer conversion.
> 
> Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com>

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

Even as I was making these I thought it might be good to try to get some unit tests of this conversion.

[RESEND PATCH 03/12] golang/xenlight: fix string conversion in generated toC functions

[Nick Rosbrook]

In gengotypes.py, the toC functions only set C string fields when
the Go strings are non-empty. However, to prevent segfaults in some
cases, these fields should always at least be set to nil so that the C
memory is zeroed out.

Update gengotypes.py so that the generated code always sets these fields
to nil first, and then proceeds to check if the Go string is non-empty.
And, commit the new generated code.

Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com>
---
 tools/golang/xenlight/gengotypes.py  |   1 +
 tools/golang/xenlight/helpers.gen.go | 160 +++++++++++++++++++++++++++
 2 files changed, 161 insertions(+)

diff --git a/tools/golang/xenlight/gengotypes.py b/tools/golang/xenlight/gengotypes.py
index 3e40c3d5dc..e6daa9b92f 100644
--- a/tools/golang/xenlight/gengotypes.py
+++ b/tools/golang/xenlight/gengotypes.py
@@ -527,6 +527,7 @@ def xenlight_golang_convert_to_C(ty = None, outer_name = None,
 
     elif gotypename == 'string':
         # Use the cgo helper for converting C strings.
+        s += '{0}.{1} = nil\n'.format(cvarname, cname)
         s += 'if {0}.{1} != "" {{\n'.format(govarname,goname)
         s += '{0}.{1} = C.CString({2}.{3})}}\n'.format(cvarname,cname,
                                                    govarname,goname)
diff --git a/tools/golang/xenlight/helpers.gen.go b/tools/golang/xenlight/helpers.gen.go
index b454b12d52..5222898fb8 100644
--- a/tools/golang/xenlight/helpers.gen.go
+++ b/tools/golang/xenlight/helpers.gen.go
@@ -154,8 +154,10 @@ C.libxl_vnc_info_dispose(xc)}
 if err := x.Enable.toC(&xc.enable); err != nil {
 return fmt.Errorf("converting field Enable: %v", err)
 }
+xc.listen = nil
 if x.Listen != "" {
 xc.listen = C.CString(x.Listen)}
+xc.passwd = nil
 if x.Passwd != "" {
 xc.passwd = C.CString(x.Passwd)}
 xc.display = C.int(x.Display)
@@ -216,11 +218,13 @@ return fmt.Errorf("converting field Enable: %v", err)
 }
 xc.port = C.int(x.Port)
 xc.tls_port = C.int(x.TlsPort)
+xc.host = nil
 if x.Host != "" {
 xc.host = C.CString(x.Host)}
 if err := x.DisableTicketing.toC(&xc.disable_ticketing); err != nil {
 return fmt.Errorf("converting field DisableTicketing: %v", err)
 }
+xc.passwd = nil
 if x.Passwd != "" {
 xc.passwd = C.CString(x.Passwd)}
 if err := x.AgentMouse.toC(&xc.agent_mouse); err != nil {
@@ -233,8 +237,10 @@ if err := x.ClipboardSharing.toC(&xc.clipboard_sharing); err != nil {
 return fmt.Errorf("converting field ClipboardSharing: %v", err)
 }
 xc.usbredirection = C.int(x.Usbredirection)
+xc.image_compression = nil
 if x.ImageCompression != "" {
 xc.image_compression = C.CString(x.ImageCompression)}
+xc.streaming_video = nil
 if x.StreamingVideo != "" {
 xc.streaming_video = C.CString(x.StreamingVideo)}
 
@@ -278,8 +284,10 @@ return fmt.Errorf("converting field Enable: %v", err)
 if err := x.Opengl.toC(&xc.opengl); err != nil {
 return fmt.Errorf("converting field Opengl: %v", err)
 }
+xc.display = nil
 if x.Display != "" {
 xc.display = C.CString(x.Display)}
+xc.xauthority = nil
 if x.Xauthority != "" {
 xc.xauthority = C.CString(x.Xauthority)}
 
@@ -337,6 +345,7 @@ return fmt.Errorf("converting field Uuid: %v", err)
 }
 xc.domid = C.libxl_domid(x.Domid)
 xc.ssidref = C.uint32_t(x.Ssidref)
+xc.ssid_label = nil
 if x.SsidLabel != "" {
 xc.ssid_label = C.CString(x.SsidLabel)}
 xc.running = C.bool(x.Running)
@@ -391,6 +400,7 @@ C.libxl_cpupoolinfo_dispose(xc)}
 }()
 
 xc.poolid = C.uint32_t(x.Poolid)
+xc.pool_name = nil
 if x.PoolName != "" {
 xc.pool_name = C.CString(x.PoolName)}
 xc.sched = C.libxl_scheduler(x.Sched)
@@ -458,9 +468,11 @@ if err != nil{
 C.libxl_channelinfo_dispose(xc)}
 }()
 
+xc.backend = nil
 if x.Backend != "" {
 xc.backend = C.CString(x.Backend)}
 xc.backend_id = C.uint32_t(x.BackendId)
+xc.frontend = nil
 if x.Frontend != "" {
 xc.frontend = C.CString(x.Frontend)}
 xc.frontend_id = C.uint32_t(x.FrontendId)
@@ -478,6 +490,7 @@ if !ok {
 return errors.New("wrong type for union key connection")
 }
 var pty C.libxl_channelinfo_connection_union_pty
+pty.path = nil
 if tmp.Path != "" {
 pty.path = C.CString(tmp.Path)}
 ptyBytes := C.GoBytes(unsafe.Pointer(&pty),C.sizeof_libxl_channelinfo_connection_union_pty)
@@ -563,24 +576,33 @@ C.libxl_version_info_dispose(xc)}
 
 xc.xen_version_major = C.int(x.XenVersionMajor)
 xc.xen_version_minor = C.int(x.XenVersionMinor)
+xc.xen_version_extra = nil
 if x.XenVersionExtra != "" {
 xc.xen_version_extra = C.CString(x.XenVersionExtra)}
+xc.compiler = nil
 if x.Compiler != "" {
 xc.compiler = C.CString(x.Compiler)}
+xc.compile_by = nil
 if x.CompileBy != "" {
 xc.compile_by = C.CString(x.CompileBy)}
+xc.compile_domain = nil
 if x.CompileDomain != "" {
 xc.compile_domain = C.CString(x.CompileDomain)}
+xc.compile_date = nil
 if x.CompileDate != "" {
 xc.compile_date = C.CString(x.CompileDate)}
+xc.capabilities = nil
 if x.Capabilities != "" {
 xc.capabilities = C.CString(x.Capabilities)}
+xc.changeset = nil
 if x.Changeset != "" {
 xc.changeset = C.CString(x.Changeset)}
 xc.virt_start = C.uint64_t(x.VirtStart)
 xc.pagesize = C.int(x.Pagesize)
+xc.commandline = nil
 if x.Commandline != "" {
 xc.commandline = C.CString(x.Commandline)}
+xc.build_id = nil
 if x.BuildId != "" {
 xc.build_id = C.CString(x.BuildId)}
 
@@ -650,8 +672,10 @@ if err := x.Oos.toC(&xc.oos); err != nil {
 return fmt.Errorf("converting field Oos: %v", err)
 }
 xc.ssidref = C.uint32_t(x.Ssidref)
+xc.ssid_label = nil
 if x.SsidLabel != "" {
 xc.ssid_label = C.CString(x.SsidLabel)}
+xc.name = nil
 if x.Name != "" {
 xc.name = C.CString(x.Name)}
 xc.domid = C.libxl_domid(x.Domid)
@@ -665,6 +689,7 @@ if err := x.Platformdata.toC(&xc.platformdata); err != nil {
 return fmt.Errorf("converting field Platformdata: %v", err)
 }
 xc.poolid = C.uint32_t(x.Poolid)
+xc.pool_name = nil
 if x.PoolName != "" {
 xc.pool_name = C.CString(x.PoolName)}
 if err := x.RunHotplugScripts.toC(&xc.run_hotplug_scripts); err != nil {
@@ -712,6 +737,7 @@ C.libxl_domain_restore_params_dispose(xc)}
 
 xc.checkpointed_stream = C.int(x.CheckpointedStream)
 xc.stream_version = C.uint32_t(x.StreamVersion)
+xc.colo_proxy_script = nil
 if x.ColoProxyScript != "" {
 xc.colo_proxy_script = C.CString(x.ColoProxyScript)}
 if err := x.UserspaceColoProxy.toC(&xc.userspace_colo_proxy); err != nil {
@@ -1312,6 +1338,7 @@ xc.shadow_memkb = C.uint64_t(x.ShadowMemkb)
 xc.iommu_memkb = C.uint64_t(x.IommuMemkb)
 xc.rtc_timeoffset = C.uint32_t(x.RtcTimeoffset)
 xc.exec_ssidref = C.uint32_t(x.ExecSsidref)
+xc.exec_ssid_label = nil
 if x.ExecSsidLabel != "" {
 xc.exec_ssid_label = C.CString(x.ExecSsidLabel)}
 if err := x.Localtime.toC(&xc.localtime); err != nil {
@@ -1323,6 +1350,7 @@ return fmt.Errorf("converting field DisableMigrate: %v", err)
 if err := x.Cpuid.toC(&xc.cpuid); err != nil {
 return fmt.Errorf("converting field Cpuid: %v", err)
 }
+xc.blkdev_start = nil
 if x.BlkdevStart != "" {
 xc.blkdev_start = C.CString(x.BlkdevStart)}
 if numVnumaNodes := len(x.VnumaNodes); numVnumaNodes > 0 {
@@ -1342,15 +1370,20 @@ if err := x.DeviceModelStubdomain.toC(&xc.device_model_stubdomain); err != nil {
 return fmt.Errorf("converting field DeviceModelStubdomain: %v", err)
 }
 xc.stubdomain_memkb = C.uint64_t(x.StubdomainMemkb)
+xc.stubdomain_kernel = nil
 if x.StubdomainKernel != "" {
 xc.stubdomain_kernel = C.CString(x.StubdomainKernel)}
+xc.stubdomain_ramdisk = nil
 if x.StubdomainRamdisk != "" {
 xc.stubdomain_ramdisk = C.CString(x.StubdomainRamdisk)}
+xc.device_model = nil
 if x.DeviceModel != "" {
 xc.device_model = C.CString(x.DeviceModel)}
 xc.device_model_ssidref = C.uint32_t(x.DeviceModelSsidref)
+xc.device_model_ssid_label = nil
 if x.DeviceModelSsidLabel != "" {
 xc.device_model_ssid_label = C.CString(x.DeviceModelSsidLabel)}
+xc.device_model_user = nil
 if x.DeviceModelUser != "" {
 xc.device_model_user = C.CString(x.DeviceModelUser)}
 if err := x.Extra.toC(&xc.extra); err != nil {
@@ -1397,17 +1430,22 @@ if err := x.ClaimMode.toC(&xc.claim_mode); err != nil {
 return fmt.Errorf("converting field ClaimMode: %v", err)
 }
 xc.event_channels = C.uint32_t(x.EventChannels)
+xc.kernel = nil
 if x.Kernel != "" {
 xc.kernel = C.CString(x.Kernel)}
+xc.cmdline = nil
 if x.Cmdline != "" {
 xc.cmdline = C.CString(x.Cmdline)}
+xc.ramdisk = nil
 if x.Ramdisk != "" {
 xc.ramdisk = C.CString(x.Ramdisk)}
+xc.device_tree = nil
 if x.DeviceTree != "" {
 xc.device_tree = C.CString(x.DeviceTree)}
 if err := x.Acpi.toC(&xc.acpi); err != nil {
 return fmt.Errorf("converting field Acpi: %v", err)
 }
+xc.bootloader = nil
 if x.Bootloader != "" {
 xc.bootloader = C.CString(x.Bootloader)}
 if err := x.BootloaderArgs.toC(&xc.bootloader_args); err != nil {
@@ -1432,6 +1470,7 @@ if !ok {
 return errors.New("wrong type for union key type")
 }
 var hvm C.libxl_domain_build_info_type_union_hvm
+hvm.firmware = nil
 if tmp.Firmware != "" {
 hvm.firmware = C.CString(tmp.Firmware)}
 hvm.bios = C.libxl_bios_type(tmp.Bios)
@@ -1465,6 +1504,7 @@ return fmt.Errorf("converting field ViridianEnable: %v", err)
 if err := tmp.ViridianDisable.toC(&hvm.viridian_disable); err != nil {
 return fmt.Errorf("converting field ViridianDisable: %v", err)
 }
+hvm.timeoffset = nil
 if tmp.Timeoffset != "" {
 hvm.timeoffset = C.CString(tmp.Timeoffset)}
 if err := tmp.Hpet.toC(&hvm.hpet); err != nil {
@@ -1481,10 +1521,13 @@ return fmt.Errorf("converting field NestedHvm: %v", err)
 if err := tmp.Altp2M.toC(&hvm.altp2m); err != nil {
 return fmt.Errorf("converting field Altp2M: %v", err)
 }
+hvm.system_firmware = nil
 if tmp.SystemFirmware != "" {
 hvm.system_firmware = C.CString(tmp.SystemFirmware)}
+hvm.smbios_firmware = nil
 if tmp.SmbiosFirmware != "" {
 hvm.smbios_firmware = C.CString(tmp.SmbiosFirmware)}
+hvm.acpi_firmware = nil
 if tmp.AcpiFirmware != "" {
 hvm.acpi_firmware = C.CString(tmp.AcpiFirmware)}
 hvm.hdtype = C.libxl_hdtype(tmp.Hdtype)
@@ -1497,6 +1540,7 @@ return fmt.Errorf("converting field Vga: %v", err)
 if err := tmp.Vnc.toC(&hvm.vnc); err != nil {
 return fmt.Errorf("converting field Vnc: %v", err)
 }
+hvm.keymap = nil
 if tmp.Keymap != "" {
 hvm.keymap = C.CString(tmp.Keymap)}
 if err := tmp.Sdl.toC(&hvm.sdl); err != nil {
@@ -1509,19 +1553,23 @@ if err := tmp.GfxPassthru.toC(&hvm.gfx_passthru); err != nil {
 return fmt.Errorf("converting field GfxPassthru: %v", err)
 }
 hvm.gfx_passthru_kind = C.libxl_gfx_passthru_kind(tmp.GfxPassthruKind)
+hvm.serial = nil
 if tmp.Serial != "" {
 hvm.serial = C.CString(tmp.Serial)}
+hvm.boot = nil
 if tmp.Boot != "" {
 hvm.boot = C.CString(tmp.Boot)}
 if err := tmp.Usb.toC(&hvm.usb); err != nil {
 return fmt.Errorf("converting field Usb: %v", err)
 }
 hvm.usbversion = C.int(tmp.Usbversion)
+hvm.usbdevice = nil
 if tmp.Usbdevice != "" {
 hvm.usbdevice = C.CString(tmp.Usbdevice)}
 if err := tmp.VkbDevice.toC(&hvm.vkb_device); err != nil {
 return fmt.Errorf("converting field VkbDevice: %v", err)
 }
+hvm.soundhw = nil
 if tmp.Soundhw != "" {
 hvm.soundhw = C.CString(tmp.Soundhw)}
 if err := tmp.XenPlatformPci.toC(&hvm.xen_platform_pci); err != nil {
@@ -1550,18 +1598,23 @@ if !ok {
 return errors.New("wrong type for union key type")
 }
 var pv C.libxl_domain_build_info_type_union_pv
+pv.kernel = nil
 if tmp.Kernel != "" {
 pv.kernel = C.CString(tmp.Kernel)}
 pv.slack_memkb = C.uint64_t(tmp.SlackMemkb)
+pv.bootloader = nil
 if tmp.Bootloader != "" {
 pv.bootloader = C.CString(tmp.Bootloader)}
 if err := tmp.BootloaderArgs.toC(&pv.bootloader_args); err != nil {
 return fmt.Errorf("converting field BootloaderArgs: %v", err)
 }
+pv.cmdline = nil
 if tmp.Cmdline != "" {
 pv.cmdline = C.CString(tmp.Cmdline)}
+pv.ramdisk = nil
 if tmp.Ramdisk != "" {
 pv.ramdisk = C.CString(tmp.Ramdisk)}
+pv.features = nil
 if tmp.Features != "" {
 pv.features = C.CString(tmp.Features)}
 if err := tmp.E820Host.toC(&pv.e820_host); err != nil {
@@ -1578,10 +1631,13 @@ var pvh C.libxl_domain_build_info_type_union_pvh
 if err := tmp.Pvshim.toC(&pvh.pvshim); err != nil {
 return fmt.Errorf("converting field Pvshim: %v", err)
 }
+pvh.pvshim_path = nil
 if tmp.PvshimPath != "" {
 pvh.pvshim_path = C.CString(tmp.PvshimPath)}
+pvh.pvshim_cmdline = nil
 if tmp.PvshimCmdline != "" {
 pvh.pvshim_cmdline = C.CString(tmp.PvshimCmdline)}
+pvh.pvshim_extra = nil
 if tmp.PvshimExtra != "" {
 pvh.pvshim_extra = C.CString(tmp.PvshimExtra)}
 pvhBytes := C.GoBytes(unsafe.Pointer(&pvh),C.sizeof_libxl_domain_build_info_type_union_pvh)
@@ -1635,6 +1691,7 @@ C.libxl_device_vfb_dispose(xc)}
 }()
 
 xc.backend_domid = C.libxl_domid(x.BackendDomid)
+xc.backend_domname = nil
 if x.BackendDomname != "" {
 xc.backend_domname = C.CString(x.BackendDomname)}
 xc.devid = C.libxl_devid(x.Devid)
@@ -1644,6 +1701,7 @@ return fmt.Errorf("converting field Vnc: %v", err)
 if err := x.Sdl.toC(&xc.sdl); err != nil {
 return fmt.Errorf("converting field Sdl: %v", err)
 }
+xc.keymap = nil
 if x.Keymap != "" {
 xc.keymap = C.CString(x.Keymap)}
 
@@ -1689,10 +1747,12 @@ C.libxl_device_vkb_dispose(xc)}
 }()
 
 xc.backend_domid = C.libxl_domid(x.BackendDomid)
+xc.backend_domname = nil
 if x.BackendDomname != "" {
 xc.backend_domname = C.CString(x.BackendDomname)}
 xc.devid = C.libxl_devid(x.Devid)
 xc.backend_type = C.libxl_vkb_backend(x.BackendType)
+xc.unique_id = nil
 if x.UniqueId != "" {
 xc.unique_id = C.CString(x.UniqueId)}
 xc.feature_disable_keyboard = C.bool(x.FeatureDisableKeyboard)
@@ -1758,14 +1818,18 @@ C.libxl_device_disk_dispose(xc)}
 }()
 
 xc.backend_domid = C.libxl_domid(x.BackendDomid)
+xc.backend_domname = nil
 if x.BackendDomname != "" {
 xc.backend_domname = C.CString(x.BackendDomname)}
+xc.pdev_path = nil
 if x.PdevPath != "" {
 xc.pdev_path = C.CString(x.PdevPath)}
+xc.vdev = nil
 if x.Vdev != "" {
 xc.vdev = C.CString(x.Vdev)}
 xc.backend = C.libxl_disk_backend(x.Backend)
 xc.format = C.libxl_disk_format(x.Format)
+xc.script = nil
 if x.Script != "" {
 xc.script = C.CString(x.Script)}
 xc.removable = C.int(x.Removable)
@@ -1781,13 +1845,17 @@ return fmt.Errorf("converting field ColoEnable: %v", err)
 if err := x.ColoRestoreEnable.toC(&xc.colo_restore_enable); err != nil {
 return fmt.Errorf("converting field ColoRestoreEnable: %v", err)
 }
+xc.colo_host = nil
 if x.ColoHost != "" {
 xc.colo_host = C.CString(x.ColoHost)}
 xc.colo_port = C.int(x.ColoPort)
+xc.colo_export = nil
 if x.ColoExport != "" {
 xc.colo_export = C.CString(x.ColoExport)}
+xc.active_disk = nil
 if x.ActiveDisk != "" {
 xc.active_disk = C.CString(x.ActiveDisk)}
+xc.hidden_disk = nil
 if x.HiddenDisk != "" {
 xc.hidden_disk = C.CString(x.HiddenDisk)}
 
@@ -1883,124 +1951,180 @@ C.libxl_device_nic_dispose(xc)}
 }()
 
 xc.backend_domid = C.libxl_domid(x.BackendDomid)
+xc.backend_domname = nil
 if x.BackendDomname != "" {
 xc.backend_domname = C.CString(x.BackendDomname)}
 xc.devid = C.libxl_devid(x.Devid)
 xc.mtu = C.int(x.Mtu)
+xc.model = nil
 if x.Model != "" {
 xc.model = C.CString(x.Model)}
 if err := x.Mac.toC(&xc.mac); err != nil {
 return fmt.Errorf("converting field Mac: %v", err)
 }
+xc.ip = nil
 if x.Ip != "" {
 xc.ip = C.CString(x.Ip)}
+xc.bridge = nil
 if x.Bridge != "" {
 xc.bridge = C.CString(x.Bridge)}
+xc.ifname = nil
 if x.Ifname != "" {
 xc.ifname = C.CString(x.Ifname)}
+xc.script = nil
 if x.Script != "" {
 xc.script = C.CString(x.Script)}
 xc.nictype = C.libxl_nic_type(x.Nictype)
 xc.rate_bytes_per_interval = C.uint64_t(x.RateBytesPerInterval)
 xc.rate_interval_usecs = C.uint32_t(x.RateIntervalUsecs)
+xc.gatewaydev = nil
 if x.Gatewaydev != "" {
 xc.gatewaydev = C.CString(x.Gatewaydev)}
+xc.coloft_forwarddev = nil
 if x.ColoftForwarddev != "" {
 xc.coloft_forwarddev = C.CString(x.ColoftForwarddev)}
+xc.colo_sock_mirror_id = nil
 if x.ColoSockMirrorId != "" {
 xc.colo_sock_mirror_id = C.CString(x.ColoSockMirrorId)}
+xc.colo_sock_mirror_ip = nil
 if x.ColoSockMirrorIp != "" {
 xc.colo_sock_mirror_ip = C.CString(x.ColoSockMirrorIp)}
+xc.colo_sock_mirror_port = nil
 if x.ColoSockMirrorPort != "" {
 xc.colo_sock_mirror_port = C.CString(x.ColoSockMirrorPort)}
+xc.colo_sock_compare_pri_in_id = nil
 if x.ColoSockComparePriInId != "" {
 xc.colo_sock_compare_pri_in_id = C.CString(x.ColoSockComparePriInId)}
+xc.colo_sock_compare_pri_in_ip = nil
 if x.ColoSockComparePriInIp != "" {
 xc.colo_sock_compare_pri_in_ip = C.CString(x.ColoSockComparePriInIp)}
+xc.colo_sock_compare_pri_in_port = nil
 if x.ColoSockComparePriInPort != "" {
 xc.colo_sock_compare_pri_in_port = C.CString(x.ColoSockComparePriInPort)}
+xc.colo_sock_compare_sec_in_id = nil
 if x.ColoSockCompareSecInId != "" {
 xc.colo_sock_compare_sec_in_id = C.CString(x.ColoSockCompareSecInId)}
+xc.colo_sock_compare_sec_in_ip = nil
 if x.ColoSockCompareSecInIp != "" {
 xc.colo_sock_compare_sec_in_ip = C.CString(x.ColoSockCompareSecInIp)}
+xc.colo_sock_compare_sec_in_port = nil
 if x.ColoSockCompareSecInPort != "" {
 xc.colo_sock_compare_sec_in_port = C.CString(x.ColoSockCompareSecInPort)}
+xc.colo_sock_compare_notify_id = nil
 if x.ColoSockCompareNotifyId != "" {
 xc.colo_sock_compare_notify_id = C.CString(x.ColoSockCompareNotifyId)}
+xc.colo_sock_compare_notify_ip = nil
 if x.ColoSockCompareNotifyIp != "" {
 xc.colo_sock_compare_notify_ip = C.CString(x.ColoSockCompareNotifyIp)}
+xc.colo_sock_compare_notify_port = nil
 if x.ColoSockCompareNotifyPort != "" {
 xc.colo_sock_compare_notify_port = C.CString(x.ColoSockCompareNotifyPort)}
+xc.colo_sock_redirector0_id = nil
 if x.ColoSockRedirector0Id != "" {
 xc.colo_sock_redirector0_id = C.CString(x.ColoSockRedirector0Id)}
+xc.colo_sock_redirector0_ip = nil
 if x.ColoSockRedirector0Ip != "" {
 xc.colo_sock_redirector0_ip = C.CString(x.ColoSockRedirector0Ip)}
+xc.colo_sock_redirector0_port = nil
 if x.ColoSockRedirector0Port != "" {
 xc.colo_sock_redirector0_port = C.CString(x.ColoSockRedirector0Port)}
+xc.colo_sock_redirector1_id = nil
 if x.ColoSockRedirector1Id != "" {
 xc.colo_sock_redirector1_id = C.CString(x.ColoSockRedirector1Id)}
+xc.colo_sock_redirector1_ip = nil
 if x.ColoSockRedirector1Ip != "" {
 xc.colo_sock_redirector1_ip = C.CString(x.ColoSockRedirector1Ip)}
+xc.colo_sock_redirector1_port = nil
 if x.ColoSockRedirector1Port != "" {
 xc.colo_sock_redirector1_port = C.CString(x.ColoSockRedirector1Port)}
+xc.colo_sock_redirector2_id = nil
 if x.ColoSockRedirector2Id != "" {
 xc.colo_sock_redirector2_id = C.CString(x.ColoSockRedirector2Id)}
+xc.colo_sock_redirector2_ip = nil
 if x.ColoSockRedirector2Ip != "" {
 xc.colo_sock_redirector2_ip = C.CString(x.ColoSockRedirector2Ip)}
+xc.colo_sock_redirector2_port = nil
 if x.ColoSockRedirector2Port != "" {
 xc.colo_sock_redirector2_port = C.CString(x.ColoSockRedirector2Port)}
+xc.colo_filter_mirror_queue = nil
 if x.ColoFilterMirrorQueue != "" {
 xc.colo_filter_mirror_queue = C.CString(x.ColoFilterMirrorQueue)}
+xc.colo_filter_mirror_outdev = nil
 if x.ColoFilterMirrorOutdev != "" {
 xc.colo_filter_mirror_outdev = C.CString(x.ColoFilterMirrorOutdev)}
+xc.colo_filter_redirector0_queue = nil
 if x.ColoFilterRedirector0Queue != "" {
 xc.colo_filter_redirector0_queue = C.CString(x.ColoFilterRedirector0Queue)}
+xc.colo_filter_redirector0_indev = nil
 if x.ColoFilterRedirector0Indev != "" {
 xc.colo_filter_redirector0_indev = C.CString(x.ColoFilterRedirector0Indev)}
+xc.colo_filter_redirector0_outdev = nil
 if x.ColoFilterRedirector0Outdev != "" {
 xc.colo_filter_redirector0_outdev = C.CString(x.ColoFilterRedirector0Outdev)}
+xc.colo_filter_redirector1_queue = nil
 if x.ColoFilterRedirector1Queue != "" {
 xc.colo_filter_redirector1_queue = C.CString(x.ColoFilterRedirector1Queue)}
+xc.colo_filter_redirector1_indev = nil
 if x.ColoFilterRedirector1Indev != "" {
 xc.colo_filter_redirector1_indev = C.CString(x.ColoFilterRedirector1Indev)}
+xc.colo_filter_redirector1_outdev = nil
 if x.ColoFilterRedirector1Outdev != "" {
 xc.colo_filter_redirector1_outdev = C.CString(x.ColoFilterRedirector1Outdev)}
+xc.colo_compare_pri_in = nil
 if x.ColoComparePriIn != "" {
 xc.colo_compare_pri_in = C.CString(x.ColoComparePriIn)}
+xc.colo_compare_sec_in = nil
 if x.ColoCompareSecIn != "" {
 xc.colo_compare_sec_in = C.CString(x.ColoCompareSecIn)}
+xc.colo_compare_out = nil
 if x.ColoCompareOut != "" {
 xc.colo_compare_out = C.CString(x.ColoCompareOut)}
+xc.colo_compare_notify_dev = nil
 if x.ColoCompareNotifyDev != "" {
 xc.colo_compare_notify_dev = C.CString(x.ColoCompareNotifyDev)}
+xc.colo_sock_sec_redirector0_id = nil
 if x.ColoSockSecRedirector0Id != "" {
 xc.colo_sock_sec_redirector0_id = C.CString(x.ColoSockSecRedirector0Id)}
+xc.colo_sock_sec_redirector0_ip = nil
 if x.ColoSockSecRedirector0Ip != "" {
 xc.colo_sock_sec_redirector0_ip = C.CString(x.ColoSockSecRedirector0Ip)}
+xc.colo_sock_sec_redirector0_port = nil
 if x.ColoSockSecRedirector0Port != "" {
 xc.colo_sock_sec_redirector0_port = C.CString(x.ColoSockSecRedirector0Port)}
+xc.colo_sock_sec_redirector1_id = nil
 if x.ColoSockSecRedirector1Id != "" {
 xc.colo_sock_sec_redirector1_id = C.CString(x.ColoSockSecRedirector1Id)}
+xc.colo_sock_sec_redirector1_ip = nil
 if x.ColoSockSecRedirector1Ip != "" {
 xc.colo_sock_sec_redirector1_ip = C.CString(x.ColoSockSecRedirector1Ip)}
+xc.colo_sock_sec_redirector1_port = nil
 if x.ColoSockSecRedirector1Port != "" {
 xc.colo_sock_sec_redirector1_port = C.CString(x.ColoSockSecRedirector1Port)}
+xc.colo_filter_sec_redirector0_queue = nil
 if x.ColoFilterSecRedirector0Queue != "" {
 xc.colo_filter_sec_redirector0_queue = C.CString(x.ColoFilterSecRedirector0Queue)}
+xc.colo_filter_sec_redirector0_indev = nil
 if x.ColoFilterSecRedirector0Indev != "" {
 xc.colo_filter_sec_redirector0_indev = C.CString(x.ColoFilterSecRedirector0Indev)}
+xc.colo_filter_sec_redirector0_outdev = nil
 if x.ColoFilterSecRedirector0Outdev != "" {
 xc.colo_filter_sec_redirector0_outdev = C.CString(x.ColoFilterSecRedirector0Outdev)}
+xc.colo_filter_sec_redirector1_queue = nil
 if x.ColoFilterSecRedirector1Queue != "" {
 xc.colo_filter_sec_redirector1_queue = C.CString(x.ColoFilterSecRedirector1Queue)}
+xc.colo_filter_sec_redirector1_indev = nil
 if x.ColoFilterSecRedirector1Indev != "" {
 xc.colo_filter_sec_redirector1_indev = C.CString(x.ColoFilterSecRedirector1Indev)}
+xc.colo_filter_sec_redirector1_outdev = nil
 if x.ColoFilterSecRedirector1Outdev != "" {
 xc.colo_filter_sec_redirector1_outdev = C.CString(x.ColoFilterSecRedirector1Outdev)}
+xc.colo_filter_sec_rewriter0_queue = nil
 if x.ColoFilterSecRewriter0Queue != "" {
 xc.colo_filter_sec_rewriter0_queue = C.CString(x.ColoFilterSecRewriter0Queue)}
+xc.colo_checkpoint_host = nil
 if x.ColoCheckpointHost != "" {
 xc.colo_checkpoint_host = C.CString(x.ColoCheckpointHost)}
+xc.colo_checkpoint_port = nil
 if x.ColoCheckpointPort != "" {
 xc.colo_checkpoint_port = C.CString(x.ColoCheckpointPort)}
 
@@ -2053,6 +2177,7 @@ xc.power_mgmt = C.bool(x.PowerMgmt)
 xc.permissive = C.bool(x.Permissive)
 xc.seize = C.bool(x.Seize)
 xc.rdm_policy = C.libxl_rdm_reserve_policy(x.RdmPolicy)
+xc.name = nil
 if x.Name != "" {
 xc.name = C.CString(x.Name)}
 
@@ -2126,6 +2251,7 @@ xc.devid = C.libxl_devid(x.Devid)
 xc.version = C.int(x.Version)
 xc.ports = C.int(x.Ports)
 xc.backend_domid = C.libxl_domid(x.BackendDomid)
+xc.backend_domname = nil
 if x.BackendDomname != "" {
 xc.backend_domname = C.CString(x.BackendDomname)}
 
@@ -2223,6 +2349,7 @@ if err != nil{
 C.libxl_device_dtdev_dispose(xc)}
 }()
 
+xc.path = nil
 if x.Path != "" {
 xc.path = C.CString(x.Path)}
 
@@ -2259,6 +2386,7 @@ C.libxl_device_vtpm_dispose(xc)}
 }()
 
 xc.backend_domid = C.libxl_domid(x.BackendDomid)
+xc.backend_domname = nil
 if x.BackendDomname != "" {
 xc.backend_domname = C.CString(x.BackendDomname)}
 xc.devid = C.libxl_devid(x.Devid)
@@ -2299,12 +2427,16 @@ C.libxl_device_p9_dispose(xc)}
 }()
 
 xc.backend_domid = C.libxl_domid(x.BackendDomid)
+xc.backend_domname = nil
 if x.BackendDomname != "" {
 xc.backend_domname = C.CString(x.BackendDomname)}
+xc.tag = nil
 if x.Tag != "" {
 xc.tag = C.CString(x.Tag)}
+xc.path = nil
 if x.Path != "" {
 xc.path = C.CString(x.Path)}
+xc.security_model = nil
 if x.SecurityModel != "" {
 xc.security_model = C.CString(x.SecurityModel)}
 xc.devid = C.libxl_devid(x.Devid)
@@ -2339,6 +2471,7 @@ C.libxl_device_pvcallsif_dispose(xc)}
 }()
 
 xc.backend_domid = C.libxl_domid(x.BackendDomid)
+xc.backend_domname = nil
 if x.BackendDomname != "" {
 xc.backend_domname = C.CString(x.BackendDomname)}
 xc.devid = C.libxl_devid(x.Devid)
@@ -2399,9 +2532,11 @@ C.libxl_device_channel_dispose(xc)}
 }()
 
 xc.backend_domid = C.libxl_domid(x.BackendDomid)
+xc.backend_domname = nil
 if x.BackendDomname != "" {
 xc.backend_domname = C.CString(x.BackendDomname)}
 xc.devid = C.libxl_devid(x.Devid)
+xc.name = nil
 if x.Name != "" {
 xc.name = C.CString(x.Name)}
 xc.connection = C.libxl_channel_connection(x.Connection)
@@ -2416,6 +2551,7 @@ if !ok {
 return errors.New("wrong type for union key connection")
 }
 var socket C.libxl_device_channel_connection_union_socket
+socket.path = nil
 if tmp.Path != "" {
 socket.path = C.CString(tmp.Path)}
 socketBytes := C.GoBytes(unsafe.Pointer(&socket),C.sizeof_libxl_device_channel_connection_union_socket)
@@ -2452,6 +2588,7 @@ if err != nil{
 C.libxl_connector_param_dispose(xc)}
 }()
 
+xc.unique_id = nil
 if x.UniqueId != "" {
 xc.unique_id = C.CString(x.UniqueId)}
 xc.width = C.uint32_t(x.Width)
@@ -2497,6 +2634,7 @@ C.libxl_device_vdispl_dispose(xc)}
 }()
 
 xc.backend_domid = C.libxl_domid(x.BackendDomid)
+xc.backend_domname = nil
 if x.BackendDomname != "" {
 xc.backend_domname = C.CString(x.BackendDomname)}
 xc.devid = C.libxl_devid(x.Devid)
@@ -2608,6 +2746,7 @@ if err != nil{
 C.libxl_vsnd_stream_dispose(xc)}
 }()
 
+xc.unique_id = nil
 if x.UniqueId != "" {
 xc.unique_id = C.CString(x.UniqueId)}
 xc._type = C.libxl_vsnd_stream_type(x.Type)
@@ -2654,6 +2793,7 @@ if err != nil{
 C.libxl_vsnd_pcm_dispose(xc)}
 }()
 
+xc.name = nil
 if x.Name != "" {
 xc.name = C.CString(x.Name)}
 if err := x.Params.toC(&xc.params); err != nil {
@@ -2714,11 +2854,14 @@ C.libxl_device_vsnd_dispose(xc)}
 }()
 
 xc.backend_domid = C.libxl_domid(x.BackendDomid)
+xc.backend_domname = nil
 if x.BackendDomname != "" {
 xc.backend_domname = C.CString(x.BackendDomname)}
 xc.devid = C.libxl_devid(x.Devid)
+xc.short_name = nil
 if x.ShortName != "" {
 xc.short_name = C.CString(x.ShortName)}
+xc.long_name = nil
 if x.LongName != "" {
 xc.long_name = C.CString(x.LongName)}
 if err := x.Params.toC(&xc.params); err != nil {
@@ -3103,9 +3246,11 @@ if err != nil{
 C.libxl_diskinfo_dispose(xc)}
 }()
 
+xc.backend = nil
 if x.Backend != "" {
 xc.backend = C.CString(x.Backend)}
 xc.backend_id = C.uint32_t(x.BackendId)
+xc.frontend = nil
 if x.Frontend != "" {
 xc.frontend = C.CString(x.Frontend)}
 xc.frontend_id = C.uint32_t(x.FrontendId)
@@ -3149,9 +3294,11 @@ if err != nil{
 C.libxl_nicinfo_dispose(xc)}
 }()
 
+xc.backend = nil
 if x.Backend != "" {
 xc.backend = C.CString(x.Backend)}
 xc.backend_id = C.uint32_t(x.BackendId)
+xc.frontend = nil
 if x.Frontend != "" {
 xc.frontend = C.CString(x.Frontend)}
 xc.frontend_id = C.uint32_t(x.FrontendId)
@@ -3198,9 +3345,11 @@ if err != nil{
 C.libxl_vtpminfo_dispose(xc)}
 }()
 
+xc.backend = nil
 if x.Backend != "" {
 xc.backend = C.CString(x.Backend)}
 xc.backend_id = C.uint32_t(x.BackendId)
+xc.frontend = nil
 if x.Frontend != "" {
 xc.frontend = C.CString(x.Frontend)}
 xc.frontend_id = C.uint32_t(x.FrontendId)
@@ -3254,9 +3403,11 @@ xc._type = C.libxl_usbctrl_type(x.Type)
 xc.devid = C.libxl_devid(x.Devid)
 xc.version = C.int(x.Version)
 xc.ports = C.int(x.Ports)
+xc.backend = nil
 if x.Backend != "" {
 xc.backend = C.CString(x.Backend)}
 xc.backend_id = C.uint32_t(x.BackendId)
+xc.frontend = nil
 if x.Frontend != "" {
 xc.frontend = C.CString(x.Frontend)}
 xc.frontend_id = C.uint32_t(x.FrontendId)
@@ -3422,6 +3573,7 @@ if err != nil{
 C.libxl_connectorinfo_dispose(xc)}
 }()
 
+xc.unique_id = nil
 if x.UniqueId != "" {
 xc.unique_id = C.CString(x.UniqueId)}
 xc.width = C.uint32_t(x.Width)
@@ -3473,9 +3625,11 @@ if err != nil{
 C.libxl_vdisplinfo_dispose(xc)}
 }()
 
+xc.backend = nil
 if x.Backend != "" {
 xc.backend = C.CString(x.Backend)}
 xc.backend_id = C.uint32_t(x.BackendId)
+xc.frontend = nil
 if x.Frontend != "" {
 xc.frontend = C.CString(x.Frontend)}
 xc.frontend_id = C.uint32_t(x.FrontendId)
@@ -3611,9 +3765,11 @@ if err != nil{
 C.libxl_vsndinfo_dispose(xc)}
 }()
 
+xc.backend = nil
 if x.Backend != "" {
 xc.backend = C.CString(x.Backend)}
 xc.backend_id = C.uint32_t(x.BackendId)
+xc.frontend = nil
 if x.Frontend != "" {
 xc.frontend = C.CString(x.Frontend)}
 xc.frontend_id = C.uint32_t(x.FrontendId)
@@ -3664,9 +3820,11 @@ if err != nil{
 C.libxl_vkbinfo_dispose(xc)}
 }()
 
+xc.backend = nil
 if x.Backend != "" {
 xc.backend = C.CString(x.Backend)}
 xc.backend_id = C.uint32_t(x.BackendId)
+xc.frontend = nil
 if x.Frontend != "" {
 xc.frontend = C.CString(x.Frontend)}
 xc.frontend_id = C.uint32_t(x.FrontendId)
@@ -3902,6 +4060,7 @@ return fmt.Errorf("converting field Compression: %v", err)
 if err := x.Netbuf.toC(&xc.netbuf); err != nil {
 return fmt.Errorf("converting field Netbuf: %v", err)
 }
+xc.netbufscript = nil
 if x.Netbufscript != "" {
 xc.netbufscript = C.CString(x.Netbufscript)}
 if err := x.Diskbuf.toC(&xc.diskbuf); err != nil {
@@ -4035,6 +4194,7 @@ if !ok {
 return errors.New("wrong type for union key type")
 }
 var disk_eject C.libxl_event_type_union_disk_eject
+disk_eject.vdev = nil
 if tmp.Vdev != "" {
 disk_eject.vdev = C.CString(tmp.Vdev)}
 if err := tmp.Disk.toC(&disk_eject.disk); err != nil {
-- 
2.17.1

Re: [RESEND PATCH 03/12] golang/xenlight: fix string conversion in generated toC functions

[George Dunlap]

> On May 24, 2021, at 9:36 PM, Nick Rosbrook <rosbrookn@gmail.com> wrote:
> 
> In gengotypes.py, the toC functions only set C string fields when
> the Go strings are non-empty. However, to prevent segfaults in some
> cases, these fields should always at least be set to nil so that the C
> memory is zeroed out.
> 
> Update gengotypes.py so that the generated code always sets these fields
> to nil first, and then proceeds to check if the Go string is non-empty.
> And, commit the new generated code.
> 
> Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com>

So wait — if you do

var foo C.typename

Then golang won’t automatically zero out `foo`?

That seems like a bug really; but assuming this fixes real behavior you’ve encountered:

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

> ---
> tools/golang/xenlight/gengotypes.py  |   1 +
> tools/golang/xenlight/helpers.gen.go | 160 +++++++++++++++++++++++++++
> 2 files changed, 161 insertions(+)
> 
> diff --git a/tools/golang/xenlight/gengotypes.py b/tools/golang/xenlight/gengotypes.py
> index 3e40c3d5dc..e6daa9b92f 100644
> --- a/tools/golang/xenlight/gengotypes.py
> +++ b/tools/golang/xenlight/gengotypes.py
> @@ -527,6 +527,7 @@ def xenlight_golang_convert_to_C(ty = None, outer_name = None,
> 
>     elif gotypename == 'string':
>         # Use the cgo helper for converting C strings.
> +        s += '{0}.{1} = nil\n'.format(cvarname, cname)
>         s += 'if {0}.{1} != "" {{\n'.format(govarname,goname)
>         s += '{0}.{1} = C.CString({2}.{3})}}\n'.format(cvarname,cname,
>                                                    govarname,goname)
> diff --git a/tools/golang/xenlight/helpers.gen.go b/tools/golang/xenlight/helpers.gen.go
> index b454b12d52..5222898fb8 100644
> --- a/tools/golang/xenlight/helpers.gen.go
> +++ b/tools/golang/xenlight/helpers.gen.go
> @@ -154,8 +154,10 @@ C.libxl_vnc_info_dispose(xc)}
> if err := x.Enable.toC(&xc.enable); err != nil {
> return fmt.Errorf("converting field Enable: %v", err)
> }
> +xc.listen = nil
> if x.Listen != "" {
> xc.listen = C.CString(x.Listen)}
> +xc.passwd = nil
> if x.Passwd != "" {
> xc.passwd = C.CString(x.Passwd)}
> xc.display = C.int(x.Display)
> @@ -216,11 +218,13 @@ return fmt.Errorf("converting field Enable: %v", err)
> }
> xc.port = C.int(x.Port)
> xc.tls_port = C.int(x.TlsPort)
> +xc.host = nil
> if x.Host != "" {
> xc.host = C.CString(x.Host)}
> if err := x.DisableTicketing.toC(&xc.disable_ticketing); err != nil {
> return fmt.Errorf("converting field DisableTicketing: %v", err)
> }
> +xc.passwd = nil
> if x.Passwd != "" {
> xc.passwd = C.CString(x.Passwd)}
> if err := x.AgentMouse.toC(&xc.agent_mouse); err != nil {
> @@ -233,8 +237,10 @@ if err := x.ClipboardSharing.toC(&xc.clipboard_sharing); err != nil {
> return fmt.Errorf("converting field ClipboardSharing: %v", err)
> }
> xc.usbredirection = C.int(x.Usbredirection)
> +xc.image_compression = nil
> if x.ImageCompression != "" {
> xc.image_compression = C.CString(x.ImageCompression)}
> +xc.streaming_video = nil
> if x.StreamingVideo != "" {
> xc.streaming_video = C.CString(x.StreamingVideo)}
> 
> @@ -278,8 +284,10 @@ return fmt.Errorf("converting field Enable: %v", err)
> if err := x.Opengl.toC(&xc.opengl); err != nil {
> return fmt.Errorf("converting field Opengl: %v", err)
> }
> +xc.display = nil
> if x.Display != "" {
> xc.display = C.CString(x.Display)}
> +xc.xauthority = nil
> if x.Xauthority != "" {
> xc.xauthority = C.CString(x.Xauthority)}
> 
> @@ -337,6 +345,7 @@ return fmt.Errorf("converting field Uuid: %v", err)
> }
> xc.domid = C.libxl_domid(x.Domid)
> xc.ssidref = C.uint32_t(x.Ssidref)
> +xc.ssid_label = nil
> if x.SsidLabel != "" {
> xc.ssid_label = C.CString(x.SsidLabel)}
> xc.running = C.bool(x.Running)
> @@ -391,6 +400,7 @@ C.libxl_cpupoolinfo_dispose(xc)}
> }()
> 
> xc.poolid = C.uint32_t(x.Poolid)
> +xc.pool_name = nil
> if x.PoolName != "" {
> xc.pool_name = C.CString(x.PoolName)}
> xc.sched = C.libxl_scheduler(x.Sched)
> @@ -458,9 +468,11 @@ if err != nil{
> C.libxl_channelinfo_dispose(xc)}
> }()
> 
> +xc.backend = nil
> if x.Backend != "" {
> xc.backend = C.CString(x.Backend)}
> xc.backend_id = C.uint32_t(x.BackendId)
> +xc.frontend = nil
> if x.Frontend != "" {
> xc.frontend = C.CString(x.Frontend)}
> xc.frontend_id = C.uint32_t(x.FrontendId)
> @@ -478,6 +490,7 @@ if !ok {
> return errors.New("wrong type for union key connection")
> }
> var pty C.libxl_channelinfo_connection_union_pty
> +pty.path = nil
> if tmp.Path != "" {
> pty.path = C.CString(tmp.Path)}
> ptyBytes := C.GoBytes(unsafe.Pointer(&pty),C.sizeof_libxl_channelinfo_connection_union_pty)
> @@ -563,24 +576,33 @@ C.libxl_version_info_dispose(xc)}
> 
> xc.xen_version_major = C.int(x.XenVersionMajor)
> xc.xen_version_minor = C.int(x.XenVersionMinor)
> +xc.xen_version_extra = nil
> if x.XenVersionExtra != "" {
> xc.xen_version_extra = C.CString(x.XenVersionExtra)}
> +xc.compiler = nil
> if x.Compiler != "" {
> xc.compiler = C.CString(x.Compiler)}
> +xc.compile_by = nil
> if x.CompileBy != "" {
> xc.compile_by = C.CString(x.CompileBy)}
> +xc.compile_domain = nil
> if x.CompileDomain != "" {
> xc.compile_domain = C.CString(x.CompileDomain)}
> +xc.compile_date = nil
> if x.CompileDate != "" {
> xc.compile_date = C.CString(x.CompileDate)}
> +xc.capabilities = nil
> if x.Capabilities != "" {
> xc.capabilities = C.CString(x.Capabilities)}
> +xc.changeset = nil
> if x.Changeset != "" {
> xc.changeset = C.CString(x.Changeset)}
> xc.virt_start = C.uint64_t(x.VirtStart)
> xc.pagesize = C.int(x.Pagesize)
> +xc.commandline = nil
> if x.Commandline != "" {
> xc.commandline = C.CString(x.Commandline)}
> +xc.build_id = nil
> if x.BuildId != "" {
> xc.build_id = C.CString(x.BuildId)}
> 
> @@ -650,8 +672,10 @@ if err := x.Oos.toC(&xc.oos); err != nil {
> return fmt.Errorf("converting field Oos: %v", err)
> }
> xc.ssidref = C.uint32_t(x.Ssidref)
> +xc.ssid_label = nil
> if x.SsidLabel != "" {
> xc.ssid_label = C.CString(x.SsidLabel)}
> +xc.name = nil
> if x.Name != "" {
> xc.name = C.CString(x.Name)}
> xc.domid = C.libxl_domid(x.Domid)
> @@ -665,6 +689,7 @@ if err := x.Platformdata.toC(&xc.platformdata); err != nil {
> return fmt.Errorf("converting field Platformdata: %v", err)
> }
> xc.poolid = C.uint32_t(x.Poolid)
> +xc.pool_name = nil
> if x.PoolName != "" {
> xc.pool_name = C.CString(x.PoolName)}
> if err := x.RunHotplugScripts.toC(&xc.run_hotplug_scripts); err != nil {
> @@ -712,6 +737,7 @@ C.libxl_domain_restore_params_dispose(xc)}
> 
> xc.checkpointed_stream = C.int(x.CheckpointedStream)
> xc.stream_version = C.uint32_t(x.StreamVersion)
> +xc.colo_proxy_script = nil
> if x.ColoProxyScript != "" {
> xc.colo_proxy_script = C.CString(x.ColoProxyScript)}
> if err := x.UserspaceColoProxy.toC(&xc.userspace_colo_proxy); err != nil {
> @@ -1312,6 +1338,7 @@ xc.shadow_memkb = C.uint64_t(x.ShadowMemkb)
> xc.iommu_memkb = C.uint64_t(x.IommuMemkb)
> xc.rtc_timeoffset = C.uint32_t(x.RtcTimeoffset)
> xc.exec_ssidref = C.uint32_t(x.ExecSsidref)
> +xc.exec_ssid_label = nil
> if x.ExecSsidLabel != "" {
> xc.exec_ssid_label = C.CString(x.ExecSsidLabel)}
> if err := x.Localtime.toC(&xc.localtime); err != nil {
> @@ -1323,6 +1350,7 @@ return fmt.Errorf("converting field DisableMigrate: %v", err)
> if err := x.Cpuid.toC(&xc.cpuid); err != nil {
> return fmt.Errorf("converting field Cpuid: %v", err)
> }
> +xc.blkdev_start = nil
> if x.BlkdevStart != "" {
> xc.blkdev_start = C.CString(x.BlkdevStart)}
> if numVnumaNodes := len(x.VnumaNodes); numVnumaNodes > 0 {
> @@ -1342,15 +1370,20 @@ if err := x.DeviceModelStubdomain.toC(&xc.device_model_stubdomain); err != nil {
> return fmt.Errorf("converting field DeviceModelStubdomain: %v", err)
> }
> xc.stubdomain_memkb = C.uint64_t(x.StubdomainMemkb)
> +xc.stubdomain_kernel = nil
> if x.StubdomainKernel != "" {
> xc.stubdomain_kernel = C.CString(x.StubdomainKernel)}
> +xc.stubdomain_ramdisk = nil
> if x.StubdomainRamdisk != "" {
> xc.stubdomain_ramdisk = C.CString(x.StubdomainRamdisk)}
> +xc.device_model = nil
> if x.DeviceModel != "" {
> xc.device_model = C.CString(x.DeviceModel)}
> xc.device_model_ssidref = C.uint32_t(x.DeviceModelSsidref)
> +xc.device_model_ssid_label = nil
> if x.DeviceModelSsidLabel != "" {
> xc.device_model_ssid_label = C.CString(x.DeviceModelSsidLabel)}
> +xc.device_model_user = nil
> if x.DeviceModelUser != "" {
> xc.device_model_user = C.CString(x.DeviceModelUser)}
> if err := x.Extra.toC(&xc.extra); err != nil {
> @@ -1397,17 +1430,22 @@ if err := x.ClaimMode.toC(&xc.claim_mode); err != nil {
> return fmt.Errorf("converting field ClaimMode: %v", err)
> }
> xc.event_channels = C.uint32_t(x.EventChannels)
> +xc.kernel = nil
> if x.Kernel != "" {
> xc.kernel = C.CString(x.Kernel)}
> +xc.cmdline = nil
> if x.Cmdline != "" {
> xc.cmdline = C.CString(x.Cmdline)}
> +xc.ramdisk = nil
> if x.Ramdisk != "" {
> xc.ramdisk = C.CString(x.Ramdisk)}
> +xc.device_tree = nil
> if x.DeviceTree != "" {
> xc.device_tree = C.CString(x.DeviceTree)}
> if err := x.Acpi.toC(&xc.acpi); err != nil {
> return fmt.Errorf("converting field Acpi: %v", err)
> }
> +xc.bootloader = nil
> if x.Bootloader != "" {
> xc.bootloader = C.CString(x.Bootloader)}
> if err := x.BootloaderArgs.toC(&xc.bootloader_args); err != nil {
> @@ -1432,6 +1470,7 @@ if !ok {
> return errors.New("wrong type for union key type")
> }
> var hvm C.libxl_domain_build_info_type_union_hvm
> +hvm.firmware = nil
> if tmp.Firmware != "" {
> hvm.firmware = C.CString(tmp.Firmware)}
> hvm.bios = C.libxl_bios_type(tmp.Bios)
> @@ -1465,6 +1504,7 @@ return fmt.Errorf("converting field ViridianEnable: %v", err)
> if err := tmp.ViridianDisable.toC(&hvm.viridian_disable); err != nil {
> return fmt.Errorf("converting field ViridianDisable: %v", err)
> }
> +hvm.timeoffset = nil
> if tmp.Timeoffset != "" {
> hvm.timeoffset = C.CString(tmp.Timeoffset)}
> if err := tmp.Hpet.toC(&hvm.hpet); err != nil {
> @@ -1481,10 +1521,13 @@ return fmt.Errorf("converting field NestedHvm: %v", err)
> if err := tmp.Altp2M.toC(&hvm.altp2m); err != nil {
> return fmt.Errorf("converting field Altp2M: %v", err)
> }
> +hvm.system_firmware = nil
> if tmp.SystemFirmware != "" {
> hvm.system_firmware = C.CString(tmp.SystemFirmware)}
> +hvm.smbios_firmware = nil
> if tmp.SmbiosFirmware != "" {
> hvm.smbios_firmware = C.CString(tmp.SmbiosFirmware)}
> +hvm.acpi_firmware = nil
> if tmp.AcpiFirmware != "" {
> hvm.acpi_firmware = C.CString(tmp.AcpiFirmware)}
> hvm.hdtype = C.libxl_hdtype(tmp.Hdtype)
> @@ -1497,6 +1540,7 @@ return fmt.Errorf("converting field Vga: %v", err)
> if err := tmp.Vnc.toC(&hvm.vnc); err != nil {
> return fmt.Errorf("converting field Vnc: %v", err)
> }
> +hvm.keymap = nil
> if tmp.Keymap != "" {
> hvm.keymap = C.CString(tmp.Keymap)}
> if err := tmp.Sdl.toC(&hvm.sdl); err != nil {
> @@ -1509,19 +1553,23 @@ if err := tmp.GfxPassthru.toC(&hvm.gfx_passthru); err != nil {
> return fmt.Errorf("converting field GfxPassthru: %v", err)
> }
> hvm.gfx_passthru_kind = C.libxl_gfx_passthru_kind(tmp.GfxPassthruKind)
> +hvm.serial = nil
> if tmp.Serial != "" {
> hvm.serial = C.CString(tmp.Serial)}
> +hvm.boot = nil
> if tmp.Boot != "" {
> hvm.boot = C.CString(tmp.Boot)}
> if err := tmp.Usb.toC(&hvm.usb); err != nil {
> return fmt.Errorf("converting field Usb: %v", err)
> }
> hvm.usbversion = C.int(tmp.Usbversion)
> +hvm.usbdevice = nil
> if tmp.Usbdevice != "" {
> hvm.usbdevice = C.CString(tmp.Usbdevice)}
> if err := tmp.VkbDevice.toC(&hvm.vkb_device); err != nil {
> return fmt.Errorf("converting field VkbDevice: %v", err)
> }
> +hvm.soundhw = nil
> if tmp.Soundhw != "" {
> hvm.soundhw = C.CString(tmp.Soundhw)}
> if err := tmp.XenPlatformPci.toC(&hvm.xen_platform_pci); err != nil {
> @@ -1550,18 +1598,23 @@ if !ok {
> return errors.New("wrong type for union key type")
> }
> var pv C.libxl_domain_build_info_type_union_pv
> +pv.kernel = nil
> if tmp.Kernel != "" {
> pv.kernel = C.CString(tmp.Kernel)}
> pv.slack_memkb = C.uint64_t(tmp.SlackMemkb)
> +pv.bootloader = nil
> if tmp.Bootloader != "" {
> pv.bootloader = C.CString(tmp.Bootloader)}
> if err := tmp.BootloaderArgs.toC(&pv.bootloader_args); err != nil {
> return fmt.Errorf("converting field BootloaderArgs: %v", err)
> }
> +pv.cmdline = nil
> if tmp.Cmdline != "" {
> pv.cmdline = C.CString(tmp.Cmdline)}
> +pv.ramdisk = nil
> if tmp.Ramdisk != "" {
> pv.ramdisk = C.CString(tmp.Ramdisk)}
> +pv.features = nil
> if tmp.Features != "" {
> pv.features = C.CString(tmp.Features)}
> if err := tmp.E820Host.toC(&pv.e820_host); err != nil {
> @@ -1578,10 +1631,13 @@ var pvh C.libxl_domain_build_info_type_union_pvh
> if err := tmp.Pvshim.toC(&pvh.pvshim); err != nil {
> return fmt.Errorf("converting field Pvshim: %v", err)
> }
> +pvh.pvshim_path = nil
> if tmp.PvshimPath != "" {
> pvh.pvshim_path = C.CString(tmp.PvshimPath)}
> +pvh.pvshim_cmdline = nil
> if tmp.PvshimCmdline != "" {
> pvh.pvshim_cmdline = C.CString(tmp.PvshimCmdline)}
> +pvh.pvshim_extra = nil
> if tmp.PvshimExtra != "" {
> pvh.pvshim_extra = C.CString(tmp.PvshimExtra)}
> pvhBytes := C.GoBytes(unsafe.Pointer(&pvh),C.sizeof_libxl_domain_build_info_type_union_pvh)
> @@ -1635,6 +1691,7 @@ C.libxl_device_vfb_dispose(xc)}
> }()
> 
> xc.backend_domid = C.libxl_domid(x.BackendDomid)
> +xc.backend_domname = nil
> if x.BackendDomname != "" {
> xc.backend_domname = C.CString(x.BackendDomname)}
> xc.devid = C.libxl_devid(x.Devid)
> @@ -1644,6 +1701,7 @@ return fmt.Errorf("converting field Vnc: %v", err)
> if err := x.Sdl.toC(&xc.sdl); err != nil {
> return fmt.Errorf("converting field Sdl: %v", err)
> }
> +xc.keymap = nil
> if x.Keymap != "" {
> xc.keymap = C.CString(x.Keymap)}
> 
> @@ -1689,10 +1747,12 @@ C.libxl_device_vkb_dispose(xc)}
> }()
> 
> xc.backend_domid = C.libxl_domid(x.BackendDomid)
> +xc.backend_domname = nil
> if x.BackendDomname != "" {
> xc.backend_domname = C.CString(x.BackendDomname)}
> xc.devid = C.libxl_devid(x.Devid)
> xc.backend_type = C.libxl_vkb_backend(x.BackendType)
> +xc.unique_id = nil
> if x.UniqueId != "" {
> xc.unique_id = C.CString(x.UniqueId)}
> xc.feature_disable_keyboard = C.bool(x.FeatureDisableKeyboard)
> @@ -1758,14 +1818,18 @@ C.libxl_device_disk_dispose(xc)}
> }()
> 
> xc.backend_domid = C.libxl_domid(x.BackendDomid)
> +xc.backend_domname = nil
> if x.BackendDomname != "" {
> xc.backend_domname = C.CString(x.BackendDomname)}
> +xc.pdev_path = nil
> if x.PdevPath != "" {
> xc.pdev_path = C.CString(x.PdevPath)}
> +xc.vdev = nil
> if x.Vdev != "" {
> xc.vdev = C.CString(x.Vdev)}
> xc.backend = C.libxl_disk_backend(x.Backend)
> xc.format = C.libxl_disk_format(x.Format)
> +xc.script = nil
> if x.Script != "" {
> xc.script = C.CString(x.Script)}
> xc.removable = C.int(x.Removable)
> @@ -1781,13 +1845,17 @@ return fmt.Errorf("converting field ColoEnable: %v", err)
> if err := x.ColoRestoreEnable.toC(&xc.colo_restore_enable); err != nil {
> return fmt.Errorf("converting field ColoRestoreEnable: %v", err)
> }
> +xc.colo_host = nil
> if x.ColoHost != "" {
> xc.colo_host = C.CString(x.ColoHost)}
> xc.colo_port = C.int(x.ColoPort)
> +xc.colo_export = nil
> if x.ColoExport != "" {
> xc.colo_export = C.CString(x.ColoExport)}
> +xc.active_disk = nil
> if x.ActiveDisk != "" {
> xc.active_disk = C.CString(x.ActiveDisk)}
> +xc.hidden_disk = nil
> if x.HiddenDisk != "" {
> xc.hidden_disk = C.CString(x.HiddenDisk)}
> 
> @@ -1883,124 +1951,180 @@ C.libxl_device_nic_dispose(xc)}
> }()
> 
> xc.backend_domid = C.libxl_domid(x.BackendDomid)
> +xc.backend_domname = nil
> if x.BackendDomname != "" {
> xc.backend_domname = C.CString(x.BackendDomname)}
> xc.devid = C.libxl_devid(x.Devid)
> xc.mtu = C.int(x.Mtu)
> +xc.model = nil
> if x.Model != "" {
> xc.model = C.CString(x.Model)}
> if err := x.Mac.toC(&xc.mac); err != nil {
> return fmt.Errorf("converting field Mac: %v", err)
> }
> +xc.ip = nil
> if x.Ip != "" {
> xc.ip = C.CString(x.Ip)}
> +xc.bridge = nil
> if x.Bridge != "" {
> xc.bridge = C.CString(x.Bridge)}
> +xc.ifname = nil
> if x.Ifname != "" {
> xc.ifname = C.CString(x.Ifname)}
> +xc.script = nil
> if x.Script != "" {
> xc.script = C.CString(x.Script)}
> xc.nictype = C.libxl_nic_type(x.Nictype)
> xc.rate_bytes_per_interval = C.uint64_t(x.RateBytesPerInterval)
> xc.rate_interval_usecs = C.uint32_t(x.RateIntervalUsecs)
> +xc.gatewaydev = nil
> if x.Gatewaydev != "" {
> xc.gatewaydev = C.CString(x.Gatewaydev)}
> +xc.coloft_forwarddev = nil
> if x.ColoftForwarddev != "" {
> xc.coloft_forwarddev = C.CString(x.ColoftForwarddev)}
> +xc.colo_sock_mirror_id = nil
> if x.ColoSockMirrorId != "" {
> xc.colo_sock_mirror_id = C.CString(x.ColoSockMirrorId)}
> +xc.colo_sock_mirror_ip = nil
> if x.ColoSockMirrorIp != "" {
> xc.colo_sock_mirror_ip = C.CString(x.ColoSockMirrorIp)}
> +xc.colo_sock_mirror_port = nil
> if x.ColoSockMirrorPort != "" {
> xc.colo_sock_mirror_port = C.CString(x.ColoSockMirrorPort)}
> +xc.colo_sock_compare_pri_in_id = nil
> if x.ColoSockComparePriInId != "" {
> xc.colo_sock_compare_pri_in_id = C.CString(x.ColoSockComparePriInId)}
> +xc.colo_sock_compare_pri_in_ip = nil
> if x.ColoSockComparePriInIp != "" {
> xc.colo_sock_compare_pri_in_ip = C.CString(x.ColoSockComparePriInIp)}
> +xc.colo_sock_compare_pri_in_port = nil
> if x.ColoSockComparePriInPort != "" {
> xc.colo_sock_compare_pri_in_port = C.CString(x.ColoSockComparePriInPort)}
> +xc.colo_sock_compare_sec_in_id = nil
> if x.ColoSockCompareSecInId != "" {
> xc.colo_sock_compare_sec_in_id = C.CString(x.ColoSockCompareSecInId)}
> +xc.colo_sock_compare_sec_in_ip = nil
> if x.ColoSockCompareSecInIp != "" {
> xc.colo_sock_compare_sec_in_ip = C.CString(x.ColoSockCompareSecInIp)}
> +xc.colo_sock_compare_sec_in_port = nil
> if x.ColoSockCompareSecInPort != "" {
> xc.colo_sock_compare_sec_in_port = C.CString(x.ColoSockCompareSecInPort)}
> +xc.colo_sock_compare_notify_id = nil
> if x.ColoSockCompareNotifyId != "" {
> xc.colo_sock_compare_notify_id = C.CString(x.ColoSockCompareNotifyId)}
> +xc.colo_sock_compare_notify_ip = nil
> if x.ColoSockCompareNotifyIp != "" {
> xc.colo_sock_compare_notify_ip = C.CString(x.ColoSockCompareNotifyIp)}
> +xc.colo_sock_compare_notify_port = nil
> if x.ColoSockCompareNotifyPort != "" {
> xc.colo_sock_compare_notify_port = C.CString(x.ColoSockCompareNotifyPort)}
> +xc.colo_sock_redirector0_id = nil
> if x.ColoSockRedirector0Id != "" {
> xc.colo_sock_redirector0_id = C.CString(x.ColoSockRedirector0Id)}
> +xc.colo_sock_redirector0_ip = nil
> if x.ColoSockRedirector0Ip != "" {
> xc.colo_sock_redirector0_ip = C.CString(x.ColoSockRedirector0Ip)}
> +xc.colo_sock_redirector0_port = nil
> if x.ColoSockRedirector0Port != "" {
> xc.colo_sock_redirector0_port = C.CString(x.ColoSockRedirector0Port)}
> +xc.colo_sock_redirector1_id = nil
> if x.ColoSockRedirector1Id != "" {
> xc.colo_sock_redirector1_id = C.CString(x.ColoSockRedirector1Id)}
> +xc.colo_sock_redirector1_ip = nil
> if x.ColoSockRedirector1Ip != "" {
> xc.colo_sock_redirector1_ip = C.CString(x.ColoSockRedirector1Ip)}
> +xc.colo_sock_redirector1_port = nil
> if x.ColoSockRedirector1Port != "" {
> xc.colo_sock_redirector1_port = C.CString(x.ColoSockRedirector1Port)}
> +xc.colo_sock_redirector2_id = nil
> if x.ColoSockRedirector2Id != "" {
> xc.colo_sock_redirector2_id = C.CString(x.ColoSockRedirector2Id)}
> +xc.colo_sock_redirector2_ip = nil
> if x.ColoSockRedirector2Ip != "" {
> xc.colo_sock_redirector2_ip = C.CString(x.ColoSockRedirector2Ip)}
> +xc.colo_sock_redirector2_port = nil
> if x.ColoSockRedirector2Port != "" {
> xc.colo_sock_redirector2_port = C.CString(x.ColoSockRedirector2Port)}
> +xc.colo_filter_mirror_queue = nil
> if x.ColoFilterMirrorQueue != "" {
> xc.colo_filter_mirror_queue = C.CString(x.ColoFilterMirrorQueue)}
> +xc.colo_filter_mirror_outdev = nil
> if x.ColoFilterMirrorOutdev != "" {
> xc.colo_filter_mirror_outdev = C.CString(x.ColoFilterMirrorOutdev)}
> +xc.colo_filter_redirector0_queue = nil
> if x.ColoFilterRedirector0Queue != "" {
> xc.colo_filter_redirector0_queue = C.CString(x.ColoFilterRedirector0Queue)}
> +xc.colo_filter_redirector0_indev = nil
> if x.ColoFilterRedirector0Indev != "" {
> xc.colo_filter_redirector0_indev = C.CString(x.ColoFilterRedirector0Indev)}
> +xc.colo_filter_redirector0_outdev = nil
> if x.ColoFilterRedirector0Outdev != "" {
> xc.colo_filter_redirector0_outdev = C.CString(x.ColoFilterRedirector0Outdev)}
> +xc.colo_filter_redirector1_queue = nil
> if x.ColoFilterRedirector1Queue != "" {
> xc.colo_filter_redirector1_queue = C.CString(x.ColoFilterRedirector1Queue)}
> +xc.colo_filter_redirector1_indev = nil
> if x.ColoFilterRedirector1Indev != "" {
> xc.colo_filter_redirector1_indev = C.CString(x.ColoFilterRedirector1Indev)}
> +xc.colo_filter_redirector1_outdev = nil
> if x.ColoFilterRedirector1Outdev != "" {
> xc.colo_filter_redirector1_outdev = C.CString(x.ColoFilterRedirector1Outdev)}
> +xc.colo_compare_pri_in = nil
> if x.ColoComparePriIn != "" {
> xc.colo_compare_pri_in = C.CString(x.ColoComparePriIn)}
> +xc.colo_compare_sec_in = nil
> if x.ColoCompareSecIn != "" {
> xc.colo_compare_sec_in = C.CString(x.ColoCompareSecIn)}
> +xc.colo_compare_out = nil
> if x.ColoCompareOut != "" {
> xc.colo_compare_out = C.CString(x.ColoCompareOut)}
> +xc.colo_compare_notify_dev = nil
> if x.ColoCompareNotifyDev != "" {
> xc.colo_compare_notify_dev = C.CString(x.ColoCompareNotifyDev)}
> +xc.colo_sock_sec_redirector0_id = nil
> if x.ColoSockSecRedirector0Id != "" {
> xc.colo_sock_sec_redirector0_id = C.CString(x.ColoSockSecRedirector0Id)}
> +xc.colo_sock_sec_redirector0_ip = nil
> if x.ColoSockSecRedirector0Ip != "" {
> xc.colo_sock_sec_redirector0_ip = C.CString(x.ColoSockSecRedirector0Ip)}
> +xc.colo_sock_sec_redirector0_port = nil
> if x.ColoSockSecRedirector0Port != "" {
> xc.colo_sock_sec_redirector0_port = C.CString(x.ColoSockSecRedirector0Port)}
> +xc.colo_sock_sec_redirector1_id = nil
> if x.ColoSockSecRedirector1Id != "" {
> xc.colo_sock_sec_redirector1_id = C.CString(x.ColoSockSecRedirector1Id)}
> +xc.colo_sock_sec_redirector1_ip = nil
> if x.ColoSockSecRedirector1Ip != "" {
> xc.colo_sock_sec_redirector1_ip = C.CString(x.ColoSockSecRedirector1Ip)}
> +xc.colo_sock_sec_redirector1_port = nil
> if x.ColoSockSecRedirector1Port != "" {
> xc.colo_sock_sec_redirector1_port = C.CString(x.ColoSockSecRedirector1Port)}
> +xc.colo_filter_sec_redirector0_queue = nil
> if x.ColoFilterSecRedirector0Queue != "" {
> xc.colo_filter_sec_redirector0_queue = C.CString(x.ColoFilterSecRedirector0Queue)}
> +xc.colo_filter_sec_redirector0_indev = nil
> if x.ColoFilterSecRedirector0Indev != "" {
> xc.colo_filter_sec_redirector0_indev = C.CString(x.ColoFilterSecRedirector0Indev)}
> +xc.colo_filter_sec_redirector0_outdev = nil
> if x.ColoFilterSecRedirector0Outdev != "" {
> xc.colo_filter_sec_redirector0_outdev = C.CString(x.ColoFilterSecRedirector0Outdev)}
> +xc.colo_filter_sec_redirector1_queue = nil
> if x.ColoFilterSecRedirector1Queue != "" {
> xc.colo_filter_sec_redirector1_queue = C.CString(x.ColoFilterSecRedirector1Queue)}
> +xc.colo_filter_sec_redirector1_indev = nil
> if x.ColoFilterSecRedirector1Indev != "" {
> xc.colo_filter_sec_redirector1_indev = C.CString(x.ColoFilterSecRedirector1Indev)}
> +xc.colo_filter_sec_redirector1_outdev = nil
> if x.ColoFilterSecRedirector1Outdev != "" {
> xc.colo_filter_sec_redirector1_outdev = C.CString(x.ColoFilterSecRedirector1Outdev)}
> +xc.colo_filter_sec_rewriter0_queue = nil
> if x.ColoFilterSecRewriter0Queue != "" {
> xc.colo_filter_sec_rewriter0_queue = C.CString(x.ColoFilterSecRewriter0Queue)}
> +xc.colo_checkpoint_host = nil
> if x.ColoCheckpointHost != "" {
> xc.colo_checkpoint_host = C.CString(x.ColoCheckpointHost)}
> +xc.colo_checkpoint_port = nil
> if x.ColoCheckpointPort != "" {
> xc.colo_checkpoint_port = C.CString(x.ColoCheckpointPort)}
> 
> @@ -2053,6 +2177,7 @@ xc.power_mgmt = C.bool(x.PowerMgmt)
> xc.permissive = C.bool(x.Permissive)
> xc.seize = C.bool(x.Seize)
> xc.rdm_policy = C.libxl_rdm_reserve_policy(x.RdmPolicy)
> +xc.name = nil
> if x.Name != "" {
> xc.name = C.CString(x.Name)}
> 
> @@ -2126,6 +2251,7 @@ xc.devid = C.libxl_devid(x.Devid)
> xc.version = C.int(x.Version)
> xc.ports = C.int(x.Ports)
> xc.backend_domid = C.libxl_domid(x.BackendDomid)
> +xc.backend_domname = nil
> if x.BackendDomname != "" {
> xc.backend_domname = C.CString(x.BackendDomname)}
> 
> @@ -2223,6 +2349,7 @@ if err != nil{
> C.libxl_device_dtdev_dispose(xc)}
> }()
> 
> +xc.path = nil
> if x.Path != "" {
> xc.path = C.CString(x.Path)}
> 
> @@ -2259,6 +2386,7 @@ C.libxl_device_vtpm_dispose(xc)}
> }()
> 
> xc.backend_domid = C.libxl_domid(x.BackendDomid)
> +xc.backend_domname = nil
> if x.BackendDomname != "" {
> xc.backend_domname = C.CString(x.BackendDomname)}
> xc.devid = C.libxl_devid(x.Devid)
> @@ -2299,12 +2427,16 @@ C.libxl_device_p9_dispose(xc)}
> }()
> 
> xc.backend_domid = C.libxl_domid(x.BackendDomid)
> +xc.backend_domname = nil
> if x.BackendDomname != "" {
> xc.backend_domname = C.CString(x.BackendDomname)}
> +xc.tag = nil
> if x.Tag != "" {
> xc.tag = C.CString(x.Tag)}
> +xc.path = nil
> if x.Path != "" {
> xc.path = C.CString(x.Path)}
> +xc.security_model = nil
> if x.SecurityModel != "" {
> xc.security_model = C.CString(x.SecurityModel)}
> xc.devid = C.libxl_devid(x.Devid)
> @@ -2339,6 +2471,7 @@ C.libxl_device_pvcallsif_dispose(xc)}
> }()
> 
> xc.backend_domid = C.libxl_domid(x.BackendDomid)
> +xc.backend_domname = nil
> if x.BackendDomname != "" {
> xc.backend_domname = C.CString(x.BackendDomname)}
> xc.devid = C.libxl_devid(x.Devid)
> @@ -2399,9 +2532,11 @@ C.libxl_device_channel_dispose(xc)}
> }()
> 
> xc.backend_domid = C.libxl_domid(x.BackendDomid)
> +xc.backend_domname = nil
> if x.BackendDomname != "" {
> xc.backend_domname = C.CString(x.BackendDomname)}
> xc.devid = C.libxl_devid(x.Devid)
> +xc.name = nil
> if x.Name != "" {
> xc.name = C.CString(x.Name)}
> xc.connection = C.libxl_channel_connection(x.Connection)
> @@ -2416,6 +2551,7 @@ if !ok {
> return errors.New("wrong type for union key connection")
> }
> var socket C.libxl_device_channel_connection_union_socket
> +socket.path = nil
> if tmp.Path != "" {
> socket.path = C.CString(tmp.Path)}
> socketBytes := C.GoBytes(unsafe.Pointer(&socket),C.sizeof_libxl_device_channel_connection_union_socket)
> @@ -2452,6 +2588,7 @@ if err != nil{
> C.libxl_connector_param_dispose(xc)}
> }()
> 
> +xc.unique_id = nil
> if x.UniqueId != "" {
> xc.unique_id = C.CString(x.UniqueId)}
> xc.width = C.uint32_t(x.Width)
> @@ -2497,6 +2634,7 @@ C.libxl_device_vdispl_dispose(xc)}
> }()
> 
> xc.backend_domid = C.libxl_domid(x.BackendDomid)
> +xc.backend_domname = nil
> if x.BackendDomname != "" {
> xc.backend_domname = C.CString(x.BackendDomname)}
> xc.devid = C.libxl_devid(x.Devid)
> @@ -2608,6 +2746,7 @@ if err != nil{
> C.libxl_vsnd_stream_dispose(xc)}
> }()
> 
> +xc.unique_id = nil
> if x.UniqueId != "" {
> xc.unique_id = C.CString(x.UniqueId)}
> xc._type = C.libxl_vsnd_stream_type(x.Type)
> @@ -2654,6 +2793,7 @@ if err != nil{
> C.libxl_vsnd_pcm_dispose(xc)}
> }()
> 
> +xc.name = nil
> if x.Name != "" {
> xc.name = C.CString(x.Name)}
> if err := x.Params.toC(&xc.params); err != nil {
> @@ -2714,11 +2854,14 @@ C.libxl_device_vsnd_dispose(xc)}
> }()
> 
> xc.backend_domid = C.libxl_domid(x.BackendDomid)
> +xc.backend_domname = nil
> if x.BackendDomname != "" {
> xc.backend_domname = C.CString(x.BackendDomname)}
> xc.devid = C.libxl_devid(x.Devid)
> +xc.short_name = nil
> if x.ShortName != "" {
> xc.short_name = C.CString(x.ShortName)}
> +xc.long_name = nil
> if x.LongName != "" {
> xc.long_name = C.CString(x.LongName)}
> if err := x.Params.toC(&xc.params); err != nil {
> @@ -3103,9 +3246,11 @@ if err != nil{
> C.libxl_diskinfo_dispose(xc)}
> }()
> 
> +xc.backend = nil
> if x.Backend != "" {
> xc.backend = C.CString(x.Backend)}
> xc.backend_id = C.uint32_t(x.BackendId)
> +xc.frontend = nil
> if x.Frontend != "" {
> xc.frontend = C.CString(x.Frontend)}
> xc.frontend_id = C.uint32_t(x.FrontendId)
> @@ -3149,9 +3294,11 @@ if err != nil{
> C.libxl_nicinfo_dispose(xc)}
> }()
> 
> +xc.backend = nil
> if x.Backend != "" {
> xc.backend = C.CString(x.Backend)}
> xc.backend_id = C.uint32_t(x.BackendId)
> +xc.frontend = nil
> if x.Frontend != "" {
> xc.frontend = C.CString(x.Frontend)}
> xc.frontend_id = C.uint32_t(x.FrontendId)
> @@ -3198,9 +3345,11 @@ if err != nil{
> C.libxl_vtpminfo_dispose(xc)}
> }()
> 
> +xc.backend = nil
> if x.Backend != "" {
> xc.backend = C.CString(x.Backend)}
> xc.backend_id = C.uint32_t(x.BackendId)
> +xc.frontend = nil
> if x.Frontend != "" {
> xc.frontend = C.CString(x.Frontend)}
> xc.frontend_id = C.uint32_t(x.FrontendId)
> @@ -3254,9 +3403,11 @@ xc._type = C.libxl_usbctrl_type(x.Type)
> xc.devid = C.libxl_devid(x.Devid)
> xc.version = C.int(x.Version)
> xc.ports = C.int(x.Ports)
> +xc.backend = nil
> if x.Backend != "" {
> xc.backend = C.CString(x.Backend)}
> xc.backend_id = C.uint32_t(x.BackendId)
> +xc.frontend = nil
> if x.Frontend != "" {
> xc.frontend = C.CString(x.Frontend)}
> xc.frontend_id = C.uint32_t(x.FrontendId)
> @@ -3422,6 +3573,7 @@ if err != nil{
> C.libxl_connectorinfo_dispose(xc)}
> }()
> 
> +xc.unique_id = nil
> if x.UniqueId != "" {
> xc.unique_id = C.CString(x.UniqueId)}
> xc.width = C.uint32_t(x.Width)
> @@ -3473,9 +3625,11 @@ if err != nil{
> C.libxl_vdisplinfo_dispose(xc)}
> }()
> 
> +xc.backend = nil
> if x.Backend != "" {
> xc.backend = C.CString(x.Backend)}
> xc.backend_id = C.uint32_t(x.BackendId)
> +xc.frontend = nil
> if x.Frontend != "" {
> xc.frontend = C.CString(x.Frontend)}
> xc.frontend_id = C.uint32_t(x.FrontendId)
> @@ -3611,9 +3765,11 @@ if err != nil{
> C.libxl_vsndinfo_dispose(xc)}
> }()
> 
> +xc.backend = nil
> if x.Backend != "" {
> xc.backend = C.CString(x.Backend)}
> xc.backend_id = C.uint32_t(x.BackendId)
> +xc.frontend = nil
> if x.Frontend != "" {
> xc.frontend = C.CString(x.Frontend)}
> xc.frontend_id = C.uint32_t(x.FrontendId)
> @@ -3664,9 +3820,11 @@ if err != nil{
> C.libxl_vkbinfo_dispose(xc)}
> }()
> 
> +xc.backend = nil
> if x.Backend != "" {
> xc.backend = C.CString(x.Backend)}
> xc.backend_id = C.uint32_t(x.BackendId)
> +xc.frontend = nil
> if x.Frontend != "" {
> xc.frontend = C.CString(x.Frontend)}
> xc.frontend_id = C.uint32_t(x.FrontendId)
> @@ -3902,6 +4060,7 @@ return fmt.Errorf("converting field Compression: %v", err)
> if err := x.Netbuf.toC(&xc.netbuf); err != nil {
> return fmt.Errorf("converting field Netbuf: %v", err)
> }
> +xc.netbufscript = nil
> if x.Netbufscript != "" {
> xc.netbufscript = C.CString(x.Netbufscript)}
> if err := x.Diskbuf.toC(&xc.diskbuf); err != nil {
> @@ -4035,6 +4194,7 @@ if !ok {
> return errors.New("wrong type for union key type")
> }
> var disk_eject C.libxl_event_type_union_disk_eject
> +disk_eject.vdev = nil
> if tmp.Vdev != "" {
> disk_eject.vdev = C.CString(tmp.Vdev)}
> if err := tmp.Disk.toC(&disk_eject.disk); err != nil {
> -- 
> 2.17.1
> 

Re: [RESEND PATCH 03/12] golang/xenlight: fix string conversion in generated toC functions

[Nick Rosbrook]

On Fri, Jun 18, 2021 at 11:00:26AM +0000, George Dunlap wrote:
> 
> 
> > On May 24, 2021, at 9:36 PM, Nick Rosbrook <rosbrookn@gmail.com> wrote:
> > 
> > In gengotypes.py, the toC functions only set C string fields when
> > the Go strings are non-empty. However, to prevent segfaults in some
> > cases, these fields should always at least be set to nil so that the C
> > memory is zeroed out.
> > 
> > Update gengotypes.py so that the generated code always sets these fields
> > to nil first, and then proceeds to check if the Go string is non-empty.
> > And, commit the new generated code.
> > 
> > Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com>
> 
> So wait — if you do
> 
> var foo C.typename
> 
> Then golang won’t automatically zero out `foo`?
> 
> That seems like a bug really; but assuming this fixes real behavior you’ve encountered:

I would have to dig in again to figure out exactly what Go/cgo is doing
here, and whether or not this is a bug. But, the behavior I observed was
that without these nil assignments, I would sometimes get segfaults in
libxl_string_copy. This patch ensures that libxl__str_dup is not called
in the empty string case, thus avoiding the segfault.
> 
> Reviewed-by: George Dunlap <george.dunlap@citrix.com>

Thanks,
NR

Re: [RESEND PATCH 03/12] golang/xenlight: fix string conversion in generated toC functions

[George Dunlap]

> On Jun 21, 2021, at 5:11 PM, Nick Rosbrook <rosbrookn@gmail.com> wrote:
> 
> On Fri, Jun 18, 2021 at 11:00:26AM +0000, George Dunlap wrote:
>> 
>> 
>>> On May 24, 2021, at 9:36 PM, Nick Rosbrook <rosbrookn@gmail.com> wrote:
>>> 
>>> In gengotypes.py, the toC functions only set C string fields when
>>> the Go strings are non-empty. However, to prevent segfaults in some
>>> cases, these fields should always at least be set to nil so that the C
>>> memory is zeroed out.
>>> 
>>> Update gengotypes.py so that the generated code always sets these fields
>>> to nil first, and then proceeds to check if the Go string is non-empty.
>>> And, commit the new generated code.
>>> 
>>> Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com>
>> 
>> So wait — if you do
>> 
>> var foo C.typename
>> 
>> Then golang won’t automatically zero out `foo`?
>> 
>> That seems like a bug really; but assuming this fixes real behavior you’ve encountered:
> 
> I would have to dig in again to figure out exactly what Go/cgo is doing
> here, and whether or not this is a bug. But, the behavior I observed was
> that without these nil assignments, I would sometimes get segfaults in
> libxl_string_copy. This patch ensures that libxl__str_dup is not called
> in the empty string case, thus avoiding the segfault.

I skimmed through the CGo page again when I was looking at this, and didn’t see anything specified about what happens if something is passed to a C function before being used by golang.  If you get a chance, I think it would be good to try to file a ticket with the golang project, pointing out the observed behavior, and asking them to either:

1. Document that the golang compiler may not initialize a structure before passing it in to a C function

2. Document that it *will* initialize values to zero, and fix the bug.

 -George

Re: [RESEND PATCH 03/12] golang/xenlight: fix string conversion in generated toC functions

[Nick Rosbrook]

On Thu, Jul 01, 2021 at 02:09:47PM +0000, George Dunlap wrote:
> 
> 
> > On Jun 21, 2021, at 5:11 PM, Nick Rosbrook <rosbrookn@gmail.com> wrote:
> > 
> > On Fri, Jun 18, 2021 at 11:00:26AM +0000, George Dunlap wrote:
> >> 
> >> 
> >>> On May 24, 2021, at 9:36 PM, Nick Rosbrook <rosbrookn@gmail.com> wrote:
> >>> 
> >>> In gengotypes.py, the toC functions only set C string fields when
> >>> the Go strings are non-empty. However, to prevent segfaults in some
> >>> cases, these fields should always at least be set to nil so that the C
> >>> memory is zeroed out.
> >>> 
> >>> Update gengotypes.py so that the generated code always sets these fields
> >>> to nil first, and then proceeds to check if the Go string is non-empty.
> >>> And, commit the new generated code.
> >>> 
> >>> Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com>
> >> 
> >> So wait — if you do
> >> 
> >> var foo C.typename
> >> 
> >> Then golang won’t automatically zero out `foo`?
> >> 
> >> That seems like a bug really; but assuming this fixes real behavior you’ve encountered:
> > 
> > I would have to dig in again to figure out exactly what Go/cgo is doing
> > here, and whether or not this is a bug. But, the behavior I observed was
> > that without these nil assignments, I would sometimes get segfaults in
> > libxl_string_copy. This patch ensures that libxl__str_dup is not called
> > in the empty string case, thus avoiding the segfault.
> 
> I skimmed through the CGo page again when I was looking at this, and didn’t see anything specified about what happens if something is passed to a C function before being used by golang.  If you get a chance, I think it would be good to try to file a ticket with the golang project, pointing out the observed behavior, and asking them to either:
> 
> 1. Document that the golang compiler may not initialize a structure before passing it in to a C function
> 
> 2. Document that it *will* initialize values to zero, and fix the bug.
> 
Sorry for the late reply. But that's a good idea, I can try and come up
with a reproducible example and open an issue.

Thanks,
NR

[RESEND PATCH 04/12] golang/xenlight: export keyed union interface types

[Nick Rosbrook]

For structs that have a keyed union, e.g. DomainBuildInfo, the TypeUnion
field must be exported so that package users can get/set the fields
within. This means that users are aware of the existence of the
interface type used in those fields (see [1]), so it is awkward that the
interface itself is not exported. However, the single method within the
interface must remain unexported so that users cannot mistakenly "implement"
those interfaces.

Since there seems to be no reason to do otherwise, export the keyed
union interface types.

[1] https://pkg.go.dev/xenbits.xenproject.org/git-http/xen.git/tools/golang/xenlight?tab=doc#DeviceUsbdev

Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com>
---
 tools/golang/xenlight/gengotypes.py |  6 +--
 tools/golang/xenlight/types.gen.go  | 58 ++++++++++++++---------------
 2 files changed, 32 insertions(+), 32 deletions(-)

diff --git a/tools/golang/xenlight/gengotypes.py b/tools/golang/xenlight/gengotypes.py
index e6daa9b92f..3796632f7d 100644
--- a/tools/golang/xenlight/gengotypes.py
+++ b/tools/golang/xenlight/gengotypes.py
@@ -159,7 +159,7 @@ def xenlight_golang_define_union(ty = None, struct_name = '', union_name = ''):
     extras = []
 
     interface_name = '{0}_{1}_union'.format(struct_name, ty.keyvar.name)
-    interface_name = xenlight_golang_fmt_name(interface_name, exported=False)
+    interface_name = xenlight_golang_fmt_name(interface_name)
 
     s += 'type {0} interface {{\n'.format(interface_name)
     s += 'is{0}()\n'.format(interface_name)
@@ -341,7 +341,7 @@ def xenlight_golang_union_from_C(ty = None, union_name = '', struct_name = ''):
     field_name = xenlight_golang_fmt_name('{0}_union'.format(keyname))
 
     interface_name = '{0}_{1}_union'.format(struct_name, keyname)
-    interface_name = xenlight_golang_fmt_name(interface_name, exported=False)
+    interface_name = xenlight_golang_fmt_name(interface_name)
 
     cgo_keyname = keyname
     if cgo_keyname in go_keywords:
@@ -546,7 +546,7 @@ def xenlight_golang_union_to_C(ty = None, union_name = '',
     gokeytype = xenlight_golang_fmt_name(keytype)
 
     interface_name = '{0}_{1}_union'.format(struct_name, keyname)
-    interface_name = xenlight_golang_fmt_name(interface_name, exported=False)
+    interface_name = xenlight_golang_fmt_name(interface_name)
 
     cgo_keyname = keyname
     if cgo_keyname in go_keywords:
diff --git a/tools/golang/xenlight/types.gen.go b/tools/golang/xenlight/types.gen.go
index f2ceceb61c..a214dd9df6 100644
--- a/tools/golang/xenlight/types.gen.go
+++ b/tools/golang/xenlight/types.gen.go
@@ -337,18 +337,18 @@ State int
 Evtch int
 Rref int
 Connection ChannelConnection
-ConnectionUnion channelinfoConnectionUnion
+ConnectionUnion ChannelinfoConnectionUnion
 }
 
-type channelinfoConnectionUnion interface {
-ischannelinfoConnectionUnion()
+type ChannelinfoConnectionUnion interface {
+isChannelinfoConnectionUnion()
 }
 
 type ChannelinfoConnectionUnionPty struct {
 Path string
 }
 
-func (x ChannelinfoConnectionUnionPty) ischannelinfoConnectionUnion(){}
+func (x ChannelinfoConnectionUnionPty) isChannelinfoConnectionUnion(){}
 
 type Vminfo struct {
 Uuid Uuid
@@ -510,7 +510,7 @@ Apic Defbool
 DmRestrict Defbool
 Tee TeeType
 Type DomainType
-TypeUnion domainBuildInfoTypeUnion
+TypeUnion DomainBuildInfoTypeUnion
 ArchArm struct {
 GicVersion GicVersion
 Vuart VuartType
@@ -522,8 +522,8 @@ Altp2M Altp2MMode
 VmtraceBufKb int
 }
 
-type domainBuildInfoTypeUnion interface {
-isdomainBuildInfoTypeUnion()
+type DomainBuildInfoTypeUnion interface {
+isDomainBuildInfoTypeUnion()
 }
 
 type DomainBuildInfoTypeUnionHvm struct {
@@ -575,7 +575,7 @@ RdmMemBoundaryMemkb uint64
 McaCaps uint64
 }
 
-func (x DomainBuildInfoTypeUnionHvm) isdomainBuildInfoTypeUnion(){}
+func (x DomainBuildInfoTypeUnionHvm) isDomainBuildInfoTypeUnion(){}
 
 type DomainBuildInfoTypeUnionPv struct {
 Kernel string
@@ -588,7 +588,7 @@ Features string
 E820Host Defbool
 }
 
-func (x DomainBuildInfoTypeUnionPv) isdomainBuildInfoTypeUnion(){}
+func (x DomainBuildInfoTypeUnionPv) isDomainBuildInfoTypeUnion(){}
 
 type DomainBuildInfoTypeUnionPvh struct {
 Pvshim Defbool
@@ -597,7 +597,7 @@ PvshimCmdline string
 PvshimExtra string
 }
 
-func (x DomainBuildInfoTypeUnionPvh) isdomainBuildInfoTypeUnion(){}
+func (x DomainBuildInfoTypeUnionPvh) isDomainBuildInfoTypeUnion(){}
 
 type DeviceVfb struct {
 BackendDomid Domid
@@ -761,11 +761,11 @@ type DeviceUsbdev struct {
 Ctrl Devid
 Port int
 Type UsbdevType
-TypeUnion deviceUsbdevTypeUnion
+TypeUnion DeviceUsbdevTypeUnion
 }
 
-type deviceUsbdevTypeUnion interface {
-isdeviceUsbdevTypeUnion()
+type DeviceUsbdevTypeUnion interface {
+isDeviceUsbdevTypeUnion()
 }
 
 type DeviceUsbdevTypeUnionHostdev struct {
@@ -773,7 +773,7 @@ Hostbus byte
 Hostaddr byte
 }
 
-func (x DeviceUsbdevTypeUnionHostdev) isdeviceUsbdevTypeUnion(){}
+func (x DeviceUsbdevTypeUnionHostdev) isDeviceUsbdevTypeUnion(){}
 
 type DeviceDtdev struct {
 Path string
@@ -807,18 +807,18 @@ BackendDomname string
 Devid Devid
 Name string
 Connection ChannelConnection
-ConnectionUnion deviceChannelConnectionUnion
+ConnectionUnion DeviceChannelConnectionUnion
 }
 
-type deviceChannelConnectionUnion interface {
-isdeviceChannelConnectionUnion()
+type DeviceChannelConnectionUnion interface {
+isDeviceChannelConnectionUnion()
 }
 
 type DeviceChannelConnectionUnionSocket struct {
 Path string
 }
 
-func (x DeviceChannelConnectionUnionSocket) isdeviceChannelConnectionUnion(){}
+func (x DeviceChannelConnectionUnionSocket) isDeviceChannelConnectionUnion(){}
 
 type ConnectorParam struct {
 UniqueId string
@@ -1116,31 +1116,31 @@ Domid Domid
 Domuuid Uuid
 ForUser uint64
 Type EventType
-TypeUnion eventTypeUnion
+TypeUnion EventTypeUnion
 }
 
-type eventTypeUnion interface {
-iseventTypeUnion()
+type EventTypeUnion interface {
+isEventTypeUnion()
 }
 
 type EventTypeUnionDomainShutdown struct {
 ShutdownReason byte
 }
 
-func (x EventTypeUnionDomainShutdown) iseventTypeUnion(){}
+func (x EventTypeUnionDomainShutdown) isEventTypeUnion(){}
 
 type EventTypeUnionDiskEject struct {
 Vdev string
 Disk DeviceDisk
 }
 
-func (x EventTypeUnionDiskEject) iseventTypeUnion(){}
+func (x EventTypeUnionDiskEject) isEventTypeUnion(){}
 
 type EventTypeUnionOperationComplete struct {
 Rc int
 }
 
-func (x EventTypeUnionOperationComplete) iseventTypeUnion(){}
+func (x EventTypeUnionOperationComplete) isEventTypeUnion(){}
 
 type PsrCmtType int
 const(
@@ -1175,11 +1175,11 @@ PsrFeatTypeMba PsrFeatType = 2
 type PsrHwInfo struct {
 Id uint32
 Type PsrFeatType
-TypeUnion psrHwInfoTypeUnion
+TypeUnion PsrHwInfoTypeUnion
 }
 
-type psrHwInfoTypeUnion interface {
-ispsrHwInfoTypeUnion()
+type PsrHwInfoTypeUnion interface {
+isPsrHwInfoTypeUnion()
 }
 
 type PsrHwInfoTypeUnionCat struct {
@@ -1188,7 +1188,7 @@ CbmLen uint32
 CdpEnabled bool
 }
 
-func (x PsrHwInfoTypeUnionCat) ispsrHwInfoTypeUnion(){}
+func (x PsrHwInfoTypeUnionCat) isPsrHwInfoTypeUnion(){}
 
 type PsrHwInfoTypeUnionMba struct {
 CosMax uint32
@@ -1196,5 +1196,5 @@ ThrtlMax uint32
 Linear bool
 }
 
-func (x PsrHwInfoTypeUnionMba) ispsrHwInfoTypeUnion(){}
+func (x PsrHwInfoTypeUnionMba) isPsrHwInfoTypeUnion(){}
 
-- 
2.17.1

Re: [RESEND PATCH 04/12] golang/xenlight: export keyed union interface types

[George Dunlap]

> On May 24, 2021, at 9:36 PM, Nick Rosbrook <rosbrookn@gmail.com> wrote:
> 
> For structs that have a keyed union, e.g. DomainBuildInfo, the TypeUnion
> field must be exported so that package users can get/set the fields
> within. This means that users are aware of the existence of the
> interface type used in those fields (see [1]), so it is awkward that the
> interface itself is not exported. However, the single method within the
> interface must remain unexported so that users cannot mistakenly "implement"
> those interfaces.
> 
> Since there seems to be no reason to do otherwise, export the keyed
> union interface types.
> 
> [1] https://pkg.go.dev/xenbits.xenproject.org/git-http/xen.git/tools/golang/xenlight?tab=doc#DeviceUsbdev
> 
> Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com>

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

I wonder if at some point we should add documentation to the definitions of specific union types, saying explicitly what union type it implements, and maybe what the Type field should be set to.  e.g.:

DeviceUsbdevTypeUnionHostdev implements the DeviceUsbdevTypeUnion interface.  If DeviceUsbdev.TypeUnion is set to this type, DeviceUsbdev.Type should be set to UsbdevTypeHostdev.

Might be overkill tho.

[RESEND PATCH 05/12] golang/xenlight: use struct pointers in keyed union fields

[Nick Rosbrook]

Currently, when marshalig Go types with keyed union fields, we assign the
value of the struct (e.g. DomainBuildInfoTypeUnionHvm) which implements the
interface of the keyed union field (e.g. DomainBuildInfoTypeUnion).
As-is, this means that if a populated DomainBuildInfo is marshaled to
e.g. JSON, unmarshaling back to DomainBuildInfo will fail.

When the encoding/json is unmarshaling data into a Go type, and
encounters a JSON object, it basically can either marshal the data into
an empty interface, a map, or a struct. It cannot, however, marshal data
into an interface with at least one method defined on it (e.g.
DomainBuildInfoTypeUnion). Before this check is done, however, the
decoder will check if the Go type is a pointer, and dereference it if
so. It will then use the type of this value as the "target" type.

This means that if the TypeUnion field is populated with a
DomainBuildInfoTypeUnion, the decoder will see a non-empty interface and
fail. If the TypeUnion field is populated with a
*DomainBuildInfoTypeUnionHvm, it dereferences the pointer and sees a
struct instead, allowing decoding to continue normally.

Since there does not appear to be a strict need for NOT using pointers
in these fields, update code generation to set keyed union fields to
pointers of their implementing structs.

Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com>
---
 tools/golang/xenlight/gengotypes.py  |  4 +--
 tools/golang/xenlight/helpers.gen.go | 44 ++++++++++++++--------------
 2 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/tools/golang/xenlight/gengotypes.py b/tools/golang/xenlight/gengotypes.py
index 3796632f7d..57f2576468 100644
--- a/tools/golang/xenlight/gengotypes.py
+++ b/tools/golang/xenlight/gengotypes.py
@@ -404,7 +404,7 @@ def xenlight_golang_union_from_C(ty = None, union_name = '', struct_name = ''):
         s += 'if err := {0}.fromC(xc);'.format(goname)
         s += 'err != nil {{\n return fmt.Errorf("converting field {0}: %v", err)\n}}\n'.format(goname)
 
-        s += 'x.{0} = {1}\n'.format(field_name, goname)
+        s += 'x.{0} = &{1}\n'.format(field_name, goname)
 
     # End switch statement
     s += 'default:\n'
@@ -571,7 +571,7 @@ def xenlight_golang_union_to_C(ty = None, union_name = '',
         gotype  = xenlight_golang_fmt_name(cgotype)
 
         field_name = xenlight_golang_fmt_name('{0}_union'.format(keyname))
-        s += 'tmp, ok := x.{0}.({1})\n'.format(field_name,gotype)
+        s += 'tmp, ok := x.{0}.(*{1})\n'.format(field_name,gotype)
         s += 'if !ok {\n'
         s += 'return errors.New("wrong type for union key {0}")\n'.format(keyname)
         s += '}\n'
diff --git a/tools/golang/xenlight/helpers.gen.go b/tools/golang/xenlight/helpers.gen.go
index 5222898fb8..8fc5ec1649 100644
--- a/tools/golang/xenlight/helpers.gen.go
+++ b/tools/golang/xenlight/helpers.gen.go
@@ -443,7 +443,7 @@ var connectionPty ChannelinfoConnectionUnionPty
 if err := connectionPty.fromC(xc);err != nil {
  return fmt.Errorf("converting field connectionPty: %v", err)
 }
-x.ConnectionUnion = connectionPty
+x.ConnectionUnion = &connectionPty
 case ChannelConnectionSocket:
 x.ConnectionUnion = nil
 case ChannelConnectionUnknown:
@@ -485,7 +485,7 @@ switch x.Connection{
 case ChannelConnectionUnknown:
 break
 case ChannelConnectionPty:
-tmp, ok := x.ConnectionUnion.(ChannelinfoConnectionUnionPty)
+tmp, ok := x.ConnectionUnion.(*ChannelinfoConnectionUnionPty)
 if !ok {
 return errors.New("wrong type for union key connection")
 }
@@ -1120,7 +1120,7 @@ var typeHvm DomainBuildInfoTypeUnionHvm
 if err := typeHvm.fromC(xc);err != nil {
  return fmt.Errorf("converting field typeHvm: %v", err)
 }
-x.TypeUnion = typeHvm
+x.TypeUnion = &typeHvm
 case DomainTypeInvalid:
 x.TypeUnion = nil
 case DomainTypePv:
@@ -1128,13 +1128,13 @@ var typePv DomainBuildInfoTypeUnionPv
 if err := typePv.fromC(xc);err != nil {
  return fmt.Errorf("converting field typePv: %v", err)
 }
-x.TypeUnion = typePv
+x.TypeUnion = &typePv
 case DomainTypePvh:
 var typePvh DomainBuildInfoTypeUnionPvh
 if err := typePvh.fromC(xc);err != nil {
  return fmt.Errorf("converting field typePvh: %v", err)
 }
-x.TypeUnion = typePvh
+x.TypeUnion = &typePvh
 default:
 return fmt.Errorf("invalid union key '%v'", x.Type)}
 x.ArchArm.GicVersion = GicVersion(xc.arch_arm.gic_version)
@@ -1465,7 +1465,7 @@ xc.tee = C.libxl_tee_type(x.Tee)
 xc._type = C.libxl_domain_type(x.Type)
 switch x.Type{
 case DomainTypeHvm:
-tmp, ok := x.TypeUnion.(DomainBuildInfoTypeUnionHvm)
+tmp, ok := x.TypeUnion.(*DomainBuildInfoTypeUnionHvm)
 if !ok {
 return errors.New("wrong type for union key type")
 }
@@ -1593,7 +1593,7 @@ hvm.mca_caps = C.uint64_t(tmp.McaCaps)
 hvmBytes := C.GoBytes(unsafe.Pointer(&hvm),C.sizeof_libxl_domain_build_info_type_union_hvm)
 copy(xc.u[:],hvmBytes)
 case DomainTypePv:
-tmp, ok := x.TypeUnion.(DomainBuildInfoTypeUnionPv)
+tmp, ok := x.TypeUnion.(*DomainBuildInfoTypeUnionPv)
 if !ok {
 return errors.New("wrong type for union key type")
 }
@@ -1623,7 +1623,7 @@ return fmt.Errorf("converting field E820Host: %v", err)
 pvBytes := C.GoBytes(unsafe.Pointer(&pv),C.sizeof_libxl_domain_build_info_type_union_pv)
 copy(xc.u[:],pvBytes)
 case DomainTypePvh:
-tmp, ok := x.TypeUnion.(DomainBuildInfoTypeUnionPvh)
+tmp, ok := x.TypeUnion.(*DomainBuildInfoTypeUnionPvh)
 if !ok {
 return errors.New("wrong type for union key type")
 }
@@ -2283,7 +2283,7 @@ var typeHostdev DeviceUsbdevTypeUnionHostdev
 if err := typeHostdev.fromC(xc);err != nil {
  return fmt.Errorf("converting field typeHostdev: %v", err)
 }
-x.TypeUnion = typeHostdev
+x.TypeUnion = &typeHostdev
 default:
 return fmt.Errorf("invalid union key '%v'", x.Type)}
 
@@ -2310,7 +2310,7 @@ xc.port = C.int(x.Port)
 xc._type = C.libxl_usbdev_type(x.Type)
 switch x.Type{
 case UsbdevTypeHostdev:
-tmp, ok := x.TypeUnion.(DeviceUsbdevTypeUnionHostdev)
+tmp, ok := x.TypeUnion.(*DeviceUsbdevTypeUnionHostdev)
 if !ok {
 return errors.New("wrong type for union key type")
 }
@@ -2508,7 +2508,7 @@ var connectionSocket DeviceChannelConnectionUnionSocket
 if err := connectionSocket.fromC(xc);err != nil {
  return fmt.Errorf("converting field connectionSocket: %v", err)
 }
-x.ConnectionUnion = connectionSocket
+x.ConnectionUnion = &connectionSocket
 case ChannelConnectionUnknown:
 x.ConnectionUnion = nil
 default:
@@ -2546,7 +2546,7 @@ break
 case ChannelConnectionPty:
 break
 case ChannelConnectionSocket:
-tmp, ok := x.ConnectionUnion.(DeviceChannelConnectionUnionSocket)
+tmp, ok := x.ConnectionUnion.(*DeviceChannelConnectionUnionSocket)
 if !ok {
 return errors.New("wrong type for union key connection")
 }
@@ -4107,7 +4107,7 @@ var typeDiskEject EventTypeUnionDiskEject
 if err := typeDiskEject.fromC(xc);err != nil {
  return fmt.Errorf("converting field typeDiskEject: %v", err)
 }
-x.TypeUnion = typeDiskEject
+x.TypeUnion = &typeDiskEject
 case EventTypeDomainCreateConsoleAvailable:
 x.TypeUnion = nil
 case EventTypeDomainDeath:
@@ -4117,13 +4117,13 @@ var typeDomainShutdown EventTypeUnionDomainShutdown
 if err := typeDomainShutdown.fromC(xc);err != nil {
  return fmt.Errorf("converting field typeDomainShutdown: %v", err)
 }
-x.TypeUnion = typeDomainShutdown
+x.TypeUnion = &typeDomainShutdown
 case EventTypeOperationComplete:
 var typeOperationComplete EventTypeUnionOperationComplete
 if err := typeOperationComplete.fromC(xc);err != nil {
  return fmt.Errorf("converting field typeOperationComplete: %v", err)
 }
-x.TypeUnion = typeOperationComplete
+x.TypeUnion = &typeOperationComplete
 default:
 return fmt.Errorf("invalid union key '%v'", x.Type)}
 
@@ -4178,7 +4178,7 @@ xc.for_user = C.uint64_t(x.ForUser)
 xc._type = C.libxl_event_type(x.Type)
 switch x.Type{
 case EventTypeDomainShutdown:
-tmp, ok := x.TypeUnion.(EventTypeUnionDomainShutdown)
+tmp, ok := x.TypeUnion.(*EventTypeUnionDomainShutdown)
 if !ok {
 return errors.New("wrong type for union key type")
 }
@@ -4189,7 +4189,7 @@ copy(xc.u[:],domain_shutdownBytes)
 case EventTypeDomainDeath:
 break
 case EventTypeDiskEject:
-tmp, ok := x.TypeUnion.(EventTypeUnionDiskEject)
+tmp, ok := x.TypeUnion.(*EventTypeUnionDiskEject)
 if !ok {
 return errors.New("wrong type for union key type")
 }
@@ -4203,7 +4203,7 @@ return fmt.Errorf("converting field Disk: %v", err)
 disk_ejectBytes := C.GoBytes(unsafe.Pointer(&disk_eject),C.sizeof_libxl_event_type_union_disk_eject)
 copy(xc.u[:],disk_ejectBytes)
 case EventTypeOperationComplete:
-tmp, ok := x.TypeUnion.(EventTypeUnionOperationComplete)
+tmp, ok := x.TypeUnion.(*EventTypeUnionOperationComplete)
 if !ok {
 return errors.New("wrong type for union key type")
 }
@@ -4278,13 +4278,13 @@ var typeCat PsrHwInfoTypeUnionCat
 if err := typeCat.fromC(xc);err != nil {
  return fmt.Errorf("converting field typeCat: %v", err)
 }
-x.TypeUnion = typeCat
+x.TypeUnion = &typeCat
 case PsrFeatTypeMba:
 var typeMba PsrHwInfoTypeUnionMba
 if err := typeMba.fromC(xc);err != nil {
  return fmt.Errorf("converting field typeMba: %v", err)
 }
-x.TypeUnion = typeMba
+x.TypeUnion = &typeMba
 default:
 return fmt.Errorf("invalid union key '%v'", x.Type)}
 
@@ -4323,7 +4323,7 @@ xc.id = C.uint32_t(x.Id)
 xc._type = C.libxl_psr_feat_type(x.Type)
 switch x.Type{
 case PsrFeatTypeCat:
-tmp, ok := x.TypeUnion.(PsrHwInfoTypeUnionCat)
+tmp, ok := x.TypeUnion.(*PsrHwInfoTypeUnionCat)
 if !ok {
 return errors.New("wrong type for union key type")
 }
@@ -4334,7 +4334,7 @@ cat.cdp_enabled = C.bool(tmp.CdpEnabled)
 catBytes := C.GoBytes(unsafe.Pointer(&cat),C.sizeof_libxl_psr_hw_info_type_union_cat)
 copy(xc.u[:],catBytes)
 case PsrFeatTypeMba:
-tmp, ok := x.TypeUnion.(PsrHwInfoTypeUnionMba)
+tmp, ok := x.TypeUnion.(*PsrHwInfoTypeUnionMba)
 if !ok {
 return errors.New("wrong type for union key type")
 }
-- 
2.17.1

Re: [RESEND PATCH 05/12] golang/xenlight: use struct pointers in keyed union fields

[George Dunlap]

> On May 24, 2021, at 9:36 PM, Nick Rosbrook <rosbrookn@gmail.com> wrote:
> 
> Currently, when marshalig Go types with keyed union fields, we assign the
> value of the struct (e.g. DomainBuildInfoTypeUnionHvm) which implements the
> interface of the keyed union field (e.g. DomainBuildInfoTypeUnion).
> As-is, this means that if a populated DomainBuildInfo is marshaled to
> e.g. JSON, unmarshaling back to DomainBuildInfo will fail.
> 
> When the encoding/json is unmarshaling data into a Go type, and
> encounters a JSON object, it basically can either marshal the data into
> an empty interface, a map, or a struct. It cannot, however, marshal data
> into an interface with at least one method defined on it (e.g.
> DomainBuildInfoTypeUnion). Before this check is done, however, the
> decoder will check if the Go type is a pointer, and dereference it if
> so. It will then use the type of this value as the "target" type.
> 
> This means that if the TypeUnion field is populated with a
> DomainBuildInfoTypeUnion, the decoder will see a non-empty interface and
> fail. If the TypeUnion field is populated with a
> *DomainBuildInfoTypeUnionHvm, it dereferences the pointer and sees a
> struct instead, allowing decoding to continue normally.
> 
> Since there does not appear to be a strict need for NOT using pointers
> in these fields, update code generation to set keyed union fields to
> pointers of their implementing structs.
> 
> Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com>

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

[RESEND PATCH 06/12] golang/xenlight: rename Ctx receivers to ctx

[Nick Rosbrook]

As a matter of style, it is strange to see capitalized receiver names,
due to the significance of capitalized symbols in Go (although there is
in fact nothing special about a capitalized receiver name). Fix this in
xenlight.go by running:

  gofmt -w -r 'Ctx -> ctx' xenlight.go

from tools/golang/xenlight. There is no functional change.

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

diff --git a/tools/golang/xenlight/xenlight.go b/tools/golang/xenlight/xenlight.go
index 13171d0ad1..fc3eb0bf3f 100644
--- a/tools/golang/xenlight/xenlight.go
+++ b/tools/golang/xenlight/xenlight.go
@@ -203,13 +203,13 @@ type Domid uint32
 // 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) {
+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 {
+	if ret := C.libxl_name_to_domid(ctx.ctx, cname, &domid); ret != 0 {
 		return DomidInvalid, Error(ret)
 	}
 
@@ -223,8 +223,8 @@ func (Ctx *Context) NameToDomid(name string) (Domid, error) {
 // 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))
+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)
@@ -594,10 +594,10 @@ func SchedulerFromString(name string) (s Scheduler, err error) {
 
 // libxl_cpupoolinfo * libxl_list_cpupool(libxl_ctx*, int *nb_pool_out);
 // void libxl_cpupoolinfo_list_free(libxl_cpupoolinfo *list, int nb_pool);
-func (Ctx *Context) ListCpupool() (list []Cpupoolinfo) {
+func (ctx *Context) ListCpupool() (list []Cpupoolinfo) {
 	var nbPool C.int
 
-	c_cpupool_list := C.libxl_list_cpupool(Ctx.ctx, &nbPool)
+	c_cpupool_list := C.libxl_list_cpupool(ctx.ctx, &nbPool)
 
 	defer C.libxl_cpupoolinfo_list_free(c_cpupool_list, nbPool)
 
@@ -617,10 +617,10 @@ func (Ctx *Context) ListCpupool() (list []Cpupoolinfo) {
 }
 
 // int libxl_cpupool_info(libxl_ctx *ctx, libxl_cpupoolinfo *info, uint32_t poolid);
-func (Ctx *Context) CpupoolInfo(Poolid uint32) (pool Cpupoolinfo, err error) {
+func (ctx *Context) CpupoolInfo(Poolid uint32) (pool Cpupoolinfo, err error) {
 	var c_cpupool C.libxl_cpupoolinfo
 
-	ret := C.libxl_cpupool_info(Ctx.ctx, &c_cpupool, C.uint32_t(Poolid))
+	ret := C.libxl_cpupool_info(ctx.ctx, &c_cpupool, C.uint32_t(Poolid))
 	if ret != 0 {
 		err = Error(-ret)
 		return
@@ -638,7 +638,7 @@ func (Ctx *Context) CpupoolInfo(Poolid uint32) (pool Cpupoolinfo, err error) {
 //                          uint32_t *poolid);
 // FIXME: uuid
 // FIXME: Setting poolid
-func (Ctx *Context) CpupoolCreate(Name string, Scheduler Scheduler, Cpumap Bitmap) (err error, Poolid uint32) {
+func (ctx *Context) CpupoolCreate(Name string, Scheduler Scheduler, Cpumap Bitmap) (err error, Poolid uint32) {
 	poolid := C.uint32_t(C.LIBXL_CPUPOOL_POOLID_ANY)
 	name := C.CString(Name)
 	defer C.free(unsafe.Pointer(name))
@@ -653,7 +653,7 @@ func (Ctx *Context) CpupoolCreate(Name string, Scheduler Scheduler, Cpumap Bitma
 	}
 	defer C.libxl_bitmap_dispose(&cbm)
 
-	ret := C.libxl_cpupool_create(Ctx.ctx, name, C.libxl_scheduler(Scheduler),
+	ret := C.libxl_cpupool_create(ctx.ctx, name, C.libxl_scheduler(Scheduler),
 		cbm, &uuid, &poolid)
 	if ret != 0 {
 		err = Error(-ret)
@@ -666,8 +666,8 @@ func (Ctx *Context) CpupoolCreate(Name string, Scheduler Scheduler, Cpumap Bitma
 }
 
 // int libxl_cpupool_destroy(libxl_ctx *ctx, uint32_t poolid);
-func (Ctx *Context) CpupoolDestroy(Poolid uint32) (err error) {
-	ret := C.libxl_cpupool_destroy(Ctx.ctx, C.uint32_t(Poolid))
+func (ctx *Context) CpupoolDestroy(Poolid uint32) (err error) {
+	ret := C.libxl_cpupool_destroy(ctx.ctx, C.uint32_t(Poolid))
 	if ret != 0 {
 		err = Error(-ret)
 		return
@@ -677,8 +677,8 @@ func (Ctx *Context) CpupoolDestroy(Poolid uint32) (err error) {
 }
 
 // int libxl_cpupool_cpuadd(libxl_ctx *ctx, uint32_t poolid, int cpu);
-func (Ctx *Context) CpupoolCpuadd(Poolid uint32, Cpu int) (err error) {
-	ret := C.libxl_cpupool_cpuadd(Ctx.ctx, C.uint32_t(Poolid), C.int(Cpu))
+func (ctx *Context) CpupoolCpuadd(Poolid uint32, Cpu int) (err error) {
+	ret := C.libxl_cpupool_cpuadd(ctx.ctx, C.uint32_t(Poolid), C.int(Cpu))
 	if ret != 0 {
 		err = Error(-ret)
 		return
@@ -689,14 +689,14 @@ func (Ctx *Context) CpupoolCpuadd(Poolid uint32, Cpu int) (err error) {
 
 // int libxl_cpupool_cpuadd_cpumap(libxl_ctx *ctx, uint32_t poolid,
 //                                 const libxl_bitmap *cpumap);
-func (Ctx *Context) CpupoolCpuaddCpumap(Poolid uint32, Cpumap Bitmap) (err error) {
+func (ctx *Context) CpupoolCpuaddCpumap(Poolid uint32, Cpumap Bitmap) (err error) {
 	var cbm C.libxl_bitmap
 	if err = Cpumap.toC(&cbm); err != nil {
 		return
 	}
 	defer C.libxl_bitmap_dispose(&cbm)
 
-	ret := C.libxl_cpupool_cpuadd_cpumap(Ctx.ctx, C.uint32_t(Poolid), &cbm)
+	ret := C.libxl_cpupool_cpuadd_cpumap(ctx.ctx, C.uint32_t(Poolid), &cbm)
 	if ret != 0 {
 		err = Error(-ret)
 		return
@@ -706,8 +706,8 @@ func (Ctx *Context) CpupoolCpuaddCpumap(Poolid uint32, Cpumap Bitmap) (err error
 }
 
 // int libxl_cpupool_cpuremove(libxl_ctx *ctx, uint32_t poolid, int cpu);
-func (Ctx *Context) CpupoolCpuremove(Poolid uint32, Cpu int) (err error) {
-	ret := C.libxl_cpupool_cpuremove(Ctx.ctx, C.uint32_t(Poolid), C.int(Cpu))
+func (ctx *Context) CpupoolCpuremove(Poolid uint32, Cpu int) (err error) {
+	ret := C.libxl_cpupool_cpuremove(ctx.ctx, C.uint32_t(Poolid), C.int(Cpu))
 	if ret != 0 {
 		err = Error(-ret)
 		return
@@ -718,14 +718,14 @@ func (Ctx *Context) CpupoolCpuremove(Poolid uint32, Cpu int) (err error) {
 
 // int libxl_cpupool_cpuremove_cpumap(libxl_ctx *ctx, uint32_t poolid,
 //                                    const libxl_bitmap *cpumap);
-func (Ctx *Context) CpupoolCpuremoveCpumap(Poolid uint32, Cpumap Bitmap) (err error) {
+func (ctx *Context) CpupoolCpuremoveCpumap(Poolid uint32, Cpumap Bitmap) (err error) {
 	var cbm C.libxl_bitmap
 	if err = Cpumap.toC(&cbm); err != nil {
 		return
 	}
 	defer C.libxl_bitmap_dispose(&cbm)
 
-	ret := C.libxl_cpupool_cpuremove_cpumap(Ctx.ctx, C.uint32_t(Poolid), &cbm)
+	ret := C.libxl_cpupool_cpuremove_cpumap(ctx.ctx, C.uint32_t(Poolid), &cbm)
 	if ret != 0 {
 		err = Error(-ret)
 		return
@@ -735,11 +735,11 @@ func (Ctx *Context) CpupoolCpuremoveCpumap(Poolid uint32, Cpumap Bitmap) (err er
 }
 
 // int libxl_cpupool_rename(libxl_ctx *ctx, const char *name, uint32_t poolid);
-func (Ctx *Context) CpupoolRename(Name string, Poolid uint32) (err error) {
+func (ctx *Context) CpupoolRename(Name string, Poolid uint32) (err error) {
 	name := C.CString(Name)
 	defer C.free(unsafe.Pointer(name))
 
-	ret := C.libxl_cpupool_rename(Ctx.ctx, name, C.uint32_t(Poolid))
+	ret := C.libxl_cpupool_rename(ctx.ctx, name, C.uint32_t(Poolid))
 	if ret != 0 {
 		err = Error(-ret)
 		return
@@ -749,10 +749,10 @@ func (Ctx *Context) CpupoolRename(Name string, Poolid uint32) (err error) {
 }
 
 // int libxl_cpupool_cpuadd_node(libxl_ctx *ctx, uint32_t poolid, int node, int *cpus);
-func (Ctx *Context) CpupoolCpuaddNode(Poolid uint32, Node int) (Cpus int, err error) {
+func (ctx *Context) CpupoolCpuaddNode(Poolid uint32, Node int) (Cpus int, err error) {
 	ccpus := C.int(0)
 
-	ret := C.libxl_cpupool_cpuadd_node(Ctx.ctx, C.uint32_t(Poolid), C.int(Node), &ccpus)
+	ret := C.libxl_cpupool_cpuadd_node(ctx.ctx, C.uint32_t(Poolid), C.int(Node), &ccpus)
 	if ret != 0 {
 		err = Error(-ret)
 		return
@@ -764,10 +764,10 @@ func (Ctx *Context) CpupoolCpuaddNode(Poolid uint32, Node int) (Cpus int, err er
 }
 
 // int libxl_cpupool_cpuremove_node(libxl_ctx *ctx, uint32_t poolid, int node, int *cpus);
-func (Ctx *Context) CpupoolCpuremoveNode(Poolid uint32, Node int) (Cpus int, err error) {
+func (ctx *Context) CpupoolCpuremoveNode(Poolid uint32, Node int) (Cpus int, err error) {
 	ccpus := C.int(0)
 
-	ret := C.libxl_cpupool_cpuremove_node(Ctx.ctx, C.uint32_t(Poolid), C.int(Node), &ccpus)
+	ret := C.libxl_cpupool_cpuremove_node(ctx.ctx, C.uint32_t(Poolid), C.int(Node), &ccpus)
 	if ret != 0 {
 		err = Error(-ret)
 		return
@@ -779,8 +779,8 @@ func (Ctx *Context) CpupoolCpuremoveNode(Poolid uint32, Node int) (Cpus int, err
 }
 
 // int libxl_cpupool_movedomain(libxl_ctx *ctx, uint32_t poolid, uint32_t domid);
-func (Ctx *Context) CpupoolMovedomain(Poolid uint32, Id Domid) (err error) {
-	ret := C.libxl_cpupool_movedomain(Ctx.ctx, C.uint32_t(Poolid), C.uint32_t(Id))
+func (ctx *Context) CpupoolMovedomain(Poolid uint32, Id Domid) (err error) {
+	ret := C.libxl_cpupool_movedomain(ctx.ctx, C.uint32_t(Poolid), C.uint32_t(Id))
 	if ret != 0 {
 		err = Error(-ret)
 		return
@@ -792,8 +792,8 @@ func (Ctx *Context) CpupoolMovedomain(Poolid uint32, Id Domid) (err error) {
 //
 // Utility functions
 //
-func (Ctx *Context) CpupoolFindByName(name string) (info Cpupoolinfo, found bool) {
-	plist := Ctx.ListCpupool()
+func (ctx *Context) CpupoolFindByName(name string) (info Cpupoolinfo, found bool) {
+	plist := ctx.ListCpupool()
 
 	for i := range plist {
 		if plist[i].PoolName == name {
@@ -805,14 +805,14 @@ func (Ctx *Context) CpupoolFindByName(name string) (info Cpupoolinfo, found bool
 	return
 }
 
-func (Ctx *Context) CpupoolMakeFree(Cpumap Bitmap) (err error) {
-	plist := Ctx.ListCpupool()
+func (ctx *Context) CpupoolMakeFree(Cpumap Bitmap) (err error) {
+	plist := ctx.ListCpupool()
 
 	for i := range plist {
 		var Intersection Bitmap
 		Intersection = Cpumap.And(plist[i].Cpumap)
 		if !Intersection.IsEmpty() {
-			err = Ctx.CpupoolCpuremoveCpumap(plist[i].Poolid, Intersection)
+			err = ctx.CpupoolCpuremoveCpumap(plist[i].Poolid, Intersection)
 			if err != nil {
 				return
 			}
@@ -940,8 +940,8 @@ func (bm Bitmap) String() (s string) {
 }
 
 //int libxl_get_max_cpus(libxl_ctx *ctx);
-func (Ctx *Context) GetMaxCpus() (maxCpus int, err error) {
-	ret := C.libxl_get_max_cpus(Ctx.ctx)
+func (ctx *Context) GetMaxCpus() (maxCpus int, err error) {
+	ret := C.libxl_get_max_cpus(ctx.ctx)
 	if ret < 0 {
 		err = Error(-ret)
 		return
@@ -951,8 +951,8 @@ func (Ctx *Context) GetMaxCpus() (maxCpus int, err error) {
 }
 
 //int libxl_get_online_cpus(libxl_ctx *ctx);
-func (Ctx *Context) GetOnlineCpus() (onCpus int, err error) {
-	ret := C.libxl_get_online_cpus(Ctx.ctx)
+func (ctx *Context) GetOnlineCpus() (onCpus int, err error) {
+	ret := C.libxl_get_online_cpus(ctx.ctx)
 	if ret < 0 {
 		err = Error(-ret)
 		return
@@ -962,8 +962,8 @@ func (Ctx *Context) GetOnlineCpus() (onCpus int, err error) {
 }
 
 //int libxl_get_max_nodes(libxl_ctx *ctx);
-func (Ctx *Context) GetMaxNodes() (maxNodes int, err error) {
-	ret := C.libxl_get_max_nodes(Ctx.ctx)
+func (ctx *Context) GetMaxNodes() (maxNodes int, err error) {
+	ret := C.libxl_get_max_nodes(ctx.ctx)
 	if ret < 0 {
 		err = Error(-ret)
 		return
@@ -973,9 +973,9 @@ func (Ctx *Context) GetMaxNodes() (maxNodes int, err error) {
 }
 
 //int libxl_get_free_memory(libxl_ctx *ctx, uint64_t *memkb);
-func (Ctx *Context) GetFreeMemory() (memkb uint64, err error) {
+func (ctx *Context) GetFreeMemory() (memkb uint64, err error) {
 	var cmem C.uint64_t
-	ret := C.libxl_get_free_memory(Ctx.ctx, &cmem)
+	ret := C.libxl_get_free_memory(ctx.ctx, &cmem)
 
 	if ret < 0 {
 		err = Error(-ret)
@@ -988,12 +988,12 @@ func (Ctx *Context) GetFreeMemory() (memkb uint64, err error) {
 }
 
 //int libxl_get_physinfo(libxl_ctx *ctx, libxl_physinfo *physinfo)
-func (Ctx *Context) GetPhysinfo() (physinfo *Physinfo, err error) {
+func (ctx *Context) GetPhysinfo() (physinfo *Physinfo, err error) {
 	var cphys C.libxl_physinfo
 	C.libxl_physinfo_init(&cphys)
 	defer C.libxl_physinfo_dispose(&cphys)
 
-	ret := C.libxl_get_physinfo(Ctx.ctx, &cphys)
+	ret := C.libxl_get_physinfo(ctx.ctx, &cphys)
 
 	if ret < 0 {
 		err = Error(ret)
@@ -1005,22 +1005,22 @@ func (Ctx *Context) GetPhysinfo() (physinfo *Physinfo, err error) {
 }
 
 //const libxl_version_info* libxl_get_version_info(libxl_ctx *ctx);
-func (Ctx *Context) GetVersionInfo() (info *VersionInfo, err error) {
+func (ctx *Context) GetVersionInfo() (info *VersionInfo, err error) {
 	var cinfo *C.libxl_version_info
 
-	cinfo = C.libxl_get_version_info(Ctx.ctx)
+	cinfo = C.libxl_get_version_info(ctx.ctx)
 
 	err = info.fromC(cinfo)
 
 	return
 }
 
-func (Ctx *Context) DomainInfo(Id Domid) (di *Dominfo, err error) {
+func (ctx *Context) DomainInfo(Id Domid) (di *Dominfo, err error) {
 	var cdi C.libxl_dominfo
 	C.libxl_dominfo_init(&cdi)
 	defer C.libxl_dominfo_dispose(&cdi)
 
-	ret := C.libxl_domain_info(Ctx.ctx, &cdi, C.uint32_t(Id))
+	ret := C.libxl_domain_info(ctx.ctx, &cdi, C.uint32_t(Id))
 
 	if ret != 0 {
 		err = Error(-ret)
@@ -1032,8 +1032,8 @@ func (Ctx *Context) DomainInfo(Id Domid) (di *Dominfo, err error) {
 	return
 }
 
-func (Ctx *Context) DomainUnpause(Id Domid) (err error) {
-	ret := C.libxl_domain_unpause(Ctx.ctx, C.uint32_t(Id), nil)
+func (ctx *Context) DomainUnpause(Id Domid) (err error) {
+	ret := C.libxl_domain_unpause(ctx.ctx, C.uint32_t(Id), nil)
 
 	if ret != 0 {
 		err = Error(-ret)
@@ -1042,8 +1042,8 @@ func (Ctx *Context) DomainUnpause(Id Domid) (err error) {
 }
 
 //int libxl_domain_pause(libxl_ctx *ctx, uint32_t domain);
-func (Ctx *Context) DomainPause(id Domid) (err error) {
-	ret := C.libxl_domain_pause(Ctx.ctx, C.uint32_t(id), nil)
+func (ctx *Context) DomainPause(id Domid) (err error) {
+	ret := C.libxl_domain_pause(ctx.ctx, C.uint32_t(id), nil)
 
 	if ret != 0 {
 		err = Error(-ret)
@@ -1052,8 +1052,8 @@ func (Ctx *Context) DomainPause(id Domid) (err error) {
 }
 
 //int libxl_domain_shutdown(libxl_ctx *ctx, uint32_t domid);
-func (Ctx *Context) DomainShutdown(id Domid) (err error) {
-	ret := C.libxl_domain_shutdown(Ctx.ctx, C.uint32_t(id), nil)
+func (ctx *Context) DomainShutdown(id Domid) (err error) {
+	ret := C.libxl_domain_shutdown(ctx.ctx, C.uint32_t(id), nil)
 
 	if ret != 0 {
 		err = Error(-ret)
@@ -1062,8 +1062,8 @@ func (Ctx *Context) DomainShutdown(id Domid) (err error) {
 }
 
 //int libxl_domain_reboot(libxl_ctx *ctx, uint32_t domid);
-func (Ctx *Context) DomainReboot(id Domid) (err error) {
-	ret := C.libxl_domain_reboot(Ctx.ctx, C.uint32_t(id), nil)
+func (ctx *Context) DomainReboot(id Domid) (err error) {
+	ret := C.libxl_domain_reboot(ctx.ctx, C.uint32_t(id), nil)
 
 	if ret != 0 {
 		err = Error(-ret)
@@ -1073,9 +1073,9 @@ func (Ctx *Context) DomainReboot(id Domid) (err error) {
 
 //libxl_dominfo * libxl_list_domain(libxl_ctx*, int *nb_domain_out);
 //void libxl_dominfo_list_free(libxl_dominfo *list, int nb_domain);
-func (Ctx *Context) ListDomain() (glist []Dominfo) {
+func (ctx *Context) ListDomain() (glist []Dominfo) {
 	var nbDomain C.int
-	clist := C.libxl_list_domain(Ctx.ctx, &nbDomain)
+	clist := C.libxl_list_domain(ctx.ctx, &nbDomain)
 	defer C.libxl_dominfo_list_free(clist, nbDomain)
 
 	if int(nbDomain) == 0 {
@@ -1095,11 +1095,11 @@ func (Ctx *Context) ListDomain() (glist []Dominfo) {
 //libxl_vcpuinfo *libxl_list_vcpu(libxl_ctx *ctx, uint32_t domid,
 //				int *nb_vcpu, int *nr_cpus_out);
 //void libxl_vcpuinfo_list_free(libxl_vcpuinfo *, int nr_vcpus);
-func (Ctx *Context) ListVcpu(id Domid) (glist []Vcpuinfo) {
+func (ctx *Context) ListVcpu(id Domid) (glist []Vcpuinfo) {
 	var nbVcpu C.int
 	var nrCpu C.int
 
-	clist := C.libxl_list_vcpu(Ctx.ctx, C.uint32_t(id), &nbVcpu, &nrCpu)
+	clist := C.libxl_list_vcpu(ctx.ctx, C.uint32_t(id), &nbVcpu, &nrCpu)
 	defer C.libxl_vcpuinfo_list_free(clist, nbVcpu)
 
 	if int(nbVcpu) == 0 {
@@ -1125,9 +1125,9 @@ func (ct ConsoleType) String() (str string) {
 
 //int libxl_console_get_tty(libxl_ctx *ctx, uint32_t domid, int cons_num,
 //libxl_console_type type, char **path);
-func (Ctx *Context) ConsoleGetTty(id Domid, consNum int, conType ConsoleType) (path string, err error) {
+func (ctx *Context) ConsoleGetTty(id Domid, consNum int, conType ConsoleType) (path string, err error) {
 	var cpath *C.char
-	ret := C.libxl_console_get_tty(Ctx.ctx, C.uint32_t(id), C.int(consNum), C.libxl_console_type(conType), &cpath)
+	ret := C.libxl_console_get_tty(ctx.ctx, C.uint32_t(id), C.int(consNum), C.libxl_console_type(conType), &cpath)
 	if ret != 0 {
 		err = Error(-ret)
 		return
@@ -1140,9 +1140,9 @@ func (Ctx *Context) ConsoleGetTty(id Domid, consNum int, conType ConsoleType) (p
 
 //int libxl_primary_console_get_tty(libxl_ctx *ctx, uint32_t domid_vm,
 //					char **path);
-func (Ctx *Context) PrimaryConsoleGetTty(domid uint32) (path string, err error) {
+func (ctx *Context) PrimaryConsoleGetTty(domid uint32) (path string, err error) {
 	var cpath *C.char
-	ret := C.libxl_primary_console_get_tty(Ctx.ctx, C.uint32_t(domid), &cpath)
+	ret := C.libxl_primary_console_get_tty(ctx.ctx, C.uint32_t(domid), &cpath)
 	if ret != 0 {
 		err = Error(-ret)
 		return
@@ -1154,7 +1154,7 @@ func (Ctx *Context) PrimaryConsoleGetTty(domid uint32) (path string, err error)
 }
 
 // DeviceNicAdd adds a nic to a domain.
-func (Ctx *Context) DeviceNicAdd(domid Domid, nic *DeviceNic) error {
+func (ctx *Context) DeviceNicAdd(domid Domid, nic *DeviceNic) error {
 	var cnic C.libxl_device_nic
 
 	if err := nic.toC(&cnic); err != nil {
@@ -1162,7 +1162,7 @@ func (Ctx *Context) DeviceNicAdd(domid Domid, nic *DeviceNic) error {
 	}
 	defer C.libxl_device_nic_dispose(&cnic)
 
-	ret := C.libxl_device_nic_add(Ctx.ctx, C.uint32_t(domid), &cnic, nil)
+	ret := C.libxl_device_nic_add(ctx.ctx, C.uint32_t(domid), &cnic, nil)
 	if ret != 0 {
 		return Error(ret)
 	}
@@ -1171,7 +1171,7 @@ func (Ctx *Context) DeviceNicAdd(domid Domid, nic *DeviceNic) error {
 }
 
 // DeviceNicRemove removes a nic from a domain.
-func (Ctx *Context) DeviceNicRemove(domid Domid, nic *DeviceNic) error {
+func (ctx *Context) DeviceNicRemove(domid Domid, nic *DeviceNic) error {
 	var cnic C.libxl_device_nic
 
 	if err := nic.toC(&cnic); err != nil {
@@ -1179,7 +1179,7 @@ func (Ctx *Context) DeviceNicRemove(domid Domid, nic *DeviceNic) error {
 	}
 	defer C.libxl_device_nic_dispose(&cnic)
 
-	ret := C.libxl_device_nic_remove(Ctx.ctx, C.uint32_t(domid), &cnic, nil)
+	ret := C.libxl_device_nic_remove(ctx.ctx, C.uint32_t(domid), &cnic, nil)
 	if ret != 0 {
 		return Error(ret)
 	}
@@ -1188,7 +1188,7 @@ func (Ctx *Context) DeviceNicRemove(domid Domid, nic *DeviceNic) error {
 }
 
 // DevicePciAdd is used to passthrough a PCI device to a domain.
-func (Ctx *Context) DevicePciAdd(domid Domid, pci *DevicePci) error {
+func (ctx *Context) DevicePciAdd(domid Domid, pci *DevicePci) error {
 	var cpci C.libxl_device_pci
 
 	if err := pci.toC(&cpci); err != nil {
@@ -1196,7 +1196,7 @@ func (Ctx *Context) DevicePciAdd(domid Domid, pci *DevicePci) error {
 	}
 	defer C.libxl_device_pci_dispose(&cpci)
 
-	ret := C.libxl_device_pci_add(Ctx.ctx, C.uint32_t(domid), &cpci, nil)
+	ret := C.libxl_device_pci_add(ctx.ctx, C.uint32_t(domid), &cpci, nil)
 	if ret != 0 {
 		return Error(ret)
 	}
@@ -1205,7 +1205,7 @@ func (Ctx *Context) DevicePciAdd(domid Domid, pci *DevicePci) error {
 }
 
 // DevicePciRemove removes a PCI device from a domain.
-func (Ctx *Context) DevicePciRemove(domid Domid, pci *DevicePci) error {
+func (ctx *Context) DevicePciRemove(domid Domid, pci *DevicePci) error {
 	var cpci C.libxl_device_pci
 
 	if err := pci.toC(&cpci); err != nil {
@@ -1213,7 +1213,7 @@ func (Ctx *Context) DevicePciRemove(domid Domid, pci *DevicePci) error {
 	}
 	defer C.libxl_device_pci_dispose(&cpci)
 
-	ret := C.libxl_device_pci_remove(Ctx.ctx, C.uint32_t(domid), &cpci, nil)
+	ret := C.libxl_device_pci_remove(ctx.ctx, C.uint32_t(domid), &cpci, nil)
 	if ret != 0 {
 		return Error(ret)
 	}
@@ -1222,7 +1222,7 @@ func (Ctx *Context) DevicePciRemove(domid Domid, pci *DevicePci) error {
 }
 
 // DeviceUsbdevAdd adds a USB device to a domain.
-func (Ctx *Context) DeviceUsbdevAdd(domid Domid, usbdev *DeviceUsbdev) error {
+func (ctx *Context) DeviceUsbdevAdd(domid Domid, usbdev *DeviceUsbdev) error {
 	var cusbdev C.libxl_device_usbdev
 
 	if err := usbdev.toC(&cusbdev); err != nil {
@@ -1230,7 +1230,7 @@ func (Ctx *Context) DeviceUsbdevAdd(domid Domid, usbdev *DeviceUsbdev) error {
 	}
 	defer C.libxl_device_usbdev_dispose(&cusbdev)
 
-	ret := C.libxl_device_usbdev_add(Ctx.ctx, C.uint32_t(domid), &cusbdev, nil)
+	ret := C.libxl_device_usbdev_add(ctx.ctx, C.uint32_t(domid), &cusbdev, nil)
 	if ret != 0 {
 		return Error(ret)
 	}
@@ -1239,7 +1239,7 @@ func (Ctx *Context) DeviceUsbdevAdd(domid Domid, usbdev *DeviceUsbdev) error {
 }
 
 // DeviceUsbdevRemove removes a USB device from a domain.
-func (Ctx *Context) DeviceUsbdevRemove(domid Domid, usbdev *DeviceUsbdev) error {
+func (ctx *Context) DeviceUsbdevRemove(domid Domid, usbdev *DeviceUsbdev) error {
 	var cusbdev C.libxl_device_usbdev
 
 	if err := usbdev.toC(&cusbdev); err != nil {
@@ -1247,7 +1247,7 @@ func (Ctx *Context) DeviceUsbdevRemove(domid Domid, usbdev *DeviceUsbdev) error
 	}
 	defer C.libxl_device_usbdev_dispose(&cusbdev)
 
-	ret := C.libxl_device_usbdev_remove(Ctx.ctx, C.uint32_t(domid), &cusbdev, nil)
+	ret := C.libxl_device_usbdev_remove(ctx.ctx, C.uint32_t(domid), &cusbdev, nil)
 	if ret != 0 {
 		return Error(ret)
 	}
@@ -1256,7 +1256,7 @@ func (Ctx *Context) DeviceUsbdevRemove(domid Domid, usbdev *DeviceUsbdev) error
 }
 
 // DomainCreateNew creates a new domain.
-func (Ctx *Context) DomainCreateNew(config *DomainConfig) (Domid, error) {
+func (ctx *Context) DomainCreateNew(config *DomainConfig) (Domid, error) {
 	var cdomid C.uint32_t
 	var cconfig C.libxl_domain_config
 	err := config.toC(&cconfig)
@@ -1265,7 +1265,7 @@ func (Ctx *Context) DomainCreateNew(config *DomainConfig) (Domid, error) {
 	}
 	defer C.libxl_domain_config_dispose(&cconfig)
 
-	ret := C.libxl_domain_create_new(Ctx.ctx, &cconfig, &cdomid, nil, nil)
+	ret := C.libxl_domain_create_new(ctx.ctx, &cconfig, &cdomid, nil, nil)
 	if ret != 0 {
 		return Domid(0), Error(ret)
 	}
-- 
2.17.1

Re: [RESEND PATCH 06/12] golang/xenlight: rename Ctx receivers to ctx

[George Dunlap]

> On May 24, 2021, at 9:36 PM, Nick Rosbrook <rosbrookn@gmail.com> wrote:
> 
> As a matter of style, it is strange to see capitalized receiver names,
> due to the significance of capitalized symbols in Go (although there is
> in fact nothing special about a capitalized receiver name). Fix this in
> xenlight.go by running:
> 
>  gofmt -w -r 'Ctx -> ctx' xenlight.go
> 
> from tools/golang/xenlight. There is no functional change.
> 
> Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com>

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

[RESEND PATCH 07/12] golang/xenlight: add logging conveniences for within xenlight

[Nick Rosbrook]

Add some logging methods to Context to provide easy use of the
Contenxt's xentoollog_logger. These are not exported, but the LogLevel
type is so that a later commit can allow the Context's log level to be
configurable.

Becuase cgo does not support calling C functions with variable
arguments, e.g. xtl_log, add an xtl_log_wrap function to the cgo preamble
that accepts an already formatted string, and handle the formatting in
Go.

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

diff --git a/tools/golang/xenlight/xenlight.go b/tools/golang/xenlight/xenlight.go
index fc3eb0bf3f..f68d7b6e97 100644
--- a/tools/golang/xenlight/xenlight.go
+++ b/tools/golang/xenlight/xenlight.go
@@ -32,6 +32,15 @@ static const libxl_childproc_hooks childproc_hooks = { .chldowner = libxl_sigchl
 void xenlight_set_chldproc(libxl_ctx *ctx) {
 	libxl_childproc_setmode(ctx, &childproc_hooks, NULL);
 }
+
+void xtl_log_wrap(struct xentoollog_logger *logger,
+		  xentoollog_level level,
+		  int errnoval,
+		  const char *context,
+		  const char *msg)
+{
+    xtl_log(logger, level, errnoval, context, "%s", msg);
+}
 */
 import "C"
 
@@ -192,6 +201,42 @@ func (ctx *Context) Close() error {
 	return nil
 }
 
+// LogLevel represents an xentoollog_level, and can be used to configre the log
+// level of a Context's logger.
+type LogLevel int
+
+const (
+	//LogLevelNone     LogLevel = C.XTL_NONE
+	LogLevelDebug    LogLevel = C.XTL_DEBUG
+	LogLevelVerbose  LogLevel = C.XTL_VERBOSE
+	LogLevelDetail   LogLevel = C.XTL_DETAIL
+	LogLevelProgress LogLevel = C.XTL_PROGRESS
+	LogLevelInfo     LogLevel = C.XTL_INFO
+	LogLevelNotice   LogLevel = C.XTL_NOTICE
+	LogLevelWarn     LogLevel = C.XTL_WARN
+	LogLevelError    LogLevel = C.XTL_ERROR
+	LogLevelCritical LogLevel = C.XTL_CRITICAL
+	//LogLevelNumLevels LogLevel = C.XTL_NUM_LEVELS
+)
+
+func (ctx *Context) log(lvl LogLevel, errnoval int, format string, a ...interface{}) {
+	msg := C.CString(fmt.Sprintf(format, a...))
+	defer C.free(unsafe.Pointer(msg))
+	context := C.CString("xenlight")
+	defer C.free(unsafe.Pointer(context))
+
+	C.xtl_log_wrap((*C.xentoollog_logger)(unsafe.Pointer(ctx.logger)),
+		C.xentoollog_level(lvl), C.int(errnoval), context, msg)
+}
+
+func (ctx *Context) logd(format string, a ...interface{}) {
+	ctx.log(LogLevelDebug, -1, format, a...)
+}
+
+func (ctx *Context) logw(format string, a ...interface{}) {
+	ctx.log(LogLevelWarn, -1, format, a...)
+}
+
 /*
  * Types: Builtins
  */
-- 
2.17.1

Re: [RESEND PATCH 07/12] golang/xenlight: add logging conveniences for within xenlight

[George Dunlap]

> On May 24, 2021, at 9:36 PM, Nick Rosbrook <rosbrookn@gmail.com> wrote:
> 
> Add some logging methods to Context to provide easy use of the
> Contenxt's xentoollog_logger. These are not exported, but the LogLevel
> type is so that a later commit can allow the Context's log level to be
> configurable.
> 
> Becuase cgo does not support calling C functions with variable
> arguments, e.g. xtl_log, add an xtl_log_wrap function to the cgo preamble
> that accepts an already formatted string, and handle the formatting in
> Go.
> 
> Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com>

Looks good.  One comment:

> ---
> tools/golang/xenlight/xenlight.go | 45 +++++++++++++++++++++++++++++++
> 1 file changed, 45 insertions(+)
> 
> diff --git a/tools/golang/xenlight/xenlight.go b/tools/golang/xenlight/xenlight.go
> index fc3eb0bf3f..f68d7b6e97 100644
> --- a/tools/golang/xenlight/xenlight.go
> +++ b/tools/golang/xenlight/xenlight.go
> @@ -32,6 +32,15 @@ static const libxl_childproc_hooks childproc_hooks = { .chldowner = libxl_sigchl
> void xenlight_set_chldproc(libxl_ctx *ctx) {
> 	libxl_childproc_setmode(ctx, &childproc_hooks, NULL);
> }
> +
> +void xtl_log_wrap(struct xentoollog_logger *logger,
> +		  xentoollog_level level,
> +		  int errnoval,
> +		  const char *context,
> +		  const char *msg)
> +{
> +    xtl_log(logger, level, errnoval, context, "%s", msg);
> +}
> */
> import "C"
> 
> @@ -192,6 +201,42 @@ func (ctx *Context) Close() error {
> 	return nil
> }
> 
> +// LogLevel represents an xentoollog_level, and can be used to configre the log
> +// level of a Context's logger.
> +type LogLevel int
> +
> +const (
> +	//LogLevelNone     LogLevel = C.XTL_NONE

Why are we not defining this one?  Don’t we want to be able to disable logging entirely?

> +	LogLevelDebug    LogLevel = C.XTL_DEBUG
> +	LogLevelVerbose  LogLevel = C.XTL_VERBOSE
> +	LogLevelDetail   LogLevel = C.XTL_DETAIL
> +	LogLevelProgress LogLevel = C.XTL_PROGRESS
> +	LogLevelInfo     LogLevel = C.XTL_INFO
> +	LogLevelNotice   LogLevel = C.XTL_NOTICE
> +	LogLevelWarn     LogLevel = C.XTL_WARN
> +	LogLevelError    LogLevel = C.XTL_ERROR
> +	LogLevelCritical LogLevel = C.XTL_CRITICAL
> +	//LogLevelNumLevels LogLevel = C.XTL_NUM_LEVELS
> +)
> +
> +func (ctx *Context) log(lvl LogLevel, errnoval int, format string, a ...interface{}) {
> +	msg := C.CString(fmt.Sprintf(format, a...))
> +	defer C.free(unsafe.Pointer(msg))
> +	context := C.CString("xenlight")
> +	defer C.free(unsafe.Pointer(context))

Hmm, allocating and freeing a fixed string every time seems pretty wasteful.  Would it make more sense to either use a static C string in the CGo code at the top instead?  Or if not, to make context a global variable we allocate once at the package level and re-use?

Also, is ‘xenlight’ informative enough?  I haven’t looked at the other “context” strings; would “go-xenlight” or something be better?

> +
> +	C.xtl_log_wrap((*C.xentoollog_logger)(unsafe.Pointer(ctx.logger)),
> +		C.xentoollog_level(lvl), C.int(errnoval), context, msg)
> +}

I think we want to make it possible long-term to configure your own logger or have no logger at all; so maybe we should add a `if (ctx.logger == nil) return;` at then top?

Other than that looks good, thanks!

 -George

Re: [RESEND PATCH 07/12] golang/xenlight: add logging conveniences for within xenlight

[George Dunlap]

> On Jun 18, 2021, at 2:17 PM, George Dunlap <George.Dunlap@citrix.com> wrote:
> 
> 
> 
>> On May 24, 2021, at 9:36 PM, Nick Rosbrook <rosbrookn@gmail.com> wrote:
>> 
>> Add some logging methods to Context to provide easy use of the
>> Contenxt's xentoollog_logger. These are not exported, but the LogLevel
>> type is so that a later commit can allow the Context's log level to be
>> configurable.
>> 
>> Becuase cgo does not support calling C functions with variable
>> arguments, e.g. xtl_log, add an xtl_log_wrap function to the cgo preamble
>> that accepts an already formatted string, and handle the formatting in
>> Go.
>> 
>> Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com>
> 
> Looks good.  One comment:

Er, sorry, turns out I had rather more than one comment.  Here’s one more:

Is there any particular reason not to export the Ctx.Log[X]() functions?

 -George

Re: [RESEND PATCH 07/12] golang/xenlight: add logging conveniences for within xenlight

[Nick Rosbrook]

On Fri, Jun 18, 2021 at 01:21:40PM +0000, George Dunlap wrote:
> 
> 
> > On Jun 18, 2021, at 2:17 PM, George Dunlap <George.Dunlap@citrix.com> wrote:
> > 
> > 
> > 
> >> On May 24, 2021, at 9:36 PM, Nick Rosbrook <rosbrookn@gmail.com> wrote:
> >> 
> >> Add some logging methods to Context to provide easy use of the
> >> Contenxt's xentoollog_logger. These are not exported, but the LogLevel
> >> type is so that a later commit can allow the Context's log level to be
> >> configurable.
> >> 
> >> Becuase cgo does not support calling C functions with variable
> >> arguments, e.g. xtl_log, add an xtl_log_wrap function to the cgo preamble
> >> that accepts an already formatted string, and handle the formatting in
> >> Go.
> >> 
> >> Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com>
> > 
> > Looks good.  One comment:
> 
> Er, sorry, turns out I had rather more than one comment.  Here’s one more:
> 
> Is there any particular reason not to export the Ctx.Log[X]() functions?
> 
No reason other than I tend to only export functions when I know they
need to be exported. My motivation for adding these at the time was to
help debug development. Would you prefer to export them then?

Thanks,
NR

Re: [RESEND PATCH 07/12] golang/xenlight: add logging conveniences for within xenlight

[George Dunlap]

> On Jun 18, 2021, at 4:26 PM, Nick Rosbrook <rosbrookn@gmail.com> wrote:
> 
> On Fri, Jun 18, 2021 at 01:21:40PM +0000, George Dunlap wrote:
>> 
>> 
>>> On Jun 18, 2021, at 2:17 PM, George Dunlap <George.Dunlap@citrix.com> wrote:
>>> 
>>> 
>>> 
>>>> On May 24, 2021, at 9:36 PM, Nick Rosbrook <rosbrookn@gmail.com> wrote:
>>>> 
>>>> Add some logging methods to Context to provide easy use of the
>>>> Contenxt's xentoollog_logger. These are not exported, but the LogLevel
>>>> type is so that a later commit can allow the Context's log level to be
>>>> configurable.
>>>> 
>>>> Becuase cgo does not support calling C functions with variable
>>>> arguments, e.g. xtl_log, add an xtl_log_wrap function to the cgo preamble
>>>> that accepts an already formatted string, and handle the formatting in
>>>> Go.
>>>> 
>>>> Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com>
>>> 
>>> Looks good.  One comment:
>> 
>> Er, sorry, turns out I had rather more than one comment.  Here’s one more:
>> 
>> Is there any particular reason not to export the Ctx.Log[X]() functions?
>> 
> No reason other than I tend to only export functions when I know they
> need to be exported. My motivation for adding these at the time was to
> help debug development. Would you prefer to export them then?

I don’t have a super-strong preference.  I was just thinking that xl and libxl both use the same logger, so it would make sense for whatever was built on top of xenlight to use the same logger.

But I guess we’d want the exported version to be able to pass in its own “context”; since it’s more work than just capitalizing the method names, I’d say go ahead and postpone that for now.

Thanks,
 -George

Re: [RESEND PATCH 07/12] golang/xenlight: add logging conveniences for within xenlight

[Nick Rosbrook]

On Fri, Jun 18, 2021 at 01:17:07PM +0000, George Dunlap wrote:
> 
> 
> > On May 24, 2021, at 9:36 PM, Nick Rosbrook <rosbrookn@gmail.com> wrote:
> > 
> > Add some logging methods to Context to provide easy use of the
> > Contenxt's xentoollog_logger. These are not exported, but the LogLevel
> > type is so that a later commit can allow the Context's log level to be
> > configurable.
> > 
> > Becuase cgo does not support calling C functions with variable
> > arguments, e.g. xtl_log, add an xtl_log_wrap function to the cgo preamble
> > that accepts an already formatted string, and handle the formatting in
> > Go.
> > 
> > Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com>
> 
> Looks good.  One comment:
> 
> > ---
> > tools/golang/xenlight/xenlight.go | 45 +++++++++++++++++++++++++++++++
> > 1 file changed, 45 insertions(+)
> > 
> > diff --git a/tools/golang/xenlight/xenlight.go b/tools/golang/xenlight/xenlight.go
> > index fc3eb0bf3f..f68d7b6e97 100644
> > --- a/tools/golang/xenlight/xenlight.go
> > +++ b/tools/golang/xenlight/xenlight.go
> > @@ -32,6 +32,15 @@ static const libxl_childproc_hooks childproc_hooks = { .chldowner = libxl_sigchl
> > void xenlight_set_chldproc(libxl_ctx *ctx) {
> > 	libxl_childproc_setmode(ctx, &childproc_hooks, NULL);
> > }
> > +
> > +void xtl_log_wrap(struct xentoollog_logger *logger,
> > +		  xentoollog_level level,
> > +		  int errnoval,
> > +		  const char *context,
> > +		  const char *msg)
> > +{
> > +    xtl_log(logger, level, errnoval, context, "%s", msg);
> > +}
> > */
> > import "C"
> > 
> > @@ -192,6 +201,42 @@ func (ctx *Context) Close() error {
> > 	return nil
> > }
> > 
> > +// LogLevel represents an xentoollog_level, and can be used to configre the log
> > +// level of a Context's logger.
> > +type LogLevel int
> > +
> > +const (
> > +	//LogLevelNone     LogLevel = C.XTL_NONE
> 
> Why are we not defining this one?  Don’t we want to be able to disable logging entirely?

Hm, I'm not sure. I'll poke around to see if I had a reason for this,
otherwise I will un-comment this line.

> 
> > +	LogLevelDebug    LogLevel = C.XTL_DEBUG
> > +	LogLevelVerbose  LogLevel = C.XTL_VERBOSE
> > +	LogLevelDetail   LogLevel = C.XTL_DETAIL
> > +	LogLevelProgress LogLevel = C.XTL_PROGRESS
> > +	LogLevelInfo     LogLevel = C.XTL_INFO
> > +	LogLevelNotice   LogLevel = C.XTL_NOTICE
> > +	LogLevelWarn     LogLevel = C.XTL_WARN
> > +	LogLevelError    LogLevel = C.XTL_ERROR
> > +	LogLevelCritical LogLevel = C.XTL_CRITICAL
> > +	//LogLevelNumLevels LogLevel = C.XTL_NUM_LEVELS
> > +)
> > +
> > +func (ctx *Context) log(lvl LogLevel, errnoval int, format string, a ...interface{}) {
> > +	msg := C.CString(fmt.Sprintf(format, a...))
> > +	defer C.free(unsafe.Pointer(msg))
> > +	context := C.CString("xenlight")
> > +	defer C.free(unsafe.Pointer(context))
> 
> Hmm, allocating and freeing a fixed string every time seems pretty wasteful.  Would it make more sense to either use a static C string in the CGo code at the top instead?  Or if not, to make context a global variable we allocate once at the package level and re-use?

You're right, we should probably define a static C string in the
preamble.
> 
> Also, is ‘xenlight’ informative enough?  I haven’t looked at the other “context” strings; would “go-xenlight” or something be better?
> 

I believe libxl uses "libxl." I would be fine with "go-xenlight" if you
prefer that. 

> > +
> > +	C.xtl_log_wrap((*C.xentoollog_logger)(unsafe.Pointer(ctx.logger)),
> > +		C.xentoollog_level(lvl), C.int(errnoval), context, msg)
> > +}
> 
> I think we want to make it possible long-term to configure your own logger or have no logger at all; so maybe we should add a `if (ctx.logger == nil) return;` at then top?
> 
Yeah, that's a good idea.

> Other than that looks good, thanks!
> 
>  -George

Thanks,
NR

Re: [RESEND PATCH 07/12] golang/xenlight: add logging conveniences for within xenlight

[George Dunlap]

> On Jun 18, 2021, at 4:17 PM, Nick Rosbrook <rosbrookn@gmail.com> wrote:
> 
> On Fri, Jun 18, 2021 at 01:17:07PM +0000, George Dunlap wrote:
>> Also, is ‘xenlight’ informative enough?  I haven’t looked at the other “context” strings; would “go-xenlight” or something be better?
>> 
> 
> I believe libxl uses "libxl." I would be fine with "go-xenlight" if you
> prefer that. 

I think so, just so there’s no confusion if someone decides to write Python / Rust / Lua / whatever bindings.

Thanks,
 -George

[RESEND PATCH 08/12] golang/xenlight: add functional options to configure Context

[Nick Rosbrook]

Add a ContextOption type to support functional options in NewContext.
Then, add a variadic ContextOption parameter to NewContext, which allows
callers to specify 0 or more configuration options.

For now, just add the WithLogLevel option so that callers can set the
log level of the Context's xentoollog_logger. Future configuration
options can be created by adding an appropriate field to the
contextOptions struct and creating a With<OptionName> function to return
a ContextOption

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

diff --git a/tools/golang/xenlight/xenlight.go b/tools/golang/xenlight/xenlight.go
index f68d7b6e97..65f93abe32 100644
--- a/tools/golang/xenlight/xenlight.go
+++ b/tools/golang/xenlight/xenlight.go
@@ -136,7 +136,7 @@ func sigchldHandler(ctx *Context) {
 }
 
 // NewContext returns a new Context.
-func NewContext() (ctx *Context, err error) {
+func NewContext(opts ...ContextOption) (ctx *Context, err error) {
 	ctx = &Context{}
 
 	defer func() {
@@ -146,8 +146,19 @@ func NewContext() (ctx *Context, err error) {
 		}
 	}()
 
+	// Set the default context options. These fields may
+	// be modified by the provided opts.
+	copts := &contextOptions{
+		logLevel: LogLevelError,
+	}
+
+	for _, opt := range opts {
+		opt.apply(copts)
+	}
+
 	// Create a logger
-	ctx.logger = C.xtl_createlogger_stdiostream(C.stderr, C.XTL_ERROR, 0)
+	ctx.logger = C.xtl_createlogger_stdiostream(C.stderr,
+		C.xentoollog_level(copts.logLevel), 0)
 
 	// Allocate a context
 	ret := C.libxl_ctx_alloc(&ctx.ctx, C.LIBXL_VERSION, 0,
@@ -201,6 +212,35 @@ func (ctx *Context) Close() error {
 	return nil
 }
 
+type contextOptions struct {
+	logLevel LogLevel
+}
+
+// ContextOption is used to configure options for a Context.
+type ContextOption interface {
+	apply(*contextOptions)
+}
+
+type funcContextOption struct {
+	f func(*contextOptions)
+}
+
+func (fco *funcContextOption) apply(c *contextOptions) {
+	fco.f(c)
+}
+
+func newFuncContextOption(f func(*contextOptions)) *funcContextOption {
+	return &funcContextOption{f}
+}
+
+// WithLogLevel sets the log level for a Context's logger. The default level is
+// LogLevelError.
+func WithLogLevel(level LogLevel) ContextOption {
+	return newFuncContextOption(func(co *contextOptions) {
+		co.logLevel = level
+	})
+}
+
 // LogLevel represents an xentoollog_level, and can be used to configre the log
 // level of a Context's logger.
 type LogLevel int
-- 
2.17.1

Re: [RESEND PATCH 08/12] golang/xenlight: add functional options to configure Context

[George Dunlap]

> On May 24, 2021, at 9:36 PM, Nick Rosbrook <rosbrookn@gmail.com> wrote:
> 
> Add a ContextOption type to support functional options in NewContext.
> Then, add a variadic ContextOption parameter to NewContext, which allows
> callers to specify 0 or more configuration options.
> 
> For now, just add the WithLogLevel option so that callers can set the
> log level of the Context's xentoollog_logger. Future configuration
> options can be created by adding an appropriate field to the
> contextOptions struct and creating a With<OptionName> function to return
> a ContextOption
> 
> Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com>
> ---
> tools/golang/xenlight/xenlight.go | 44 +++++++++++++++++++++++++++++--
> 1 file changed, 42 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/golang/xenlight/xenlight.go b/tools/golang/xenlight/xenlight.go
> index f68d7b6e97..65f93abe32 100644
> --- a/tools/golang/xenlight/xenlight.go
> +++ b/tools/golang/xenlight/xenlight.go
> @@ -136,7 +136,7 @@ func sigchldHandler(ctx *Context) {
> }
> 
> // NewContext returns a new Context.
> -func NewContext() (ctx *Context, err error) {
> +func NewContext(opts ...ContextOption) (ctx *Context, err error) {
> 	ctx = &Context{}
> 
> 	defer func() {
> @@ -146,8 +146,19 @@ func NewContext() (ctx *Context, err error) {
> 		}
> 	}()
> 
> +	// Set the default context options. These fields may
> +	// be modified by the provided opts.
> +	copts := &contextOptions{
> +		logLevel: LogLevelError,
> +	}
> +
> +	for _, opt := range opts {
> +		opt.apply(copts)
> +	}
> +
> 	// Create a logger
> -	ctx.logger = C.xtl_createlogger_stdiostream(C.stderr, C.XTL_ERROR, 0)
> +	ctx.logger = C.xtl_createlogger_stdiostream(C.stderr,
> +		C.xentoollog_level(copts.logLevel), 0)
> 
> 	// Allocate a context
> 	ret := C.libxl_ctx_alloc(&ctx.ctx, C.LIBXL_VERSION, 0,
> @@ -201,6 +212,35 @@ func (ctx *Context) Close() error {
> 	return nil
> }
> 
> +type contextOptions struct {
> +	logLevel LogLevel
> +}
> +
> +// ContextOption is used to configure options for a Context.
> +type ContextOption interface {
> +	apply(*contextOptions)
> +}
> +
> +type funcContextOption struct {
> +	f func(*contextOptions)
> +}
> +
> +func (fco *funcContextOption) apply(c *contextOptions) {
> +	fco.f(c)
> +}

Why all this convolution with interfaces and such, rather than just defining ContextOption as a function pointer?  Is it just to keep contextOptions out of the documentation page?

 -George

Re: [RESEND PATCH 08/12] golang/xenlight: add functional options to configure Context

[Nick Rosbrook]

On Fri, Jun 18, 2021 at 02:44:15PM +0000, George Dunlap wrote:
> 
> 
> > On May 24, 2021, at 9:36 PM, Nick Rosbrook <rosbrookn@gmail.com> wrote:
> > 
> > Add a ContextOption type to support functional options in NewContext.
> > Then, add a variadic ContextOption parameter to NewContext, which allows
> > callers to specify 0 or more configuration options.
> > 
> > For now, just add the WithLogLevel option so that callers can set the
> > log level of the Context's xentoollog_logger. Future configuration
> > options can be created by adding an appropriate field to the
> > contextOptions struct and creating a With<OptionName> function to return
> > a ContextOption
> > 
> > Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com>
> > ---
> > tools/golang/xenlight/xenlight.go | 44 +++++++++++++++++++++++++++++--
> > 1 file changed, 42 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/golang/xenlight/xenlight.go b/tools/golang/xenlight/xenlight.go
> > index f68d7b6e97..65f93abe32 100644
> > --- a/tools/golang/xenlight/xenlight.go
> > +++ b/tools/golang/xenlight/xenlight.go
> > @@ -136,7 +136,7 @@ func sigchldHandler(ctx *Context) {
> > }
> > 
> > // NewContext returns a new Context.
> > -func NewContext() (ctx *Context, err error) {
> > +func NewContext(opts ...ContextOption) (ctx *Context, err error) {
> > 	ctx = &Context{}
> > 
> > 	defer func() {
> > @@ -146,8 +146,19 @@ func NewContext() (ctx *Context, err error) {
> > 		}
> > 	}()
> > 
> > +	// Set the default context options. These fields may
> > +	// be modified by the provided opts.
> > +	copts := &contextOptions{
> > +		logLevel: LogLevelError,
> > +	}
> > +
> > +	for _, opt := range opts {
> > +		opt.apply(copts)
> > +	}
> > +
> > 	// Create a logger
> > -	ctx.logger = C.xtl_createlogger_stdiostream(C.stderr, C.XTL_ERROR, 0)
> > +	ctx.logger = C.xtl_createlogger_stdiostream(C.stderr,
> > +		C.xentoollog_level(copts.logLevel), 0)
> > 
> > 	// Allocate a context
> > 	ret := C.libxl_ctx_alloc(&ctx.ctx, C.LIBXL_VERSION, 0,
> > @@ -201,6 +212,35 @@ func (ctx *Context) Close() error {
> > 	return nil
> > }
> > 
> > +type contextOptions struct {
> > +	logLevel LogLevel
> > +}
> > +
> > +// ContextOption is used to configure options for a Context.
> > +type ContextOption interface {
> > +	apply(*contextOptions)
> > +}
> > +
> > +type funcContextOption struct {
> > +	f func(*contextOptions)
> > +}
> > +
> > +func (fco *funcContextOption) apply(c *contextOptions) {
> > +	fco.f(c)
> > +}
> 
> Why all this convolution with interfaces and such, rather than just defining ContextOption as a function pointer?  Is it just to keep contextOptions out of the documentation page?

Part of the motivation for using functional options is to abstract the
"options" struct, yes. This allows internal defaults to be applied more
easily -- if you require e.g. a ContextOptions struct to be passed by
the caller, how do you know if they intended to override a default, or
if they just didn't set the field? Additionally, using the ContextOption
as an interface allows variadic arguments, which are just convenient for
API users -- the same NewContext function can be used whether you need
to pass 3 options or 0.

The reason we use ContextOption as an interface, rather than function
pointer of sorts is for flexibility in the signatures of ContextOption
implementations. E.g., we could have

func WithLogLevel(lvl LogLevel) ContextOption
func WithLogContext(s string) ContextOption
func WithFooAndBar(s string, n int) ContextOption

See [1] for more background on this pattern.

Thanks,
NR

[1] https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis

Re: [RESEND PATCH 08/12] golang/xenlight: add functional options to configure Context

[George Dunlap]

> On Jun 18, 2021, at 4:08 PM, Nick Rosbrook <rosbrookn@gmail.com> wrote:
> 
> On Fri, Jun 18, 2021 at 02:44:15PM +0000, George Dunlap wrote:
>> 
>> 
>>> On May 24, 2021, at 9:36 PM, Nick Rosbrook <rosbrookn@gmail.com> wrote:
>>> 
>>> Add a ContextOption type to support functional options in NewContext.
>>> Then, add a variadic ContextOption parameter to NewContext, which allows
>>> callers to specify 0 or more configuration options.
>>> 
>>> For now, just add the WithLogLevel option so that callers can set the
>>> log level of the Context's xentoollog_logger. Future configuration
>>> options can be created by adding an appropriate field to the
>>> contextOptions struct and creating a With<OptionName> function to return
>>> a ContextOption
>>> 
>>> Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com>
>>> ---
>>> tools/golang/xenlight/xenlight.go | 44 +++++++++++++++++++++++++++++--
>>> 1 file changed, 42 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/tools/golang/xenlight/xenlight.go b/tools/golang/xenlight/xenlight.go
>>> index f68d7b6e97..65f93abe32 100644
>>> --- a/tools/golang/xenlight/xenlight.go
>>> +++ b/tools/golang/xenlight/xenlight.go
>>> @@ -136,7 +136,7 @@ func sigchldHandler(ctx *Context) {
>>> }
>>> 
>>> // NewContext returns a new Context.
>>> -func NewContext() (ctx *Context, err error) {
>>> +func NewContext(opts ...ContextOption) (ctx *Context, err error) {
>>> 	ctx = &Context{}
>>> 
>>> 	defer func() {
>>> @@ -146,8 +146,19 @@ func NewContext() (ctx *Context, err error) {
>>> 		}
>>> 	}()
>>> 
>>> +	// Set the default context options. These fields may
>>> +	// be modified by the provided opts.
>>> +	copts := &contextOptions{
>>> +		logLevel: LogLevelError,
>>> +	}
>>> +
>>> +	for _, opt := range opts {
>>> +		opt.apply(copts)
>>> +	}
>>> +
>>> 	// Create a logger
>>> -	ctx.logger = C.xtl_createlogger_stdiostream(C.stderr, C.XTL_ERROR, 0)
>>> +	ctx.logger = C.xtl_createlogger_stdiostream(C.stderr,
>>> +		C.xentoollog_level(copts.logLevel), 0)
>>> 
>>> 	// Allocate a context
>>> 	ret := C.libxl_ctx_alloc(&ctx.ctx, C.LIBXL_VERSION, 0,
>>> @@ -201,6 +212,35 @@ func (ctx *Context) Close() error {
>>> 	return nil
>>> }
>>> 
>>> +type contextOptions struct {
>>> +	logLevel LogLevel
>>> +}
>>> +
>>> +// ContextOption is used to configure options for a Context.
>>> +type ContextOption interface {
>>> +	apply(*contextOptions)
>>> +}
>>> +
>>> +type funcContextOption struct {
>>> +	f func(*contextOptions)
>>> +}
>>> +
>>> +func (fco *funcContextOption) apply(c *contextOptions) {
>>> +	fco.f(c)
>>> +}
>> 
>> Why all this convolution with interfaces and such, rather than just defining ContextOption as a function pointer?  Is it just to keep contextOptions out of the documentation page?
> 
> Part of the motivation for using functional options is to abstract the
> "options" struct, yes. This allows internal defaults to be applied more
> easily -- if you require e.g. a ContextOptions struct to be passed by
> the caller, how do you know if they intended to override a default, or
> if they just didn't set the field? Additionally, using the ContextOption
> as an interface allows variadic arguments, which are just convenient for
> API users -- the same NewContext function can be used whether you need
> to pass 3 options or 0.
> 
> The reason we use ContextOption as an interface, rather than function
> pointer of sorts is for flexibility in the signatures of ContextOption
> implementations. E.g., we could have
> 
> func WithLogLevel(lvl LogLevel) ContextOption
> func WithLogContext(s string) ContextOption
> func WithFooAndBar(s string, n int) ContextOption
> 
> See [1] for more background on this pattern.
> 
> Thanks,
> NR
> 
> [1] https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis

Yes, I frequently use a pattern like the one described in that blog post myself.  But that blog post doesn’t use interfaces — the final slide actually has the “option function” type as an open-coded function pointer type.

So my question was, why not do something like this:

type ContextOption func(*contextOptions) error

func WithLogLevel(level LogLevel) ContextOption {
  return func(co *contextOptions) {
    co.logLevel = level
  }
}

ATM the only advantage I can see of defining ContextOption as an interface rather than as a function pointer is that the godoc for ContextOption would look like:

type ContextOption interface {
   // contains filtered or unexported fields
}

Rather than

type ContextOption func(*contextOptions) error

Which shows you the name of the unexported field.

Is there another reason I missed?

 -George

Re: [RESEND PATCH 08/12] golang/xenlight: add functional options to configure Context

[Nick Rosbrook]

On Fri, Jun 18, 2021 at 04:18:44PM +0000, George Dunlap wrote:
> 
> 
> > On Jun 18, 2021, at 4:08 PM, Nick Rosbrook <rosbrookn@gmail.com> wrote:
> > 
> > On Fri, Jun 18, 2021 at 02:44:15PM +0000, George Dunlap wrote:
> >> 
> >> 
> >>> On May 24, 2021, at 9:36 PM, Nick Rosbrook <rosbrookn@gmail.com> wrote:
> >>> 
> >>> Add a ContextOption type to support functional options in NewContext.
> >>> Then, add a variadic ContextOption parameter to NewContext, which allows
> >>> callers to specify 0 or more configuration options.
> >>> 
> >>> For now, just add the WithLogLevel option so that callers can set the
> >>> log level of the Context's xentoollog_logger. Future configuration
> >>> options can be created by adding an appropriate field to the
> >>> contextOptions struct and creating a With<OptionName> function to return
> >>> a ContextOption
> >>> 
> >>> Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com>
> >>> ---
> >>> tools/golang/xenlight/xenlight.go | 44 +++++++++++++++++++++++++++++--
> >>> 1 file changed, 42 insertions(+), 2 deletions(-)
> >>> 
> >>> diff --git a/tools/golang/xenlight/xenlight.go b/tools/golang/xenlight/xenlight.go
> >>> index f68d7b6e97..65f93abe32 100644
> >>> --- a/tools/golang/xenlight/xenlight.go
> >>> +++ b/tools/golang/xenlight/xenlight.go
> >>> @@ -136,7 +136,7 @@ func sigchldHandler(ctx *Context) {
> >>> }
> >>> 
> >>> // NewContext returns a new Context.
> >>> -func NewContext() (ctx *Context, err error) {
> >>> +func NewContext(opts ...ContextOption) (ctx *Context, err error) {
> >>> 	ctx = &Context{}
> >>> 
> >>> 	defer func() {
> >>> @@ -146,8 +146,19 @@ func NewContext() (ctx *Context, err error) {
> >>> 		}
> >>> 	}()
> >>> 
> >>> +	// Set the default context options. These fields may
> >>> +	// be modified by the provided opts.
> >>> +	copts := &contextOptions{
> >>> +		logLevel: LogLevelError,
> >>> +	}
> >>> +
> >>> +	for _, opt := range opts {
> >>> +		opt.apply(copts)
> >>> +	}
> >>> +
> >>> 	// Create a logger
> >>> -	ctx.logger = C.xtl_createlogger_stdiostream(C.stderr, C.XTL_ERROR, 0)
> >>> +	ctx.logger = C.xtl_createlogger_stdiostream(C.stderr,
> >>> +		C.xentoollog_level(copts.logLevel), 0)
> >>> 
> >>> 	// Allocate a context
> >>> 	ret := C.libxl_ctx_alloc(&ctx.ctx, C.LIBXL_VERSION, 0,
> >>> @@ -201,6 +212,35 @@ func (ctx *Context) Close() error {
> >>> 	return nil
> >>> }
> >>> 
> >>> +type contextOptions struct {
> >>> +	logLevel LogLevel
> >>> +}
> >>> +
> >>> +// ContextOption is used to configure options for a Context.
> >>> +type ContextOption interface {
> >>> +	apply(*contextOptions)
> >>> +}
> >>> +
> >>> +type funcContextOption struct {
> >>> +	f func(*contextOptions)
> >>> +}
> >>> +
> >>> +func (fco *funcContextOption) apply(c *contextOptions) {
> >>> +	fco.f(c)
> >>> +}
> >> 
> >> Why all this convolution with interfaces and such, rather than just defining ContextOption as a function pointer?  Is it just to keep contextOptions out of the documentation page?
> > 
> > Part of the motivation for using functional options is to abstract the
> > "options" struct, yes. This allows internal defaults to be applied more
> > easily -- if you require e.g. a ContextOptions struct to be passed by
> > the caller, how do you know if they intended to override a default, or
> > if they just didn't set the field? Additionally, using the ContextOption
> > as an interface allows variadic arguments, which are just convenient for
> > API users -- the same NewContext function can be used whether you need
> > to pass 3 options or 0.
> > 
> > The reason we use ContextOption as an interface, rather than function
> > pointer of sorts is for flexibility in the signatures of ContextOption
> > implementations. E.g., we could have
> > 
> > func WithLogLevel(lvl LogLevel) ContextOption
> > func WithLogContext(s string) ContextOption
> > func WithFooAndBar(s string, n int) ContextOption
> > 
> > See [1] for more background on this pattern.
> > 
> > Thanks,
> > NR
> > 
> > [1] https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis
> 
> Yes, I frequently use a pattern like the one described in that blog post myself.  But that blog post doesn’t use interfaces — the final slide actually has the “option function” type as an open-coded function pointer type.
> 
> So my question was, why not do something like this:
> 
> type ContextOption func(*contextOptions) error
> 
> func WithLogLevel(level LogLevel) ContextOption {
>   return func(co *contextOptions) {
>     co.logLevel = level
>   }
> }
> 
> ATM the only advantage I can see of defining ContextOption as an interface rather than as a function pointer is that the godoc for ContextOption would look like:
> 
> type ContextOption interface {
>    // contains filtered or unexported fields
> }
> 
> Rather than
> 
> type ContextOption func(*contextOptions) error
> 
> Which shows you the name of the unexported field.
> 
> Is there another reason I missed?

Technically it does allow more flexibility in implementing
ContextOption, e.g. you could do...

func (lvl LogLevel) apply(co *contextOptions) { co.logLevel = lvl }

...and then pass a LogLevel directly as a ContextOption. But generally
everyone implements these things as funcs.

I will admit that when it comes to my choice of using the interface
version instead of function pointers, I am just more familiar with the
former and encounter it more often in other Go packages I use.

Thanks,
NR

Re: [RESEND PATCH 08/12] golang/xenlight: add functional options to configure Context

[George Dunlap]

> On Jun 18, 2021, at 6:00 PM, Nick Rosbrook <rosbrookn@gmail.com> wrote:
> 
> On Fri, Jun 18, 2021 at 04:18:44PM +0000, George Dunlap wrote:
>> 
>> 
>>> On Jun 18, 2021, at 4:08 PM, Nick Rosbrook <rosbrookn@gmail.com> wrote:
>>> 
>>> On Fri, Jun 18, 2021 at 02:44:15PM +0000, George Dunlap wrote:
>>>> 
>>>> 
>>>>> On May 24, 2021, at 9:36 PM, Nick Rosbrook <rosbrookn@gmail.com> wrote:
>>>>> 
>>>>> Add a ContextOption type to support functional options in NewContext.
>>>>> Then, add a variadic ContextOption parameter to NewContext, which allows
>>>>> callers to specify 0 or more configuration options.
>>>>> 
>>>>> For now, just add the WithLogLevel option so that callers can set the
>>>>> log level of the Context's xentoollog_logger. Future configuration
>>>>> options can be created by adding an appropriate field to the
>>>>> contextOptions struct and creating a With<OptionName> function to return
>>>>> a ContextOption
>>>>> 
>>>>> Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com>
>>>>> ---
>>>>> tools/golang/xenlight/xenlight.go | 44 +++++++++++++++++++++++++++++--
>>>>> 1 file changed, 42 insertions(+), 2 deletions(-)
>>>>> 
>>>>> diff --git a/tools/golang/xenlight/xenlight.go b/tools/golang/xenlight/xenlight.go
>>>>> index f68d7b6e97..65f93abe32 100644
>>>>> --- a/tools/golang/xenlight/xenlight.go
>>>>> +++ b/tools/golang/xenlight/xenlight.go
>>>>> @@ -136,7 +136,7 @@ func sigchldHandler(ctx *Context) {
>>>>> }
>>>>> 
>>>>> // NewContext returns a new Context.
>>>>> -func NewContext() (ctx *Context, err error) {
>>>>> +func NewContext(opts ...ContextOption) (ctx *Context, err error) {
>>>>> 	ctx = &Context{}
>>>>> 
>>>>> 	defer func() {
>>>>> @@ -146,8 +146,19 @@ func NewContext() (ctx *Context, err error) {
>>>>> 		}
>>>>> 	}()
>>>>> 
>>>>> +	// Set the default context options. These fields may
>>>>> +	// be modified by the provided opts.
>>>>> +	copts := &contextOptions{
>>>>> +		logLevel: LogLevelError,
>>>>> +	}
>>>>> +
>>>>> +	for _, opt := range opts {
>>>>> +		opt.apply(copts)
>>>>> +	}
>>>>> +
>>>>> 	// Create a logger
>>>>> -	ctx.logger = C.xtl_createlogger_stdiostream(C.stderr, C.XTL_ERROR, 0)
>>>>> +	ctx.logger = C.xtl_createlogger_stdiostream(C.stderr,
>>>>> +		C.xentoollog_level(copts.logLevel), 0)
>>>>> 
>>>>> 	// Allocate a context
>>>>> 	ret := C.libxl_ctx_alloc(&ctx.ctx, C.LIBXL_VERSION, 0,
>>>>> @@ -201,6 +212,35 @@ func (ctx *Context) Close() error {
>>>>> 	return nil
>>>>> }
>>>>> 
>>>>> +type contextOptions struct {
>>>>> +	logLevel LogLevel
>>>>> +}
>>>>> +
>>>>> +// ContextOption is used to configure options for a Context.
>>>>> +type ContextOption interface {
>>>>> +	apply(*contextOptions)
>>>>> +}
>>>>> +
>>>>> +type funcContextOption struct {
>>>>> +	f func(*contextOptions)
>>>>> +}
>>>>> +
>>>>> +func (fco *funcContextOption) apply(c *contextOptions) {
>>>>> +	fco.f(c)
>>>>> +}
>>>> 
>>>> Why all this convolution with interfaces and such, rather than just defining ContextOption as a function pointer?  Is it just to keep contextOptions out of the documentation page?
>>> 
>>> Part of the motivation for using functional options is to abstract the
>>> "options" struct, yes. This allows internal defaults to be applied more
>>> easily -- if you require e.g. a ContextOptions struct to be passed by
>>> the caller, how do you know if they intended to override a default, or
>>> if they just didn't set the field? Additionally, using the ContextOption
>>> as an interface allows variadic arguments, which are just convenient for
>>> API users -- the same NewContext function can be used whether you need
>>> to pass 3 options or 0.
>>> 
>>> The reason we use ContextOption as an interface, rather than function
>>> pointer of sorts is for flexibility in the signatures of ContextOption
>>> implementations. E.g., we could have
>>> 
>>> func WithLogLevel(lvl LogLevel) ContextOption
>>> func WithLogContext(s string) ContextOption
>>> func WithFooAndBar(s string, n int) ContextOption
>>> 
>>> See [1] for more background on this pattern.
>>> 
>>> Thanks,
>>> NR
>>> 
>>> [1] https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis
>> 
>> Yes, I frequently use a pattern like the one described in that blog post myself. But that blog post doesn’t use interfaces — the final slide actually has the “option function” type as an open-coded function pointer type.
>> 
>> So my question was, why not do something like this:
>> 
>> type ContextOption func(*contextOptions) error
>> 
>> func WithLogLevel(level LogLevel) ContextOption {
>>  return func(co *contextOptions) {
>>    co.logLevel = level
>>  }
>> }
>> 
>> ATM the only advantage I can see of defining ContextOption as an interface rather than as a function pointer is that the godoc for ContextOption would look like:
>> 
>> type ContextOption interface {
>>   // contains filtered or unexported fields
>> }
>> 
>> Rather than
>> 
>> type ContextOption func(*contextOptions) error
>> 
>> Which shows you the name of the unexported field.
>> 
>> Is there another reason I missed?
> 
> Technically it does allow more flexibility in implementing
> ContextOption, e.g. you could do...
> 
> func (lvl LogLevel) apply(co *contextOptions) { co.logLevel = lvl }
> 
> ...and then pass a LogLevel directly as a ContextOption. But generally
> everyone implements these things as funcs.
> 
> I will admit that when it comes to my choice of using the interface
> version instead of function pointers, I am just more familiar with the
> former and encounter it more often in other Go packages I use.

OK.  It seems a bit weird to me, but that’s not really a good reason to block it. :-) I just wanted to make sure I understood why it was being chosen.

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

[RESEND PATCH 09/12] golang/xenlight: add DomainDestroy wrapper

[Nick Rosbrook]

Add a wrapper around libxl_domain_destroy.

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

diff --git a/tools/golang/xenlight/xenlight.go b/tools/golang/xenlight/xenlight.go
index 65f93abe32..1e0ed109e4 100644
--- a/tools/golang/xenlight/xenlight.go
+++ b/tools/golang/xenlight/xenlight.go
@@ -1357,3 +1357,13 @@ func (ctx *Context) DomainCreateNew(config *DomainConfig) (Domid, error) {
 
 	return Domid(cdomid), nil
 }
+
+// DomainDestroy destroys a domain given a domid.
+func (ctx *Context) DomainDestroy(domid Domid) error {
+	ret := C.libxl_domain_destroy(ctx.ctx, C.uint32_t(domid), nil)
+	if ret != 0 {
+		return Error(ret)
+	}
+
+	return nil
+}
-- 
2.17.1

Re: [RESEND PATCH 09/12] golang/xenlight: add DomainDestroy wrapper

[George Dunlap]

> On May 24, 2021, at 9:36 PM, Nick Rosbrook <rosbrookn@gmail.com> wrote:
> 
> Add a wrapper around libxl_domain_destroy.
> 
> Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com>

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

[RESEND PATCH 10/12] golang/xenlight: add SendTrigger wrapper

[Nick Rosbrook]

Add a warpper around libxl_send_trigger.

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

diff --git a/tools/golang/xenlight/xenlight.go b/tools/golang/xenlight/xenlight.go
index 1e0ed109e4..d153feb851 100644
--- a/tools/golang/xenlight/xenlight.go
+++ b/tools/golang/xenlight/xenlight.go
@@ -1367,3 +1367,14 @@ func (ctx *Context) DomainDestroy(domid Domid) error {
 
 	return nil
 }
+
+// SendTrigger sends a Trigger to the domain specified by domid.
+func (ctx *Context) SendTrigger(domid Domid, trigger Trigger, vcpuid int) error {
+	ret := C.libxl_send_trigger(ctx.ctx, C.uint32_t(domid),
+		C.libxl_trigger(trigger), C.uint32_t(vcpuid), nil)
+	if ret != 0 {
+		return Error(ret)
+	}
+
+	return nil
+}
-- 
2.17.1

Re: [RESEND PATCH 10/12] golang/xenlight: add SendTrigger wrapper

[George Dunlap]

> On May 24, 2021, at 9:36 PM, Nick Rosbrook <rosbrookn@gmail.com> wrote:
> 
> Add a warpper around libxl_send_trigger.
> 
> Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com>

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

[RESEND PATCH 11/12] golang/xenlight: do not negate ret when converting to Error

[Nick Rosbrook]

There are several locations where the return code from calling into C is
negated when being converted to Error. This results in error strings
like "libxl error: <x>", rather than the correct message. Fix all
occurrances of this by running:

  gofmt -w -r 'Error(-ret) -> Error(ret)' xenlight.go

from tools/golang/xenlight.

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

diff --git a/tools/golang/xenlight/xenlight.go b/tools/golang/xenlight/xenlight.go
index d153feb851..6fb22665cc 100644
--- a/tools/golang/xenlight/xenlight.go
+++ b/tools/golang/xenlight/xenlight.go
@@ -668,7 +668,7 @@ func SchedulerFromString(name string) (s Scheduler, err error) {
 
 	ret := C.libxl_scheduler_from_string(cname, &cs)
 	if ret != 0 {
-		err = Error(-ret)
+		err = Error(ret)
 		return
 	}
 
@@ -707,7 +707,7 @@ func (ctx *Context) CpupoolInfo(Poolid uint32) (pool Cpupoolinfo, err error) {
 
 	ret := C.libxl_cpupool_info(ctx.ctx, &c_cpupool, C.uint32_t(Poolid))
 	if ret != 0 {
-		err = Error(-ret)
+		err = Error(ret)
 		return
 	}
 	defer C.libxl_cpupoolinfo_dispose(&c_cpupool)
@@ -741,7 +741,7 @@ func (ctx *Context) CpupoolCreate(Name string, Scheduler Scheduler, Cpumap Bitma
 	ret := C.libxl_cpupool_create(ctx.ctx, name, C.libxl_scheduler(Scheduler),
 		cbm, &uuid, &poolid)
 	if ret != 0 {
-		err = Error(-ret)
+		err = Error(ret)
 		return
 	}
 
@@ -754,7 +754,7 @@ func (ctx *Context) CpupoolCreate(Name string, Scheduler Scheduler, Cpumap Bitma
 func (ctx *Context) CpupoolDestroy(Poolid uint32) (err error) {
 	ret := C.libxl_cpupool_destroy(ctx.ctx, C.uint32_t(Poolid))
 	if ret != 0 {
-		err = Error(-ret)
+		err = Error(ret)
 		return
 	}
 
@@ -765,7 +765,7 @@ func (ctx *Context) CpupoolDestroy(Poolid uint32) (err error) {
 func (ctx *Context) CpupoolCpuadd(Poolid uint32, Cpu int) (err error) {
 	ret := C.libxl_cpupool_cpuadd(ctx.ctx, C.uint32_t(Poolid), C.int(Cpu))
 	if ret != 0 {
-		err = Error(-ret)
+		err = Error(ret)
 		return
 	}
 
@@ -783,7 +783,7 @@ func (ctx *Context) CpupoolCpuaddCpumap(Poolid uint32, Cpumap Bitmap) (err error
 
 	ret := C.libxl_cpupool_cpuadd_cpumap(ctx.ctx, C.uint32_t(Poolid), &cbm)
 	if ret != 0 {
-		err = Error(-ret)
+		err = Error(ret)
 		return
 	}
 
@@ -794,7 +794,7 @@ func (ctx *Context) CpupoolCpuaddCpumap(Poolid uint32, Cpumap Bitmap) (err error
 func (ctx *Context) CpupoolCpuremove(Poolid uint32, Cpu int) (err error) {
 	ret := C.libxl_cpupool_cpuremove(ctx.ctx, C.uint32_t(Poolid), C.int(Cpu))
 	if ret != 0 {
-		err = Error(-ret)
+		err = Error(ret)
 		return
 	}
 
@@ -812,7 +812,7 @@ func (ctx *Context) CpupoolCpuremoveCpumap(Poolid uint32, Cpumap Bitmap) (err er
 
 	ret := C.libxl_cpupool_cpuremove_cpumap(ctx.ctx, C.uint32_t(Poolid), &cbm)
 	if ret != 0 {
-		err = Error(-ret)
+		err = Error(ret)
 		return
 	}
 
@@ -826,7 +826,7 @@ func (ctx *Context) CpupoolRename(Name string, Poolid uint32) (err error) {
 
 	ret := C.libxl_cpupool_rename(ctx.ctx, name, C.uint32_t(Poolid))
 	if ret != 0 {
-		err = Error(-ret)
+		err = Error(ret)
 		return
 	}
 
@@ -839,7 +839,7 @@ func (ctx *Context) CpupoolCpuaddNode(Poolid uint32, Node int) (Cpus int, err er
 
 	ret := C.libxl_cpupool_cpuadd_node(ctx.ctx, C.uint32_t(Poolid), C.int(Node), &ccpus)
 	if ret != 0 {
-		err = Error(-ret)
+		err = Error(ret)
 		return
 	}
 
@@ -854,7 +854,7 @@ func (ctx *Context) CpupoolCpuremoveNode(Poolid uint32, Node int) (Cpus int, err
 
 	ret := C.libxl_cpupool_cpuremove_node(ctx.ctx, C.uint32_t(Poolid), C.int(Node), &ccpus)
 	if ret != 0 {
-		err = Error(-ret)
+		err = Error(ret)
 		return
 	}
 
@@ -867,7 +867,7 @@ func (ctx *Context) CpupoolCpuremoveNode(Poolid uint32, Node int) (Cpus int, err
 func (ctx *Context) CpupoolMovedomain(Poolid uint32, Id Domid) (err error) {
 	ret := C.libxl_cpupool_movedomain(ctx.ctx, C.uint32_t(Poolid), C.uint32_t(Id))
 	if ret != 0 {
-		err = Error(-ret)
+		err = Error(ret)
 		return
 	}
 
@@ -1028,7 +1028,7 @@ func (bm Bitmap) String() (s string) {
 func (ctx *Context) GetMaxCpus() (maxCpus int, err error) {
 	ret := C.libxl_get_max_cpus(ctx.ctx)
 	if ret < 0 {
-		err = Error(-ret)
+		err = Error(ret)
 		return
 	}
 	maxCpus = int(ret)
@@ -1039,7 +1039,7 @@ func (ctx *Context) GetMaxCpus() (maxCpus int, err error) {
 func (ctx *Context) GetOnlineCpus() (onCpus int, err error) {
 	ret := C.libxl_get_online_cpus(ctx.ctx)
 	if ret < 0 {
-		err = Error(-ret)
+		err = Error(ret)
 		return
 	}
 	onCpus = int(ret)
@@ -1050,7 +1050,7 @@ func (ctx *Context) GetOnlineCpus() (onCpus int, err error) {
 func (ctx *Context) GetMaxNodes() (maxNodes int, err error) {
 	ret := C.libxl_get_max_nodes(ctx.ctx)
 	if ret < 0 {
-		err = Error(-ret)
+		err = Error(ret)
 		return
 	}
 	maxNodes = int(ret)
@@ -1063,7 +1063,7 @@ func (ctx *Context) GetFreeMemory() (memkb uint64, err error) {
 	ret := C.libxl_get_free_memory(ctx.ctx, &cmem)
 
 	if ret < 0 {
-		err = Error(-ret)
+		err = Error(ret)
 		return
 	}
 
@@ -1108,7 +1108,7 @@ func (ctx *Context) DomainInfo(Id Domid) (di *Dominfo, err error) {
 	ret := C.libxl_domain_info(ctx.ctx, &cdi, C.uint32_t(Id))
 
 	if ret != 0 {
-		err = Error(-ret)
+		err = Error(ret)
 		return
 	}
 
@@ -1121,7 +1121,7 @@ func (ctx *Context) DomainUnpause(Id Domid) (err error) {
 	ret := C.libxl_domain_unpause(ctx.ctx, C.uint32_t(Id), nil)
 
 	if ret != 0 {
-		err = Error(-ret)
+		err = Error(ret)
 	}
 	return
 }
@@ -1131,7 +1131,7 @@ func (ctx *Context) DomainPause(id Domid) (err error) {
 	ret := C.libxl_domain_pause(ctx.ctx, C.uint32_t(id), nil)
 
 	if ret != 0 {
-		err = Error(-ret)
+		err = Error(ret)
 	}
 	return
 }
@@ -1141,7 +1141,7 @@ func (ctx *Context) DomainShutdown(id Domid) (err error) {
 	ret := C.libxl_domain_shutdown(ctx.ctx, C.uint32_t(id), nil)
 
 	if ret != 0 {
-		err = Error(-ret)
+		err = Error(ret)
 	}
 	return
 }
@@ -1151,7 +1151,7 @@ func (ctx *Context) DomainReboot(id Domid) (err error) {
 	ret := C.libxl_domain_reboot(ctx.ctx, C.uint32_t(id), nil)
 
 	if ret != 0 {
-		err = Error(-ret)
+		err = Error(ret)
 	}
 	return
 }
@@ -1214,7 +1214,7 @@ func (ctx *Context) ConsoleGetTty(id Domid, consNum int, conType ConsoleType) (p
 	var cpath *C.char
 	ret := C.libxl_console_get_tty(ctx.ctx, C.uint32_t(id), C.int(consNum), C.libxl_console_type(conType), &cpath)
 	if ret != 0 {
-		err = Error(-ret)
+		err = Error(ret)
 		return
 	}
 	defer C.free(unsafe.Pointer(cpath))
@@ -1229,7 +1229,7 @@ func (ctx *Context) PrimaryConsoleGetTty(domid uint32) (path string, err error)
 	var cpath *C.char
 	ret := C.libxl_primary_console_get_tty(ctx.ctx, C.uint32_t(domid), &cpath)
 	if ret != 0 {
-		err = Error(-ret)
+		err = Error(ret)
 		return
 	}
 	defer C.free(unsafe.Pointer(cpath))
-- 
2.17.1

Re: [RESEND PATCH 11/12] golang/xenlight: do not negate ret when converting to Error

[George Dunlap]

> On May 24, 2021, at 9:36 PM, Nick Rosbrook <rosbrookn@gmail.com> wrote:
> 
> There are several locations where the return code from calling into C is
> negated when being converted to Error. This results in error strings
> like "libxl error: <x>", rather than the correct message. Fix all
> occurrances of this by running:
> 
>  gofmt -w -r 'Error(-ret) -> Error(ret)' xenlight.go
> 
> from tools/golang/xenlight.
> 
> Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com>

Erk.  I’d be tempted to say something more like:

"Commit 871e51d2d4 changed the sign on the xenlight error types (making the values negative, same as the C-generated constants), but failed to remove the code changing the sign before casting to Error().  This results in…”

I can edit this on check-in if you’re OK with it.  Either way:

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

Re: [RESEND PATCH 11/12] golang/xenlight: do not negate ret when converting to Error

[Nick Rosbrook]

On Fri, Jun 18, 2021 at 03:13:03PM +0000, George Dunlap wrote:
> 
> 
> > On May 24, 2021, at 9:36 PM, Nick Rosbrook <rosbrookn@gmail.com> wrote:
> > 
> > There are several locations where the return code from calling into C is
> > negated when being converted to Error. This results in error strings
> > like "libxl error: <x>", rather than the correct message. Fix all
> > occurrances of this by running:
> > 
> >  gofmt -w -r 'Error(-ret) -> Error(ret)' xenlight.go
> > 
> > from tools/golang/xenlight.
> > 
> > Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com>
> 
> Erk.  I’d be tempted to say something more like:
> 
> "Commit 871e51d2d4 changed the sign on the xenlight error types (making the values negative, same as the C-generated constants), but failed to remove the code changing the sign before casting to Error().  This results in…”
> 
> I can edit this on check-in if you’re OK with it.  Either way:

I would appreciate that. Thanks!

-NR

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

[RESEND PATCH 12/12] golang/xenlight: add NotifyDomainDeath method to Context

[Nick Rosbrook]

Add a helper function to wait for domain death events, and then write
the events to a provided channel. This handles the enabling/disabling of
the event type, freeing the event, and converting it to a Go type. The
caller can then handle the event however they need to. This function
will run until a provided context.Context is cancelled.

NotifyDomainDeath spawns two goroutines that return when the
context.Context is done. The first will make sure that the domain death
event is disabled, and that the corresponding event queue is cleared.
The second calls libxl_event_wait, and writes the event to the provided
channel.

With this, callers should be able to manage a full domain life cycle.
Add to the comment of DomainCreateNew so that package uses know they
should use this method in conjunction with DomainCreateNew.

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

diff --git a/tools/golang/xenlight/xenlight.go b/tools/golang/xenlight/xenlight.go
index 6fb22665cc..8406883433 100644
--- a/tools/golang/xenlight/xenlight.go
+++ b/tools/golang/xenlight/xenlight.go
@@ -53,6 +53,7 @@ import "C"
  */
 
 import (
+	"context"
 	"fmt"
 	"os"
 	"os/signal"
@@ -1340,7 +1341,9 @@ func (ctx *Context) DeviceUsbdevRemove(domid Domid, usbdev *DeviceUsbdev) error
 	return nil
 }
 
-// DomainCreateNew creates a new domain.
+// DomainCreateNew creates a new domain. Callers of DomainCreateNew are
+// responsible for handling the death of the resulting domain. This should be
+// done using NotifyDomainDeath.
 func (ctx *Context) DomainCreateNew(config *DomainConfig) (Domid, error) {
 	var cdomid C.uint32_t
 	var cconfig C.libxl_domain_config
@@ -1358,6 +1361,84 @@ func (ctx *Context) DomainCreateNew(config *DomainConfig) (Domid, error) {
 	return Domid(cdomid), nil
 }
 
+// NotifyDomainDeath registers an event handler for domain death events for a
+// given domnid, and writes events received to ec. NotifyDomainDeath returns an
+// error if it cannot register the event handler, but other errors encountered
+// are just logged. The goroutine spawned by calling NotifyDomainDeath runs
+// until the provided context.Context's Done channel is closed.
+func (ctx *Context) NotifyDomainDeath(c context.Context, domid Domid, ec chan<- Event) error {
+	var deathw *C.libxl_evgen_domain_death
+
+	ret := C.libxl_evenable_domain_death(ctx.ctx, C.uint32_t(domid), 0, &deathw)
+	if ret != 0 {
+		return Error(ret)
+	}
+
+	// Spawn a goroutine that is responsible for cleaning up when the
+	// passed context.Context's Done channel is closed.
+	go func() {
+		<-c.Done()
+
+		ctx.logd("cleaning up domain death event handler for domain %d", domid)
+
+		// Disable the event generation.
+		C.libxl_evdisable_domain_death(ctx.ctx, deathw)
+
+		// Make sure any events that were generated get cleaned up so they
+		// do not linger in the libxl event queue.
+		var evc *C.libxl_event
+		for {
+			ret := C.libxl_event_check(ctx.ctx, &evc, C.LIBXL_EVENTMASK_ALL, nil, nil)
+			if ret != 0 {
+				return
+			}
+			C.libxl_event_free(ctx.ctx, evc)
+		}
+	}()
+
+	go func() {
+		var (
+			ev  Event
+			evc *C.libxl_event
+		)
+
+		for {
+			select {
+			case <-c.Done():
+				return
+			default:
+				// Go on and check for an event...
+			}
+
+			ret := C.libxl_event_wait(ctx.ctx, &evc, C.LIBXL_EVENTMASK_ALL, nil, nil)
+			if ret != 0 {
+				ctx.logw("unexpected error waiting for event: %s", Error(ret))
+				continue
+			}
+
+			// Try to convert the event to Go, and then free the
+			// C.libxl_event no matter what.
+			err := ev.fromC(evc)
+			C.libxl_event_free(ctx.ctx, evc)
+			if err != nil {
+				ctx.logw("error converting event from C: %v", err)
+				continue
+			}
+
+			ctx.logd("received domain death event (domid=%v, type=%v)", ev.Domid, ev.Type)
+
+			// Write the event to the channel
+			select {
+			case ec <- ev:
+			case <-c.Done():
+				return
+			}
+		}
+	}()
+
+	return nil
+}
+
 // DomainDestroy destroys a domain given a domid.
 func (ctx *Context) DomainDestroy(domid Domid) error {
 	ret := C.libxl_domain_destroy(ctx.ctx, C.uint32_t(domid), nil)
-- 
2.17.1

Re: [RESEND PATCH 12/12] golang/xenlight: add NotifyDomainDeath method to Context

[George Dunlap]

> On May 24, 2021, at 9:36 PM, Nick Rosbrook <rosbrookn@gmail.com> wrote:
> 
> Add a helper function to wait for domain death events, and then write
> the events to a provided channel. This handles the enabling/disabling of
> the event type, freeing the event, and converting it to a Go type. The
> caller can then handle the event however they need to. This function
> will run until a provided context.Context is cancelled.
> 
> NotifyDomainDeath spawns two goroutines that return when the
> context.Context is done. The first will make sure that the domain death
> event is disabled, and that the corresponding event queue is cleared.
> The second calls libxl_event_wait, and writes the event to the provided
> channel.
> 
> With this, callers should be able to manage a full domain life cycle.
> Add to the comment of DomainCreateNew so that package uses know they
> should use this method in conjunction with DomainCreateNew.
> 
> Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com>
> ---
> tools/golang/xenlight/xenlight.go | 83 ++++++++++++++++++++++++++++++-
> 1 file changed, 82 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/golang/xenlight/xenlight.go b/tools/golang/xenlight/xenlight.go
> index 6fb22665cc..8406883433 100644
> --- a/tools/golang/xenlight/xenlight.go
> +++ b/tools/golang/xenlight/xenlight.go
> @@ -53,6 +53,7 @@ import "C"
>  */
> 
> import (
> +	"context"
> 	"fmt"
> 	"os"
> 	"os/signal"
> @@ -1340,7 +1341,9 @@ func (ctx *Context) DeviceUsbdevRemove(domid Domid, usbdev *DeviceUsbdev) error
> 	return nil
> }
> 
> -// DomainCreateNew creates a new domain.
> +// DomainCreateNew creates a new domain. Callers of DomainCreateNew are
> +// responsible for handling the death of the resulting domain. This should be
> +// done using NotifyDomainDeath.
> func (ctx *Context) DomainCreateNew(config *DomainConfig) (Domid, error) {
> 	var cdomid C.uint32_t
> 	var cconfig C.libxl_domain_config
> @@ -1358,6 +1361,84 @@ func (ctx *Context) DomainCreateNew(config *DomainConfig) (Domid, error) {
> 	return Domid(cdomid), nil
> }
> 
> +// NotifyDomainDeath registers an event handler for domain death events for a
> +// given domnid, and writes events received to ec. NotifyDomainDeath returns an
> +// error if it cannot register the event handler, but other errors encountered
> +// are just logged. The goroutine spawned by calling NotifyDomainDeath runs
> +// until the provided context.Context's Done channel is closed.
> +func (ctx *Context) NotifyDomainDeath(c context.Context, domid Domid, ec chan<- Event) error {
> +	var deathw *C.libxl_evgen_domain_death
> +
> +	ret := C.libxl_evenable_domain_death(ctx.ctx, C.uint32_t(domid), 0, &deathw)
> +	if ret != 0 {
> +		return Error(ret)
> +	}
> +
> +	// Spawn a goroutine that is responsible for cleaning up when the
> +	// passed context.Context's Done channel is closed.
> +	go func() {
> +		<-c.Done()
> +
> +		ctx.logd("cleaning up domain death event handler for domain %d", domid)
> +
> +		// Disable the event generation.
> +		C.libxl_evdisable_domain_death(ctx.ctx, deathw)
> +
> +		// Make sure any events that were generated get cleaned up so they
> +		// do not linger in the libxl event queue.
> +		var evc *C.libxl_event
> +		for {
> +			ret := C.libxl_event_check(ctx.ctx, &evc, C.LIBXL_EVENTMASK_ALL, nil, nil)
> +			if ret != 0 {
> +				return
> +			}
> +			C.libxl_event_free(ctx.ctx, evc)

I have to admit, I don’t really understand how the libxl event stuff is supposed to work.  But it looks like this will drain all events of any type, for any domain, associated with this context?

So if you had two domains, and called NotifyDomainDeath() on both with different contexts, and you closed the one context, you might miss events from the other context?

Or, suppose you did this:
 * ctx.NotifyDomainDeath(ctx1, dom1, ec1)
 * ctx.NotifyDiskEject(ctx2, dom1, ec2)
 * ctx1CancelFunc()

Wouldn’t there be a risk that the disk eject message would get lost?

Ian, any suggestions for the right way to use these functions in this scenario?

 -George

Re: [RESEND PATCH 12/12] golang/xenlight: add NotifyDomainDeath method to Context

[George Dunlap]

> On Jun 18, 2021, at 7:28 PM, George Dunlap <george.dunlap@citrix.com> wrote:
> 
> 
> 
>> On May 24, 2021, at 9:36 PM, Nick Rosbrook <rosbrookn@gmail.com> wrote:
>> 
>> Add a helper function to wait for domain death events, and then write
>> the events to a provided channel. This handles the enabling/disabling of
>> the event type, freeing the event, and converting it to a Go type. The
>> caller can then handle the event however they need to. This function
>> will run until a provided context.Context is cancelled.
>> 
>> NotifyDomainDeath spawns two goroutines that return when the
>> context.Context is done. The first will make sure that the domain death
>> event is disabled, and that the corresponding event queue is cleared.
>> The second calls libxl_event_wait, and writes the event to the provided
>> channel.
>> 
>> With this, callers should be able to manage a full domain life cycle.
>> Add to the comment of DomainCreateNew so that package uses know they
>> should use this method in conjunction with DomainCreateNew.
>> 
>> Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com>
>> ---
>> tools/golang/xenlight/xenlight.go | 83 ++++++++++++++++++++++++++++++-
>> 1 file changed, 82 insertions(+), 1 deletion(-)
>> 
>> diff --git a/tools/golang/xenlight/xenlight.go b/tools/golang/xenlight/xenlight.go
>> index 6fb22665cc..8406883433 100644
>> --- a/tools/golang/xenlight/xenlight.go
>> +++ b/tools/golang/xenlight/xenlight.go
>> @@ -53,6 +53,7 @@ import "C"
>> */
>> 
>> import (
>> +	"context"
>> 	"fmt"
>> 	"os"
>> 	"os/signal"
>> @@ -1340,7 +1341,9 @@ func (ctx *Context) DeviceUsbdevRemove(domid Domid, usbdev *DeviceUsbdev) error
>> 	return nil
>> }
>> 
>> -// DomainCreateNew creates a new domain.
>> +// DomainCreateNew creates a new domain. Callers of DomainCreateNew are
>> +// responsible for handling the death of the resulting domain. This should be
>> +// done using NotifyDomainDeath.
>> func (ctx *Context) DomainCreateNew(config *DomainConfig) (Domid, error) {
>> 	var cdomid C.uint32_t
>> 	var cconfig C.libxl_domain_config
>> @@ -1358,6 +1361,84 @@ func (ctx *Context) DomainCreateNew(config *DomainConfig) (Domid, error) {
>> 	return Domid(cdomid), nil
>> }
>> 
>> +// NotifyDomainDeath registers an event handler for domain death events for a
>> +// given domnid, and writes events received to ec. NotifyDomainDeath returns an
>> +// error if it cannot register the event handler, but other errors encountered
>> +// are just logged. The goroutine spawned by calling NotifyDomainDeath runs
>> +// until the provided context.Context's Done channel is closed.
>> +func (ctx *Context) NotifyDomainDeath(c context.Context, domid Domid, ec chan<- Event) error {
>> +	var deathw *C.libxl_evgen_domain_death
>> +
>> +	ret := C.libxl_evenable_domain_death(ctx.ctx, C.uint32_t(domid), 0, &deathw)
>> +	if ret != 0 {
>> +		return Error(ret)
>> +	}
>> +
>> +	// Spawn a goroutine that is responsible for cleaning up when the
>> +	// passed context.Context's Done channel is closed.
>> +	go func() {
>> +		<-c.Done()
>> +
>> +		ctx.logd("cleaning up domain death event handler for domain %d", domid)
>> +
>> +		// Disable the event generation.
>> +		C.libxl_evdisable_domain_death(ctx.ctx, deathw)
>> +
>> +		// Make sure any events that were generated get cleaned up so they
>> +		// do not linger in the libxl event queue.
>> +		var evc *C.libxl_event
>> +		for {
>> +			ret := C.libxl_event_check(ctx.ctx, &evc, C.LIBXL_EVENTMASK_ALL, nil, nil)
>> +			if ret != 0 {
>> +				return
>> +			}
>> +			C.libxl_event_free(ctx.ctx, evc)
> 
> I have to admit, I don’t really understand how the libxl event stuff is supposed to work.  But it looks like this will drain all events of any type, for any domain, associated with this context?
> 
> So if you had two domains, and called NotifyDomainDeath() on both with different contexts, and you closed the one context, you might miss events from the other context?
> 
> Or, suppose you did this:
> * ctx.NotifyDomainDeath(ctx1, dom1, ec1)
> * ctx.NotifyDiskEject(ctx2, dom1, ec2)
> * ctx1CancelFunc()
> 
> Wouldn’t there be a risk that the disk eject message would get lost?
> 
> Ian, any suggestions for the right way to use these functions in this scenario?

It looks like one option would be to add a “predicate” function filter, to filter by type and domid.

It looks like the other option would be to try to use libxl_event_register_callbacks().  We could have the C callback pass all the events to a goroutine which would act as a dispatcher.

 -George

Re: [RESEND PATCH 12/12] golang/xenlight: add NotifyDomainDeath method to Context

[Nick Rosbrook]

On Fri, Jun 18, 2021 at 07:31:46PM +0000, George Dunlap wrote:
> 
> 
> > On Jun 18, 2021, at 7:28 PM, George Dunlap <george.dunlap@citrix.com> wrote:
> > 
> > 
> > 
> >> On May 24, 2021, at 9:36 PM, Nick Rosbrook <rosbrookn@gmail.com> wrote:
> >> 
> >> Add a helper function to wait for domain death events, and then write
> >> the events to a provided channel. This handles the enabling/disabling of
> >> the event type, freeing the event, and converting it to a Go type. The
> >> caller can then handle the event however they need to. This function
> >> will run until a provided context.Context is cancelled.
> >> 
> >> NotifyDomainDeath spawns two goroutines that return when the
> >> context.Context is done. The first will make sure that the domain death
> >> event is disabled, and that the corresponding event queue is cleared.
> >> The second calls libxl_event_wait, and writes the event to the provided
> >> channel.
> >> 
> >> With this, callers should be able to manage a full domain life cycle.
> >> Add to the comment of DomainCreateNew so that package uses know they
> >> should use this method in conjunction with DomainCreateNew.
> >> 
> >> Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com>
> >> ---
> >> tools/golang/xenlight/xenlight.go | 83 ++++++++++++++++++++++++++++++-
> >> 1 file changed, 82 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/tools/golang/xenlight/xenlight.go b/tools/golang/xenlight/xenlight.go
> >> index 6fb22665cc..8406883433 100644
> >> --- a/tools/golang/xenlight/xenlight.go
> >> +++ b/tools/golang/xenlight/xenlight.go
> >> @@ -53,6 +53,7 @@ import "C"
> >> */
> >> 
> >> import (
> >> +	"context"
> >> 	"fmt"
> >> 	"os"
> >> 	"os/signal"
> >> @@ -1340,7 +1341,9 @@ func (ctx *Context) DeviceUsbdevRemove(domid Domid, usbdev *DeviceUsbdev) error
> >> 	return nil
> >> }
> >> 
> >> -// DomainCreateNew creates a new domain.
> >> +// DomainCreateNew creates a new domain. Callers of DomainCreateNew are
> >> +// responsible for handling the death of the resulting domain. This should be
> >> +// done using NotifyDomainDeath.
> >> func (ctx *Context) DomainCreateNew(config *DomainConfig) (Domid, error) {
> >> 	var cdomid C.uint32_t
> >> 	var cconfig C.libxl_domain_config
> >> @@ -1358,6 +1361,84 @@ func (ctx *Context) DomainCreateNew(config *DomainConfig) (Domid, error) {
> >> 	return Domid(cdomid), nil
> >> }
> >> 
> >> +// NotifyDomainDeath registers an event handler for domain death events for a
> >> +// given domnid, and writes events received to ec. NotifyDomainDeath returns an
> >> +// error if it cannot register the event handler, but other errors encountered
> >> +// are just logged. The goroutine spawned by calling NotifyDomainDeath runs
> >> +// until the provided context.Context's Done channel is closed.
> >> +func (ctx *Context) NotifyDomainDeath(c context.Context, domid Domid, ec chan<- Event) error {
> >> +	var deathw *C.libxl_evgen_domain_death
> >> +
> >> +	ret := C.libxl_evenable_domain_death(ctx.ctx, C.uint32_t(domid), 0, &deathw)
> >> +	if ret != 0 {
> >> +		return Error(ret)
> >> +	}
> >> +
> >> +	// Spawn a goroutine that is responsible for cleaning up when the
> >> +	// passed context.Context's Done channel is closed.
> >> +	go func() {
> >> +		<-c.Done()
> >> +
> >> +		ctx.logd("cleaning up domain death event handler for domain %d", domid)
> >> +
> >> +		// Disable the event generation.
> >> +		C.libxl_evdisable_domain_death(ctx.ctx, deathw)
> >> +
> >> +		// Make sure any events that were generated get cleaned up so they
> >> +		// do not linger in the libxl event queue.
> >> +		var evc *C.libxl_event
> >> +		for {
> >> +			ret := C.libxl_event_check(ctx.ctx, &evc, C.LIBXL_EVENTMASK_ALL, nil, nil)
> >> +			if ret != 0 {
> >> +				return
> >> +			}
> >> +			C.libxl_event_free(ctx.ctx, evc)
> > 
> > I have to admit, I don’t really understand how the libxl event stuff is supposed to work.  But it looks like this will drain all events of any type, for any domain, associated with this context?
> > 
> > So if you had two domains, and called NotifyDomainDeath() on both with different contexts, and you closed the one context, you might miss events from the other context?
> > 
> > Or, suppose you did this:
> > * ctx.NotifyDomainDeath(ctx1, dom1, ec1)
> > * ctx.NotifyDiskEject(ctx2, dom1, ec2)
> > * ctx1CancelFunc()
> > 
> > Wouldn’t there be a risk that the disk eject message would get lost?
> > 
> > Ian, any suggestions for the right way to use these functions in this scenario?
> 
> It looks like one option would be to add a “predicate” function filter, to filter by type and domid.
> 
> It looks like the other option would be to try to use libxl_event_register_callbacks().  We could have the C callback pass all the events to a goroutine which would act as a dispatcher.
> 
After a brief look at the documentation within libxl_event.h, it seems
using predicate functions would be the easiest solution given the
current layout. Though I will look closer at using
libxl_event_register_callbacks before sending a v2.

Thanks,
NR

Re: [RESEND PATCH 00/12] golang/xenlight: domain life cycle support

[Nick Rosbrook]

On Mon, May 24, 2021 at 4:37 PM Nick Rosbrook <rosbrookn@gmail.com> wrote:
>
> The primary goal of this patch series is to allow users of the xenlight
> package to manage a full domain life cycle. In particular, it provides
> support for receiving domain death events so that domain shutdown,
> reboot, destroy, etc. can be handled. And, it addresses issues found
> when using the package to boot domains with various configurations.
>
> These patches address several things (e.g. bug fixes, code style,
> conveniences, new wrapper functions), but are all work towards the final
> goal of allowing a package user to manage a full domain life cycle.
>

George,

I know you have leave coming up, and are likely very busy preparing
for that. Is there any chance this series will get attention before
then?

Thanks,
NR

Re: [RESEND PATCH 00/12] golang/xenlight: domain life cycle support

[George Dunlap]

> On May 24, 2021, at 9:36 PM, Nick Rosbrook <rosbrookn@gmail.com> wrote:
> 
> The primary goal of this patch series is to allow users of the xenlight
> package to manage a full domain life cycle. In particular, it provides
> support for receiving domain death events so that domain shutdown,
> reboot, destroy, etc. can be handled. And, it addresses issues found
> when using the package to boot domains with various configurations.
> 
> These patches address several things (e.g. bug fixes, code style,
> conveniences, new wrapper functions), but are all work towards the final
> goal of allowing a package user to manage a full domain life cycle.
> 
> Nick Rosbrook (12):

OK, I’ve checked in the following patches: (1, 2, 4, 5, 6, 9, 10, 11):

>  golang/xenlight: update generated code
>  golang/xenlight: fix StringList toC conversion
>  golang/xenlight: export keyed union interface types
>  golang/xenlight: use struct pointers in keyed union fields
>  golang/xenlight: rename Ctx receivers to ctx

>  golang/xenlight: add DomainDestroy wrapper
>  golang/xenlight: add SendTrigger wrapper
>  golang/xenlight: do not negate ret when converting to Error

The following have not been checked in due outsanding review comments (patches 3, 7, 12), or because they depend on a patch not being checked in (patch 8):

>  golang/xenlight: fix string conversion in generated toC functions
>  golang/xenlight: add logging conveniences for within xenlight
>  golang/xenlight: add functional options to configure Context
>  golang/xenlight: add NotifyDomainDeath method to Context

Thanks,
 -George

Re: [RESEND PATCH 00/12] golang/xenlight: domain life cycle support

[Nick Rosbrook]

On Mon, Jun 21, 2021 at 03:53:39PM +0000, George Dunlap wrote:
> 
> 
> > On May 24, 2021, at 9:36 PM, Nick Rosbrook <rosbrookn@gmail.com> wrote:
> > 
> > The primary goal of this patch series is to allow users of the xenlight
> > package to manage a full domain life cycle. In particular, it provides
> > support for receiving domain death events so that domain shutdown,
> > reboot, destroy, etc. can be handled. And, it addresses issues found
> > when using the package to boot domains with various configurations.
> > 
> > These patches address several things (e.g. bug fixes, code style,
> > conveniences, new wrapper functions), but are all work towards the final
> > goal of allowing a package user to manage a full domain life cycle.
> > 
> > Nick Rosbrook (12):
> 
> OK, I’ve checked in the following patches: (1, 2, 4, 5, 6, 9, 10, 11):
> 
> >  golang/xenlight: update generated code
> >  golang/xenlight: fix StringList toC conversion
> >  golang/xenlight: export keyed union interface types
> >  golang/xenlight: use struct pointers in keyed union fields
> >  golang/xenlight: rename Ctx receivers to ctx
> 
> >  golang/xenlight: add DomainDestroy wrapper
> >  golang/xenlight: add SendTrigger wrapper
> >  golang/xenlight: do not negate ret when converting to Error
> 
> The following have not been checked in due outsanding review comments (patches 3, 7, 12), or because they depend on a patch not being checked in (patch 8):
> 
> >  golang/xenlight: fix string conversion in generated toC functions
> >  golang/xenlight: add logging conveniences for within xenlight
> >  golang/xenlight: add functional options to configure Context
> >  golang/xenlight: add NotifyDomainDeath method to Context
> 

Thanks! I am planning on addressing patch 12 comments later today.

-NR