7:32 p.m.
John Ferlan wrote:
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(a)gmail.com>
> Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
> ---
> 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?
Don't know, but I'll add one.
> + *
> + * 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(a)gmail.com>
> + */
> +
> +#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
Thanks. I need to use libxl_device_disk_dispose. I also need to
dispose of the XLU_Config object. I've reworked this function a bit to
address this and other issues you've pointed out.
Regards,
Jim