[libvirt] [PATCH 0/5] Testing libvirt XML -> libxl_domain_config conversion

[libvirt] [PATCH 0/5] Testing libvirt XML -> libxl_domain_config conversion

[Daniel P. Berrange]

At the Xen Hackathon I learnt that libxl provides an API which
can serialize a libxl_domain_config instance to a JSON document.
This is exactly what we need for testing the XML -> libxl_domain_config
conversion process, so I spent the afternoon trying to get such a test
working. The result is that we can now just add pairs of XML, JSON
files to libvirt to test handling of new config features.

I hit a couple of small issues with libxl, which I worked around, when
writing this test which I why I'm copying xen-devel

 - libxl_ctx_alloc() will call xs_daemon_open and xc_interface_open,
   and stat /var/run/xenstored.pid to see if Xen is actually running.
   This fails when run on non-Xen hosts (and also possibly if run
   unprivileged).

   I used an evil LD_PRELOAD hack to stub out xs_daemon_open and
   xc_interface_open to return (void*)0x1, and also turn
   xc_interface_close and xs_daemon_close to no-ops, and make
   stat() always return success for xenstored.pid.

   This works (evidenced by the fact that if something was needing
   these xs/xc handles they would have crashed referencing 0x1), 
   but at the same time it might be an idea to have an officially
   supported  "non live" mode for libxl_ctx_alloc() turned on by a
   flag of some sort.


 - The libxl_json.h header file is relying on conditionals that
   are only set by Xen's build process

    eg HAVE_YAJL_YAJL_VERSION_H

   I hacked around this, but it is a little dirty too. libvirt
   already links to libyajl for the QEMU driver, but we  don't
   really need the raw YAJL objects. It'd be nice to have a

      char * libxl_domain_config_as_json(libxl_domain_config *p)

   as a higher level wrapper around libxl_domain_config_gen_json
   avoiding the pain of dealing with YAJL's different APIs.

   Ian J mentioned to me that he thought there was already such a
   method, but AFAICT, the only such code is in the 'xl' command
   line tool itself (xl_cmdimpl.c - printf_info_one_json)


A few further ideas that could be done as a followup

 - Make virConnectDomainXMLToNative accept 'xl-json" as a
   data format, so you can feed in a libvirt XML and get back
   out a JSON document. This could be a useful debugging tool
   for Xen developers trying to identify bugs in libvirt.

 - Write out the JSON document to /var/log/libvirt/libxl/$GUEST.log
   whenever starting a guest. Again this would be a useful debugging
   aid to Xen developers / support people trying to identify why a
   guest might be mis-behaving


Regards,
Daniel

Daniel P. Berrange (5):
  Don't pass virDomainObjPtr to libxlBuildDomainConfig
  Don't pass libxlDriverPrivatePtr into libxlBuildDomainConfig
  libxl: Move virDomainXMLOptionNew into libxlCreateXMLConf
  Add more test suite mock helpers
  Add a test suite for libxl option generator

 src/libxl/libxl_conf.c               |  38 +++---
 src/libxl/libxl_conf.h               |  10 +-
 src/libxl/libxl_domain.c             |   7 +-
 src/libxl/libxl_driver.c             |   4 +-
 tests/Makefile.am                    |  25 +++-
 tests/libxlxml2jsondata/minimal.json | 172 +++++++++++++++++++++++++++
 tests/libxlxml2jsondata/minimal.xml  |  36 ++++++
 tests/libxlxml2jsontest.c            | 219 +++++++++++++++++++++++++++++++++++
 tests/virfirewalltest.c              |   4 +-
 tests/virmock.h                      |  54 ++++++---
 tests/virmocklibxl.c                 |  87 ++++++++++++++
 tests/virsystemdtest.c               |   4 +-
 12 files changed, 617 insertions(+), 43 deletions(-)
 create mode 100644 tests/libxlxml2jsondata/minimal.json
 create mode 100644 tests/libxlxml2jsondata/minimal.xml
 create mode 100644 tests/libxlxml2jsontest.c
 create mode 100644 tests/virmocklibxl.c

-- 
1.9.3

[libvirt] [PATCH 1/5] Don't pass virDomainObjPtr to libxlBuildDomainConfig

[Daniel P. Berrange]

To make it easier to unit test, change libxlBuildDomainConfig
so that it takes 'virDomainDefPtr' and 'libxl_ctx *' objects
as separate parameters.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 src/libxl/libxl_conf.c   | 19 +++++++++----------
 src/libxl/libxl_conf.h   |  4 +++-
 src/libxl/libxl_domain.c |  3 ++-
 3 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index b7fed7f..a98e27a 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -567,10 +567,10 @@ libxlMakeChrdevStr(virDomainChrDefPtr def, char **buf)
 }
 
 static int
-libxlMakeDomBuildInfo(virDomainObjPtr vm, libxl_domain_config *d_config)
+libxlMakeDomBuildInfo(virDomainDefPtr def,
+                      libxl_ctx *ctx,
+                      libxl_domain_config *d_config)
 {
-    virDomainDefPtr def = vm->def;
-    libxlDomainObjPrivatePtr priv = vm->privateData;
     libxl_domain_build_info *b_info = &d_config->b_info;
     int hvm = STREQ(def->os.type, "hvm");
     size_t i;
@@ -583,7 +583,7 @@ libxlMakeDomBuildInfo(virDomainObjPtr vm, libxl_domain_config *d_config)
         libxl_domain_build_info_init_type(b_info, LIBXL_DOMAIN_TYPE_PV);
 
     b_info->max_vcpus = def->maxvcpus;
-    if (libxl_cpu_bitmap_alloc(priv->ctx, &b_info->avail_vcpus, def->maxvcpus))
+    if (libxl_cpu_bitmap_alloc(ctx, &b_info->avail_vcpus, def->maxvcpus))
         goto error;
     libxl_bitmap_set_none(&b_info->avail_vcpus);
     for (i = 0; i < def->vcpus; i++)
@@ -1306,17 +1306,16 @@ libxlMakeCapabilities(libxl_ctx *ctx)
 
 int
 libxlBuildDomainConfig(libxlDriverPrivatePtr driver,
-                       virDomainObjPtr vm, libxl_domain_config *d_config)
+                       virDomainDefPtr def,
+                       libxl_ctx *ctx,
+                       libxl_domain_config *d_config)
 {
-    virDomainDefPtr def = vm->def;
-    libxlDomainObjPrivatePtr priv = vm->privateData;
-
     libxl_domain_config_init(d_config);
 
-    if (libxlMakeDomCreateInfo(priv->ctx, def, &d_config->c_info) < 0)
+    if (libxlMakeDomCreateInfo(ctx, def, &d_config->c_info) < 0)
         return -1;
 
-    if (libxlMakeDomBuildInfo(vm, d_config) < 0)
+    if (libxlMakeDomBuildInfo(def, ctx, d_config) < 0)
         return -1;
 
     if (libxlMakeDiskList(def, d_config) < 0)
diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
index 24e1720..1dd9de3 100644
--- a/src/libxl/libxl_conf.h
+++ b/src/libxl/libxl_conf.h
@@ -160,7 +160,9 @@ libxlMakePCI(virDomainHostdevDefPtr hostdev, libxl_device_pci *pcidev);
 
 int
 libxlBuildDomainConfig(libxlDriverPrivatePtr driver,
-                       virDomainObjPtr vm, libxl_domain_config *d_config);
+                       virDomainDefPtr def,
+                       libxl_ctx *ctx,
+                       libxl_domain_config *d_config);
 
 static inline void
 libxlDriverLock(libxlDriverPrivatePtr driver)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index 80d5280..f95f8ce 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -1151,7 +1151,8 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
 
     libxl_domain_config_init(&d_config);
 
-    if (libxlBuildDomainConfig(driver, vm, &d_config) < 0)
+    if (libxlBuildDomainConfig(driver, vm->def,
+                               priv->ctx, &d_config) < 0)
         goto endjob;
 
     if (cfg->autoballoon && libxlDomainFreeMem(priv, &d_config) < 0) {
-- 
1.9.3

[libvirt] [PATCH 2/5] Don't pass libxlDriverPrivatePtr into libxlBuildDomainConfig

[Daniel P. Berrange]

To make it easier to test, change libxlBuildDomainConfig so
that it takes a virPortAllocatorPtr instead of the larger
libxlDriverPrivatePtr object.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 src/libxl/libxl_conf.c   | 12 ++++++------
 src/libxl/libxl_conf.h   |  4 ++--
 src/libxl/libxl_domain.c |  2 +-
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index a98e27a..f9e3a1b 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -959,7 +959,7 @@ libxlMakeNicList(virDomainDefPtr def,  libxl_domain_config *d_config)
 }
 
 int
-libxlMakeVfb(libxlDriverPrivatePtr driver,
+libxlMakeVfb(virPortAllocatorPtr graphicsports,
              virDomainGraphicsDefPtr l_vfb,
              libxl_device_vfb *x_vfb)
 {
@@ -982,7 +982,7 @@ libxlMakeVfb(libxlDriverPrivatePtr driver,
             libxl_defbool_set(&x_vfb->vnc.findunused, 0);
             if (l_vfb->data.vnc.autoport) {
 
-                if (virPortAllocatorAcquire(driver->reservedVNCPorts, &port) < 0)
+                if (virPortAllocatorAcquire(graphicsports, &port) < 0)
                     return -1;
                 l_vfb->data.vnc.port = port;
             }
@@ -1004,7 +1004,7 @@ libxlMakeVfb(libxlDriverPrivatePtr driver,
 }
 
 static int
-libxlMakeVfbList(libxlDriverPrivatePtr driver,
+libxlMakeVfbList(virPortAllocatorPtr graphicsports,
                  virDomainDefPtr def,
                  libxl_domain_config *d_config)
 {
@@ -1027,7 +1027,7 @@ libxlMakeVfbList(libxlDriverPrivatePtr driver,
     for (i = 0; i < nvfbs; i++) {
         libxl_device_vkb_init(&x_vkbs[i]);
 
-        if (libxlMakeVfb(driver, l_vfbs[i], &x_vfbs[i]) < 0)
+        if (libxlMakeVfb(graphicsports, l_vfbs[i], &x_vfbs[i]) < 0)
             goto error;
     }
 
@@ -1305,7 +1305,7 @@ libxlMakeCapabilities(libxl_ctx *ctx)
 }
 
 int
-libxlBuildDomainConfig(libxlDriverPrivatePtr driver,
+libxlBuildDomainConfig(virPortAllocatorPtr graphicsports,
                        virDomainDefPtr def,
                        libxl_ctx *ctx,
                        libxl_domain_config *d_config)
@@ -1324,7 +1324,7 @@ libxlBuildDomainConfig(libxlDriverPrivatePtr driver,
     if (libxlMakeNicList(def, d_config) < 0)
         return -1;
 
-    if (libxlMakeVfbList(driver, def, d_config) < 0)
+    if (libxlMakeVfbList(graphicsports, def, d_config) < 0)
         return -1;
 
     if (libxlMakePCIList(def, d_config) < 0)
diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
index 1dd9de3..2dcd0b8 100644
--- a/src/libxl/libxl_conf.h
+++ b/src/libxl/libxl_conf.h
@@ -152,14 +152,14 @@ libxlMakeNic(virDomainDefPtr def,
              virDomainNetDefPtr l_nic,
              libxl_device_nic *x_nic);
 int
-libxlMakeVfb(libxlDriverPrivatePtr driver,
+libxlMakeVfb(virPortAllocatorPtr graphicsports,
              virDomainGraphicsDefPtr l_vfb, libxl_device_vfb *x_vfb);
 
 int
 libxlMakePCI(virDomainHostdevDefPtr hostdev, libxl_device_pci *pcidev);
 
 int
-libxlBuildDomainConfig(libxlDriverPrivatePtr driver,
+libxlBuildDomainConfig(virPortAllocatorPtr graphicsports,
                        virDomainDefPtr def,
                        libxl_ctx *ctx,
                        libxl_domain_config *d_config);
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index f95f8ce..e00a3fb 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -1151,7 +1151,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
 
     libxl_domain_config_init(&d_config);
 
-    if (libxlBuildDomainConfig(driver, vm->def,
+    if (libxlBuildDomainConfig(driver->reservedVNCPorts, vm->def,
                                priv->ctx, &d_config) < 0)
         goto endjob;
 
-- 
1.9.3

[libvirt] [PATCH 3/5] libxl: Move virDomainXMLOptionNew into libxlCreateXMLConf

[Daniel P. Berrange]

To allow the test suite to creat the XML option object,
move the virDomainXMLOptionNew call into a libxlCreateXMLConf
method.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 src/libxl/libxl_conf.c   | 7 +++++++
 src/libxl/libxl_conf.h   | 2 ++
 src/libxl/libxl_domain.c | 4 ++--
 src/libxl/libxl_driver.c | 4 +---
 4 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index f9e3a1b..967759c 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1336,3 +1336,10 @@ libxlBuildDomainConfig(virPortAllocatorPtr graphicsports,
 
     return 0;
 }
+
+virDomainXMLOptionPtr libxlCreateXMLConf(void)
+{
+    return virDomainXMLOptionNew(&libxlDomainDefParserConfig,
+                                 &libxlDomainXMLPrivateDataCallbacks,
+                                 NULL);
+}
diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
index 2dcd0b8..7a9a7d5 100644
--- a/src/libxl/libxl_conf.h
+++ b/src/libxl/libxl_conf.h
@@ -158,6 +158,8 @@ libxlMakeVfb(virPortAllocatorPtr graphicsports,
 int
 libxlMakePCI(virDomainHostdevDefPtr hostdev, libxl_device_pci *pcidev);
 
+virDomainXMLOptionPtr libxlCreateXMLConf(void);
+
 int
 libxlBuildDomainConfig(virPortAllocatorPtr graphicsports,
                        virDomainDefPtr def,
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index e00a3fb..00ff14f 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -1100,6 +1100,8 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
 #endif
     virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr;
 
+    libxl_domain_config_init(&d_config);
+
     if (libxlDomainObjPrivateInitCtx(vm) < 0)
         return ret;
 
@@ -1149,8 +1151,6 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
         VIR_FREE(managed_save_path);
     }
 
-    libxl_domain_config_init(&d_config);
-
     if (libxlBuildDomainConfig(driver->reservedVNCPorts, vm->def,
                                priv->ctx, &d_config) < 0)
         goto endjob;
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index df7d510..515d5c9 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -353,9 +353,7 @@ libxlStateInitialize(bool privileged,
         goto error;
     }
 
-    if (!(libxl_driver->xmlopt = virDomainXMLOptionNew(&libxlDomainDefParserConfig,
-                                                       &libxlDomainXMLPrivateDataCallbacks,
-                                                       NULL)))
+    if (!(libxl_driver->xmlopt = libxlCreateXMLConf()))
         goto error;
 
     /* Load running domains first. */
-- 
1.9.3

[libvirt] [PATCH 4/5] Add more test suite mock helpers

[Daniel P. Berrange]

Rename the VIR_MOCK_IMPL* macros to VIR_MOCK_WRAP*
and add new VIR_MOCK_IMPL macros which let you directly
implement overrides in the preloaded source.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 tests/virfirewalltest.c |  4 ++--
 tests/virmock.h         | 54 +++++++++++++++++++++++++++++++++++++------------
 tests/virsystemdtest.c  |  4 ++--
 3 files changed, 45 insertions(+), 17 deletions(-)

diff --git a/tests/virfirewalltest.c b/tests/virfirewalltest.c
index ba2e6ad..81c5557 100644
--- a/tests/virfirewalltest.c
+++ b/tests/virfirewalltest.c
@@ -67,7 +67,7 @@ static bool fwError;
     "target     prot opt source               destination\n"
 
 # if WITH_DBUS
-VIR_MOCK_IMPL_RET_ARGS(dbus_connection_send_with_reply_and_block,
+VIR_MOCK_WRAP_RET_ARGS(dbus_connection_send_with_reply_and_block,
                        DBusMessage *,
                        DBusConnection *, connection,
                        DBusMessage *, message,
@@ -82,7 +82,7 @@ VIR_MOCK_IMPL_RET_ARGS(dbus_connection_send_with_reply_and_block,
     char **args = NULL;
     char *type = NULL;
 
-    VIR_MOCK_IMPL_INIT_REAL(dbus_connection_send_with_reply_and_block);
+    VIR_MOCK_REAL_INIT(dbus_connection_send_with_reply_and_block);
 
     if (STREQ(service, "org.freedesktop.DBus") &&
         STREQ(member, "ListNames")) {
diff --git a/tests/virmock.h b/tests/virmock.h
index 0dd8bb5..8352e30 100644
--- a/tests/virmock.h
+++ b/tests/virmock.h
@@ -234,33 +234,61 @@
  */
 
 # define VIR_MOCK_IMPL_RET_ARGS(name, rettype, ...)                     \
+    rettype name(VIR_MOCK_ARGTYPENAMES(__VA_ARGS__));                   \
+    static rettype (*real_##name)(VIR_MOCK_ARGTYPES(__VA_ARGS__));      \
+    rettype name(VIR_MOCK_ARGTYPENAMES_UNUSED(__VA_ARGS__))
+
+# define VIR_MOCK_IMPL_RET_VOID(name, rettype)                          \
+    rettype name(void);                                                 \
+    static rettype (*real_##name)(void);                                \
+    rettype name(void)
+
+# define VIR_MOCK_IMPL_VOID_ARGS(name, ...)                             \
+    void name(VIR_MOCK_ARGTYPENAMES(__VA_ARGS__));                      \
+    static void (*real_##name)(VIR_MOCK_ARGTYPES(__VA_ARGS__));         \
+    void name(VIR_MOCK_ARGTYPENAMES_UNUSED(__VA_ARGS__))
+
+# define VIR_MOCK_IMPL_VOID_VOID(name)                                  \
+    void name(void);                                                    \
+    static void (*real_##name)(void);                                   \
+    void name(void)
+
+/*
+ * The VIR_MOCK_WRAP_NNN_MMM() macros are intended for use in the
+ * individual test suites. The define a stub implementation of
+ * the wrapped method and insert the caller provided code snippet
+ * as the body of the method.
+ */
+
+# define VIR_MOCK_WRAP_RET_ARGS(name, rettype, ...)                     \
     rettype wrap_##name(VIR_MOCK_ARGTYPENAMES(__VA_ARGS__));            \
     static rettype (*real_##name)(VIR_MOCK_ARGTYPES(__VA_ARGS__));      \
     rettype wrap_##name(VIR_MOCK_ARGTYPENAMES_UNUSED(__VA_ARGS__))
 
-# define VIR_MOCK_IMPL_INIT_REAL(name)                                  \
-    do {                                                                \
-        if (real_##name == NULL &&                                      \
-            !(real_##name = dlsym(RTLD_NEXT,                            \
-                                  #name))) {                            \
-            fprintf(stderr, "Missing symbol '" #name "'\n");            \
-            abort();                                                    \
-        }                                                               \
-    } while (0)
-
-# define VIR_MOCK_IMPL_RET_VOID(name, rettype)                          \
+# define VIR_MOCK_WRAP_RET_VOID(name, rettype)                          \
     rettype wrap_##name(void);                                          \
     static rettype (*real_##name)(void);                                \
     rettype wrap_##name(void)
 
-# define VIR_MOCK_IMPL_VOID_ARGS(name, ...)                             \
+# define VIR_MOCK_WRAP_VOID_ARGS(name, ...)                             \
     void wrap_##name(VIR_MOCK_ARGTYPENAMES(__VA_ARGS__));               \
     static void (*real_##name)(VIR_MOCK_ARGTYPES(__VA_ARGS__));         \
     void wrap_##name(VIR_MOCK_ARGTYPENAMES_UNUSED(__VA_ARGS__))
 
-# define VIR_MOCK_IMPL_VOID_VOID(name)                                  \
+# define VIR_MOCK_WRAP_VOID_VOID(name)                                  \
     void wrap_##name(void);                                             \
     static void (*real_##name)(void);                                   \
     void wrap_##name(void)
 
+
+# define VIR_MOCK_REAL_INIT(name)                                       \
+    do {                                                                \
+        if (real_##name == NULL &&                                      \
+            !(real_##name = dlsym(RTLD_NEXT,                            \
+                                  #name))) {                            \
+            fprintf(stderr, "Missing symbol '" #name "'\n");            \
+            abort();                                                    \
+        }                                                               \
+    } while (0)
+
 #endif /* __VIR_MOCK_H__ */
diff --git a/tests/virsystemdtest.c b/tests/virsystemdtest.c
index 8f7b47e..0d57a6a 100644
--- a/tests/virsystemdtest.c
+++ b/tests/virsystemdtest.c
@@ -34,7 +34,7 @@
 
 VIR_LOG_INIT("tests.systemdtest");
 
-VIR_MOCK_IMPL_RET_ARGS(dbus_connection_send_with_reply_and_block,
+VIR_MOCK_WRAP_RET_ARGS(dbus_connection_send_with_reply_and_block,
                        DBusMessage *,
                        DBusConnection *, connection,
                        DBusMessage *, message,
@@ -45,7 +45,7 @@ VIR_MOCK_IMPL_RET_ARGS(dbus_connection_send_with_reply_and_block,
     const char *service = dbus_message_get_destination(message);
     const char *member = dbus_message_get_member(message);
 
-    VIR_MOCK_IMPL_INIT_REAL(dbus_connection_send_with_reply_and_block);
+    VIR_MOCK_REAL_INIT(dbus_connection_send_with_reply_and_block);
 
     if (STREQ(service, "org.freedesktop.machine1")) {
         if (getenv("FAIL_BAD_SERVICE")) {
-- 
1.9.3

[PATCH 5/5] Add a test suite for libxl option generator

[Daniel P. Berrange]

The libxl library allows a libxl_domain_config object to be
serialized to a JSON string. Use this to allow testing of
the XML -> libxl_domain_config conversion process

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 tests/Makefile.am                    |  25 +++-
 tests/libxlxml2jsondata/minimal.json | 172 +++++++++++++++++++++++++++
 tests/libxlxml2jsondata/minimal.xml  |  36 ++++++
 tests/libxlxml2jsontest.c            | 219 +++++++++++++++++++++++++++++++++++
 tests/virmocklibxl.c                 |  87 ++++++++++++++
 5 files changed, 538 insertions(+), 1 deletion(-)
 create mode 100644 tests/libxlxml2jsondata/minimal.json
 create mode 100644 tests/libxlxml2jsondata/minimal.xml
 create mode 100644 tests/libxlxml2jsontest.c
 create mode 100644 tests/virmocklibxl.c

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 5ef8940..a988342 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -82,6 +82,7 @@ EXTRA_DIST =		\
 	domainsnapshotxml2xmlout \
 	fchostdata \
 	interfaceschemadata \
+	libxlxml2jsondata \
 	lxcconf2xmldata \
 	lxcxml2xmldata \
 	lxcxml2xmloutdata \
@@ -216,6 +217,9 @@ if WITH_XEN
 test_programs += xml2sexprtest sexpr2xmltest \
 	xmconfigtest xencapstest statstest reconnect
 endif WITH_XEN
+if WITH_LIBXL
+test_programs += libxlxml2jsontest
+endif WITH_LIBXL
 if WITH_QEMU
 test_programs += qemuxml2argvtest qemuxml2xmltest qemuxmlnstest \
 	qemuargv2xmltest qemuhelptest domainsnapshotxml2xmltest \
@@ -378,7 +382,9 @@ test_libraries += libqemumonitortestutils.la \
 		qemuxml2argvmock.la \
 		$(NULL)
 endif WITH_QEMU
-
+if WITH_LIBXL
+test_libraries += virmocklibxl.la
+endif WITH_LIBXL
 if WITH_BHYVE
 test_libraries += bhyvexml2argvmock.la
 endif WITH_BHYVE
@@ -577,6 +583,23 @@ EXTRA_DIST += qemuxml2argvtest.c qemuxml2xmltest.c qemuargv2xmltest.c \
 	$(QEMUMONITORTESTUTILS_SOURCES)
 endif ! WITH_QEMU
 
+if WITH_LIBXL
+libxl_LDADDS = ../src/libvirt_driver_libxl_impl.la
+libxl_LDADDS += $(LDADDS)
+
+libxlxml2jsontest_SOURCES = \
+	libxlxml2jsontest.c testutilsxen.c testutilsxen.h \
+	testutils.c testutils.h
+libxlxml2jsontest_LDADD = $(libxl_LDADDS) $(LIBXML_LIBS)
+
+virmocklibxl_la_SOURCES = \
+	virmocklibxl.c
+virmocklibxl_la_CFLAGS = $(AM_CFLAGS)
+virmocklibxl_la_LDFLAGS = -module -avoid-version \
+        -rpath /evil/libtool/hack/to/force/shared/lib/creation
+
+endif WITH_LIBXL
+
 if WITH_LXC
 
 lxc_LDADDS = ../src/libvirt_driver_lxc_impl.la
diff --git a/tests/libxlxml2jsondata/minimal.json b/tests/libxlxml2jsondata/minimal.json
new file mode 100644
index 0000000..930e1d8
--- /dev/null
+++ b/tests/libxlxml2jsondata/minimal.json
@@ -0,0 +1,172 @@
+{
+    "c_info": {
+        "type": "pv",
+        "hap": "<default>",
+        "oos": "<default>",
+        "ssidref": 0,
+        "name": "rhel5pv",
+        "uuid": "8f07fe28-753f-2729-d76d-bdbd892f949a",
+        "xsdata": {
+
+        },
+        "platformdata": {
+
+        },
+        "poolid": 0,
+        "run_hotplug_scripts": "<default>"
+    },
+    "b_info": {
+        "max_vcpus": 4,
+        "avail_vcpus": [
+            0,
+            1,
+            2,
+            3
+        ],
+        "cpumap": [
+
+        ],
+        "nodemap": [
+
+        ],
+        "numa_placement": "<default>",
+        "tsc_mode": "default",
+        "max_memkb": 2560000,
+        "target_memkb": 307200,
+        "video_memkb": -1,
+        "shadow_memkb": -1,
+        "rtc_timeoffset": 0,
+        "exec_ssidref": 0,
+        "localtime": "<default>",
+        "disable_migrate": "<default>",
+        "cpuid": [
+
+        ],
+        "blkdev_start": null,
+        "device_model_version": null,
+        "device_model_stubdomain": "<default>",
+        "device_model": null,
+        "device_model_ssidref": 0,
+        "extra": [
+
+        ],
+        "extra_pv": [
+
+        ],
+        "extra_hvm": [
+
+        ],
+        "sched_params": {
+            "sched": "unknown",
+            "weight": 1000,
+            "cap": -1,
+            "period": -1,
+            "slice": -1,
+            "latency": -1,
+            "extratime": -1
+        },
+        "ioports": [
+
+        ],
+        "irqs": [
+
+        ],
+        "iomem": [
+
+        ],
+        "claim_mode": "<default>",
+        "u": {
+            "kernel": null,
+            "slack_memkb": -1,
+            "bootloader": "/usr/bin/pygrub",
+            "bootloader_args": [
+
+            ],
+            "cmdline": null,
+            "ramdisk": null,
+            "e820_host": "<default>"
+        }
+    },
+    "disks": [
+        {
+            "backend_domid": 0,
+            "backend_domname": null,
+            "pdev_path": "/var/lib/xen/images/rhel5pv.img",
+            "vdev": "xvda",
+            "backend": "tap",
+            "format": "raw",
+            "script": null,
+            "removable": 1,
+            "readwrite": 1,
+            "is_cdrom": 0
+        },
+        {
+            "backend_domid": 0,
+            "backend_domname": null,
+            "pdev_path": "/root/qcow1-xen.img",
+            "vdev": "xvdd",
+            "backend": "qdisk",
+            "format": "qcow",
+            "script": null,
+            "removable": 1,
+            "readwrite": 1,
+            "is_cdrom": 0
+        }
+    ],
+    "nics": [
+        {
+            "backend_domid": 0,
+            "backend_domname": null,
+            "devid": 0,
+            "mtu": 0,
+            "model": null,
+            "mac": "00:16:3e:60:36:ba",
+            "ip": null,
+            "bridge": "xenbr0",
+            "ifname": null,
+            "script": null,
+            "nictype": "vif",
+            "rate_bytes_per_interval": 0,
+            "rate_interval_usecs": 0,
+            "gatewaydev": null
+        }
+    ],
+    "pcidevs": [
+
+    ],
+    "vfbs": [
+        {
+            "backend_domid": 0,
+            "backend_domname": null,
+            "devid": -1,
+            "vnc": {
+                "enable": "True",
+                "listen": "0.0.0.0",
+                "passwd": null,
+                "display": 0,
+                "findunused": "False"
+            },
+            "sdl": {
+                "enable": "<default>",
+                "opengl": "<default>",
+                "display": null,
+                "xauthority": null
+            },
+            "keymap": null
+        }
+    ],
+    "vkbs": [
+        {
+            "backend_domid": 0,
+            "backend_domname": null,
+            "devid": -1
+        }
+    ],
+    "vtpms": [
+
+    ],
+    "on_poweroff": null,
+    "on_reboot": "destroy",
+    "on_watchdog": null,
+    "on_crash": "destroy"
+}
diff --git a/tests/libxlxml2jsondata/minimal.xml b/tests/libxlxml2jsondata/minimal.xml
new file mode 100644
index 0000000..1361028
--- /dev/null
+++ b/tests/libxlxml2jsondata/minimal.xml
@@ -0,0 +1,36 @@
+<domain type='xen'>
+  <name>rhel5pv</name>
+  <uuid>8f07fe28-753f-2729-d76d-bdbd892f949a</uuid>
+  <memory>2560000</memory>
+  <currentMemory>307200</currentMemory>
+  <vcpu>4</vcpu>
+  <bootloader>/usr/bin/pygrub</bootloader>
+  <os>
+    <type arch='i686' machine='xenpv'>linux</type>
+  </os>
+  <clock offset='utc'/>
+  <on_poweroff>destroy</on_poweroff>
+  <on_reboot>restart</on_reboot>
+  <on_crash>restart</on_crash>
+  <devices>
+    <disk type='file' device='disk'>
+      <driver name='tap' type='aio'/>
+      <source file='/var/lib/xen/images/rhel5pv.img'/>
+      <target dev='xvda' bus='xen'/>
+    </disk>
+    <disk type='file' device='disk'>
+      <driver name='tap' type='qcow'/>
+      <source file='/root/qcow1-xen.img'/>
+      <target dev='xvdd' bus='xen'/>
+    </disk>
+    <interface type='bridge'>
+      <mac address='00:16:3e:60:36:ba'/>
+      <source bridge='xenbr0'/>
+    </interface>
+    <console type='pty'>
+      <target port='0'/>
+    </console>
+    <input type='mouse' bus='xen'/>
+    <graphics type='vnc' port='-1' autoport='yes' listen='0.0.0.0'/>
+  </devices>
+</domain>
diff --git a/tests/libxlxml2jsontest.c b/tests/libxlxml2jsontest.c
new file mode 100644
index 0000000..02808b7
--- /dev/null
+++ b/tests/libxlxml2jsontest.c
@@ -0,0 +1,219 @@
+/*
+ * Copyright (C) 2014 Red Hat, Inc.
+ *
+ * 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: Daniel P. Berrange <berrange@redhat.com>
+ */
+
+#include <config.h>
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <string.h>
+
+#include <sys/types.h>
+#include <fcntl.h>
+
+#include "testutils.h"
+
+#if defined(WITH_LIBXL) && defined(WITH_YAJL)
+
+# include "internal.h"
+# include "viralloc.h"
+# include "libxl/libxl_conf.h"
+# include "datatypes.h"
+# include "virstring.h"
+# include "virmock.h"
+# include "testutilsxen.h"
+
+# ifdef WITH_YAJL2
+#  define HAVE_YAJL_V2
+# endif
+
+# include <yajl/yajl_gen.h>
+# include <libxl_json.h>
+
+# define VIR_FROM_THIS VIR_FROM_XEN
+
+static const char *abs_top_srcdir;
+static virCapsPtr xencaps;
+
+# ifdef WITH_YAJL2
+#  define yajl_size_t size_t
+# else
+#  define yajl_size_t unsigned int
+# endif
+
+static int testCompareXMLToJSONFiles(const char *xml,
+                                     const char *cmdline)
+{
+    char *expectargv = NULL;
+    int ret = -1;
+    virDomainDefPtr vmdef = NULL;
+    virPortAllocatorPtr gports = NULL;
+    libxl_ctx *ctx = NULL;
+    libxl_domain_config config;
+    xentoollog_logger *log = NULL;
+    virDomainXMLOptionPtr xmlopt = NULL;
+    yajl_gen gen;
+# ifndef WITH_YAJL2
+    yajl_gen_config conf = { 1, "    " };
+# endif
+    const unsigned char *actualargv;
+    yajl_size_t actualargvlen;
+
+    libxl_domain_config_init(&config);
+
+# ifdef WITH_YAJL2
+    gen = yajl_gen_alloc(NULL);
+    if (gen) {
+        yajl_gen_config(gen, yajl_gen_beautify, 1);
+        yajl_gen_config(gen, yajl_gen_indent_string, "    ");
+        yajl_gen_config(gen, yajl_gen_validate_utf8, 1);
+    }
+# else
+    gen = yajl_gen_alloc(&conf, NULL);
+# endif
+    if (!gen) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                       "Unable to create JSON formatter");
+        goto cleanup;
+    }
+
+    if (!(log = (xentoollog_logger *)xtl_createlogger_stdiostream(stderr, XTL_DEBUG, 0)))
+        goto cleanup;
+
+    if (libxl_ctx_alloc(&ctx, LIBXL_VERSION, 0, log) < 0)
+        goto cleanup;
+
+    if (!(gports = virPortAllocatorNew("vnc", 5900, 6000)))
+        goto cleanup;
+
+    if (!(xmlopt = libxlCreateXMLConf()))
+        goto cleanup;
+
+    if (!(vmdef = virDomainDefParseFile(xml, xencaps, xmlopt,
+                                        1 << VIR_DOMAIN_VIRT_XEN,
+                                        VIR_DOMAIN_XML_INACTIVE)))
+        goto cleanup;
+
+    if (libxlBuildDomainConfig(gports, vmdef, ctx, &config) < 0)
+        goto cleanup;
+
+    libxl_domain_config_gen_json(gen, &config);
+
+    if (yajl_gen_get_buf(gen, &actualargv, &actualargvlen) != yajl_gen_status_ok) {
+        virReportOOMError();
+        goto cleanup;
+    }
+
+    virtTestLoadFile(cmdline, &expectargv);
+
+    if (STRNEQ(expectargv, (char *)actualargv)) {
+        virtTestDifference(stderr, expectargv, (char *)actualargv);
+        goto cleanup;
+    }
+
+    ret = 0;
+
+ cleanup:
+    yajl_gen_free(gen);
+    VIR_FREE(expectargv);
+    virDomainDefFree(vmdef);
+    virObjectUnref(gports);
+    virObjectUnref(xmlopt);
+    libxl_ctx_free(ctx);
+    libxl_domain_config_dispose(&config);
+    xtl_logger_destroy(log);
+    return ret;
+}
+
+
+struct testInfo {
+    const char *name;
+};
+
+static int
+testCompareXMLToJSONHelper(const void *data)
+{
+    int ret = -1;
+    const struct testInfo *info = data;
+    char *xml = NULL;
+    char *args = NULL;
+
+    if (virAsprintf(&xml, "%s/libxlxml2jsondata/%s.xml",
+                    abs_srcdir, info->name) < 0 ||
+        virAsprintf(&args, "%s/libxlxml2jsondata/%s.json",
+                    abs_srcdir, info->name) < 0)
+        goto cleanup;
+
+    ret = testCompareXMLToJSONFiles(xml, args);
+
+ cleanup:
+    VIR_FREE(xml);
+    VIR_FREE(args);
+    return ret;
+}
+
+
+static int
+mymain(void)
+{
+    int ret = 0;
+
+    abs_top_srcdir = getenv("abs_top_srcdir");
+    if (!abs_top_srcdir)
+        abs_top_srcdir = abs_srcdir "/..";
+
+    /* Set the timezone because we are mocking the time() function.
+     * If we don't do that, then localtime() may return unpredictable
+     * results. In order to detect things that just work by a blind
+     * chance, we need to set an virtual timezone that no libvirt
+     * developer resides in. */
+    if (setenv("TZ", "VIR00:30", 1) < 0) {
+        perror("setenv");
+        return EXIT_FAILURE;
+    }
+
+    if ((xencaps = testXenCapsInit()) == NULL)
+        return EXIT_FAILURE;
+
+# define DO_TEST(name)                                                  \
+    do {                                                                \
+        static struct testInfo info = {                                 \
+            name,                                                       \
+        };                                                              \
+        if (virtTestRun("LibXL XML-2-JSON " name,                       \
+                        testCompareXMLToJSONHelper, &info) < 0)         \
+            ret = -1;                                                   \
+    } while (0)
+
+    DO_TEST("minimal");
+
+    return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
+}
+
+VIRT_TEST_MAIN_PRELOAD(mymain, abs_builddir "/.libs/virmocklibxl.so")
+
+#else
+
+int main(void)
+{
+    return EXIT_AM_SKIP;
+}
+
+#endif /* WITH_XEN && WITH_YAJL */
diff --git a/tests/virmocklibxl.c b/tests/virmocklibxl.c
new file mode 100644
index 0000000..bc4b53d
--- /dev/null
+++ b/tests/virmocklibxl.c
@@ -0,0 +1,87 @@
+/*
+ * virmocklibxl.c: mocking of xenstore/libxs for libxl
+ *
+ * Copyright (C) 2014 Red Hat, Inc.
+ *
+ * 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: Daniel P. Berrange <berrange@redhat.com>
+ */
+
+#include <config.h>
+
+#if defined(WITH_LIBXL) && defined(WITH_YAJL)
+# include "virmock.h"
+# include <sys/stat.h>
+# include <unistd.h>
+# include <libxl.h>
+# include <xenstore.h>
+# include <xenctrl.h>
+
+VIR_MOCK_IMPL_RET_VOID(xs_daemon_open,
+                       struct xs_handle *)
+{
+    VIR_MOCK_REAL_INIT(xs_daemon_open);
+    return (void*)0x1;
+}
+
+VIR_MOCK_IMPL_RET_ARGS(xc_interface_open,
+                       xc_interface *,
+                       xentoollog_logger *, logger,
+                       xentoollog_logger *, dombuild_logger,
+                       unsigned, open_flags)
+{
+    VIR_MOCK_REAL_INIT(xc_interface_open);
+    return (void*)0x1;
+}
+
+
+VIR_MOCK_STUB_RET_ARGS(xc_interface_close,
+                       int, 0,
+                       xc_interface *, handle)
+
+VIR_MOCK_STUB_VOID_ARGS(xs_daemon_close,
+                        struct xs_handle *, handle)
+
+VIR_MOCK_IMPL_RET_ARGS(__xstat, int,
+                       int, ver,
+                       const char *, path,
+                       struct stat *, sb)
+{
+    VIR_MOCK_REAL_INIT(__xstat);
+
+    if (strstr(path, "xenstored.pid")) {
+        memset(sb, 0, sizeof(*sb));
+        return 0;
+    }
+
+    return real___xstat(ver, path, sb);
+}
+
+VIR_MOCK_IMPL_RET_ARGS(stat, int,
+                       const char *, path,
+                       struct stat *, sb)
+{
+    VIR_MOCK_REAL_INIT(stat);
+
+    if (strstr(path, "xenstored.pid")) {
+        memset(sb, 0, sizeof(*sb));
+        return 0;
+    }
+
+    return real_stat(path, sb);
+}
+
+#endif /* WITH_LIBXL && WITH_YAJL */
-- 
1.9.3

Re: [libvirt] [PATCH 1/5] Don't pass virDomainObjPtr to libxlBuildDomainConfig

[Jim Fehlig]

Daniel P. Berrange wrote:
> To make it easier to unit test, change libxlBuildDomainConfig
> so that it takes 'virDomainDefPtr' and 'libxl_ctx *' objects
> as separate parameters.
>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  src/libxl/libxl_conf.c   | 19 +++++++++----------
>  src/libxl/libxl_conf.h   |  4 +++-
>  src/libxl/libxl_domain.c |  3 ++-
>  3 files changed, 14 insertions(+), 12 deletions(-)
>   

ACK.

Regards,
Jim

> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index b7fed7f..a98e27a 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -567,10 +567,10 @@ libxlMakeChrdevStr(virDomainChrDefPtr def, char **buf)
>  }
>  
>  static int
> -libxlMakeDomBuildInfo(virDomainObjPtr vm, libxl_domain_config *d_config)
> +libxlMakeDomBuildInfo(virDomainDefPtr def,
> +                      libxl_ctx *ctx,
> +                      libxl_domain_config *d_config)
>  {
> -    virDomainDefPtr def = vm->def;
> -    libxlDomainObjPrivatePtr priv = vm->privateData;
>      libxl_domain_build_info *b_info = &d_config->b_info;
>      int hvm = STREQ(def->os.type, "hvm");
>      size_t i;
> @@ -583,7 +583,7 @@ libxlMakeDomBuildInfo(virDomainObjPtr vm, libxl_domain_config *d_config)
>          libxl_domain_build_info_init_type(b_info, LIBXL_DOMAIN_TYPE_PV);
>  
>      b_info->max_vcpus = def->maxvcpus;
> -    if (libxl_cpu_bitmap_alloc(priv->ctx, &b_info->avail_vcpus, def->maxvcpus))
> +    if (libxl_cpu_bitmap_alloc(ctx, &b_info->avail_vcpus, def->maxvcpus))
>          goto error;
>      libxl_bitmap_set_none(&b_info->avail_vcpus);
>      for (i = 0; i < def->vcpus; i++)
> @@ -1306,17 +1306,16 @@ libxlMakeCapabilities(libxl_ctx *ctx)
>  
>  int
>  libxlBuildDomainConfig(libxlDriverPrivatePtr driver,
> -                       virDomainObjPtr vm, libxl_domain_config *d_config)
> +                       virDomainDefPtr def,
> +                       libxl_ctx *ctx,
> +                       libxl_domain_config *d_config)
>  {
> -    virDomainDefPtr def = vm->def;
> -    libxlDomainObjPrivatePtr priv = vm->privateData;
> -
>      libxl_domain_config_init(d_config);
>  
> -    if (libxlMakeDomCreateInfo(priv->ctx, def, &d_config->c_info) < 0)
> +    if (libxlMakeDomCreateInfo(ctx, def, &d_config->c_info) < 0)
>          return -1;
>  
> -    if (libxlMakeDomBuildInfo(vm, d_config) < 0)
> +    if (libxlMakeDomBuildInfo(def, ctx, d_config) < 0)
>          return -1;
>  
>      if (libxlMakeDiskList(def, d_config) < 0)
> diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
> index 24e1720..1dd9de3 100644
> --- a/src/libxl/libxl_conf.h
> +++ b/src/libxl/libxl_conf.h
> @@ -160,7 +160,9 @@ libxlMakePCI(virDomainHostdevDefPtr hostdev, libxl_device_pci *pcidev);
>  
>  int
>  libxlBuildDomainConfig(libxlDriverPrivatePtr driver,
> -                       virDomainObjPtr vm, libxl_domain_config *d_config);
> +                       virDomainDefPtr def,
> +                       libxl_ctx *ctx,
> +                       libxl_domain_config *d_config);
>  
>  static inline void
>  libxlDriverLock(libxlDriverPrivatePtr driver)
> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> index 80d5280..f95f8ce 100644
> --- a/src/libxl/libxl_domain.c
> +++ b/src/libxl/libxl_domain.c
> @@ -1151,7 +1151,8 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
>  
>      libxl_domain_config_init(&d_config);
>  
> -    if (libxlBuildDomainConfig(driver, vm, &d_config) < 0)
> +    if (libxlBuildDomainConfig(driver, vm->def,
> +                               priv->ctx, &d_config) < 0)
>          goto endjob;
>  
>      if (cfg->autoballoon && libxlDomainFreeMem(priv, &d_config) < 0) {
>   

Re: [libvirt] [PATCH 2/5] Don't pass libxlDriverPrivatePtr into libxlBuildDomainConfig

[Jim Fehlig]

Daniel P. Berrange wrote:
> To make it easier to test, change libxlBuildDomainConfig so
> that it takes a virPortAllocatorPtr instead of the larger
> libxlDriverPrivatePtr object.
>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  src/libxl/libxl_conf.c   | 12 ++++++------
>  src/libxl/libxl_conf.h   |  4 ++--
>  src/libxl/libxl_domain.c |  2 +-
>  3 files changed, 9 insertions(+), 9 deletions(-)
>   

ACK.

Regards,
Jim

> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index a98e27a..f9e3a1b 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -959,7 +959,7 @@ libxlMakeNicList(virDomainDefPtr def,  libxl_domain_config *d_config)
>  }
>  
>  int
> -libxlMakeVfb(libxlDriverPrivatePtr driver,
> +libxlMakeVfb(virPortAllocatorPtr graphicsports,
>               virDomainGraphicsDefPtr l_vfb,
>               libxl_device_vfb *x_vfb)
>  {
> @@ -982,7 +982,7 @@ libxlMakeVfb(libxlDriverPrivatePtr driver,
>              libxl_defbool_set(&x_vfb->vnc.findunused, 0);
>              if (l_vfb->data.vnc.autoport) {
>  
> -                if (virPortAllocatorAcquire(driver->reservedVNCPorts, &port) < 0)
> +                if (virPortAllocatorAcquire(graphicsports, &port) < 0)
>                      return -1;
>                  l_vfb->data.vnc.port = port;
>              }
> @@ -1004,7 +1004,7 @@ libxlMakeVfb(libxlDriverPrivatePtr driver,
>  }
>  
>  static int
> -libxlMakeVfbList(libxlDriverPrivatePtr driver,
> +libxlMakeVfbList(virPortAllocatorPtr graphicsports,
>                   virDomainDefPtr def,
>                   libxl_domain_config *d_config)
>  {
> @@ -1027,7 +1027,7 @@ libxlMakeVfbList(libxlDriverPrivatePtr driver,
>      for (i = 0; i < nvfbs; i++) {
>          libxl_device_vkb_init(&x_vkbs[i]);
>  
> -        if (libxlMakeVfb(driver, l_vfbs[i], &x_vfbs[i]) < 0)
> +        if (libxlMakeVfb(graphicsports, l_vfbs[i], &x_vfbs[i]) < 0)
>              goto error;
>      }
>  
> @@ -1305,7 +1305,7 @@ libxlMakeCapabilities(libxl_ctx *ctx)
>  }
>  
>  int
> -libxlBuildDomainConfig(libxlDriverPrivatePtr driver,
> +libxlBuildDomainConfig(virPortAllocatorPtr graphicsports,
>                         virDomainDefPtr def,
>                         libxl_ctx *ctx,
>                         libxl_domain_config *d_config)
> @@ -1324,7 +1324,7 @@ libxlBuildDomainConfig(libxlDriverPrivatePtr driver,
>      if (libxlMakeNicList(def, d_config) < 0)
>          return -1;
>  
> -    if (libxlMakeVfbList(driver, def, d_config) < 0)
> +    if (libxlMakeVfbList(graphicsports, def, d_config) < 0)
>          return -1;
>  
>      if (libxlMakePCIList(def, d_config) < 0)
> diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
> index 1dd9de3..2dcd0b8 100644
> --- a/src/libxl/libxl_conf.h
> +++ b/src/libxl/libxl_conf.h
> @@ -152,14 +152,14 @@ libxlMakeNic(virDomainDefPtr def,
>               virDomainNetDefPtr l_nic,
>               libxl_device_nic *x_nic);
>  int
> -libxlMakeVfb(libxlDriverPrivatePtr driver,
> +libxlMakeVfb(virPortAllocatorPtr graphicsports,
>               virDomainGraphicsDefPtr l_vfb, libxl_device_vfb *x_vfb);
>  
>  int
>  libxlMakePCI(virDomainHostdevDefPtr hostdev, libxl_device_pci *pcidev);
>  
>  int
> -libxlBuildDomainConfig(libxlDriverPrivatePtr driver,
> +libxlBuildDomainConfig(virPortAllocatorPtr graphicsports,
>                         virDomainDefPtr def,
>                         libxl_ctx *ctx,
>                         libxl_domain_config *d_config);
> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> index f95f8ce..e00a3fb 100644
> --- a/src/libxl/libxl_domain.c
> +++ b/src/libxl/libxl_domain.c
> @@ -1151,7 +1151,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
>  
>      libxl_domain_config_init(&d_config);
>  
> -    if (libxlBuildDomainConfig(driver, vm->def,
> +    if (libxlBuildDomainConfig(driver->reservedVNCPorts, vm->def,
>                                 priv->ctx, &d_config) < 0)
>          goto endjob;
>  
>   

Re: [libvirt] [PATCH 3/5] libxl: Move virDomainXMLOptionNew into libxlCreateXMLConf

[Jim Fehlig]

Daniel P. Berrange wrote:
> To allow the test suite to creat the XML option object,
> move the virDomainXMLOptionNew call into a libxlCreateXMLConf
> method.
>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  src/libxl/libxl_conf.c   | 7 +++++++
>  src/libxl/libxl_conf.h   | 2 ++
>  src/libxl/libxl_domain.c | 4 ++--
>  src/libxl/libxl_driver.c | 4 +---
>  4 files changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index f9e3a1b..967759c 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -1336,3 +1336,10 @@ libxlBuildDomainConfig(virPortAllocatorPtr graphicsports,
>  
>      return 0;
>  }
> +
> +virDomainXMLOptionPtr libxlCreateXMLConf(void)
>   

Return type and function name on separate lines.

> +{
> +    return virDomainXMLOptionNew(&libxlDomainDefParserConfig,
> +                                 &libxlDomainXMLPrivateDataCallbacks,
> +                                 NULL);
> +}
> diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
> index 2dcd0b8..7a9a7d5 100644
> --- a/src/libxl/libxl_conf.h
> +++ b/src/libxl/libxl_conf.h
> @@ -158,6 +158,8 @@ libxlMakeVfb(virPortAllocatorPtr graphicsports,
>  int
>  libxlMakePCI(virDomainHostdevDefPtr hostdev, libxl_device_pci *pcidev);
>  
> +virDomainXMLOptionPtr libxlCreateXMLConf(void);
>   

Same here.

> +
>  int
>  libxlBuildDomainConfig(virPortAllocatorPtr graphicsports,
>                         virDomainDefPtr def,
> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> index e00a3fb..00ff14f 100644
> --- a/src/libxl/libxl_domain.c
> +++ b/src/libxl/libxl_domain.c
> @@ -1100,6 +1100,8 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
>  #endif
>      virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr;
>  
> +    libxl_domain_config_init(&d_config);
> +
>      if (libxlDomainObjPrivateInitCtx(vm) < 0)
>          return ret;
>  
> @@ -1149,8 +1151,6 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
>          VIR_FREE(managed_save_path);
>      }
>  
> -    libxl_domain_config_init(&d_config);
> -
>      if (libxlBuildDomainConfig(driver->reservedVNCPorts, vm->def,
>                                 priv->ctx, &d_config) < 0)
>          goto endjob;
>   

Are these two hunks fixing a bug you found? :-)

Regards,
Jim

> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index df7d510..515d5c9 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -353,9 +353,7 @@ libxlStateInitialize(bool privileged,
>          goto error;
>      }
>  
> -    if (!(libxl_driver->xmlopt = virDomainXMLOptionNew(&libxlDomainDefParserConfig,
> -                                                       &libxlDomainXMLPrivateDataCallbacks,
> -                                                       NULL)))
> +    if (!(libxl_driver->xmlopt = libxlCreateXMLConf()))
>          goto error;
>  
>      /* Load running domains first. */
>   

Re: [libvirt] [PATCH 3/5] libxl: Move virDomainXMLOptionNew into libxlCreateXMLConf

[Daniel P. Berrange]

On Fri, May 30, 2014 at 04:41:28PM -0600, Jim Fehlig wrote:
> Daniel P. Berrange wrote:
> > To allow the test suite to creat the XML option object,
> > move the virDomainXMLOptionNew call into a libxlCreateXMLConf
> > method.
> >
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
> >  src/libxl/libxl_conf.c   | 7 +++++++
> >  src/libxl/libxl_conf.h   | 2 ++
> >  src/libxl/libxl_domain.c | 4 ++--
> >  src/libxl/libxl_driver.c | 4 +---
> >  4 files changed, 12 insertions(+), 5 deletions(-)
> >

> > diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> > index e00a3fb..00ff14f 100644
> > --- a/src/libxl/libxl_domain.c
> > +++ b/src/libxl/libxl_domain.c
> > @@ -1100,6 +1100,8 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
> >  #endif
> >      virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr;
> >  
> > +    libxl_domain_config_init(&d_config);
> > +
> >      if (libxlDomainObjPrivateInitCtx(vm) < 0)
> >          return ret;
> >  
> > @@ -1149,8 +1151,6 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
> >          VIR_FREE(managed_save_path);
> >      }
> >  
> > -    libxl_domain_config_init(&d_config);
> > -
> >      if (libxlBuildDomainConfig(driver->reservedVNCPorts, vm->def,
> >                                 priv->ctx, &d_config) < 0)
> >          goto endjob;
> >   
> 
> Are these two hunks fixing a bug you found? :-)

Hmm, yes, I should have done those as a separate patch.


The 'd_config' variable is stack allocated so has undefined initial
state. 'libxl_domain_config_init' is basically doing a memset(0,...)
on it to give it predictable value. So if we call that late in the
function, there's a chance that a 'goto endjob' call will jump to
the place where we call libxl_domain_config_dispose(), which will
then access uninitialized memory, will unpredictably bad results.


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [PATCH 0/5] Testing libvirt XML -> libxl_domain_config conversion

[Ian Campbell]

On Fri, 2014-05-30 at 18:24 +0100, Daniel P. Berrange wrote:
> At the Xen Hackathon I learnt that libxl provides an API which
> can serialize a libxl_domain_config instance to a JSON document.
> This is exactly what we need for testing the XML -> libxl_domain_config
> conversion process, so I spent the afternoon trying to get such a test
> working. The result is that we can now just add pairs of XML, JSON
> files to libvirt to test handling of new config features.
> 
> I hit a couple of small issues with libxl, which I worked around, when
> writing this test which I why I'm copying xen-devel
> 
>  - libxl_ctx_alloc() will call xs_daemon_open and xc_interface_open,
>    and stat /var/run/xenstored.pid to see if Xen is actually running.
>    This fails when run on non-Xen hosts (and also possibly if run
>    unprivileged).
> 
>    I used an evil LD_PRELOAD hack to stub out xs_daemon_open and
>    xc_interface_open to return (void*)0x1, and also turn
>    xc_interface_close and xs_daemon_close to no-ops, and make
>    stat() always return success for xenstored.pid.
> 
>    This works (evidenced by the fact that if something was needing
>    these xs/xc handles they would have crashed referencing 0x1), 
>    but at the same time it might be an idea to have an officially
>    supported  "non live" mode for libxl_ctx_alloc() turned on by a
>    flag of some sort.

Yes, we absolutely should have this.

>  - The libxl_json.h header file is relying on conditionals that
>    are only set by Xen's build process
> 
>     eg HAVE_YAJL_YAJL_VERSION_H
> 

It looks like this header has ended up with a mixture of library
internal and user facing stuff, which is wrong. I think splitting the
internal stuff into libxl_json_internal.h or similar would solve this.
I'll take a look.

>    I hacked around this, but it is a little dirty too. libvirt
>    already links to libyajl for the QEMU driver, but we  don't
>    really need the raw YAJL objects. It'd be nice to have a
> 
>       char * libxl_domain_config_as_json(libxl_domain_config *p)
> 
>    as a higher level wrapper around libxl_domain_config_gen_json
>    avoiding the pain of dealing with YAJL's different APIs.
> 
>    Ian J mentioned to me that he thought there was already such a
>    method, but AFAICT, the only such code is in the 'xl' command
>    line tool itself (xl_cmdimpl.c - printf_info_one_json)

That was me not Ian J I think.

The function you need is libxl_domain_config_to_json (which is
autogenerated, so in _libxl_types.[hc]). I think the general
libxl_*_to_json support has been there since Xen 4.2, but IIRC
libxl_domain_config only got moved into the autogenerated/IDL layer in
4.3.

For compatibility with 4.2 you could probably use libxl_*_to_json on the
various members of libxl_domain_config individually, or just punt on the
unit tests in that case of course...

Ian.

Re: [libvirt] [Xen-devel] [PATCH 0/5] Testing libvirt XML -> libxl_domain_config conversion

[Daniel P. Berrange]

On Mon, Jun 02, 2014 at 01:41:58PM +0100, Ian Campbell wrote:
> >    I hacked around this, but it is a little dirty too. libvirt
> >    already links to libyajl for the QEMU driver, but we  don't
> >    really need the raw YAJL objects. It'd be nice to have a
> > 
> >       char * libxl_domain_config_as_json(libxl_domain_config *p)
> > 
> >    as a higher level wrapper around libxl_domain_config_gen_json
> >    avoiding the pain of dealing with YAJL's different APIs.
> > 
> >    Ian J mentioned to me that he thought there was already such a
> >    method, but AFAICT, the only such code is in the 'xl' command
> >    line tool itself (xl_cmdimpl.c - printf_info_one_json)
> 
> That was me not Ian J I think.

Opps, my bad, was talking to too many people to remember things clearly :-)
 
> The function you need is libxl_domain_config_to_json (which is
> autogenerated, so in _libxl_types.[hc]). I think the general
> libxl_*_to_json support has been there since Xen 4.2, but IIRC
> libxl_domain_config only got moved into the autogenerated/IDL layer in
> 4.3.

Ahhh, so I was looking in the wrong place - no wonder 'git grep' failed
to find it :-)

> For compatibility with 4.2 you could probably use libxl_*_to_json on the
> various members of libxl_domain_config individually, or just punt on the
> unit tests in that case of course...

Yeah, I think it is reasonable to just disable the test case if
this function is missing.  So I can avoid libxl_json.h entirely
in this case.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [PATCH 5/5] Add a test suite for libxl option generator

[Ian Campbell]

On Fri, 2014-05-30 at 18:24 +0100, Daniel P. Berrange wrote:
> +    if (STRNEQ(expectargv, (char *)actualargv)) {
> +        virtTestDifference(stderr, expectargv, (char *)actualargv);
> +        goto cleanup;
> +    }

Since you are using libxl_domain_config_gen_json you can control the
pretty printing, but if you were to use the libxl_domain_config_to_json
you might have problems if the library was to do something slightly
different e.g. with whitespace.

In 4.5 we will have libxl_*_from_json and (I think) libxl_*_compare, so
you could read in the template and compare it with the generated struct.
That doesn't help you now of course.

Also in 4.5 the json will omit fields which are set to the their
explicit default value. libxl_*_from_json will still do the right thing,
but it'd be another annoyance for you here I think.

Lastly, when we add new fields to the API they will start showing up in
the json (modulo the omission of defaults discussed above).

Other than having a template per libxl version (yuck) I don't have any
particularly smart suggestions except for to aim long
term for:
  libxl_domain_config_init(..., &template)
  libxl_domain_config_from_json(..., &template, "{ json json json ....")

  libxl_domain_config_init(..., &xml)
  virtLibxlFromXML(..., &xml, "<domain>....")

  libxl_domain_config_compare(&template, &xml)

which will likely handle all of those corner cases, except perhaps the
case where libvirt is using new fields on new versions of libxl etc. I
suppose that would be handled by specific unit tests for >= and <
versions of libxl separately?

Ian.

Re: [libvirt] [Xen-devel] [PATCH 5/5] Add a test suite for libxl option generator

[Daniel P. Berrange]

On Mon, Jun 02, 2014 at 01:53:46PM +0100, Ian Campbell wrote:
> 
> On Fri, 2014-05-30 at 18:24 +0100, Daniel P. Berrange wrote:
> > +    if (STRNEQ(expectargv, (char *)actualargv)) {
> > +        virtTestDifference(stderr, expectargv, (char *)actualargv);
> > +        goto cleanup;
> > +    }
> 
> Since you are using libxl_domain_config_gen_json you can control the
> pretty printing, but if you were to use the libxl_domain_config_to_json
> you might have problems if the library was to do something slightly
> different e.g. with whitespace.
> 
> In 4.5 we will have libxl_*_from_json and (I think) libxl_*_compare, so
> you could read in the template and compare it with the generated struct.
> That doesn't help you now of course.
> 
> Also in 4.5 the json will omit fields which are set to the their
> explicit default value. libxl_*_from_json will still do the right thing,
> but it'd be another annoyance for you here I think.
> 
> Lastly, when we add new fields to the API they will start showing up in
> the json (modulo the omission of defaults discussed above).

Hmm, that's a v good point.

> Other than having a template per libxl version (yuck) I don't have any
> particularly smart suggestions except for to aim long
> term for:
>   libxl_domain_config_init(..., &template)
>   libxl_domain_config_from_json(..., &template, "{ json json json ....")
> 
>   libxl_domain_config_init(..., &xml)
>   virtLibxlFromXML(..., &xml, "<domain>....")
> 
>   libxl_domain_config_compare(&template, &xml)
> 
> which will likely handle all of those corner cases, except perhaps the
> case where libvirt is using new fields on new versions of libxl etc. I
> suppose that would be handled by specific unit tests for >= and <
> versions of libxl separately?

I think that perhaps I should just add a general JSON "DOM" comparison
function in libvirt - no need for it to be libxl specific, as it might
be useful for libvirt JSON usage elsewhere.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [PATCH 0/5] Testing libvirt XML -> libxl_domain_config conversion

[Ian Campbell]

create !
title it libxl_ctx_alloc should have dummy mode which does not require a Xen host
severity it wishlist
thanks
(just creating a bug for this issue)

On Mon, 2014-06-02 at 13:41 +0100, Ian Campbell wrote:
> On Fri, 2014-05-30 at 18:24 +0100, Daniel P. Berrange wrote:
> > I hit a couple of small issues with libxl, which I worked around, when
> > writing this test which I why I'm copying xen-devel
> > 
> >  - libxl_ctx_alloc() will call xs_daemon_open and xc_interface_open,
> >    and stat /var/run/xenstored.pid to see if Xen is actually running.
> >    This fails when run on non-Xen hosts (and also possibly if run
> >    unprivileged).
> > 
> >    I used an evil LD_PRELOAD hack to stub out xs_daemon_open and
> >    xc_interface_open to return (void*)0x1, and also turn
> >    xc_interface_close and xs_daemon_close to no-ops, and make
> >    stat() always return success for xenstored.pid.
> > 
> >    This works (evidenced by the fact that if something was needing
> >    these xs/xc handles they would have crashed referencing 0x1), 
> >    but at the same time it might be an idea to have an officially
> >    supported  "non live" mode for libxl_ctx_alloc() turned on by a
> >    flag of some sort.
> 
> Yes, we absolutely should have this.

Processed: Re: [PATCH 0/5] Testing libvirt XML -> libxl_domain_config conversion

[xen]

Processing commands for xen@bugs.xenproject.org:

> create !
Created new bug #41 rooted at `<1401713875.19553.9.camel@kazak.uk.xensource.com>'
Title: `Re: [Xen-devel] [PATCH 0/5] Testing libvirt XML -> libxl_domain_config conversion'
> title it libxl_ctx_alloc should have dummy mode which does not require a Xen host
Set title for #41 to `libxl_ctx_alloc should have dummy mode which does not require a Xen host'
> severity it wishlist
Change severity for #41 to `wishlist'
> thanks
Finished processing.

Modified/created Bugs:
 - 41: http://bugs.xenproject.org/xen/bug/41 (new)

---
Xen Hypervisor Bug Tracker
See http://wiki.xen.org/wiki/Reporting_Bugs_against_Xen for information on reporting bugs
Contact xen-bugs-owner@bugs.xenproject.org with any infrastructure issues

Re: [PATCH 5/5] Add a test suite for libxl option generator

[Jim Fehlig]

Daniel P. Berrange wrote:
> On Mon, Jun 02, 2014 at 01:53:46PM +0100, Ian Campbell wrote:
>   
>> On Fri, 2014-05-30 at 18:24 +0100, Daniel P. Berrange wrote:
>>     
>>> +    if (STRNEQ(expectargv, (char *)actualargv)) {
>>> +        virtTestDifference(stderr, expectargv, (char *)actualargv);
>>> +        goto cleanup;
>>> +    }
>>>       
>> Since you are using libxl_domain_config_gen_json you can control the
>> pretty printing, but if you were to use the libxl_domain_config_to_json
>> you might have problems if the library was to do something slightly
>> different e.g. with whitespace.
>>
>> In 4.5 we will have libxl_*_from_json and (I think) libxl_*_compare, so
>> you could read in the template and compare it with the generated struct.
>> That doesn't help you now of course.
>>
>> Also in 4.5 the json will omit fields which are set to the their
>> explicit default value. libxl_*_from_json will still do the right thing,
>> but it'd be another annoyance for you here I think.
>>
>> Lastly, when we add new fields to the API they will start showing up in
>> the json (modulo the omission of defaults discussed above).
>>     
>
> Hmm, that's a v good point.
>   

One that can already be seen.  The test works when run on Xen 4.3, but
fails on 4.4.  E.g. c_info gained some new fields

        "poolid": 0,
        "run_hotplug_scripts": "<default>",
+        "pvh": "<default>",
+        "driver_domain": "<default>"
    },

And on_watchdog's value changed

    "on_poweroff": null,
    "on_reboot": "destroy",
-    "on_watchdog": null,
+    "on_watchdog": "destroy",
    "on_crash": "destroy"

Regards,
Jim

Re: [PATCH 0/5] Testing libvirt XML -> libxl_domain_config conversion

[Jim Fehlig]

Daniel P. Berrange wrote:
> On Mon, Jun 02, 2014 at 01:41:58PM +0100, Ian Campbell wrote:
>   
>>>    I hacked around this, but it is a little dirty too. libvirt
>>>    already links to libyajl for the QEMU driver, but we  don't
>>>    really need the raw YAJL objects. It'd be nice to have a
>>>
>>>       char * libxl_domain_config_as_json(libxl_domain_config *p)
>>>
>>>    as a higher level wrapper around libxl_domain_config_gen_json
>>>    avoiding the pain of dealing with YAJL's different APIs.
>>>
>>>    Ian J mentioned to me that he thought there was already such a
>>>    method, but AFAICT, the only such code is in the 'xl' command
>>>    line tool itself (xl_cmdimpl.c - printf_info_one_json)
>>>       
>> That was me not Ian J I think.
>>     
>
> Opps, my bad, was talking to too many people to remember things clearly :-)
>  
>   
>> The function you need is libxl_domain_config_to_json (which is
>> autogenerated, so in _libxl_types.[hc]). I think the general
>> libxl_*_to_json support has been there since Xen 4.2, but IIRC
>> libxl_domain_config only got moved into the autogenerated/IDL layer in
>> 4.3.
>>     
>
> Ahhh, so I was looking in the wrong place - no wonder 'git grep' failed
> to find it :-)
>   

This patch becomes a bit smaller by using libxl_domain_config_to_json()
- see below diff.  It works with Xen 4.2-4.4, although the test fails on
all but 4.4 due to changes in the json noted earlier (e.g. additional
c_info fields added to the returned json).

Thanks for looking at this btw!  Great topic for the hackathon.  I was
groaning about the need for round-trip config testing last week while
reviewing some other patches, just before you posted this series!  Awesome!

Regards,
Jim


diff --git a/tests/libxlxml2jsontest.c b/tests/libxlxml2jsontest.c
index 02808b7..ed7c256 100644
--- a/tests/libxlxml2jsontest.c
+++ b/tests/libxlxml2jsontest.c
@@ -40,24 +40,11 @@
 # include "virmock.h"
 # include "testutilsxen.h"
 
-# ifdef WITH_YAJL2
-#  define HAVE_YAJL_V2
-# endif
-
-# include <yajl/yajl_gen.h>
-# include <libxl_json.h>
-
 # define VIR_FROM_THIS VIR_FROM_XEN
 
 static const char *abs_top_srcdir;
 static virCapsPtr xencaps;
 
-# ifdef WITH_YAJL2
-#  define yajl_size_t size_t
-# else
-#  define yajl_size_t unsigned int
-# endif
-
 static int testCompareXMLToJSONFiles(const char *xml,
                                      const char *cmdline)
 {
@@ -69,31 +56,10 @@ static int testCompareXMLToJSONFiles(const char *xml,
     libxl_domain_config config;
     xentoollog_logger *log = NULL;
     virDomainXMLOptionPtr xmlopt = NULL;
-    yajl_gen gen;
-# ifndef WITH_YAJL2
-    yajl_gen_config conf = { 1, "    " };
-# endif
-    const unsigned char *actualargv;
-    yajl_size_t actualargvlen;
+    char *actualargv = NULL;
 
     libxl_domain_config_init(&config);
 
-# ifdef WITH_YAJL2
-    gen = yajl_gen_alloc(NULL);
-    if (gen) {
-        yajl_gen_config(gen, yajl_gen_beautify, 1);
-        yajl_gen_config(gen, yajl_gen_indent_string, "    ");
-        yajl_gen_config(gen, yajl_gen_validate_utf8, 1);
-    }
-# else
-    gen = yajl_gen_alloc(&conf, NULL);
-# endif
-    if (!gen) {
-        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-                       "Unable to create JSON formatter");
-        goto cleanup;
-    }
-
     if (!(log = (xentoollog_logger
*)xtl_createlogger_stdiostream(stderr, XTL_DEBUG, 0)))
         goto cleanup;
 
@@ -114,24 +80,18 @@ static int testCompareXMLToJSONFiles(const char *xml,
     if (libxlBuildDomainConfig(gports, vmdef, ctx, &config) < 0)
         goto cleanup;
 
-    libxl_domain_config_gen_json(gen, &config);
-
-    if (yajl_gen_get_buf(gen, &actualargv, &actualargvlen) !=
yajl_gen_status_ok) {
-        virReportOOMError();
-        goto cleanup;
-    }
-
+    actualargv = libxl_domain_config_to_json(ctx, &config);
     virtTestLoadFile(cmdline, &expectargv);
 
-    if (STRNEQ(expectargv, (char *)actualargv)) {
-        virtTestDifference(stderr, expectargv, (char *)actualargv);
+    if (STRNEQ(expectargv, actualargv)) {
+        virtTestDifference(stderr, expectargv, actualargv);
         goto cleanup;
     }
 
     ret = 0;
 
  cleanup:
-    yajl_gen_free(gen);
+    VIR_FREE(actualargv);
     VIR_FREE(expectargv);
     virDomainDefFree(vmdef);
     virObjectUnref(gports);

Re: [libvirt] [Xen-devel] [PATCH 0/5] Testing libvirt XML -> libxl_domain_config conversion

[Daniel P. Berrange]

On Mon, Jun 02, 2014 at 04:43:25PM -0600, Jim Fehlig wrote:
> Daniel P. Berrange wrote:
> > On Mon, Jun 02, 2014 at 01:41:58PM +0100, Ian Campbell wrote:
> >   
> >>>    I hacked around this, but it is a little dirty too. libvirt
> >>>    already links to libyajl for the QEMU driver, but we  don't
> >>>    really need the raw YAJL objects. It'd be nice to have a
> >>>
> >>>       char * libxl_domain_config_as_json(libxl_domain_config *p)
> >>>
> >>>    as a higher level wrapper around libxl_domain_config_gen_json
> >>>    avoiding the pain of dealing with YAJL's different APIs.
> >>>
> >>>    Ian J mentioned to me that he thought there was already such a
> >>>    method, but AFAICT, the only such code is in the 'xl' command
> >>>    line tool itself (xl_cmdimpl.c - printf_info_one_json)
> >>>       
> >> That was me not Ian J I think.
> >>     
> >
> > Opps, my bad, was talking to too many people to remember things clearly :-)
> >  
> >   
> >> The function you need is libxl_domain_config_to_json (which is
> >> autogenerated, so in _libxl_types.[hc]). I think the general
> >> libxl_*_to_json support has been there since Xen 4.2, but IIRC
> >> libxl_domain_config only got moved into the autogenerated/IDL layer in
> >> 4.3.
> >>     
> >
> > Ahhh, so I was looking in the wrong place - no wonder 'git grep' failed
> > to find it :-)
> >   
> 
> This patch becomes a bit smaller by using libxl_domain_config_to_json()
> - see below diff.  It works with Xen 4.2-4.4, although the test fails on
> all but 4.4 due to changes in the json noted earlier (e.g. additional
> c_info fields added to the returned json).

Thanks, I've got changes I'm testing which will do DOM level compares
instead of string compares, which will hopefully solve the compat
problem.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|