Kiarie Kahurani wrote: > Introduce the new xen-xl parser that {formats,parses} xl disk > and spice graphics format > This is a big patch with a lot of changes that aren't directly related parsing/formating xl config. I think it would be best to split this into three patches: One that exports all the convenience functions in src/xenconfig/xen_common.[ch], one that implements the xl parser and tests, and one that then uses the parser in the libxl driver. > Signed-off-by: Kiarie Kahurani <davidkiarie4@xxxxxxxxx> > --- > po/POTFILES.in | 1 + > src/Makefile.am | 3 +- > src/libvirt_xenconfig.syms | 5 + > src/libxl/libxl_driver.c | 44 ++- > src/xenconfig/xen_common.c | 140 ++++---- > src/xenconfig/xen_common.h | 24 +- > src/xenconfig/xen_xl.c | 461 ++++++++++++++++++++++++++ > src/xenconfig/xen_xl.h | 29 ++ > tests/Makefile.am | 9 +- > tests/testutilsxen.c | 50 +++ > tests/testutilsxen.h | 9 +- > tests/xlconfigdata/test-fullvirt-new-disk.cfg | 27 ++ > tests/xlconfigdata/test-fullvirt-new-disk.xml | 46 +++ > tests/xlconfigtest.c | 222 +++++++++++++ > 14 files changed, 981 insertions(+), 89 deletions(-) > create mode 100644 src/xenconfig/xen_xl.c > create mode 100644 src/xenconfig/xen_xl.h > create mode 100644 tests/xlconfigdata/test-fullvirt-new-disk.cfg > create mode 100644 tests/xlconfigdata/test-fullvirt-new-disk.xml > create mode 100644 tests/xlconfigtest.c > > diff --git a/po/POTFILES.in b/po/POTFILES.in > index f17b35f..3ac31fe 100644 > --- a/po/POTFILES.in > +++ b/po/POTFILES.in > @@ -234,6 +234,7 @@ src/xenapi/xenapi_driver.c > src/xenapi/xenapi_utils.c > src/xenconfig/xen_common.c > src/xenconfig/xen_sxpr.c > +src/xenconfig/xen_xl.c > src/xenconfig/xen_xm.c > tools/libvirt-guests.sh.in > tools/virsh.c > diff --git a/src/Makefile.am b/src/Makefile.am > index 538530e..f0c05b3 100644 > --- a/src/Makefile.am > +++ b/src/Makefile.am > @@ -968,7 +968,8 @@ XENCONFIG_SOURCES = \ > xenconfig/xenxs_private.h \ > xenconfig/xen_common.c xenconfig/xen_common.h \ > xenconfig/xen_sxpr.c xenconfig/xen_sxpr.h \ > - xenconfig/xen_xm.c xenconfig/xen_xm.h > + xenconfig/xen_xm.c xenconfig/xen_xm.h \ > Trailing '\' should be aligned with the other ones. > + xenconfig/xen_xl.c xenconfig/xen_xl.h > > pkgdata_DATA = cpu/cpu_map.xml > > diff --git a/src/libvirt_xenconfig.syms b/src/libvirt_xenconfig.syms > index 6541685..efd7a00 100644 > --- a/src/libvirt_xenconfig.syms > +++ b/src/libvirt_xenconfig.syms > @@ -16,10 +16,15 @@ xenParseSxprChar; > xenParseSxprSound; > xenParseSxprString; > > +#xenconfig/xen_xl.h > +xenFormatXL; > +xenParseXL; > + > # xenconfig/xen_xm.h > xenFormatXM; > xenParseXM; > > + > Spurious whitespace change. > # Let emacs know we want case-insensitive sorting > # Local Variables: > # sort-fold-case: t > diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c > index 67fd7bc6..aa7d861 100644 > --- a/src/libxl/libxl_driver.c > +++ b/src/libxl/libxl_driver.c > @@ -48,6 +48,7 @@ > #include "libxl_migration.h" > #include "xen_xm.h" > #include "xen_sxpr.h" > +#include "xen_xl.h" > #include "virtypedparam.h" > #include "viruri.h" > #include "virstring.h" > @@ -67,6 +68,7 @@ VIR_LOG_INIT("libxl.libxl_driver"); > #define LIBXL_DOM_REQ_CRASH 3 > #define LIBXL_DOM_REQ_HALT 4 > > +#define LIBXL_CONFIG_FORMAT_XL "xen-xl" > #define LIBXL_CONFIG_FORMAT_XM "xen-xm" > #define LIBXL_CONFIG_FORMAT_SEXPR "xen-sxpr" > > @@ -2217,7 +2219,17 @@ libxlConnectDomainXMLFromNative(virConnectPtr conn, > if (virConnectDomainXMLFromNativeEnsureACL(conn) < 0) > goto cleanup; > > - if (STREQ(nativeFormat, LIBXL_CONFIG_FORMAT_XM)) { > + if (STREQ(nativeFormat, LIBXL_CONFIG_FORMAT_XL)) { > + if (!(conf = virConfReadMem(nativeConfig, strlen(nativeConfig), 0))) > + goto cleanup; > + if (!(def = xenParseXL(conf, > + cfg->caps, > + cfg->verInfo->xen_version_major))) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("parsing xl config failed")); > + goto cleanup; > + } > + } else if (STREQ(nativeFormat, LIBXL_CONFIG_FORMAT_XM)) { > if (!(conf = virConfReadMem(nativeConfig, strlen(nativeConfig), 0))) > goto cleanup; > > @@ -2272,21 +2284,31 @@ libxlConnectDomainXMLToNative(virConnectPtr conn, const char * nativeFormat, > if (virConnectDomainXMLToNativeEnsureACL(conn) < 0) > goto cleanup; > > - if (STRNEQ(nativeFormat, LIBXL_CONFIG_FORMAT_XM)) { > + if (STREQ(nativeFormat, LIBXL_CONFIG_FORMAT_XL)) { > + if (!(def = virDomainDefParseString(domainXml, > + cfg->caps, driver->xmlopt, > + 1 << VIR_DOMAIN_VIRT_XEN, > + VIR_DOMAIN_XML_INACTIVE))) > + goto cleanup; > + > + if (!(conf = xenFormatXL(def, conn, cfg->verInfo->xen_version_major))) > + goto cleanup; > + } else if (STREQ(nativeFormat, LIBXL_CONFIG_FORMAT_XM)) { > + if (!(def = virDomainDefParseString(domainXml, > + cfg->caps, driver->xmlopt, > + 1 << VIR_DOMAIN_VIRT_XEN, > + VIR_DOMAIN_XML_INACTIVE))) > + goto cleanup; > + > + if (!(conf = xenFormatXM(conn, def, cfg->verInfo->xen_version_major))) > + goto cleanup; > + } else { > + > virReportError(VIR_ERR_INVALID_ARG, > _("unsupported config type %s"), nativeFormat); > goto cleanup; > } > > - if (!(def = virDomainDefParseString(domainXml, > - cfg->caps, driver->xmlopt, > - 1 << VIR_DOMAIN_VIRT_XEN, > - VIR_DOMAIN_XML_INACTIVE))) > - goto cleanup; > - > - if (!(conf = xenFormatXM(conn, def, cfg->verInfo->xen_version_major))) > - goto cleanup; > - > if (VIR_ALLOC_N(ret, len) < 0) > goto cleanup; > > diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c > index 398e9ec..4666b8c 100644 > --- a/src/xenconfig/xen_common.c > +++ b/src/xenconfig/xen_common.c > @@ -43,7 +43,7 @@ > /* > * Convenience method to grab a long int from the config file object > */ > -static int > +int > xenConfigGetBool(virConfPtr conf, > const char *name, > int *value, > @@ -73,7 +73,7 @@ xenConfigGetBool(virConfPtr conf, > /* > * Convenience method to grab a int from the config file object > */ > -static int > +int > xenConfigGetULong(virConfPtr conf, > const char *name, > unsigned long *value, > @@ -179,7 +179,7 @@ xenConfigCopyString(virConfPtr conf, const char *name, char **value) > } > > > -static int > +int > xenConfigCopyStringOpt(virConfPtr conf, const char *name, char **value) > { > return xenConfigCopyStringInternal(conf, name, value, 1); > @@ -262,8 +262,8 @@ xenConfigGetString(virConfPtr conf, > } > > > -static int > -xenXMConfigSetInt(virConfPtr conf, const char *setting, long long l) > +int > +xenConfigSetInt(virConfPtr conf, const char *setting, long long l) > { > virConfValuePtr value = NULL; > > @@ -283,8 +283,8 @@ xenXMConfigSetInt(virConfPtr conf, const char *setting, long long l) > } > > > -static int > -xenXMConfigSetString(virConfPtr conf, const char *setting, const char *str) > +int > +xenConfigSetString(virConfPtr conf, const char *setting, const char *str) > { > virConfValuePtr value = NULL; > > @@ -1387,11 +1387,11 @@ xenFormatGeneralMeta(virConfPtr conf, virDomainDefPtr def) > { > char uuid[VIR_UUID_STRING_BUFLEN]; > > - if (xenXMConfigSetString(conf, "name", def->name) < 0) > + if (xenConfigSetString(conf, "name", def->name) < 0) > return -1; > > virUUIDFormat(def->uuid, uuid); > - if (xenXMConfigSetString(conf, "uuid", uuid) < 0) > + if (xenConfigSetString(conf, "uuid", uuid) < 0) > return -1; > > return 0; > @@ -1401,12 +1401,12 @@ xenFormatGeneralMeta(virConfPtr conf, virDomainDefPtr def) > static int > xenFormatMem(virConfPtr conf, virDomainDefPtr def) > { > - if (xenXMConfigSetInt(conf, "maxmem", > - VIR_DIV_UP(def->mem.max_balloon, 1024)) < 0) > + if (xenConfigSetInt(conf, "maxmem", > + VIR_DIV_UP(def->mem.max_balloon, 1024)) < 0) > return -1; > > - if (xenXMConfigSetInt(conf, "memory", > - VIR_DIV_UP(def->mem.cur_balloon, 1024)) < 0) > + if (xenConfigSetInt(conf, "memory", > + VIR_DIV_UP(def->mem.cur_balloon, 1024)) < 0) > return -1; > > return 0; > @@ -1468,7 +1468,7 @@ xenFormatTimeOffset(virConfPtr conf, virDomainDefPtr def, int xendConfigVersion) > virDomainClockOffsetTypeToString(def->clock.offset)); > return -1; > } > - if (xenXMConfigSetInt(conf, "rtc_timeoffset", rtc_timeoffset) < 0) > + if (xenConfigSetInt(conf, "rtc_timeoffset", rtc_timeoffset) < 0) > return -1; > > } else { > @@ -1489,7 +1489,7 @@ xenFormatTimeOffset(virConfPtr conf, virDomainDefPtr def, int xendConfigVersion) > } /* !hvm */ > } > > - if (xenXMConfigSetInt(conf, "localtime", vmlocaltime) < 0) > + if (xenConfigSetInt(conf, "localtime", vmlocaltime) < 0) > return -1; > > return 0; > @@ -1506,7 +1506,7 @@ xenFormatEventActions(virConfPtr conf, virDomainDefPtr def) > _("unexpected lifecycle action %d"), def->onPoweroff); > return -1; > } > - if (xenXMConfigSetString(conf, "on_poweroff", lifecycle) < 0) > + if (xenConfigSetString(conf, "on_poweroff", lifecycle) < 0) > return -1; > > > @@ -1515,7 +1515,7 @@ xenFormatEventActions(virConfPtr conf, virDomainDefPtr def) > _("unexpected lifecycle action %d"), def->onReboot); > return -1; > } > - if (xenXMConfigSetString(conf, "on_reboot", lifecycle) < 0) > + if (xenConfigSetString(conf, "on_reboot", lifecycle) < 0) > return -1; > > > @@ -1524,7 +1524,7 @@ xenFormatEventActions(virConfPtr conf, virDomainDefPtr def) > _("unexpected lifecycle action %d"), def->onCrash); > return -1; > } > - if (xenXMConfigSetString(conf, "on_crash", lifecycle) < 0) > + if (xenConfigSetString(conf, "on_crash", lifecycle) < 0) > return -1; > > return 0; > @@ -1545,12 +1545,12 @@ xenFormatCharDev(virConfPtr conf, virDomainDefPtr def) > ret = xenFormatSxprChr(def->parallels[0], &buf); > str = virBufferContentAndReset(&buf); > if (ret == 0) > - ret = xenXMConfigSetString(conf, "parallel", str); > + ret = xenConfigSetString(conf, "parallel", str); > VIR_FREE(str); > if (ret < 0) > return -1; > } else { > - if (xenXMConfigSetString(conf, "parallel", "none") < 0) > + if (xenConfigSetString(conf, "parallel", "none") < 0) > return -1; > } > > @@ -1563,7 +1563,7 @@ xenFormatCharDev(virConfPtr conf, virDomainDefPtr def) > ret = xenFormatSxprChr(def->serials[0], &buf); > str = virBufferContentAndReset(&buf); > if (ret == 0) > - ret = xenXMConfigSetString(conf, "serial", str); > + ret = xenConfigSetString(conf, "serial", str); > VIR_FREE(str); > if (ret < 0) > return -1; > @@ -1608,7 +1608,7 @@ xenFormatCharDev(virConfPtr conf, virDomainDefPtr def) > VIR_FREE(serialVal); > } > } else { > - if (xenXMConfigSetString(conf, "serial", "none") < 0) > + if (xenConfigSetString(conf, "serial", "none") < 0) > return -1; > } > } > @@ -1623,13 +1623,13 @@ xenFormatCPUAllocation(virConfPtr conf, virDomainDefPtr def) > int ret = -1; > char *cpus = NULL; > > - if (xenXMConfigSetInt(conf, "vcpus", def->maxvcpus) < 0) > + if (xenConfigSetInt(conf, "vcpus", def->maxvcpus) < 0) > goto cleanup; > > /* Computing the vcpu_avail bitmask works because MAX_VIRT_CPUS is > either 32, or 64 on a platform where long is big enough. */ > if (def->vcpus < def->maxvcpus && > - xenXMConfigSetInt(conf, "vcpu_avail", (1UL << def->vcpus) - 1) < 0) > + xenConfigSetInt(conf, "vcpu_avail", (1UL << def->vcpus) - 1) < 0) > goto cleanup; > > if ((def->cpumask != NULL) && > @@ -1638,7 +1638,7 @@ xenFormatCPUAllocation(virConfPtr conf, virDomainDefPtr def) > } > > if (cpus && > - xenXMConfigSetString(conf, "cpus", cpus) < 0) > + xenConfigSetString(conf, "cpus", cpus) < 0) > goto cleanup; > > ret = 0; > @@ -1655,37 +1655,37 @@ xenFormatCPUFeatures(virConfPtr conf, virDomainDefPtr def, int xendConfigVersion > size_t i; > > if (STREQ(def->os.type, "hvm")) { > - if (xenXMConfigSetInt(conf, "pae", > - (def->features[VIR_DOMAIN_FEATURE_PAE] == > - VIR_TRISTATE_SWITCH_ON) ? 1 : 0) < 0) > + if (xenConfigSetInt(conf, "pae", > + (def->features[VIR_DOMAIN_FEATURE_PAE] == > + VIR_TRISTATE_SWITCH_ON) ? 1 : 0) < 0) > return -1; > > - if (xenXMConfigSetInt(conf, "acpi", > - (def->features[VIR_DOMAIN_FEATURE_ACPI] == > - VIR_TRISTATE_SWITCH_ON) ? 1 : 0) < 0) > + if (xenConfigSetInt(conf, "acpi", > + (def->features[VIR_DOMAIN_FEATURE_ACPI] == > + VIR_TRISTATE_SWITCH_ON) ? 1 : 0) < 0) > return -1; > > - if (xenXMConfigSetInt(conf, "apic", > - (def->features[VIR_DOMAIN_FEATURE_APIC] == > - VIR_TRISTATE_SWITCH_ON) ? 1 : 0) < 0) > + if (xenConfigSetInt(conf, "apic", > + (def->features[VIR_DOMAIN_FEATURE_APIC] == > + VIR_TRISTATE_SWITCH_ON) ? 1 : 0) < 0) > return -1; > > if (xendConfigVersion >= XEND_CONFIG_VERSION_3_0_4) { > - if (xenXMConfigSetInt(conf, "hap", > - (def->features[VIR_DOMAIN_FEATURE_HAP] == > - VIR_TRISTATE_SWITCH_ON) ? 1 : 0) < 0) > + if (xenConfigSetInt(conf, "hap", > + (def->features[VIR_DOMAIN_FEATURE_HAP] == > + VIR_TRISTATE_SWITCH_ON) ? 1 : 0) < 0) > return -1; > > - if (xenXMConfigSetInt(conf, "viridian", > - (def->features[VIR_DOMAIN_FEATURE_VIRIDIAN] == > - VIR_TRISTATE_SWITCH_ON) ? 1 : 0) < 0) > + if (xenConfigSetInt(conf, "viridian", > + (def->features[VIR_DOMAIN_FEATURE_VIRIDIAN] == > + VIR_TRISTATE_SWITCH_ON) ? 1 : 0) < 0) > return -1; > } > > for (i = 0; i < def->clock.ntimers; i++) { > if (def->clock.timers[i]->name == VIR_DOMAIN_TIMER_NAME_HPET && > def->clock.timers[i]->present != -1 && > - xenXMConfigSetInt(conf, "hpet", def->clock.timers[i]->present) < 0) > + xenConfigSetInt(conf, "hpet", def->clock.timers[i]->present) < 0) > return -1; > } > } > @@ -1698,7 +1698,7 @@ static int > xenFormatEmulator(virConfPtr conf, virDomainDefPtr def) > { > if (def->emulator && > - xenXMConfigSetString(conf, "device_model", def->emulator) < 0) > + xenConfigSetString(conf, "device_model", def->emulator) < 0) > return -1; > > return 0; > @@ -1717,7 +1717,7 @@ xenFormatCDROM(virConfPtr conf, virDomainDefPtr def, int xendConfigVersion) > def->disks[i]->dst && > STREQ(def->disks[i]->dst, "hdc") && > virDomainDiskGetSource(def->disks[i])) { > - if (xenXMConfigSetString(conf, "cdrom", > + if (xenConfigSetString(conf, "cdrom", > virDomainDiskGetSource(def->disks[i])) < 0) > return -1; > break; > @@ -1737,11 +1737,11 @@ xenFormatOS(virConfPtr conf, virDomainDefPtr def) > > if (STREQ(def->os.type, "hvm")) { > char boot[VIR_DOMAIN_BOOT_LAST+1]; > - if (xenXMConfigSetString(conf, "builder", "hvm") < 0) > + if (xenConfigSetString(conf, "builder", "hvm") < 0) > return -1; > > if (def->os.loader && > - xenXMConfigSetString(conf, "kernel", def->os.loader) < 0) > + xenConfigSetString(conf, "kernel", def->os.loader) < 0) > return -1; > > for (i = 0; i < def->os.nBootDevs; i++) { > @@ -1769,29 +1769,29 @@ xenFormatOS(virConfPtr conf, virDomainDefPtr def) > boot[def->os.nBootDevs] = '\0'; > } > > - if (xenXMConfigSetString(conf, "boot", boot) < 0) > + if (xenConfigSetString(conf, "boot", boot) < 0) > return -1; > > /* XXX floppy disks */ > } else { > if (def->os.bootloader && > - xenXMConfigSetString(conf, "bootloader", def->os.bootloader) < 0) > + xenConfigSetString(conf, "bootloader", def->os.bootloader) < 0) > return -1; > > if (def->os.bootloaderArgs && > - xenXMConfigSetString(conf, "bootargs", def->os.bootloaderArgs) < 0) > + xenConfigSetString(conf, "bootargs", def->os.bootloaderArgs) < 0) > return -1; > > if (def->os.kernel && > - xenXMConfigSetString(conf, "kernel", def->os.kernel) < 0) > + xenConfigSetString(conf, "kernel", def->os.kernel) < 0) > return -1; > > if (def->os.initrd && > - xenXMConfigSetString(conf, "ramdisk", def->os.initrd) < 0) > + xenConfigSetString(conf, "ramdisk", def->os.initrd) < 0) > return -1; > > if (def->os.cmdline && > - xenXMConfigSetString(conf, "extra", def->os.cmdline) < 0) > + xenConfigSetString(conf, "extra", def->os.cmdline) < 0) > return -1; > } /* !hvm */ > > @@ -1807,52 +1807,52 @@ xenFormatVfb(virConfPtr conf, virDomainDefPtr def, int xendConfigVersion) > if (def->ngraphics == 1) { > if (hvm || (xendConfigVersion < XEND_CONFIG_MIN_VERS_PVFB_NEWCONF)) { > if (def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_SDL) { > - if (xenXMConfigSetInt(conf, "sdl", 1) < 0) > + if (xenConfigSetInt(conf, "sdl", 1) < 0) > return -1; > > - if (xenXMConfigSetInt(conf, "vnc", 0) < 0) > + if (xenConfigSetInt(conf, "vnc", 0) < 0) > return -1; > > if (def->graphics[0]->data.sdl.display && > - xenXMConfigSetString(conf, "display", > + xenConfigSetString(conf, "display", > def->graphics[0]->data.sdl.display) < 0) > return -1; > > if (def->graphics[0]->data.sdl.xauth && > - xenXMConfigSetString(conf, "xauthority", > + xenConfigSetString(conf, "xauthority", > def->graphics[0]->data.sdl.xauth) < 0) > return -1; > } else { > const char *listenAddr; > > - if (xenXMConfigSetInt(conf, "sdl", 0) < 0) > + if (xenConfigSetInt(conf, "sdl", 0) < 0) > return -1; > > - if (xenXMConfigSetInt(conf, "vnc", 1) < 0) > + if (xenConfigSetInt(conf, "vnc", 1) < 0) > return -1; > > - if (xenXMConfigSetInt(conf, "vncunused", > + if (xenConfigSetInt(conf, "vncunused", > def->graphics[0]->data.vnc.autoport ? 1 : 0) < 0) > return -1; > > if (!def->graphics[0]->data.vnc.autoport && > - xenXMConfigSetInt(conf, "vncdisplay", > - def->graphics[0]->data.vnc.port - 5900) < 0) > + xenConfigSetInt(conf, "vncdisplay", > + def->graphics[0]->data.vnc.port - 5900) < 0) > return -1; > > listenAddr = virDomainGraphicsListenGetAddress(def->graphics[0], 0); > if (listenAddr && > - xenXMConfigSetString(conf, "vnclisten", listenAddr) < 0) > + xenConfigSetString(conf, "vnclisten", listenAddr) < 0) > return -1; > > if (def->graphics[0]->data.vnc.auth.passwd && > - xenXMConfigSetString(conf, "vncpasswd", > - def->graphics[0]->data.vnc.auth.passwd) < 0) > + xenConfigSetString(conf, "vncpasswd", > + def->graphics[0]->data.vnc.auth.passwd) < 0) > return -1; > > if (def->graphics[0]->data.vnc.keymap && > - xenXMConfigSetString(conf, "keymap", > - def->graphics[0]->data.vnc.keymap) < 0) > + xenConfigSetString(conf, "keymap", > + def->graphics[0]->data.vnc.keymap) < 0) > return -1; > } > } else { > @@ -1928,7 +1928,7 @@ xenFormatSound(virConfPtr conf, virDomainDefPtr def) > > str = virBufferContentAndReset(&buf); > if (ret == 0) > - ret = xenXMConfigSetString(conf, "soundhw", str); > + ret = xenConfigSetString(conf, "soundhw", str); > > VIR_FREE(str); > if (ret < 0) > @@ -1948,22 +1948,22 @@ xenFormatInputDevs(virConfPtr conf, virDomainDefPtr def) > if (STREQ(def->os.type, "hvm")) { > for (i = 0; i < def->ninputs; i++) { > if (def->inputs[i]->bus == VIR_DOMAIN_INPUT_BUS_USB) { > - if (xenXMConfigSetInt(conf, "usb", 1) < 0) > + if (xenConfigSetInt(conf, "usb", 1) < 0) > return -1; > > switch (def->inputs[i]->type) { > case VIR_DOMAIN_INPUT_TYPE_MOUSE: > - if (xenXMConfigSetString(conf, "usbdevice", "mouse") < 0) > + if (xenConfigSetString(conf, "usbdevice", "mouse") < 0) > return -1; > > break; > case VIR_DOMAIN_INPUT_TYPE_TABLET: > - if (xenXMConfigSetString(conf, "usbdevice", "tablet") < 0) > + if (xenConfigSetString(conf, "usbdevice", "tablet") < 0) > return -1; > > break; > case VIR_DOMAIN_INPUT_TYPE_KBD: > - if (xenXMConfigSetString(conf, "usbdevice", "keyboard") < 0) > + if (xenConfigSetString(conf, "usbdevice", "keyboard") < 0) > return -1; > > break; > diff --git a/src/xenconfig/xen_common.h b/src/xenconfig/xen_common.h > index 9f50aef..e993b21 100644 > --- a/src/xenconfig/xen_common.h > +++ b/src/xenconfig/xen_common.h > @@ -27,11 +27,25 @@ > # include "virconf.h" > # include "domain_conf.h" > > -int > -xenConfigGetString(virConfPtr conf, > - const char *name, > - const char **value, > - const char *def); > +int xenConfigGetString(virConfPtr conf, > + const char *name, > + const char **value, > + const char *def); > + > +int xenConfigGetBool(virConfPtr conf, const char *name, int *value, int def); > + > +int xenConfigSetInt(virConfPtr conf, const char *name, long long value); > + > +int xenConfigSetString(virConfPtr conf, const char *setting, const char *value); > + > +int xenConfigGetULong(virConfPtr conf, > + const char *name, > + unsigned long *value, > + unsigned long def); > + > +int xenConfigCopyStringOpt(virConfPtr conf, > + const char *name, > + char **value); > > int xenParseConfigCommon(virConfPtr conf, > virDomainDefPtr def, > diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c > new file mode 100644 > index 0000000..294b82d > --- /dev/null > +++ b/src/xenconfig/xen_xl.c > @@ -0,0 +1,461 @@ > +/* > + * xen_xl.c: Xen XL parsing functions > + * > + * This library is free software; you can redistribute it and/or > + * modify it under the terms of the GNU Lesser General Public > + * License as published by the Free Software Foundation; either > + * version 2.1 of the License, or (at your option) any later version. > + * > + * This library is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * Lesser General Public License for more details. > + * > + * You should have received a copy of the GNU Lesser General Public > + * License along with this library. If not, see > + * <http://www.gnu.org/licenses/>. > + * > + * Author: Kiarie Kahurani <davidkiarie4@xxxxxxxxx> > + */ > +#include <config.h> > +#include "virconf.h" > +#include "domain_conf.h" > +#include "viralloc.h" > +#include "virstring.h" > +#include "xen_xl.h" > + > +#define VIR_FROM_THIS VIR_FROM_NONE > + > +static int > +xenParseXLSpice(virConfPtr conf, virDomainDefPtr def) > +{ > + virDomainGraphicsDefPtr graphics = NULL; > + unsigned long port; > + char *listenAddr = NULL; > + int val = 0; > No need to initialize val. > + > + if (STREQ(def->os.type, "hvm")) { > + if (xenConfigGetBool(conf, "spice", &val, 0) < 0) > + return -1; > + > + if (val) { > + if (VIR_ALLOC(graphics) < 0) > + return -1; > + > + graphics->type = VIR_DOMAIN_GRAPHICS_TYPE_SPICE; > + if (xenConfigCopyStringOpt(conf, "spicehost", &listenAddr) < 0) > + goto cleanup; > + if (listenAddr && > + virDomainGraphicsListenSetAddress(graphics, 0, listenAddr, > + -1, true) < 0) { > + goto cleanup; > + } > + > + VIR_FREE(listenAddr); > The more common pattern is to have a blank line after VIR_FREE, not before. > + if (xenConfigGetULong(conf, "spicetls_port", &port, 0) < 0) > + goto cleanup; > + graphics->data.spice.tlsPort = (int)port; > + > + if (xenConfigGetULong(conf, "spiceport", &port, 0) < 0) > + goto cleanup; > + > + graphics->data.spice.port = (int)port; > + > + if (!graphics->data.spice.tlsPort && > + !graphics->data.spice.port) > This 'if' statement fits on one line. > + graphics->data.spice.autoport = 1; > + > + if (xenConfigGetBool(conf, "spicedisable_ticketing", &val, 0) < 0) > + goto cleanup; > + if (val) { > + if (xenConfigCopyStringOpt(conf, "spicepasswd", > + &graphics->data.spice.auth.passwd) < 0) > + goto cleanup; > + } > + > + if (xenConfigGetBool(conf, "spiceagent_mouse", > + &graphics->data.spice.mousemode, 0) < 0) > + goto cleanup; > + if (xenConfigGetBool(conf, "spicedvagent", &val, 0) < 0) > + goto cleanup; > + if (val) { > + if (xenConfigGetBool(conf, "spice_clipboard_sharing", > + &graphics->data.spice.copypaste, > + 0) < 0) > + goto cleanup; > + } > + > + if (VIR_ALLOC_N(def->graphics, 1) < 0) > + goto cleanup; > + def->graphics[0] = graphics; > + def->ngraphics = 1; > + graphics = NULL; > No need to set graphics NULL here since we are just going to fall through and return success. > + } > + } > + > + return 0; > + > + cleanup: > + virDomainGraphicsDefFree(graphics); > + return -1; > +} > + > + > +static int > +xenParseXLDisk(virConfPtr conf, virDomainDefPtr def) > You should include some comments about the xl disk format. If nothing else, just reference docs/misc/xl-disk-configuration.txt in the Xen sources. > +{ > + virDomainDiskDefPtr disk = NULL; > + virConfValuePtr list = virConfGetValue(conf, "disk"); > + > + if (list && list->type == VIR_CONF_LIST) { > + list = list->list; > + while (list) { > + char *head; > + char *offset; > + char *tmp; > + > + if ((list->type != VIR_CONF_STRING) || (list->str == NULL)) > + goto skipdisk; > + > + head = list->str; > + if (!(disk = virDomainDiskDefNew())) > + return -1; > + > + if (!(offset = strchr(head, ','))) > + goto skipdisk; > + > + if (offset == head) { > + ignore_value(virDomainDiskSetSource(disk, NULL)); > + } else { > + if (VIR_STRNDUP(tmp, head, offset - head) < 0) > + goto cleanup; > + > + if (virDomainDiskSetSource(disk, tmp) < 0) { > + VIR_FREE(tmp); > + goto cleanup; > + } > + > + VIR_FREE(tmp); > + } > + > + head = (offset + 1); > + > + if (!(offset = strchr(head, ','))) > + goto skipdisk; > + > + if (VIR_STRNDUP(tmp, head, offset - head) < 0) > + goto cleanup; > + > + if (STREQ(tmp, "raw")) { > + virDomainDiskSetFormat(disk, VIR_STORAGE_FILE_RAW); > + > + } else if (STREQ(tmp, "qcow2")) { > + virDomainDiskSetFormat(disk, VIR_STORAGE_FILE_QCOW2); > + > + } else if (STREQ(tmp, "qcow")) { > + virDomainDiskSetFormat(disk, VIR_STORAGE_FILE_QCOW); > + > + } else if (STREQ(tmp, "xvhd")) { > + virDomainDiskSetFormat(disk, VIR_STORAGE_FILE_VHD); > + } else { > + virDomainDiskSetFormat(disk, VIR_STORAGE_FILE_RAW); > + } > This isn't quite right since you are only considering positional parameters, and ignoring keyword/value pairs. E.g. your example disk configuration in the test data file disk = [ "/dev/HostVG/XenGuest2,raw,hda,w,", "/root/boot.iso,raw,hdc,r,cdrom" ] could be written equally as disk = [ "format=raw, vdev=hda, access=rw, target=/dev/HostVG/XenGuest2", "format=raw, vdev=hdc, access=ro, devtype=cdrom, target=/root/boot.iso" ] xl/libxl contains a flex-based analyzer for parsing the disk config. Perhaps that code could be used since it is already hardened and tested. It is internal to xl/libxl, so would have to be copied into libvirt. I see libvirt doesn't currently use bison or flex, but not sure if that is due to lack of need or an aversion to using these tools in the project. We can see what other libvirt devs have to say about it. > + > + VIR_FREE(tmp); > + head = offset + 1; > + > + if (!(offset = strchr(head, ','))) > + goto skipdisk; > + > + if (VIR_ALLOC_N(disk->dst, (offset - head) + 1) < 0) > + goto cleanup; > + > + if (virStrncpy(disk->dst, head, offset - head, > + (offset - head) + 1) == NULL) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("Dest device %s too big for destination"), head); > + goto cleanup; > + } > + > + head = offset + 1; > + > + if (!(offset = strchr(head, ','))) > + goto skipdisk; > + > + if (VIR_STRNDUP(tmp, head, offset - head) < 0) > + goto cleanup; > + > + if (STREQ(tmp, "r") || STREQ(tmp, "ro")) > + disk->src->readonly = true; > + else if ((STREQ(tmp, "w!")) || (STREQ(tmp, "!"))) > + disk->src->shared = true; > + > + switch (virDomainDiskGetFormat(disk)) { > + > + case VIR_STORAGE_FILE_RAW: > + ignore_value(virDomainDiskSetDriver(disk, "phy")); > + break; > + case VIR_STORAGE_FILE_QCOW: > + case VIR_STORAGE_FILE_QCOW2: > + ignore_value(virDomainDiskSetDriver(disk, "qemu")); > + case VIR_STORAGE_FILE_VHD: > + ignore_value(virDomainDiskSetDriver(disk, "tap")); > + } > + > + virDomainDiskSetType(disk, STREQ(virDomainDiskGetDriver(disk), "phy") ? > + VIR_STORAGE_TYPE_BLOCK: > + VIR_STORAGE_TYPE_FILE); > + > + head = offset + 1; > + > + if (!(offset = strchr(head, ',')) < 0) > + goto skipdisk; > + > + if (VIR_STRNDUP(tmp, head, (offset - head)) < 0) > + goto cleanup; > + > + if (STREQ(tmp, "cdrom")) { > + disk->device = VIR_DOMAIN_DISK_DEVICE_CDROM; > + } else { > + disk->device = VIR_DOMAIN_DISK_DEVICE_DISK; > + } > + > + if (STRPREFIX(disk->dst, "xvhd") || !STREQ(def->os.type, "hvm")) { > + disk->bus = VIR_DOMAIN_DISK_BUS_XEN; > + } else if (STRPREFIX(disk->dst, "sd")) { > + disk->bus = VIR_DOMAIN_DISK_BUS_SCSI; > + } else { > + disk->bus = VIR_DOMAIN_DISK_BUS_IDE; > + } > + > + if (VIR_APPEND_ELEMENT(def->disks, def->ndisks, disk) < 0) > + return -1; > + > + skipdisk: > + list = list->next; > + virDomainDiskDefFree(disk); > + } > + } > + > + return 0; > + > + cleanup: > + virDomainDiskDefFree(disk); > + return -1; > +} > + > + > +virDomainDefPtr > +xenParseXL(virConfPtr conf, virCapsPtr caps, > + int xendConfigVersion) > +{ > + virDomainDefPtr def = NULL; > + > + if (VIR_ALLOC(def) < 0) > + return NULL; > + > + def->virtType = VIR_DOMAIN_VIRT_XEN; > + def->id = -1; > + > + if (xenParseConfigCommon(conf, def, caps, xendConfigVersion) < 0) > + goto cleanup; > + > + if (xenParseXLDisk(conf, def) < 0) > + goto cleanup; > + > + if (xenParseXLSpice(conf, def) < 0) > + goto cleanup; > + > + return def; > + > + cleanup: > + virDomainDefFree(def); > + return NULL; > +} > + > + > +static int > +xenFormatXLDisk(virConfValuePtr list, virDomainDiskDefPtr disk) > +{ > + virBuffer buf = VIR_BUFFER_INITIALIZER; > + virConfValuePtr val, tmp; > + const char *src = virDomainDiskGetSource(disk); > + int format = virDomainDiskGetFormat(disk); > + > + /* target */ > + virBufferAsprintf(&buf, "%s,", src); > + /* format */ > + switch (format) { > + case VIR_STORAGE_FILE_RAW: > + virBufferAddLit(&buf, "raw,"); > + break; > + case VIR_STORAGE_FILE_VHD: > + virBufferAddLit(&buf, "xvhd,"); > + break; > + case VIR_STORAGE_FILE_QCOW: > + virBufferAddLit(&buf, "qcow,"); > + break; > + case VIR_STORAGE_FILE_QCOW2: > + virBufferAddLit(&buf, "qcow2,"); > + break; > + /* set default */ > + default: > + virBufferAddLit(&buf, "raw,"); > + } > + > + /* device */ > + virBufferAdd(&buf, disk->dst, -1); > + > + virBufferAddLit(&buf, ","); > + > + if (disk->src->readonly) > + virBufferAddLit(&buf, "r,"); > + else if (disk->src->shared) > + virBufferAddLit(&buf, "!,"); > + else > + virBufferAddLit(&buf, "w,"); > + if (disk->transient) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("transient disks not supported yet")); > + goto cleanup; > + } > + > + if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) > + virBufferAddLit(&buf, "cdrom"); > + > + if (virBufferCheckError(&buf) < 0) > + goto cleanup; > + > + if (VIR_ALLOC(val) < 0) > + goto cleanup; > + > + val->type = VIR_CONF_STRING; > + val->str = virBufferContentAndReset(&buf); > + tmp = list->list; > + while (tmp && tmp->next) > + tmp = tmp->next; > + if (tmp) > + tmp->next = val; > + else > + list->list = val; > + return 0; > + > + cleanup: > + virBufferFreeAndReset(&buf); > + return -1; > +} > + > + > +static int > +xenFormatXLDomainDisks(virConfPtr conf, virDomainDefPtr def) > +{ > + virConfValuePtr diskVal = NULL; > + size_t i = 0; > + > + if (VIR_ALLOC(diskVal) < 0) > + return -1; > + > + diskVal->type = VIR_CONF_LIST; > + diskVal->list = NULL; > + > + for (i = 0; i < def->ndisks; i++) { > + if (def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) > + continue; > + if (xenFormatXLDisk(diskVal, def->disks[i]) < 0) > + > + goto cleanup; > + } > + > + if (diskVal->list != NULL) { > + int ret = virConfSetValue(conf, "disk", diskVal); > + diskVal = NULL; > + if (ret < 0) > + goto cleanup; > + } > + > + return 0; > + > + cleanup: > + virConfFreeValue(diskVal); > + return 0; > +} > + > + > +static int > +xenFormatXLSpice(virConfPtr conf, virDomainDefPtr def) > +{ > + const char *listenAddr = NULL; > + > + if (STREQ(def->os.type, "hvm")) { /*save's CPU :-) */ > + if (def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) { > + /* set others to false but may not be necessary */ > + if (xenConfigSetInt(conf, "sdl", 0) < 0) > + return -1; > + > + if (xenConfigSetInt(conf, "vnc", 0) < 0) > + return -1; > + > + if (xenConfigSetInt(conf, "spice", 1) < 0) > + return -1; > + > + if (xenConfigSetInt(conf, "spiceport", > + def->graphics[0]->data.spice.port) < 0) > + return -1; > + > + if (xenConfigSetInt(conf, "spicetls_port", > + def->graphics[0]->data.spice.tlsPort) < 0) > + return -1; > + > + if (def->graphics[0]->data.spice.auth.passwd && > + xenConfigSetString(conf, "spicepasswd", > + def->graphics[0]->data.spice.auth.passwd) < 0) > + return -1; > + > + listenAddr = virDomainGraphicsListenGetAddress(def->graphics[0], 0); > + if (listenAddr && > + xenConfigSetString(conf, "spice_host", listenAddr) < 0) { > + return -1; > + } > + > + if (xenConfigSetInt(conf, "spice_mouse_agent", > + def->graphics[0]->data.spice.mousemode) < 0) > + return -1; > + > + if (xenConfigSetInt(conf, "disable_ticketing", > + def->graphics[0]->data.spice.mousemode) < 0) > + return -1; > + > + if (xenConfigSetInt(conf, "spice_clipboard_sharing", > + def->graphics[0]->data.spice.copypaste) < 0) > + return -1; > + } > + } > + > + return 0; > +} > + > + > +virConfPtr xenFormatXL(virDomainDefPtr def, virConnectPtr conn, > + int xendConfigVersion) > Return type and function name on a separate line. > +{ > + virConfPtr conf = NULL; > + > + if (!(conf = virConfNew())) > + goto cleanup; > + > + if (xenFormatConfigCommon(conf, def, conn, xendConfigVersion) < 0) > + goto cleanup; > + > + if (xenFormatXLDomainDisks(conf, def) < 0) > + goto cleanup; > + > + if (xenFormatXLSpice(conf, def) < 0) > + goto cleanup; > + > + return conf; > + > + cleanup: > + if (conf) > + virConfFree(conf); > + return NULL; > +} > diff --git a/src/xenconfig/xen_xl.h b/src/xenconfig/xen_xl.h > new file mode 100644 > index 0000000..d60fe5e > --- /dev/null > +++ b/src/xenconfig/xen_xl.h > @@ -0,0 +1,29 @@ > +/* > + * xen_xl.h: Xen XL parsing functions > + * > + * This library is free software; you can redistribute it and/or > + * modify it under the terms of the GNU Lesser General Public > + * License as published by the Free Software Foundation; either > + * version 2.1 of the License, or (at your option) any later version. > + * > + * This library is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * Lesser General Public License for more details. > + * > + * You should have received a copy of the GNU Lesser General Public > + * License along with this library. If not, see > + * <http://www.gnu.org/licenses/>. > + * > + * Author: Kiarie Kahurani<davidkiarie4@xxxxxxxxx> > + */ > +#ifndef _XEN_XL_H_ > +# define _XEN_XL_H_ > + > +# include "xen_common.h" > + > +virDomainDefPtr xenParseXL(virConfPtr conn, virCapsPtr caps, > + int xendConfigVersion); > +virConfPtr xenFormatXL(virDomainDefPtr def, > + virConnectPtr, int xendConfigVersion); > I think it would be best to put the additional parameters after the first on separate lines, like you did with the convenience functions in xen_common.h. > +#endif /* _XEN_XL_H_ */ > diff --git a/tests/Makefile.am b/tests/Makefile.am > index 3e71069..aa18a4a 100644 > --- a/tests/Makefile.am > +++ b/tests/Makefile.am > @@ -136,6 +136,7 @@ EXTRA_DIST = \ > vmx2xmldata \ > xencapsdata \ > xmconfigdata \ > + xlconfigdata \ > xml2sexprdata \ > xml2vmxdata \ > vmwareverdata \ > @@ -220,7 +221,8 @@ ssh_LDADD = $(COVERAGE_LDFLAGS) > > if WITH_XEN > test_programs += xml2sexprtest sexpr2xmltest \ > - xmconfigtest xencapstest statstest reconnect > + xmconfigtest xencapstest statstest reconnect \ > + xlconfigtest > No need for the line continuation. xlconfigtest can be added to the end of the existing line. > endif WITH_XEN > if WITH_QEMU > test_programs += qemuxml2argvtest qemuxml2xmltest qemuxmlnstest \ > @@ -468,6 +470,11 @@ sexpr2xmltest_SOURCES = \ > testutils.c testutils.h > sexpr2xmltest_LDADD = $(xen_LDADDS) > > +xlconfigtest_SOURCES = \ > + xlconfigtest.c testutilsxen.c testutilsxen.h \ > + testutils.c testutils.h > +xlconfigtest_LDADD =$(xen_LDADDS) > + > xmconfigtest_SOURCES = \ > xmconfigtest.c testutilsxen.c testutilsxen.h \ > testutils.c testutils.h > diff --git a/tests/testutilsxen.c b/tests/testutilsxen.c > index a50a8a2..df1d124 100644 > --- a/tests/testutilsxen.c > +++ b/tests/testutilsxen.c > @@ -69,3 +69,53 @@ virCapsPtr testXenCapsInit(void) > virObjectUnref(caps); > return NULL; > } > + > + > +virCapsPtr > +testXLInitCaps(void) > +{ > + virCapsPtr caps; > + virCapsGuestPtr guest; > + virCapsGuestMachinePtr *machines; > + int nmachines; > + static const char *const x86_machines[] = { > + "xenfv" > + }; > + static const char *const xen_machines[] = { > + "xenpv" > + }; > + > + if ((caps = virCapabilitiesNew(virArchFromHost(), > + false, false)) == NULL) > + return NULL; > + nmachines = ARRAY_CARDINALITY(x86_machines); > + if ((machines = virCapabilitiesAllocMachines(x86_machines, nmachines)) == NULL) > + goto cleanup; > + if ((guest = virCapabilitiesAddGuest(caps, "hvm", VIR_ARCH_X86_64, > + "/usr/lib/xen/bin/qemu-dm", NULL, > + nmachines, machines)) == NULL) > + goto cleanup; > + machines = NULL; > + if (virCapabilitiesAddGuestDomain(guest, "xen", NULL, > + NULL, 0, NULL) == NULL) > + goto cleanup; > + nmachines = ARRAY_CARDINALITY(xen_machines); > + if ((machines = virCapabilitiesAllocMachines(xen_machines, nmachines)) == NULL) > + goto cleanup; > + > + if ((guest = virCapabilitiesAddGuest(caps, "xen", VIR_ARCH_X86_64, > + "/usr/lib/xen/bin/qemu-dm", NULL, > + nmachines, machines)) == NULL) > + goto cleanup; > + machines = NULL; > + > + if (virCapabilitiesAddGuestDomain(guest, "xen", NULL, > + NULL, 0, NULL) == NULL) > + goto cleanup; > + return caps; > + > + cleanup: > + virCapabilitiesFreeMachines(machines, nmachines); > + virObjectUnref(caps); > + return NULL; > +} > diff --git a/tests/testutilsxen.h b/tests/testutilsxen.h > index 54155e5..c78350d 100644 > --- a/tests/testutilsxen.h > +++ b/tests/testutilsxen.h > @@ -1,3 +1,10 @@ > -#include "capabilities.h" > +#ifndef _TESTUTILSXEN_H_ > +# define _TESTUTILSXEN_H_ > + > +# include "capabilities.h" > > virCapsPtr testXenCapsInit(void); > + > +virCapsPtr testXLInitCaps(void); > + > +#endif /* _TESTUTILSXEN_H_ */ > diff --git a/tests/xlconfigdata/test-fullvirt-new-disk.cfg b/tests/xlconfigdata/test-fullvirt-new-disk.cfg > new file mode 100644 > index 0000000..2e76625 > --- /dev/null > +++ b/tests/xlconfigdata/test-fullvirt-new-disk.cfg > @@ -0,0 +1,27 @@ > +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 > +rtc_timeoffset = 0 > +localtime = 1 > +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" > +vif = [ "mac=00:16:3e:66:92:9c,bridge=xenbr1,script=vif-bridge,model=e1000" ] > +parallel = "none" > +serial = "none" > +disk = [ "/dev/HostVG/XenGuest2,raw,hda,w,", "/root/boot.iso,raw,hdc,r,cdrom" ] > diff --git a/tests/xlconfigdata/test-fullvirt-new-disk.xml b/tests/xlconfigdata/test-fullvirt-new-disk.xml > We should have test files containing the newer format (everything specified with key/value pairs) too. Looking good though. Thanks for working on this David! Regards, Jim -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list