Re: [libvirt] [PATCH 10/12] Introduce support for parsing/formatting Xen xl config format

[John Ferlan <jferlan@redhat.com>]

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@gmail.com>
> Signed-off-by: Jim Fehlig <jfehlig@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?

> + *
> + * 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@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

> +                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@gmail.com>
> + */
> +
> +#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__ */
>