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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@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?
>   

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

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

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]