On 01/10/2015 12:03 AM, Jim Fehlig wrote: > Introduce a parser/formatter for the xl config format. Since the > deprecation of xm/xend, the VM config file format has diverged as > new features are added to libxl. This patch adds support for parsing > and formating the xl config format. It supports the existing xm config > format, plus adds support for spice graphics and xl disk config syntax. > > Disk config is specified a bit differently in xl as compared to xm. In > xl, disk config consists of comma-separated positional parameters and > keyword/value pairs separated by commas. Positional parameters are > specified as follows > > target, format, vdev, access > > Supported keys for key=value options are > > devtype, backend, backendtype, script, direct-io-safe, > > The positional paramters can also be specified in key/value form. For > example the following xl disk config are equivalent > > /dev/vg/guest-volume,,hda > /dev/vg/guest-volume,raw,hda,rw > format=raw, vdev=hda, access=rw, target=/dev/vg/guest-volume > > See $xen_sources/docs/misc/xl-disk-configuration.txt for more details. > > xl disk config is parsed with the help of xlu_disk_parse() from > libxlutil, libxl's utility library. Although the library exists > in all Xen versions supported by the libxl virt driver, only > recently has the corresponding header file been included. A check > for the header is done in configure.ac. If not found, xlu_disk_parse() > is declared externally. > > Signed-off-by: Kiarie Kahurani <davidkiarie4@xxxxxxxxx> > Signed-off-by: Jim Fehlig <jfehlig@xxxxxxxx> > --- > configure.ac | 3 + > po/POTFILES.in | 1 + > src/Makefile.am | 4 +- > src/libvirt_xenconfig.syms | 4 + > src/xenconfig/xen_common.c | 3 +- > src/xenconfig/xen_xl.c | 492 +++++++++++++++++++++++++++++++++++++++++++++ > src/xenconfig/xen_xl.h | 33 +++ > 7 files changed, 538 insertions(+), 2 deletions(-) > The following is just from the Coverity check... I don't have all the build environments that have proved to be problematic over the last week or so... I assume all you've done is take the generated code and use that rather than going through the problems as a result of attempting to generate the code. > diff --git a/configure.ac b/configure.ac > index 9d12079..f370475 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -891,6 +891,9 @@ if test $fail = 1; then > fi > > if test "$with_libxl" = "yes"; then > + dnl If building with libxl, use the libxl utility header and lib too > + AC_CHECK_HEADERS([libxlutil.h]) > + LIBXL_LIBS="$LIBXL_LIBS -lxlutil" > AC_DEFINE_UNQUOTED([WITH_LIBXL], 1, [whether libxenlight driver is enabled]) > fi > AM_CONDITIONAL([WITH_LIBXL], [test "$with_libxl" = "yes"]) > diff --git a/po/POTFILES.in b/po/POTFILES.in > index e7cb2cc..094c8e3 100644 > --- a/po/POTFILES.in > +++ b/po/POTFILES.in > @@ -247,6 +247,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 > tests/virpolkittest.c > tools/libvirt-guests.sh.in > diff --git a/src/Makefile.am b/src/Makefile.am > index e0e47d0..875fb5e 100644 > --- a/src/Makefile.am > +++ b/src/Makefile.am > @@ -1004,7 +1004,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 \ > + xenconfig/xen_xl.c xenconfig/xen_xl.h > > pkgdata_DATA = cpu/cpu_map.xml > > @@ -1061,6 +1062,7 @@ endif WITH_VMX > if WITH_XENCONFIG > noinst_LTLIBRARIES += libvirt_xenconfig.la > libvirt_la_BUILT_LIBADD += libvirt_xenconfig.la > +libvirt_la_LIBADD += $(LIBXL_LIBS) > libvirt_xenconfig_la_CFLAGS = \ > -I$(srcdir)/conf $(AM_CFLAGS) > libvirt_xenconfig_la_SOURCES = $(XENCONFIG_SOURCES) > diff --git a/src/libvirt_xenconfig.syms b/src/libvirt_xenconfig.syms > index 6541685..3e2e5d6 100644 > --- a/src/libvirt_xenconfig.syms > +++ b/src/libvirt_xenconfig.syms > @@ -16,6 +16,10 @@ xenParseSxprChar; > xenParseSxprSound; > xenParseSxprString; > > +#xenconfig/xen_xl.h > +xenFormatXL; > +xenParseXL; > + > # xenconfig/xen_xm.h > xenFormatXM; > xenParseXM; > diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c > index b40a722..a2a1474 100644 > --- a/src/xenconfig/xen_common.c > +++ b/src/xenconfig/xen_common.c > @@ -1812,7 +1812,8 @@ xenFormatVfb(virConfPtr conf, virDomainDefPtr def, int xendConfigVersion) > { > int hvm = STREQ(def->os.type, "hvm") ? 1 : 0; > > - if (def->ngraphics == 1) { > + if (def->ngraphics == 1 && > + def->graphics[0]->type != VIR_DOMAIN_GRAPHICS_TYPE_SPICE) { > if (hvm || (xendConfigVersion < XEND_CONFIG_MIN_VERS_PVFB_NEWCONF)) { > if (def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_SDL) { > if (xenConfigSetInt(conf, "sdl", 1) < 0) > diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c > new file mode 100644 > index 0000000..10027c1 > --- /dev/null > +++ b/src/xenconfig/xen_xl.c > @@ -0,0 +1,492 @@ > +/* > + * xen_xl.c: Xen XL parsing functions Should there be copyright date here? > + * > + * 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 <libxl.h> > + > +#include "virconf.h" > +#include "virerror.h" > +#include "domain_conf.h" > +#include "viralloc.h" > +#include "virstring.h" > +#include "virstoragefile.h" > +#include "xen_xl.h" > + > +#define VIR_FROM_THIS VIR_FROM_NONE > + > +/* > + * Xen provides a libxl utility library, with several useful functions, > + * specifically xlu_disk_parse for parsing xl disk config strings. > + * Although the libxlutil library is installed, until recently the > + * corresponding header file wasn't. Use the header file if detected during > + * configure, otherwise provide extern declarations for any functions used. > + */ > +#ifdef HAVE_LIBXLUTIL_H > +# include <libxlutil.h> > +#else > +typedef struct XLU_Config XLU_Config; > + > +extern XLU_Config *xlu_cfg_init(FILE *report, > + const char *report_filename); > + > +extern int xlu_disk_parse(XLU_Config *cfg, > + int nspecs, > + const char *const *specs, > + libxl_device_disk *disk); > +#endif > + > +static int > +xenParseXLSpice(virConfPtr conf, virDomainDefPtr def) > +{ > + virDomainGraphicsDefPtr graphics = NULL; > + unsigned long port; > + char *listenAddr = NULL; > + int 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); > + > + 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) > + 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; > + } > + } > + > + return 0; > + > + cleanup: > + virDomainGraphicsDefFree(graphics); > + return -1; > +} > + > +/* > + * positional parameters > + * (If the <diskspec> strings are not separated by "=" > + * the string is split following ',' and assigned to > + * the following options in the following order) > + * target,format,vdev,access > + * ================================================================ > + * > + * The parameters below cannot be specified as positional parameters: > + * > + * other parameters > + * devtype = <devtype> > + * backendtype = <backend-type> > + * parameters not taken care of > + * backend = <domain-name> > + * script = <script> > + * direct-io-safe > + * > + * ================================================================ > + * The parser does not take any deprecated parameters > + * > + * For more information refer to /xen/docs/misc/xl-disk-configuration.txt > + */ > +static int > +xenParseXLDisk(virConfPtr conf, virDomainDefPtr def) > +{ > + virConfValuePtr list = virConfGetValue(conf, "disk"); > + XLU_Config *xluconf; > + virDomainDiskDefPtr disk; > + libxl_device_disk *libxldisk; > + > + if (VIR_ALLOC(libxldisk) < 0) > + return -1; > + > + if (!(xluconf = xlu_cfg_init(stderr, "command line"))) > + return -1; This leaks libxldisk > + > + if (list && list->type == VIR_CONF_LIST) { > + list = list->list; > + while (list) { > + const char *disk_spec = list->str; > + > + if ((list->type != VIR_CONF_STRING) || (list->str == NULL)) > + goto skipdisk; > + > + libxl_device_disk_init(libxldisk); > + > + if (xlu_disk_parse(xluconf, 1, &disk_spec, libxldisk)) Will this allocate anything into libxldisk? That will eventually be cleared out by the libxl_device_disk_init() on the next pass? I note numberous strdup's of libxkdisk->* fields, so I am assuming that there's been an allocation, although I suppose it could use pointers to > + goto skipdisk; > + > + if (!(disk = virDomainDiskDefNew())) > + return -1; > + > + if (VIR_STRDUP(disk->dst, libxldisk->vdev) < 0) > + goto fail; > + > + if (virDomainDiskSetSource(disk, libxldisk->pdev_path) < 0) > + goto fail; > + > + disk->src->readonly = libxldisk->readwrite ? 0 : 1; > + disk->removable = libxldisk->removable; > + > + if (libxldisk->is_cdrom) { > + if (virDomainDiskSetDriver(disk, "qemu") < 0) > + goto fail; > + > + virDomainDiskSetType(disk, VIR_STORAGE_TYPE_FILE); > + disk->device = VIR_DOMAIN_DISK_DEVICE_CDROM; > + if (!disk->src->path || STREQ(disk->src->path, "")) > + disk->src->format = VIR_STORAGE_FILE_NONE; > + else > + disk->src->format = VIR_STORAGE_FILE_RAW; > + } else { > + switch (libxldisk->format) { > + case LIBXL_DISK_FORMAT_QCOW: > + disk->src->format = VIR_STORAGE_FILE_QCOW; > + break; > + > + case LIBXL_DISK_FORMAT_QCOW2: > + disk->src->format = VIR_STORAGE_FILE_QCOW2; > + break; > + > + case LIBXL_DISK_FORMAT_VHD: > + disk->src->format = VIR_STORAGE_FILE_VHD; > + break; > + > + case LIBXL_DISK_FORMAT_RAW: > + case LIBXL_DISK_FORMAT_UNKNOWN: > + disk->src->format = VIR_STORAGE_FILE_RAW; > + break; > + > + case LIBXL_DISK_FORMAT_EMPTY: > + break; > + } > + > + switch (libxldisk->backend) { > + case LIBXL_DISK_BACKEND_QDISK: > + case LIBXL_DISK_BACKEND_UNKNOWN: > + if (virDomainDiskSetDriver(disk, "qemu") < 0) > + goto fail; > + virDomainDiskSetType(disk, VIR_STORAGE_TYPE_FILE); > + break; > + > + case LIBXL_DISK_BACKEND_TAP: > + if (virDomainDiskSetDriver(disk, "tap") < 0) > + goto fail; > + virDomainDiskSetType(disk, VIR_STORAGE_TYPE_FILE); > + break; > + > + case LIBXL_DISK_BACKEND_PHY: > + if (virDomainDiskSetDriver(disk, "phy") < 0) > + goto fail; > + virDomainDiskSetType(disk, VIR_STORAGE_TYPE_BLOCK); > + break; > + } > + } > + > + if (STRPREFIX(libxldisk->vdev, "xvd") || !STREQ(def->os.type, "hvm")) > + disk->bus = VIR_DOMAIN_DISK_BUS_XEN; > + else if (STRPREFIX(libxldisk->vdev, "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) > + goto fail; > + > + skipdisk: > + list = list->next; > + } > + } Coverity notes that libxldisk is leaked here especially if list == NULL (or list->type != VIR_CONF_LIST). Although I assume by inspection that it'll be leaked whether the list finds something or not. > + return 0; > + > + fail: > + virDomainDiskDefFree(disk); I would think the libxldisk needs to be addressed here as well although Coverity didn't > + 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); > + const char *driver = virDomainDiskGetDriver(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 (STREQ_NULLABLE(driver, "qemu")) > + virBufferAddLit(&buf, "backendtype=qdisk"); > + else if (STREQ_NULLABLE(driver, "tap")) > + virBufferAddLit(&buf, "backendtype=tap"); > + else if (STREQ_NULLABLE(driver, "phy")) > + virBufferAddLit(&buf, "backendtype=phy"); > + > + if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) > + virBufferAddLit(&buf, ",devtype=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; > + } > + Coverity complains that 'diskVal' going out of scope leaks what it points to - especially if "diskVal->list != NULL" takes the false branch above. This is something I pointed out in patch 5 of 6 in my Coverity series: http://www.redhat.com/archives/libvir-list/2015-January/msg00251.html Seems there should be a virConfFreeValue(diskVal) or VIR_FREE(diskVal) here and... > + return 0; > + > + cleanup: > + virConfFreeValue(diskVal); > + return 0; shouldn't this be return -1? > +} > + > + > +static int > +xenFormatXLSpice(virConfPtr conf, virDomainDefPtr def) > +{ > + const char *listenAddr = NULL; > + > + if (STREQ(def->os.type, "hvm")) { > + 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) { > + if (xenConfigSetInt(conf, "spicedisable_ticketing", 1) < 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, "spicehost", listenAddr) < 0) > + return -1; > + > + if (xenConfigSetInt(conf, "spicemouse_mouse", > + def->graphics[0]->data.spice.mousemode) < 0) > + return -1; > + > + if (def->graphics[0]->data.spice.copypaste) { > + if (xenConfigSetInt(conf, "spicedvagent", 1) < 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) > +{ > + 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..536e9b7 > --- /dev/null > +++ b/src/xenconfig/xen_xl.h > @@ -0,0 +1,33 @@ > +/* > + * xen_xl.h: Xen XL parsing functions > + * Copyright date stuff here too... John > + * 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 __VIR_XEN_XL_H__ > +# define __VIR_XEN_XL_H__ > + > +# include "virconf.h" > +# include "domain_conf.h" > +# include "xen_common.h" > + > +virDomainDefPtr xenParseXL(virConfPtr conn, virCapsPtr caps, > + int xendConfigVersion); > +virConfPtr xenFormatXL(virDomainDefPtr def, > + virConnectPtr, int xendConfigVersion); > + > +#endif /* __VIR_XEN_XL_H__ */ > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list