[libvirt] [PATCH 0/5] Testing libvirt XML -> libxl_domain_config conversion
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
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
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) {
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
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;
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
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. */
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 :|
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
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
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.
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 :|
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
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.
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 :|
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);
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 :|
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.
participants (3)
-
Daniel P. Berrange -
Ian Campbell -
Jim Fehlig