Marek Marczykowski-Górecki wrote:

From: Marek Marczykowski <marmarek@invisiblethingslab.com>

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- src/libxl/libxl_conf.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+)

Changes in v2: - rebase on 1.2.12+ - multiple devices support Changes in v3: - reduce code duplication

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 2321660..50ef9d8 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -749,6 +749,50 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, libxl_defbool_set(&b_info->u.hvm.vnc.enable, 0); libxl_defbool_set(&b_info->u.hvm.sdl.enable, 0);

+ if (def->ninputs) { + for (i = 0; i < def->ninputs; i++) { + if (def->inputs[i]->bus != VIR_DOMAIN_INPUT_BUS_USB) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("libxenlight supports only USB input")); + return -1; + } + } +#ifdef LIBXL_HAVE_BUILDINFO_USBDEVICE_LIST + if (VIR_ALLOC_N(b_info->u.hvm.usbdevice_list, def->ninputs+1) < 0) + return -1; +#else + if (def->ninputs > 1) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("libxenlight supports only one input device")); + return -1; + } +#endif + for (i = 0; i < def->ninputs; i++) { + char **usbdevice; +#ifdef LIBXL_HAVE_BUILDINFO_USBDEVICE_LIST + usbdevice = &b_info->u.hvm.usbdevice_list[i]; +#else + usbdevice = &b_info->u.hvm.usbdevice; +#endif + switch (def->inputs[i]->type) { + case VIR_DOMAIN_INPUT_TYPE_MOUSE: + VIR_FREE(*usbdevice); + if (VIR_STRDUP(*usbdevice, "mouse") < 0) + return -1; + break; + case VIR_DOMAIN_INPUT_TYPE_TABLET: + VIR_FREE(*usbdevice); + if (VIR_STRDUP(*usbdevice, "tablet") < 0) + return -1; + break; + default: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Unknown input device type")); + return -1; + } + } + } +

Looks good now; ACK. Regards, Jim

Marek Marczykowski-Górecki wrote:

In Xen>=4.3, libxl supports new syntax for USB devices: usbdevice=[ "DEVICE", "DEVICE", ... ] Add support for that in xenconfig driver. When only one device is defined, keep using old syntax for backward compatibility.

Adjust tests for changed options order.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- src/xenconfig/xen_common.c | 66 ------------- src/xenconfig/xen_xl.c | 127 +++++++++++++++++++++++++ src/xenconfig/xen_xm.c | 72 ++++++++++++++ tests/xmconfigdata/test-fullvirt-usbmouse.cfg | 4 +- tests/xmconfigdata/test-fullvirt-usbtablet.cfg | 4 +- 5 files changed, 203 insertions(+), 70 deletions(-)

diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c index a2a1474..079f77d 100644 --- a/src/xenconfig/xen_common.c +++ b/src/xenconfig/xen_common.c @@ -972,33 +972,6 @@ xenParseEmulatedDevices(virConfPtr conf, virDomainDefPtr def) if (str && xenParseSxprSound(def, str) < 0) return -1; - - if (xenConfigGetString(conf, "usbdevice", &str, NULL) < 0) - return -1; - - if (str && - (STREQ(str, "tablet") || - STREQ(str, "mouse") || - STREQ(str, "keyboard"))) { - virDomainInputDefPtr input; - if (VIR_ALLOC(input) < 0) - return -1; - - input->bus = VIR_DOMAIN_INPUT_BUS_USB; - if (STREQ(str, "mouse")) - input->type = VIR_DOMAIN_INPUT_TYPE_MOUSE; - else if (STREQ(str, "tablet")) - input->type = VIR_DOMAIN_INPUT_TYPE_TABLET; - else if (STREQ(str, "keyboard")) - input->type = VIR_DOMAIN_INPUT_TYPE_KBD; - if (VIR_ALLOC_N(def->inputs, 1) < 0) { - virDomainInputDefFree(input); - return -1; - - } - def->inputs[0] = input; - def->ninputs = 1; - } }

return 0; @@ -1949,42 +1922,6 @@ xenFormatSound(virConfPtr conf, virDomainDefPtr def) }

-static int -xenFormatInputDevs(virConfPtr conf, virDomainDefPtr def) -{ - size_t i; - - if (STREQ(def->os.type, "hvm")) { - for (i = 0; i < def->ninputs; i++) { - if (def->inputs[i]->bus == VIR_DOMAIN_INPUT_BUS_USB) { - if (xenConfigSetInt(conf, "usb", 1) < 0) - return -1; - - switch (def->inputs[i]->type) { - case VIR_DOMAIN_INPUT_TYPE_MOUSE: - if (xenConfigSetString(conf, "usbdevice", "mouse") < 0) - return -1; - - break; - case VIR_DOMAIN_INPUT_TYPE_TABLET: - if (xenConfigSetString(conf, "usbdevice", "tablet") < 0) - return -1; - - break; - case VIR_DOMAIN_INPUT_TYPE_KBD: - if (xenConfigSetString(conf, "usbdevice", "keyboard") < 0) - return -1; - - break; - } - break; - } - } - } - - return 0; -} -

static int xenFormatVif(virConfPtr conf, @@ -2059,9 +1996,6 @@ xenFormatConfigCommon(virConfPtr conf, if (xenFormatEmulator(conf, def) < 0) return -1;

- if (xenFormatInputDevs(conf, def) < 0) - return -1; - if (xenFormatVfb(conf, def, xendConfigVersion) < 0) return -1;

diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c index 95ef5f4..f127ebe 100644 --- a/src/xenconfig/xen_xl.c +++ b/src/xenconfig/xen_xl.c @@ -289,6 +289,59 @@ xenParseXLDisk(virConfPtr conf, virDomainDefPtr def) goto cleanup; }

+static int +xenParseXLInputDevs(virConfPtr conf, virDomainDefPtr def) +{ + const char *str; + virConfValuePtr val; + + if (STREQ(def->os.type, "hvm")) { + val = virConfGetValue(conf, "usbdevice"); + /* usbdevice can be defined as either a single string or a list */ + if (val && val->type == VIR_CONF_LIST) { +#ifdef LIBXL_HAVE_BUILDINFO_USBDEVICE_LIST + val = val->list; +#else + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("multiple USB devices not supported")); + return -1; +#endif + } + /* otherwise val->next is NULL, so can be handled by the same code */ + while (val) { + if (val->type != VIR_CONF_STRING) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("config value %s was malformed"), + "usbdevice"); + return -1; + } + str = val->str; + + if (str && + (STREQ(str, "tablet") || + STREQ(str, "mouse") || + STREQ(str, "keyboard"))) { + virDomainInputDefPtr input; + if (VIR_ALLOC(input) < 0) + return -1; + + input->bus = VIR_DOMAIN_INPUT_BUS_USB; + if (STREQ(str, "mouse")) + input->type = VIR_DOMAIN_INPUT_TYPE_MOUSE; + else if (STREQ(str, "tablet")) + input->type = VIR_DOMAIN_INPUT_TYPE_TABLET; + else if (STREQ(str, "keyboard")) + input->type = VIR_DOMAIN_INPUT_TYPE_KBD; + if (VIR_APPEND_ELEMENT(def->inputs, def->ninputs, input) < 0) { + virDomainInputDefFree(input); + return -1; + } + } + val = val->next; + } + } + return 0; +}

virDomainDefPtr xenParseXL(virConfPtr conf, virCapsPtr caps, int xendConfigVersion) @@ -310,6 +363,9 @@ xenParseXL(virConfPtr conf, virCapsPtr caps, int xendConfigVersion) if (xenParseXLSpice(conf, def) < 0) goto cleanup;

+ if (xenParseXLInputDevs(conf, def) < 0) + goto cleanup; + return def;

cleanup: @@ -488,6 +544,74 @@ xenFormatXLSpice(virConfPtr conf, virDomainDefPtr def) return 0; }

+static int +xenFormatXLInputDevs(virConfPtr conf, virDomainDefPtr def) +{ + size_t i; + const char *devtype; + virConfValuePtr usbdevices = NULL, lastdev; + + if (STREQ(def->os.type, "hvm")) { + if (VIR_ALLOC(usbdevices) < 0) + goto error; + + usbdevices->type = VIR_CONF_LIST; + usbdevices->list = NULL; + lastdev = NULL; + for (i = 0; i < def->ninputs; i++) { + if (def->inputs[i]->bus == VIR_DOMAIN_INPUT_BUS_USB) { + if (xenConfigSetInt(conf, "usb", 1) < 0) + goto error; + + switch (def->inputs[i]->type) { + case VIR_DOMAIN_INPUT_TYPE_MOUSE: + devtype = "mouse"; + break; + case VIR_DOMAIN_INPUT_TYPE_TABLET: + devtype = "tablet"; + break; + case VIR_DOMAIN_INPUT_TYPE_KBD: + devtype = "keyboard"; + break; + default: + continue; + } + + if (lastdev == NULL) { + if (VIR_ALLOC(lastdev) < 0) + goto error; + usbdevices->list = lastdev; + } else { + if (VIR_ALLOC(lastdev->next) < 0) + goto error; + lastdev = lastdev->next; + } + lastdev->type = VIR_CONF_STRING; + if (VIR_STRDUP(lastdev->str, devtype) < 0) + goto error; + } + } + if (usbdevices->list != NULL) { + if (usbdevices->list->next == NULL) { + /* for compatibility with Xen <= 4.2, use old syntax when + * only one device present */ + if (xenConfigSetString(conf, "usbdevice", usbdevices->list->str) < 0) + goto error; + virConfFreeValue(usbdevices); + } else { + virConfSetValue(conf, "usbdevice", usbdevices); + } + } else { + VIR_FREE(usbdevices); + } + } + + return 0; + error: + virConfFreeValue(usbdevices); + return -1;

I had to stare at this a bit to convince myself memory was being freed properly. xenConfigSetString copies the string, so need virConfFreeValue afterwards - check. virtConfSetValue takes ownership of value, so no need to free - check. Cleanup on error - check. Caller frees values when conf is destoryed - check. Coverity, and subsequently John, will complain loudly if I've missed a check. ACK. Regards, Jim

+} +

virConfPtr xenFormatXL(virDomainDefPtr def, virConnectPtr conn, int xendConfigVersion) @@ -506,6 +630,9 @@ xenFormatXL(virDomainDefPtr def, virConnectPtr conn, int xendConfigVersion) if (xenFormatXLSpice(conf, def) < 0) goto cleanup;

+ if (xenFormatXLInputDevs(conf, def) < 0) + goto cleanup; + return conf;

cleanup: diff --git a/src/xenconfig/xen_xm.c b/src/xenconfig/xen_xm.c index 2f57cd2..5d70af6 100644 --- a/src/xenconfig/xen_xm.c +++ b/src/xenconfig/xen_xm.c @@ -365,6 +365,38 @@ xenFormatXMDisks(virConfPtr conf, virDomainDefPtr def, int xendConfigVersion) }

+static int +xenParseXMInputDevs(virConfPtr conf, virDomainDefPtr def) +{ + const char *str; + + if (STREQ(def->os.type, "hvm")) { + if (xenConfigGetString(conf, "usbdevice", &str, NULL) < 0) + return -1; + if (str && + (STREQ(str, "tablet") || + STREQ(str, "mouse") || + STREQ(str, "keyboard"))) { + virDomainInputDefPtr input; + if (VIR_ALLOC(input) < 0) + return -1; + + input->bus = VIR_DOMAIN_INPUT_BUS_USB; + if (STREQ(str, "mouse")) + input->type = VIR_DOMAIN_INPUT_TYPE_MOUSE; + else if (STREQ(str, "tablet")) + input->type = VIR_DOMAIN_INPUT_TYPE_TABLET; + else if (STREQ(str, "keyboard")) + input->type = VIR_DOMAIN_INPUT_TYPE_KBD; + if (VIR_APPEND_ELEMENT(def->inputs, def->ninputs, input) < 0) { + virDomainInputDefFree(input); + return -1; + } + } + } + return 0; +} + /* * Convert an XM config record into a virDomainDef object. */ @@ -387,6 +419,9 @@ xenParseXM(virConfPtr conf, if (xenParseXMDisk(conf, def, xendConfigVersion) < 0) goto cleanup;

+ if (xenParseXMInputDevs(conf, def) < 0) + goto cleanup; + return def;

cleanup: @@ -394,6 +429,40 @@ xenParseXM(virConfPtr conf, return NULL; }

+static int +xenFormatXMInputDevs(virConfPtr conf, virDomainDefPtr def) +{ + size_t i; + const char *devtype; + + if (STREQ(def->os.type, "hvm")) { + for (i = 0; i < def->ninputs; i++) { + if (def->inputs[i]->bus == VIR_DOMAIN_INPUT_BUS_USB) { + if (xenConfigSetInt(conf, "usb", 1) < 0) + return -1; + + switch (def->inputs[i]->type) { + case VIR_DOMAIN_INPUT_TYPE_MOUSE: + devtype = "mouse"; + break; + case VIR_DOMAIN_INPUT_TYPE_TABLET: + devtype = "tablet"; + break; + case VIR_DOMAIN_INPUT_TYPE_KBD: + devtype = "keyboard"; + break; + default: + continue; + } + if (xenConfigSetString(conf, "usbdevice", devtype) < 0) + return -1; + break; + } + } + } + return 0; +} +

/* Computing the vcpu_avail bitmask works because MAX_VIRT_CPUS is either 32, or 64 on a platform where long is big enough. */ @@ -418,6 +487,9 @@ xenFormatXM(virConnectPtr conn, if (xenFormatXMDisks(conf, def, xendConfigVersion) < 0) goto cleanup;

+ if (xenFormatXMInputDevs(conf, def) < 0) + goto cleanup; + return conf;

cleanup: diff --git a/tests/xmconfigdata/test-fullvirt-usbmouse.cfg b/tests/xmconfigdata/test-fullvirt-usbmouse.cfg index b24d7a4..7b252cf 100755 --- a/tests/xmconfigdata/test-fullvirt-usbmouse.cfg +++ b/tests/xmconfigdata/test-fullvirt-usbmouse.cfg @@ -14,8 +14,6 @@ on_poweroff = "destroy" on_reboot = "restart" on_crash = "restart" device_model = "/usr/lib/xen/bin/qemu-dm" -usb = 1 -usbdevice = "mouse" sdl = 0 vnc = 1 vncunused = 1 @@ -25,3 +23,5 @@ vif = [ "mac=00:16:3e:66:92:9c,bridge=xenbr1,script=vif-bridge,model=e1000,type= parallel = "none" serial = "none" disk = [ "phy:/dev/HostVG/XenGuest2,hda,w", "file:/root/boot.iso,hdc:cdrom,r" ] +usb = 1 +usbdevice = "mouse" diff --git a/tests/xmconfigdata/test-fullvirt-usbtablet.cfg b/tests/xmconfigdata/test-fullvirt-usbtablet.cfg index 189cd15..7e6cd36 100755 --- a/tests/xmconfigdata/test-fullvirt-usbtablet.cfg +++ b/tests/xmconfigdata/test-fullvirt-usbtablet.cfg @@ -14,8 +14,6 @@ on_poweroff = "destroy" on_reboot = "restart" on_crash = "restart" device_model = "/usr/lib/xen/bin/qemu-dm" -usb = 1 -usbdevice = "tablet" sdl = 0 vnc = 1 vncunused = 1 @@ -25,3 +23,5 @@ vif = [ "mac=00:16:3e:66:92:9c,bridge=xenbr1,script=vif-bridge,model=e1000,type= parallel = "none" serial = "none" disk = [ "phy:/dev/HostVG/XenGuest2,hda,w", "file:/root/boot.iso,hdc:cdrom,r" ] +usb = 1 +usbdevice = "tablet"

Marek Marczykowski-Górecki wrote:

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- tests/xlconfigdata/test-fullvirt-multiusb.cfg | 29 ++++++++++++++++ tests/xlconfigdata/test-fullvirt-multiusb.xml | 48 +++++++++++++++++++++++++++ tests/xlconfigtest.c | 1 + 3 files changed, 78 insertions(+) create mode 100755 tests/xlconfigdata/test-fullvirt-multiusb.cfg create mode 100644 tests/xlconfigdata/test-fullvirt-multiusb.xml

Nice, thanks! I've tested this with the various combinations of --with{out}-xen --with{out}-libxl configure options. ACK. Regards, Jim

diff --git a/tests/xlconfigdata/test-fullvirt-multiusb.cfg b/tests/xlconfigdata/test-fullvirt-multiusb.cfg new file mode 100755 index 0000000..ba4bf52 --- /dev/null +++ b/tests/xlconfigdata/test-fullvirt-multiusb.cfg @@ -0,0 +1,29 @@ +name = "XenGuest2" +uuid = "c7a5fdb2-cdaf-9455-926a-d65c16db1809" +maxmem = 579 +memory = 394 +vcpus = 1 +builder = "hvm" +kernel = "/usr/lib/xen/boot/hvmloader" +boot = "d" +pae = 1 +acpi = 1 +apic = 1 +hap = 0 +viridian = 0 +localtime = 0 +on_poweroff = "destroy" +on_reboot = "restart" +on_crash = "restart" +device_model = "/usr/lib/xen/bin/qemu-dm" +sdl = 0 +vnc = 1 +vncunused = 1 +vnclisten = "127.0.0.1" +vncpasswd = "123poi" +vif = [ "mac=00:16:3e:66:92:9c,bridge=xenbr1,script=vif-bridge,model=e1000,type=ioemu" ] +parallel = "none" +serial = "none" +disk = [ "/dev/HostVG/XenGuest2,raw,hda,w,backendtype=phy", "/root/boot.iso,raw,hdc,r,backendtype=qdisk,devtype=cdrom" ] +usb = 1 +usbdevice = [ "mouse", "tablet" ] diff --git a/tests/xlconfigdata/test-fullvirt-multiusb.xml b/tests/xlconfigdata/test-fullvirt-multiusb.xml new file mode 100644 index 0000000..642c242 --- /dev/null +++ b/tests/xlconfigdata/test-fullvirt-multiusb.xml @@ -0,0 +1,48 @@ +<domain type='xen'> + <name>XenGuest2</name> + <uuid>c7a5fdb2-cdaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>592896</memory> + <currentMemory unit='KiB'>403456</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='xenfv'>hvm</type> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> + <boot dev='cdrom'/> + </os> + <features> + <acpi/> + <apic/> + <pae/> + </features> + <clock offset='utc' adjustment='reset'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <devices> + <emulator>/usr/lib/xen/bin/qemu-dm</emulator> + <disk type='block' device='disk'> + <driver name='phy' type='raw'/> + <source dev='/dev/HostVG/XenGuest2'/> + <target dev='hda' bus='ide'/> + </disk> + <disk type='file' device='cdrom'> + <driver name='qemu' type='raw'/> + <source file='/root/boot.iso'/> + <target dev='hdc' bus='ide'/> + <readonly/> + </disk> + <interface type='bridge'> + <mac address='00:16:3e:66:92:9c'/> + <source bridge='xenbr1'/> + <script path='vif-bridge'/> + <model type='e1000'/> + </interface> + <input type='mouse' bus='usb'/> + <input type='tablet' bus='usb'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <graphics type='vnc' port='-1' autoport='yes' listen='127.0.0.1' passwd='123poi'> + <listen type='address' address='127.0.0.1'/> + </graphics> + </devices> +</domain> diff --git a/tests/xlconfigtest.c b/tests/xlconfigtest.c index 8c4c82c..6d4aa6d 100644 --- a/tests/xlconfigtest.c +++ b/tests/xlconfigtest.c @@ -215,6 +215,7 @@ mymain(void)

DO_TEST("new-disk", 3); DO_TEST("spice", 3); + DO_TEST("fullvirt-multiusb", 3);

virObjectUnref(caps); virObjectUnref(xmlopt);

Marek Marczykowski-Górecki wrote:

virDomainNetFindIdx no longer returns info whether device was not found, or there was multiple matches. Additionally it already handle error reporting. Introduce virDomainHasNet which does a simple task, without implicit error reporting.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- src/conf/domain_conf.c | 21 +++++++++++++++++++++ src/conf/domain_conf.h | 1 + src/libvirt_private.syms | 1 + 3 files changed, 23 insertions(+)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9b7ae3f..c430e03 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11908,6 +11908,27 @@ virDomainNetFindIdx(virDomainDefPtr def, virDomainNetDefPtr net) return matchidx; }

+bool virDomainHasNet(virDomainDefPtr def, virDomainNetDefPtr net)

Nit: libvirt style is to have return type on separate line.

+{ + size_t i; + bool PCIAddrSpecified = virDomainDeviceAddressIsValid(&net->info, + VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI); + + for (i = 0; i < def->nnets; i++) { + if (virMacAddrCmp(&def->nets[i]->mac, &net->mac)) + continue; + + if (PCIAddrSpecified) {

Braces here are fine, where the single statement spans multiple lines.

+ if (virDevicePCIAddressEqual(&def->nets[i]->info.addr.pci, + &net->info.addr.pci)) { + return true; + }

But not needed here. ACK. Will fix the nits before pushing. Regards, Jim

+ } else { + return true; + } + } + return false; +}

void virDomainNetRemoveHostdev(virDomainDefPtr def, diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 02ddd93..36c8131 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2601,6 +2601,7 @@ bool virDomainHasDiskMirror(virDomainObjPtr vm);

int virDomainNetFindIdx(virDomainDefPtr def, virDomainNetDefPtr net); virDomainNetDefPtr virDomainNetFind(virDomainDefPtr def, const char *device); +bool virDomainHasNet(virDomainDefPtr def, virDomainNetDefPtr net); int virDomainNetInsert(virDomainDefPtr def, virDomainNetDefPtr net); virDomainNetDefPtr virDomainNetRemove(virDomainDefPtr def, size_t i); void virDomainNetRemoveHostdev(virDomainDefPtr def, virDomainNetDefPtr net); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index ba05cc6..5fe6ae0 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -292,6 +292,7 @@ virDomainGraphicsTypeToString; virDomainGraphicsVNCSharePolicyTypeFromString; virDomainGraphicsVNCSharePolicyTypeToString; virDomainHasDiskMirror; +virDomainHasNet; virDomainHostdevCapsTypeToString; virDomainHostdevDefAlloc; virDomainHostdevDefClear;

Marek Marczykowski-Górecki wrote:

It will not be possible to detach such device later. Also improve logging in such cases.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- src/libxl/libxl_driver.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)

Changes in v4: - use virDomainHasNet instead of virDomainNetFindIdx

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index ce3a99b..1313d2e 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -2787,6 +2787,7 @@ libxlDomainAttachNetDevice(libxlDriverPrivatePtr driver, int actualType; libxl_device_nic nic; int ret = -1; + char mac[VIR_MAC_STRING_BUFLEN];

/* preallocate new slot for device */ if (VIR_REALLOC_N(vm->def->nets, vm->def->nnets + 1) < 0) @@ -2801,6 +2802,13 @@ libxlDomainAttachNetDevice(libxlDriverPrivatePtr driver,

actualType = virDomainNetGetActualType(net);

+ if (virDomainHasNet(vm->def, net)) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("device matching mac address %s already exists"),

+ virMacAddrFormat(&net->mac, mac));

I think the error should be VIR_ERR_INVALID_ARG, like you've done in the last hunk below. I'll change that, and use the same text in both error reports, before pushing. Otherwise, ACK. Regards, Jim

+ return -1; + } + if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) { /* This is really a "smart hostdev", so it should be attached * as a hostdev (the hostdev code will reach over into the @@ -2879,6 +2887,7 @@ libxlDomainAttachDeviceConfig(virDomainDefPtr vmdef, virDomainDeviceDefPtr dev) virDomainHostdevDefPtr hostdev; virDomainHostdevDefPtr found; virDomainHostdevSubsysPCIPtr pcisrc; + char mac[VIR_MAC_STRING_BUFLEN];

switch (dev->type) { case VIR_DOMAIN_DEVICE_DISK: @@ -2896,6 +2905,12 @@ libxlDomainAttachDeviceConfig(virDomainDefPtr vmdef, virDomainDeviceDefPtr dev)

case VIR_DOMAIN_DEVICE_NET: net = dev->data.net; + if (virDomainHasNet(vmdef, net)) { + virReportError(VIR_ERR_INVALID_ARG, + _("network device with mac %s already exists."), + virMacAddrFormat(&net->mac, mac)); + return -1; + } if (virDomainNetInsert(vmdef, net)) return -1; dev->data.net = NULL;

On Fri, Mar 13, 2015 at 02:09:34PM -0600, Jim Fehlig wrote:

Marek Marczykowski-Górecki wrote:

...

Xen have feature of having device model in separate domain (called stub domain). Add <stubdomain> element to allow selecting such configuration. Emulator path is still used for qemu running in dom0 (if any). Libxl currently do not allow to select stubdomain path.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- docs/formatdomain.html.in | 13 +++++++++++++ docs/schemas/domaincommon.rng | 10 ++++++++++ src/conf/domain_conf.c | 14 ++++++++++++++ src/conf/domain_conf.h | 3 ++- src/libxl/libxl_conf.c | 8 ++++++++ 5 files changed, 47 insertions(+), 1 deletion(-)

Changes in v4: - change <emulator type='stubdom'> to separate <stubdomain> element

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index fb0a0d1..054f48f 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1670,6 +1670,19 @@ </dd> </dl>

+ <dl> + <dt><code>stubdomain</code></dt> + <dd> + <span class="since">Since 1.2.13</span>, the optional <code>stubdomain</code> + element contains <code>enabled</code> yes/no/default attribute. If enabled + Emulator will be launched in stub domain (Xen only). Currently there + is no way to provide path to that emulator. Note that in most cases

+ this emulator will be in addition to one in dom0.

Why does a stubdomain need an emulator (running in dom0) to serve as a backend? Wouldn't it be better to give the stubdomain direct access to the resources it needs? Otherwise, It seems to defeat stubdomain's purpose of moving the emulator execution out of dom0. At least the Xen wiki describes this as primary motivation for studomains

http://wiki.xen.org/wiki/Device_Model_Stub_Domains

Two reasons: 1. To host video/keyboard (vfb/vkb) backends for stubdomain - stubdomain have no simple way to expose VNC server to the network. 2. To host a console backend. Xenconsoled supports only one console per domain, but qemu in stubdomain needs at least three (logs, pipe to save the state and another one to restore the state during save/restore). Actually in Qubes OS we have our own GUI agent, so the first point is not needed. And we broke the second one just to get rid of qemu in dom0... I'll be more than happy if/when xenconsoled will get support for more than one console device per domain.

...

+ <code>emulator</code> element described above still controls the path + to the dom0 instance. + </dd> + </dl>

+

I think it would be good to add an example above the description. E.g. along the lines of Daniel's previous example

<devices> <emulator>/usr/lib/xen/bin/qemu-system-i386</emulator> <stubdomain enabled="yes|no"/> </devices>

Ok, I'll add it in the next patch version (if any).

Regards, Jim

...

<h4><a name="elementsDisks">Hard drives, floppy disks, CDROMs</a></h4>

<p> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 4e4fe9f..539db39 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2567,6 +2567,13 @@ <ref name="absFilePath"/> </element> </define> + <define name="stubdomain"> + <element name="stubdomain"> + <attribute name="enabled"> + <ref name="virYesNo"/> + </attribute> + </element> + </define> <!-- A graphic description, currently in Xen only 2 types are supported: - sdl with optional display, xauth and fullscreen @@ -4007,6 +4014,9 @@ <optional> <ref name="emulator"/> </optional> + <optional> + <ref name="stubdomain"/> + </optional> <zeroOrMore> <choice> <ref name="disk"/> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c430e03..e327e55 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14107,6 +14107,9 @@ virDomainDefParseXML(xmlDocPtr xml, }

def->emulator = virXPathString("string(./devices/emulator[1])", ctxt); + tmp = virXPathString("string(./devices/stubdomain/@enabled)", ctxt); + if (tmp) + def->stubdomain = virTristateBoolTypeFromString(tmp);

/* analysis of the disk devices */ if ((n = virXPathNodeSet("./devices/disk", ctxt, &nodes)) < 0) @@ -16064,6 +16067,14 @@ virDomainDefCheckABIStability(virDomainDefPtr src, goto error; }

+ if (src->stubdomain != dst->stubdomain) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Target domain stubdomain '%s' does not match source '%s'"), + virTristateBoolTypeToString(dst->stubdomain), + virTristateBoolTypeToString(src->stubdomain)); + goto error; + } + if (!virDomainDefFeaturesCheckABIStability(src, dst)) goto error;

@@ -20303,6 +20314,9 @@ virDomainDefFormatInternal(virDomainDefPtr def,

virBufferEscapeString(buf, "<emulator>%s</emulator>\n", def->emulator); + if (def->stubdomain) + virBufferAsprintf(buf, "<stubdomain enabled='%s'/>\n", + virTristateBoolTypeToString(def->stubdomain));

for (n = 0; n < def->ndisks; n++) if (virDomainDiskDefFormat(buf, def->disks[n], flags) < 0) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 36c8131..446f4f0 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2108,9 +2108,10 @@ struct _virDomainDef {

virDomainOSDef os; char *emulator; - /* These three options are of type virTristateSwitch, + /* These four options are of type virTristateSwitch, * except VIR_DOMAIN_FEATURE_CAPABILITIES that is of type * virDomainCapabilitiesPolicy */ + int stubdomain; int features[VIR_DOMAIN_FEATURE_LAST]; int apic_eoi; int hyperv_features[VIR_DOMAIN_HYPERV_LAST]; diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 50ef9d8..8ec3c75 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -727,6 +727,14 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, b_info->device_model_version = libxlDomainGetEmulatorType(def); }

+ /* In case of stubdom there will be two qemu instances: + * - in stubdom (libxl uses hardcoded path for this one), + * - in dom0 as a backend for stubdom (if needed). + */ + if (def->stubdomain) + libxl_defbool_set(&b_info->device_model_stubdomain, + def->stubdomain == VIR_TRISTATE_BOOL_YES); + if (def->nserials) { if (def->nserials > 1) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED,

-- Best Regards, Marek Marczykowski-Górecki Invisible Things Lab A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing?

Marek Marczykowski-Górecki wrote:

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- src/libxl/libxl_conf.c | 8 ++++++++ 1 file changed, 8 insertions(+)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 8ec3c75..d78d2b2 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -735,6 +735,14 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, libxl_defbool_set(&b_info->device_model_stubdomain, def->stubdomain == VIR_TRISTATE_BOOL_YES);

+ if (def->os.cmdline && def->os.cmdline[0]) { + b_info->extra_hvm = virStringSplit(def->os.cmdline, " ", 0);

What is the difference between b_info->extra_hvm and b_info->cmdline? How are these two reconciled in libxl? I.e., what if they contained different values? Chunyan added the latter with xen commit 11dffa235 when implementing hvm direct kernel boot. She also submitted a patch for libvirt https://www.redhat.com/archives/libvir-list/2014-September/msg01006.html I reworked that patch to support the recent breakout of xl and xm config in src/xenconfig. While doing so, I fixed other <os> related issues and posted this series https://www.redhat.com/archives/libvir-list/2015-March/msg01088.html Patch 5 in that series supports <cmdline> for HVM guests, but only if LIBXL_HAVE_BUILDINFO_KERNEL is defined (introduced in Xen4.5 IIRC). Do we need to populate both b_info->extra_hvm and b_info->cmdline? Regards, Jim

On Fri, Mar 20, 2015 at 11:18:36AM -0600, Jim Fehlig wrote:

Marek Marczykowski-Górecki wrote:

...

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- src/libxl/libxl_conf.c | 8 ++++++++ 1 file changed, 8 insertions(+)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 8ec3c75..d78d2b2 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -735,6 +735,14 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, libxl_defbool_set(&b_info->device_model_stubdomain, def->stubdomain == VIR_TRISTATE_BOOL_YES);

+ if (def->os.cmdline && def->os.cmdline[0]) { + b_info->extra_hvm = virStringSplit(def->os.cmdline, " ", 0);

What is the difference between b_info->extra_hvm and b_info->cmdline? How are these two reconciled in libxl? I.e., what if they contained

In short, I don't think you should use that field at all. b_info->extra_hvm contains the parameters passed to QEMU specifically for HVM guest. Those parameters are not meant to be kernel command line. That field is introduced so that user can append arbitrary parameters to QEMU. I think it's only a stopgap for things we have not implemented in libxl or things we don't want to put into libxl. In reality I think it is rarely used.

different values? Chunyan added the latter with xen commit 11dffa235 when implementing hvm direct kernel boot. She also submitted a patch for libvirt

https://www.redhat.com/archives/libvir-list/2014-September/msg01006.html

I reworked that patch to support the recent breakout of xl and xm config in src/xenconfig. While doing so, I fixed other <os> related issues and posted this series

https://www.redhat.com/archives/libvir-list/2015-March/msg01088.html

Patch 5 in that series supports <cmdline> for HVM guests, but only if LIBXL_HAVE_BUILDINFO_KERNEL is defined (introduced in Xen4.5 IIRC). Do we need to populate both b_info->extra_hvm and b_info->cmdline?

No. See above. Wei.

Regards, Jim

On Fri, Mar 20, 2015 at 02:11:46PM -0600, Jim Fehlig wrote:

Wei Liu wrote:

...

On Fri, Mar 20, 2015 at 11:18:36AM -0600, Jim Fehlig wrote:

...

Marek Marczykowski-Górecki wrote:

...

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- src/libxl/libxl_conf.c | 8 ++++++++ 1 file changed, 8 insertions(+)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 8ec3c75..d78d2b2 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -735,6 +735,14 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, libxl_defbool_set(&b_info->device_model_stubdomain, def->stubdomain == VIR_TRISTATE_BOOL_YES);

+ if (def->os.cmdline && def->os.cmdline[0]) { + b_info->extra_hvm = virStringSplit(def->os.cmdline, " ", 0);

What is the difference between b_info->extra_hvm and b_info->cmdline? How are these two reconciled in libxl? I.e., what if they contained

In short, I don't think you should use that field at all.

b_info->extra_hvm contains the parameters passed to QEMU specifically for HVM guest. Those parameters are not meant to be kernel command line.

That field is introduced so that user can append arbitrary parameters to QEMU. I think it's only a stopgap for things we have not implemented in libxl or things we don't want to put into libxl. In reality I think it

is rarely used.

Thanks Wei and Ian for the explanation.

Marek, seems we want b_info->cmdline, if the intent is to "pass cmdline to HVM guests". Can you review (and test in your setup) the "improve xl config parsing" series I posted?

I'll definitely do. But above raises a question - how can I set extra arguments for qemu? In case of qemu in dom0, it's not a problem because I can create some wrapper script. But in case of qemu in stubdom, the only way is to set b_info->extra_hvm. Some additional attributes for <emulator> tag? If you're curious why I want to do such things, I have a custom patch for qemu in stubdom, to have a (LWIP based) DHCP server for the VM there. Just to get rid of as much as possible from backend domain (which BTW is not dom0). So I need to pass it some configuration, qemu cmdline seems like the only feasible option. -- Best Regards, Marek Marczykowski-Górecki Invisible Things Lab A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing?

On Fri, Mar 20, 2015 at 02:56:11PM -0600, Jim Fehlig wrote:

Marek Marczykowski-Górecki wrote:

...

Handle features supported only on xen: driver domains, qemu in stubdomain.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

[...]

...

diff --git a/tests/xlxml2xmltest.c b/tests/xlxml2xmltest.c new file mode 100644 index 0000000..131e43b --- /dev/null +++ b/tests/xlxml2xmltest.c @@ -0,0 +1,189 @@ +#include <config.h> + +#include <stdio.h> +#include <stdlib.h> +#include <unistd.h> +#include <string.h> + +#include <sys/types.h> +#include <fcntl.h> + +#include "testutils.h" + +#ifdef WITH_LIBXL + +# include "internal.h" +# include "libxl/libxl_conf.h" +# include "libxl/libxl_domain.h" +# include "testutilsxen.h" +# include "virstring.h" + +# define VIR_FROM_THIS VIR_FROM_NONE + +static virCapsPtr caps; +static virDomainXMLOptionPtr xmlopt; + +static int +testCompareXMLToXMLFiles(const char *inxml, const char *outxml, bool live) +{ + char *inXmlData = NULL; + char *outXmlData = NULL; + char *actual = NULL; + int ret = -1; + virDomainDefPtr def = NULL; + unsigned int parse_flags = live ? 0 : VIR_DOMAIN_DEF_PARSE_INACTIVE; + unsigned int format_flags = VIR_DOMAIN_DEF_FORMAT_SECURE; + if (!live) + format_flags |= VIR_DOMAIN_DEF_FORMAT_INACTIVE; +

Perhaps I'm being dense, but it is not clear to me what this is testing wrt libxl or xenconfig code. xml config is read from a file

...

+ if (virtTestLoadFile(inxml, &inXmlData) < 0) + goto fail; + if (virtTestLoadFile(outxml, &outXmlData) < 0) + goto fail; + + if (!(def = virDomainDefParseString(inXmlData, caps, xmlopt, + 1 << VIR_DOMAIN_VIRT_XEN, parse_flags))) + goto fail; +

parsed with virDomainDefParseString (which does invoke the libxl driver post-parse callbacks)

...

+ if (!virDomainDefCheckABIStability(def, def)) { + fprintf(stderr, "ABI stability check failed on %s", inxml); + goto fail; + }

virDomainDefCheckABIStability called with identical def

...

+ + if (!(actual = virDomainDefFormat(def, format_flags))) + goto fail;

converted back to XML

...

+ + if (STRNEQ(outXmlData, actual)) { + virtTestDifferenceFull(stderr, outXmlData, outxml, actual, inxml); + goto fail;

and then compared with xml config read from a file. I suppose the post-parse callbacks are being tested. IMO, a more useful test would be domain XML -> libxl_domain_config -> JSON doc, comparing the output JSON doc to a template doc. Daniel Berrange started work on such a test, which I toiled on later, but we've yet to find a nice solution that works across Xen >= 4.2. This work was last discussed in January

https://www.redhat.com/archives/libvir-list/2015-January/msg00924.html

Suggestions welcomed!

I've written this test before finding already existing one for XML->xenconfig and xenconfig->XML conversion. Initially it was meant to verify XML parsing/formating code, but handling those with xenconfig tests seems to be more sensible. So I think you can just drop this patch. -- Best Regards, Marek Marczykowski-Górecki Invisible Things Lab A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing?