[libvirt] [PATCH 0/9] ABI/JSON/other fixes
I've split out these patches from my work on supporting memory hotplug in qemu so that the resulting series is not extremely huge. Peter Krempa (9): conf: Improve adding of new address types conf: shmem: Add ABI stability check conf: Add compile time check that devices were checked for ABI stability qemu: hotplug: Use typecasted switch statement when plugging new devices util: json: Split out code to create json value objects util: json: Improve handling and docs for adding JSON objects qemu: monitor: Add functions for object hot-add/remove Implement empty post parse callbacks for all drivers conf: Move definition of virDomainParseMemory src/bhyve/bhyve_domain.c | 10 ++ src/conf/domain_conf.c | 179 +++++++++++++++++++++++-------- src/conf/domain_conf.h | 2 +- src/libvirt_private.syms | 2 + src/parallels/parallels_driver.c | 21 ++++ src/phyp/phyp_driver.c | 29 ++++- src/qemu/qemu_driver.c | 17 ++- src/qemu/qemu_monitor.c | 48 +++++++++ src/qemu/qemu_monitor.h | 8 ++ src/qemu/qemu_monitor_json.c | 217 ++++++++++--------------------------- src/qemu/qemu_monitor_json.h | 8 ++ src/uml/uml_driver.c | 10 ++ src/util/virjson.c | 224 +++++++++++++++++++++++++++++++++++++++ src/util/virjson.h | 5 + src/vbox/vbox_common.c | 19 ++++ src/vmware/vmware_driver.c | 24 ++++- src/vmx/vmx.c | 18 ++++ src/xenapi/xenapi_driver.c | 10 ++ 18 files changed, 645 insertions(+), 206 deletions(-) -- 2.1.0
Use typecasted switch statement and note the type used to select the address type in a comment. --- src/conf/domain_conf.c | 36 ++++++++++++++++++++++++------------ src/conf/domain_conf.h | 2 +- 2 files changed, 25 insertions(+), 13 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 35bbd91..55a1cc5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3306,7 +3306,7 @@ virDomainDeviceInfoFormat(virBufferPtr buf, virBufferAsprintf(buf, "<address type='%s'", virDomainDeviceAddressTypeToString(info->type)); - switch (info->type) { + switch ((virDomainDeviceAddressType) info->type) { case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI: virBufferAsprintf(buf, " domain='0x%.4x' bus='0x%.2x' slot='0x%.2x' function='0x%.1x'", info->addr.pci.domain, @@ -3368,10 +3368,10 @@ virDomainDeviceInfoFormat(virBufferPtr buf, virBufferAsprintf(buf, " irq='0x%x'", info->addr.isa.irq); break; - default: - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unknown address type '%d'"), info->type); - return -1; + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST: + break; } virBufferAddLit(buf, "/>\n"); @@ -3816,7 +3816,7 @@ virDomainDeviceInfoParseXML(xmlNodePtr node, type = virXMLPropString(address, "type"); if (type) { - if ((info->type = virDomainDeviceAddressTypeFromString(type)) < 0) { + if ((info->type = virDomainDeviceAddressTypeFromString(type)) <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown address type '%s'"), type); goto cleanup; @@ -3827,7 +3827,7 @@ virDomainDeviceInfoParseXML(xmlNodePtr node, goto cleanup; } - switch (info->type) { + switch ((virDomainDeviceAddressType) info->type) { case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI: if (virDevicePCIAddressParseXML(address, &info->addr.pci) < 0) goto cleanup; @@ -3873,11 +3873,14 @@ virDomainDeviceInfoParseXML(xmlNodePtr node, goto cleanup; break; - default: - /* Should not happen */ - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Unknown device address type")); + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390: + virReportError(VIR_ERR_XML_ERROR, "%s", + _("virtio-s390 bus doesn't have an address")); goto cleanup; + + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST: + break; } ret = 0; @@ -14223,7 +14226,7 @@ virDomainDeviceInfoCheckABIStability(virDomainDeviceInfoPtr src, return false; } - switch (src->type) { + switch ((virDomainDeviceAddressType) src->type) { case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI: if (src->addr.pci.domain != dst->addr.pci.domain || src->addr.pci.bus != dst->addr.pci.bus || @@ -14297,6 +14300,15 @@ virDomainDeviceInfoCheckABIStability(virDomainDeviceInfoPtr src, return false; } break; + + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST: + break; } return true; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index afa3da6..9908d88 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -308,7 +308,7 @@ struct _virDomainDeviceInfo { * to consider the new fields */ char *alias; - int type; + int type; /* virDomainDeviceAddressType */ union { virDevicePCIAddress pci; virDomainDeviceDriveAddress drive; -- 2.1.0
On 10/14/2014 03:29 AM, Peter Krempa wrote:
Use typecasted switch statement and note the type used to select the address type in a comment. --- src/conf/domain_conf.c | 36 ++++++++++++++++++++++++------------ src/conf/domain_conf.h | 2 +- 2 files changed, 25 insertions(+), 13 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 35bbd91..55a1cc5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3306,7 +3306,7 @@ virDomainDeviceInfoFormat(virBufferPtr buf, virBufferAsprintf(buf, "<address type='%s'", virDomainDeviceAddressTypeToString(info->type));
- switch (info->type) { + switch ((virDomainDeviceAddressType) info->type) { case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI: virBufferAsprintf(buf, " domain='0x%.4x' bus='0x%.2x' slot='0x%.2x' function='0x%.1x'", info->addr.pci.domain, @@ -3368,10 +3368,10 @@ virDomainDeviceInfoFormat(virBufferPtr buf, virBufferAsprintf(buf, " irq='0x%x'", info->addr.isa.irq); break;
- default: - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unknown address type '%d'"), info->type); - return -1; + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST: + break; }
virBufferAddLit(buf, "/>\n"); @@ -3816,7 +3816,7 @@ virDomainDeviceInfoParseXML(xmlNodePtr node, type = virXMLPropString(address, "type");
if (type) { - if ((info->type = virDomainDeviceAddressTypeFromString(type)) < 0) { + if ((info->type = virDomainDeviceAddressTypeFromString(type)) <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown address type '%s'"), type); goto cleanup; @@ -3827,7 +3827,7 @@ virDomainDeviceInfoParseXML(xmlNodePtr node, goto cleanup; }
- switch (info->type) { + switch ((virDomainDeviceAddressType) info->type) { case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI: if (virDevicePCIAddressParseXML(address, &info->addr.pci) < 0) goto cleanup; @@ -3873,11 +3873,14 @@ virDomainDeviceInfoParseXML(xmlNodePtr node, goto cleanup; break;
- default: - /* Should not happen */ - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Unknown device address type")); + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390: + virReportError(VIR_ERR_XML_ERROR, "%s", + _("virtio-s390 bus doesn't have an address"));
Do we really care? We could just ignore it (like MMIO) and it'll be lost. Of course since we would have fallen into 'default' before and thus resulted in error - I do see why you've added the error. I'm OK with the error - just figured I'd note it and let you decide.. ACK John
goto cleanup; + + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST: + break; }
ret = 0; @@ -14223,7 +14226,7 @@ virDomainDeviceInfoCheckABIStability(virDomainDeviceInfoPtr src, return false; }
- switch (src->type) { + switch ((virDomainDeviceAddressType) src->type) { case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI: if (src->addr.pci.domain != dst->addr.pci.domain || src->addr.pci.bus != dst->addr.pci.bus || @@ -14297,6 +14300,15 @@ virDomainDeviceInfoCheckABIStability(virDomainDeviceInfoPtr src, return false; } break; + + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST: + break; }
return true; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index afa3da6..9908d88 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -308,7 +308,7 @@ struct _virDomainDeviceInfo { * to consider the new fields */ char *alias; - int type; + int type; /* virDomainDeviceAddressType */ union { virDevicePCIAddress pci; virDomainDeviceDriveAddress drive;
On 10/14/14 10:56, John Ferlan wrote:
On 10/14/2014 03:29 AM, Peter Krempa wrote:
Use typecasted switch statement and note the type used to select the address type in a comment. --- src/conf/domain_conf.c | 36 ++++++++++++++++++++++++------------ src/conf/domain_conf.h | 2 +- 2 files changed, 25 insertions(+), 13 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 35bbd91..55a1cc5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3306,7 +3306,7 @@ virDomainDeviceInfoFormat(virBufferPtr buf, virBufferAsprintf(buf, "<address type='%s'", virDomainDeviceAddressTypeToString(info->type));
- switch (info->type) { + switch ((virDomainDeviceAddressType) info->type) { case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI: virBufferAsprintf(buf, " domain='0x%.4x' bus='0x%.2x' slot='0x%.2x' function='0x%.1x'", info->addr.pci.domain, @@ -3368,10 +3368,10 @@ virDomainDeviceInfoFormat(virBufferPtr buf, virBufferAsprintf(buf, " irq='0x%x'", info->addr.isa.irq); break;
- default: - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unknown address type '%d'"), info->type); - return -1; + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST: + break; }
virBufferAddLit(buf, "/>\n"); @@ -3816,7 +3816,7 @@ virDomainDeviceInfoParseXML(xmlNodePtr node, type = virXMLPropString(address, "type");
if (type) { - if ((info->type = virDomainDeviceAddressTypeFromString(type)) < 0) { + if ((info->type = virDomainDeviceAddressTypeFromString(type)) <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown address type '%s'"), type); goto cleanup; @@ -3827,7 +3827,7 @@ virDomainDeviceInfoParseXML(xmlNodePtr node, goto cleanup; }
- switch (info->type) { + switch ((virDomainDeviceAddressType) info->type) { case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI: if (virDevicePCIAddressParseXML(address, &info->addr.pci) < 0) goto cleanup; @@ -3873,11 +3873,14 @@ virDomainDeviceInfoParseXML(xmlNodePtr node, goto cleanup; break;
- default: - /* Should not happen */ - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Unknown device address type")); + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390: + virReportError(VIR_ERR_XML_ERROR, "%s", + _("virtio-s390 bus doesn't have an address"));
Do we really care? We could just ignore it (like MMIO) and it'll be lost. Of course since we would have fallen into 'default' before and thus resulted in error - I do see why you've added the error.
I'm OK with the error - just figured I'd note it and let you decide..
ACK
John
I deliberately chose to leave the error in place so that this patch doesn't change behavior in these cases in case something would rely on it. I'm not familiar enough with the s390 addressing scheme to make a qualified decision about that. At any rate that can be done as a follow up. Peter
Although the device will probably inhibit migration add checks to make sure that the configuration change gets caught. --- src/conf/domain_conf.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 55a1cc5..5e3c389 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14998,6 +14998,44 @@ virDomainPanicCheckABIStability(virDomainPanicDefPtr src, } +static bool +virDomainShmemDefCheckABIStability(virDomainShmemDefPtr src, + virDomainShmemDefPtr dst) +{ + if (STRNEQ_NULLABLE(src->name, dst->name)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Target shared memory name '%s' does not match source " + "'%s'"), dst->name, src->name); + return false; + } + + if (src->size != dst->size) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Target shared memory size '%llu' does not match " + "source size '%llu'"), dst->size, src->size); + return false; + } + + if (src->server.enabled != dst->server.enabled) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Target shared memory server usage doesn't match " + "source")); + return false; + } + + if (src->msi.vectors != dst->msi.vectors || + src->msi.enabled != dst->msi.enabled || + src->msi.ioeventfd != dst->msi.ioeventfd) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Target shared memory MSI configuration doesn't match " + "source")); + return false; + } + + return virDomainDeviceInfoCheckABIStability(&src->info, &dst->info); +} + + /* This compares two configurations and looks for any differences * which will affect the guest ABI. This is primarily to allow * validation of custom XML config passed in during migration @@ -15399,6 +15437,18 @@ virDomainDefCheckABIStability(virDomainDefPtr src, if (!virDomainPanicCheckABIStability(src->panic, dst->panic)) goto error; + if (src->nshmems != dst->nshmems) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Target domain shared memory device count %zu " + "does not match source %zu"), dst->nshmems, src->nshmems); + goto error; + } + + for (i = 0; i < src->nshmems; i++) { + if (!virDomainShmemDefCheckABIStability(src->shmems[i], dst->shmems[i])) + goto error; + } + return true; error: -- 2.1.0
On 10/14/2014 03:29 AM, Peter Krempa wrote:
Although the device will probably inhibit migration add checks to make sure that the configuration change gets caught. --- src/conf/domain_conf.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 55a1cc5..5e3c389 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14998,6 +14998,44 @@ virDomainPanicCheckABIStability(virDomainPanicDefPtr src, }
+static bool +virDomainShmemDefCheckABIStability(virDomainShmemDefPtr src, + virDomainShmemDefPtr dst) +{ + if (STRNEQ_NULLABLE(src->name, dst->name)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Target shared memory name '%s' does not match source " + "'%s'"), dst->name, src->name); + return false; + } + + if (src->size != dst->size) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Target shared memory size '%llu' does not match " + "source size '%llu'"), dst->size, src->size); + return false; + }
Again assuming the device even allows migration... Could it be reasonable to allow the target to be larger? I'm assuming that "something" copying <stuff> from source to target during the migration. As long as the target is at least a source size, then does it really matter if the target is larger? I didn't dig into the migration code, but I'm assuming what happens is the target gets whatever is currently in the shared memory device of the source. Obviously going larger to smaller would cause a problem, but if as a guest admin I wanted to have a larger shared memory on the target and knew I could modify my (now) former source memory segment later (or not at all if I wasn't going to migrate back), then does it hurt? Testing won't like it, but I can see a use case. Whether it's a reasonable option based on the shmem code - I'm not sure.
+ + if (src->server.enabled != dst->server.enabled) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Target shared memory server usage doesn't match " + "source")); + return false; + } + + if (src->msi.vectors != dst->msi.vectors || + src->msi.enabled != dst->msi.enabled || + src->msi.ioeventfd != dst->msi.ioeventfd) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Target shared memory MSI configuration doesn't match " + "source")); + return false; + }
I think I've convinced myself it's not a good idea to switch between server vs. non-server environment, but maybe someone has such a use case. Just don't want to see us restrict something that someone may find as a feature... I haven't closely followed all the shmem discussions so I could be off in the weeds... ACK in general to what's here - unless of course my comments trigger someone else to indicate we should allow these "differences". John
+ + return virDomainDeviceInfoCheckABIStability(&src->info, &dst->info); +} + + /* This compares two configurations and looks for any differences * which will affect the guest ABI. This is primarily to allow * validation of custom XML config passed in during migration @@ -15399,6 +15437,18 @@ virDomainDefCheckABIStability(virDomainDefPtr src, if (!virDomainPanicCheckABIStability(src->panic, dst->panic)) goto error;
+ if (src->nshmems != dst->nshmems) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Target domain shared memory device count %zu " + "does not match source %zu"), dst->nshmems, src->nshmems); + goto error; + } + + for (i = 0; i < src->nshmems; i++) { + if (!virDomainShmemDefCheckABIStability(src->shmems[i], dst->shmems[i])) + goto error; + } + return true;
error:
As in the device info iterator add a switch that will force the compiler to check that new device types are added to the ABI stability checker. --- src/conf/domain_conf.c | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5e3c389..0634116 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -15449,6 +15449,39 @@ virDomainDefCheckABIStability(virDomainDefPtr src, goto error; } + /* Coverity is not very happy with this - all dead_error_condition */ +#if !STATIC_ANALYSIS + /* This switch statement is here to trigger compiler warning when adding + * a new device type. When you are adding a new field to the switch you + * also have to add an check above. Otherwise the switch statement has no + * real function here and should be optimized out by the compiler. */ + i = VIR_DOMAIN_DEVICE_LAST; + switch ((virDomainDeviceType) i) { + case VIR_DOMAIN_DEVICE_DISK: + case VIR_DOMAIN_DEVICE_LEASE: + case VIR_DOMAIN_DEVICE_FS: + case VIR_DOMAIN_DEVICE_NET: + case VIR_DOMAIN_DEVICE_INPUT: + case VIR_DOMAIN_DEVICE_SOUND: + case VIR_DOMAIN_DEVICE_VIDEO: + case VIR_DOMAIN_DEVICE_HOSTDEV: + case VIR_DOMAIN_DEVICE_WATCHDOG: + case VIR_DOMAIN_DEVICE_CONTROLLER: + case VIR_DOMAIN_DEVICE_GRAPHICS: + case VIR_DOMAIN_DEVICE_HUB: + case VIR_DOMAIN_DEVICE_REDIRDEV: + case VIR_DOMAIN_DEVICE_NONE: + case VIR_DOMAIN_DEVICE_SMARTCARD: + case VIR_DOMAIN_DEVICE_CHR: + case VIR_DOMAIN_DEVICE_MEMBALLOON: + case VIR_DOMAIN_DEVICE_NVRAM: + case VIR_DOMAIN_DEVICE_LAST: + case VIR_DOMAIN_DEVICE_RNG: + case VIR_DOMAIN_DEVICE_SHMEM: + break; + } +#endif + return true; error: -- 2.1.0
On 10/14/2014 03:29 AM, Peter Krempa wrote:
As in the device info iterator add a switch that will force the compiler to check that new device types are added to the ABI stability checker. --- src/conf/domain_conf.c | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+)
ACK John (passed my coverity check too :-))
--- src/qemu/qemu_driver.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 7c9b1ab..e0fd4c0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6805,7 +6805,7 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm, virQEMUDriverPtr driver = dom->conn->privateData; int ret = -1; - switch (dev->type) { + switch ((virDomainDeviceType) dev->type) { case VIR_DOMAIN_DEVICE_DISK: qemuDomainObjCheckDiskTaint(driver, vm, dev->data.disk, -1); ret = qemuDomainAttachDeviceDiskLive(dom->conn, driver, vm, dev); @@ -6856,7 +6856,20 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm, dev->data.chr = NULL; break; - default: + case VIR_DOMAIN_DEVICE_NONE: + case VIR_DOMAIN_DEVICE_FS: + case VIR_DOMAIN_DEVICE_INPUT: + case VIR_DOMAIN_DEVICE_SOUND: + case VIR_DOMAIN_DEVICE_VIDEO: + case VIR_DOMAIN_DEVICE_WATCHDOG: + case VIR_DOMAIN_DEVICE_GRAPHICS: + case VIR_DOMAIN_DEVICE_HUB: + case VIR_DOMAIN_DEVICE_SMARTCARD: + case VIR_DOMAIN_DEVICE_MEMBALLOON: + case VIR_DOMAIN_DEVICE_NVRAM: + case VIR_DOMAIN_DEVICE_RNG: + case VIR_DOMAIN_DEVICE_SHMEM: + case VIR_DOMAIN_DEVICE_LAST: virReportError(VIR_ERR_OPERATION_UNSUPPORTED, _("live attach of device '%s' is not supported"), virDomainDeviceTypeToString(dev->type)); -- 2.1.0
Our qemu monitor code has a converter from key-value pairs to a json value object. I want to re-use the code later and having it part of the monitor command generator is inflexible. Split it out into a separate helper. --- src/libvirt_private.syms | 2 + src/qemu/qemu_monitor_json.c | 161 +-------------------------------- src/util/virjson.c | 211 +++++++++++++++++++++++++++++++++++++++++++ src/util/virjson.h | 5 + 4 files changed, 220 insertions(+), 159 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d6265ac..314b2b8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1503,6 +1503,8 @@ virJSONValueObjectAppendNumberLong; virJSONValueObjectAppendNumberUint; virJSONValueObjectAppendNumberUlong; virJSONValueObjectAppendString; +virJSONValueObjectCreate; +virJSONValueObjectCreateVArgs; virJSONValueObjectGet; virJSONValueObjectGetBoolean; virJSONValueObjectGetKey; diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 7f23aae..2967193 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -431,7 +431,6 @@ qemuMonitorJSONMakeCommandRaw(bool wrap, const char *cmdname, ...) virJSONValuePtr obj; virJSONValuePtr jargs = NULL; va_list args; - char *key; va_start(args, cmdname); @@ -442,164 +441,8 @@ qemuMonitorJSONMakeCommandRaw(bool wrap, const char *cmdname, ...) cmdname) < 0) goto error; - while ((key = va_arg(args, char *)) != NULL) { - int ret; - char type; - - if (strlen(key) < 3) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("argument key '%s' is too short, missing type prefix"), - key); - goto error; - } - - /* Keys look like s:name the first letter is a type code: - * Explanation of type codes: - * s: string value, must be non-null - * S: string value, omitted if null - * - * i: signed integer value - * j: signed integer value, error if negative - * z: signed integer value, omitted if zero - * y: signed integer value, omitted if zero, error if negative - * - * I: signed long integer value - * J: signed long integer value, error if negative - * Z: signed long integer value, omitted if zero - * Y: signed long integer value, omitted if zero, error if negative - * - * u: unsigned integer value - * p: unsigned integer value, omitted if zero - * - * U: unsigned long integer value (see below for quirks) - * P: unsigned long integer value, omitted if zero - * - * b: boolean value - * B: boolean value, omitted if false - * - * d: double precision floating point number - * n: json null value - * a: json array - */ - type = key[0]; - key += 2; - - if (!jargs && - !(jargs = virJSONValueNewObject())) - goto error; - - /* This doesn't support maps, but no command uses those. */ - switch (type) { - case 'S': - case 's': { - char *val = va_arg(args, char *); - if (!val) { - if (type == 'S') - continue; - - virReportError(VIR_ERR_INTERNAL_ERROR, - _("argument key '%s' must not have null value"), - key); - goto error; - } - ret = virJSONValueObjectAppendString(jargs, key, val); - } break; - - case 'z': - case 'y': - case 'j': - case 'i': { - int val = va_arg(args, int); - - if (val < 0 && (type == 'j' || type == 'y')) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("argument key '%s' must not be negative"), - key); - goto error; - } - - if (!val && (type == 'z' || type == 'y')) - continue; - - ret = virJSONValueObjectAppendNumberInt(jargs, key, val); - } break; - - case 'p': - case 'u': { - unsigned int val = va_arg(args, unsigned int); - - if (!val && type == 'p') - continue; - - ret = virJSONValueObjectAppendNumberUint(jargs, key, val); - } break; - - case 'Z': - case 'Y': - case 'J': - case 'I': { - long long val = va_arg(args, long long); - - if (val < 0 && (type == 'J' || type == 'Y')) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("argument key '%s' must not be negative"), - key); - goto error; - } - - if (!val && (type == 'Z' || type == 'Y')) - continue; - - ret = virJSONValueObjectAppendNumberLong(jargs, key, val); - } break; - - case 'P': - case 'U': { - /* qemu silently truncates numbers larger than LLONG_MAX, - * so passing the full range of unsigned 64 bit integers - * is not safe here. Pass them as signed 64 bit integers - * instead. - */ - long long val = va_arg(args, long long); - - if (!val && type == 'P') - continue; - - ret = virJSONValueObjectAppendNumberLong(jargs, key, val); - } break; - - case 'd': { - double val = va_arg(args, double); - ret = virJSONValueObjectAppendNumberDouble(jargs, key, val); - } break; - - case 'B': - case 'b': { - int val = va_arg(args, int); - - if (!val && type == 'B') - continue; - - ret = virJSONValueObjectAppendBoolean(jargs, key, val); - } break; - - case 'n': { - ret = virJSONValueObjectAppendNull(jargs, key); - } break; - - case 'a': { - virJSONValuePtr val = va_arg(args, virJSONValuePtr); - ret = virJSONValueObjectAppend(jargs, key, val); - } break; - - default: - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unsupported data type '%c' for arg '%s'"), type, key - 2); - goto error; - } - if (ret < 0) - goto error; - } + if (virJSONValueObjectCreateVArgs(&jargs, args) < 0) + goto error; if (jargs && virJSONValueObjectAppend(obj, wrap ? "data" : "arguments", jargs) < 0) diff --git a/src/util/virjson.c b/src/util/virjson.c index ec83b2f..0dfeaed 100644 --- a/src/util/virjson.c +++ b/src/util/virjson.c @@ -63,6 +63,217 @@ struct _virJSONParser { }; +/** + * virJSONValueObjectCreateVArgs: + * @obj: returns the created JSON object + * @...: a key-value argument pairs, terminated by NULL + * + * Creates a JSON value object filled with key-value pairs supplied as variable + * argument list. + * + * Keys look like s:name the first letter is a type code: + * Explanation of type codes: + * s: string value, must be non-null + * S: string value, omitted if null + * + * i: signed integer value + * j: signed integer value, error if negative + * z: signed integer value, omitted if zero + * y: signed integer value, omitted if zero, error if negative + * + * I: signed long integer value + * J: signed long integer value, error if negative + * Z: signed long integer value, omitted if zero + * Y: signed long integer value, omitted if zero, error if negative + * + * u: unsigned integer value + * p: unsigned integer value, omitted if zero + * + * U: unsigned long integer value (see below for quirks) + * P: unsigned long integer value, omitted if zero + * + * b: boolean value + * B: boolean value, omitted if false + * + * d: double precision floating point number + * n: json null value + * a: json array + * + * The value corresponds to the selected type. + * + * Returns -1 on error. 1 on success, if at least one key:pair was valid 0 + * in case of no error but nothing was filled (@obj will be NULL). + */ +int +virJSONValueObjectCreateVArgs(virJSONValuePtr *obj, va_list args) +{ + virJSONValuePtr jargs = NULL; + char type; + char *key; + int ret = -1; + int rc = -2; /* -2 is a sentinel to check if at least one entry was added */ + + *obj = NULL; + + if (!(jargs = virJSONValueNewObject())) + goto cleanup; + + while ((key = va_arg(args, char *)) != NULL) { + + if (strlen(key) < 3) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("argument key '%s' is too short, missing type prefix"), + key); + goto cleanup; + } + + type = key[0]; + key += 2; + + /* This doesn't support maps, but no command uses those. */ + switch (type) { + case 'S': + case 's': { + char *val = va_arg(args, char *); + if (!val) { + if (type == 'S') + continue; + + virReportError(VIR_ERR_INTERNAL_ERROR, + _("argument key '%s' must not have null value"), + key); + goto cleanup; + } + rc = virJSONValueObjectAppendString(jargs, key, val); + } break; + + case 'z': + case 'y': + case 'j': + case 'i': { + int val = va_arg(args, int); + + if (val < 0 && (type == 'j' || type == 'y')) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("argument key '%s' must not be negative"), + key); + goto cleanup; + } + + if (!val && (type == 'z' || type == 'y')) + continue; + + rc = virJSONValueObjectAppendNumberInt(jargs, key, val); + } break; + + case 'p': + case 'u': { + unsigned int val = va_arg(args, unsigned int); + + if (!val && type == 'p') + continue; + + rc = virJSONValueObjectAppendNumberUint(jargs, key, val); + } break; + + case 'Z': + case 'Y': + case 'J': + case 'I': { + long long val = va_arg(args, long long); + + if (val < 0 && (type == 'J' || type == 'Y')) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("argument key '%s' must not be negative"), + key); + goto cleanup; + } + + if (!val && (type == 'Z' || type == 'Y')) + continue; + + rc = virJSONValueObjectAppendNumberLong(jargs, key, val); + } break; + + case 'P': + case 'U': { + /* qemu silently truncates numbers larger than LLONG_MAX, + * so passing the full range of unsigned 64 bit integers + * is not safe here. Pass them as signed 64 bit integers + * instead. + */ + long long val = va_arg(args, long long); + + if (!val && type == 'P') + continue; + + rc = virJSONValueObjectAppendNumberLong(jargs, key, val); + } break; + + case 'd': { + double val = va_arg(args, double); + rc = virJSONValueObjectAppendNumberDouble(jargs, key, val); + } break; + + case 'B': + case 'b': { + int val = va_arg(args, int); + + if (!val && type == 'B') + continue; + + rc = virJSONValueObjectAppendBoolean(jargs, key, val); + } break; + + case 'n': { + rc = virJSONValueObjectAppendNull(jargs, key); + } break; + + case 'a': { + virJSONValuePtr val = va_arg(args, virJSONValuePtr); + rc = virJSONValueObjectAppend(jargs, key, val); + } break; + + default: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unsupported data type '%c' for arg '%s'"), type, key - 2); + goto cleanup; + } + + if (rc < 0) + goto cleanup; + } + + /* verify that we added at least one key-value pair */ + if (rc == -2) { + ret = 0; + goto cleanup; + } + + *obj = jargs; + jargs = NULL; + ret = 1; + + cleanup: + virJSONValueFree(jargs); + return ret; +} + + +int +virJSONValueObjectCreate(virJSONValuePtr *obj, ...) +{ + va_list args; + int ret; + + va_start(args, obj); + ret = virJSONValueObjectCreateVArgs(obj, args); + va_end(args); + + return ret; +} + + void virJSONValueFree(virJSONValuePtr value) { diff --git a/src/util/virjson.h b/src/util/virjson.h index 9487729..be603ae 100644 --- a/src/util/virjson.h +++ b/src/util/virjson.h @@ -26,6 +26,8 @@ # include "internal.h" +# include <stdarg.h> + typedef enum { VIR_JSON_TYPE_OBJECT, @@ -79,6 +81,9 @@ struct _virJSONValue { void virJSONValueFree(virJSONValuePtr value); +int virJSONValueObjectCreate(virJSONValuePtr *obj, ...); +int virJSONValueObjectCreateVArgs(virJSONValuePtr *obj, va_list args); + virJSONValuePtr virJSONValueNewString(const char *data); virJSONValuePtr virJSONValueNewStringLen(const char *data, size_t length); virJSONValuePtr virJSONValueNewNumberInt(int data); -- 2.1.0
On 10/14/2014 03:29 AM, Peter Krempa wrote:
Our qemu monitor code has a converter from key-value pairs to a json value object. I want to re-use the code later and having it part of the monitor command generator is inflexible. Split it out into a separate helper. --- src/libvirt_private.syms | 2 + src/qemu/qemu_monitor_json.c | 161 +-------------------------------- src/util/virjson.c | 211 +++++++++++++++++++++++++++++++++++++++++++ src/util/virjson.h | 5 + 4 files changed, 220 insertions(+), 159 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d6265ac..314b2b8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1503,6 +1503,8 @@ virJSONValueObjectAppendNumberLong; virJSONValueObjectAppendNumberUint; virJSONValueObjectAppendNumberUlong; virJSONValueObjectAppendString; +virJSONValueObjectCreate; +virJSONValueObjectCreateVArgs; virJSONValueObjectGet; virJSONValueObjectGetBoolean; virJSONValueObjectGetKey; diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 7f23aae..2967193 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -431,7 +431,6 @@ qemuMonitorJSONMakeCommandRaw(bool wrap, const char *cmdname, ...) virJSONValuePtr obj; virJSONValuePtr jargs = NULL; va_list args; - char *key;
va_start(args, cmdname);
@@ -442,164 +441,8 @@ qemuMonitorJSONMakeCommandRaw(bool wrap, const char *cmdname, ...) cmdname) < 0) goto error;
- while ((key = va_arg(args, char *)) != NULL) { - int ret; - char type; - - if (strlen(key) < 3) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("argument key '%s' is too short, missing type prefix"), - key); - goto error; - } - - /* Keys look like s:name the first letter is a type code: - * Explanation of type codes: - * s: string value, must be non-null - * S: string value, omitted if null - * - * i: signed integer value - * j: signed integer value, error if negative - * z: signed integer value, omitted if zero - * y: signed integer value, omitted if zero, error if negative - * - * I: signed long integer value - * J: signed long integer value, error if negative - * Z: signed long integer value, omitted if zero - * Y: signed long integer value, omitted if zero, error if negative - * - * u: unsigned integer value - * p: unsigned integer value, omitted if zero - * - * U: unsigned long integer value (see below for quirks) - * P: unsigned long integer value, omitted if zero - * - * b: boolean value - * B: boolean value, omitted if false - * - * d: double precision floating point number - * n: json null value - * a: json array - */ - type = key[0]; - key += 2; - - if (!jargs && - !(jargs = virJSONValueNewObject())) - goto error; - - /* This doesn't support maps, but no command uses those. */ - switch (type) { - case 'S': - case 's': { - char *val = va_arg(args, char *); - if (!val) { - if (type == 'S') - continue; - - virReportError(VIR_ERR_INTERNAL_ERROR, - _("argument key '%s' must not have null value"), - key); - goto error; - } - ret = virJSONValueObjectAppendString(jargs, key, val); - } break; - - case 'z': - case 'y': - case 'j': - case 'i': { - int val = va_arg(args, int); - - if (val < 0 && (type == 'j' || type == 'y')) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("argument key '%s' must not be negative"), - key); - goto error; - } - - if (!val && (type == 'z' || type == 'y')) - continue; - - ret = virJSONValueObjectAppendNumberInt(jargs, key, val); - } break; - - case 'p': - case 'u': { - unsigned int val = va_arg(args, unsigned int); - - if (!val && type == 'p') - continue; - - ret = virJSONValueObjectAppendNumberUint(jargs, key, val); - } break; - - case 'Z': - case 'Y': - case 'J': - case 'I': { - long long val = va_arg(args, long long); - - if (val < 0 && (type == 'J' || type == 'Y')) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("argument key '%s' must not be negative"), - key); - goto error; - } - - if (!val && (type == 'Z' || type == 'Y')) - continue; - - ret = virJSONValueObjectAppendNumberLong(jargs, key, val); - } break; - - case 'P': - case 'U': { - /* qemu silently truncates numbers larger than LLONG_MAX, - * so passing the full range of unsigned 64 bit integers - * is not safe here. Pass them as signed 64 bit integers - * instead. - */ - long long val = va_arg(args, long long); - - if (!val && type == 'P') - continue; - - ret = virJSONValueObjectAppendNumberLong(jargs, key, val); - } break; - - case 'd': { - double val = va_arg(args, double); - ret = virJSONValueObjectAppendNumberDouble(jargs, key, val); - } break; - - case 'B': - case 'b': { - int val = va_arg(args, int); - - if (!val && type == 'B') - continue; - - ret = virJSONValueObjectAppendBoolean(jargs, key, val); - } break; - - case 'n': { - ret = virJSONValueObjectAppendNull(jargs, key); - } break; - - case 'a': { - virJSONValuePtr val = va_arg(args, virJSONValuePtr); - ret = virJSONValueObjectAppend(jargs, key, val); - } break; - - default: - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unsupported data type '%c' for arg '%s'"), type, key - 2); - goto error; - } - if (ret < 0) - goto error; - } + if (virJSONValueObjectCreateVArgs(&jargs, args) < 0) + goto error;
if (jargs && virJSONValueObjectAppend(obj, wrap ? "data" : "arguments", jargs) < 0) diff --git a/src/util/virjson.c b/src/util/virjson.c index ec83b2f..0dfeaed 100644 --- a/src/util/virjson.c +++ b/src/util/virjson.c @@ -63,6 +63,217 @@ struct _virJSONParser { };
+/** + * virJSONValueObjectCreateVArgs: + * @obj: returns the created JSON object + * @...: a key-value argument pairs, terminated by NULL + * + * Creates a JSON value object filled with key-value pairs supplied as variable + * argument list. + * + * Keys look like s:name the first letter is a type code: + * Explanation of type codes: + * s: string value, must be non-null + * S: string value, omitted if null + * + * i: signed integer value + * j: signed integer value, error if negative + * z: signed integer value, omitted if zero + * y: signed integer value, omitted if zero, error if negative + * + * I: signed long integer value + * J: signed long integer value, error if negative + * Z: signed long integer value, omitted if zero + * Y: signed long integer value, omitted if zero, error if negative + * + * u: unsigned integer value + * p: unsigned integer value, omitted if zero + * + * U: unsigned long integer value (see below for quirks) + * P: unsigned long integer value, omitted if zero + * + * b: boolean value + * B: boolean value, omitted if false + * + * d: double precision floating point number + * n: json null value + * a: json array + * + * The value corresponds to the selected type. + * + * Returns -1 on error. 1 on success, if at least one key:pair was valid 0 + * in case of no error but nothing was filled (@obj will be NULL). + */ +int +virJSONValueObjectCreateVArgs(virJSONValuePtr *obj, va_list args) +{ + virJSONValuePtr jargs = NULL; + char type; + char *key; + int ret = -1; + int rc = -2; /* -2 is a sentinel to check if at least one entry was added */ + + *obj = NULL; + + if (!(jargs = virJSONValueNewObject())) + goto cleanup; + + while ((key = va_arg(args, char *)) != NULL) { + + if (strlen(key) < 3) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("argument key '%s' is too short, missing type prefix"), + key); + goto cleanup; + } + + type = key[0]; + key += 2; + + /* This doesn't support maps, but no command uses those. */ + switch (type) { + case 'S': + case 's': { + char *val = va_arg(args, char *); + if (!val) { + if (type == 'S') + continue; + + virReportError(VIR_ERR_INTERNAL_ERROR, + _("argument key '%s' must not have null value"), + key); + goto cleanup; + } + rc = virJSONValueObjectAppendString(jargs, key, val); + } break; + + case 'z': + case 'y': + case 'j': + case 'i': { + int val = va_arg(args, int); + + if (val < 0 && (type == 'j' || type == 'y')) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("argument key '%s' must not be negative"), + key); + goto cleanup; + } + + if (!val && (type == 'z' || type == 'y')) + continue; + + rc = virJSONValueObjectAppendNumberInt(jargs, key, val); + } break; + + case 'p': + case 'u': { + unsigned int val = va_arg(args, unsigned int); + + if (!val && type == 'p') + continue; + + rc = virJSONValueObjectAppendNumberUint(jargs, key, val); + } break; + + case 'Z': + case 'Y': + case 'J': + case 'I': { + long long val = va_arg(args, long long); + + if (val < 0 && (type == 'J' || type == 'Y')) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("argument key '%s' must not be negative"), + key); + goto cleanup; + } + + if (!val && (type == 'Z' || type == 'Y')) + continue; + + rc = virJSONValueObjectAppendNumberLong(jargs, key, val); + } break; + + case 'P': + case 'U': { + /* qemu silently truncates numbers larger than LLONG_MAX, + * so passing the full range of unsigned 64 bit integers + * is not safe here. Pass them as signed 64 bit integers + * instead. + */ + long long val = va_arg(args, long long); + + if (!val && type == 'P') + continue; + + rc = virJSONValueObjectAppendNumberLong(jargs, key, val); + } break; + + case 'd': { + double val = va_arg(args, double); + rc = virJSONValueObjectAppendNumberDouble(jargs, key, val); + } break; + + case 'B': + case 'b': { + int val = va_arg(args, int); + + if (!val && type == 'B') + continue; + + rc = virJSONValueObjectAppendBoolean(jargs, key, val); + } break; + + case 'n': { + rc = virJSONValueObjectAppendNull(jargs, key); + } break; + + case 'a': { + virJSONValuePtr val = va_arg(args, virJSONValuePtr); + rc = virJSONValueObjectAppend(jargs, key, val); + } break; + + default: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unsupported data type '%c' for arg '%s'"), type, key - 2); + goto cleanup; + } + + if (rc < 0) + goto cleanup; + } + + /* verify that we added at least one key-value pair */ + if (rc == -2) {
0, then we added something (eg, npairs count should be incremented)
I didn't check any of the virJSAONValue* functions called, but if any return -2 for whatever reason "sometime" in the future, then this logic is flawed. However, at this point you should have an array size/count right? And if thus virJSONValueObjectKeysNumber() should return > 0
+ ret = 0; + goto cleanup; + } + + *obj = jargs; + jargs = NULL; + ret = 1; + + cleanup: + virJSONValueFree(jargs); + return ret; +} + + +int +virJSONValueObjectCreate(virJSONValuePtr *obj, ...) +{ + va_list args; + int ret; + + va_start(args, obj); + ret = virJSONValueObjectCreateVArgs(obj, args); + va_end(args); + + return ret; +} + + void virJSONValueFree(virJSONValuePtr value) { diff --git a/src/util/virjson.h b/src/util/virjson.h index 9487729..be603ae 100644 --- a/src/util/virjson.h +++ b/src/util/virjson.h @@ -26,6 +26,8 @@
# include "internal.h"
+# include <stdarg.h> +
typedef enum { VIR_JSON_TYPE_OBJECT, @@ -79,6 +81,9 @@ struct _virJSONValue {
void virJSONValueFree(virJSONValuePtr value);
+int virJSONValueObjectCreate(virJSONValuePtr *obj, ...); +int virJSONValueObjectCreateVArgs(virJSONValuePtr *obj, va_list args);
Add an ATTRIBUTE_NONNULL(1) for this definition, since you deref in function. ACK with adjustments John
+ virJSONValuePtr virJSONValueNewString(const char *data); virJSONValuePtr virJSONValueNewStringLen(const char *data, size_t length); virJSONValuePtr virJSONValueNewNumberInt(int data);
On 10/14/14 12:46, John Ferlan wrote:
On 10/14/2014 03:29 AM, Peter Krempa wrote:
Our qemu monitor code has a converter from key-value pairs to a json value object. I want to re-use the code later and having it part of the monitor command generator is inflexible. Split it out into a separate helper. --- src/libvirt_private.syms | 2 + src/qemu/qemu_monitor_json.c | 161 +-------------------------------- src/util/virjson.c | 211 +++++++++++++++++++++++++++++++++++++++++++ src/util/virjson.h | 5 + 4 files changed, 220 insertions(+), 159 deletions(-)
...
typedef enum { VIR_JSON_TYPE_OBJECT, @@ -79,6 +81,9 @@ struct _virJSONValue {
void virJSONValueFree(virJSONValuePtr value);
+int virJSONValueObjectCreate(virJSONValuePtr *obj, ...); +int virJSONValueObjectCreateVArgs(virJSONValuePtr *obj, va_list args);
Add an ATTRIBUTE_NONNULL(1) for this definition, since you deref in function.
ACK with adjustments
I've also added an ATTRIBUTE_SENTINEL to the first function.
John
+ virJSONValuePtr virJSONValueNewString(const char *data); virJSONValuePtr virJSONValueNewStringLen(const char *data, size_t length); virJSONValuePtr virJSONValueNewNumberInt(int data);
Peter
The JSON structure constructor has an option to add JSON arrays to the constructed object. The description is inaccurate as it can add any json object even a dict. Change the docs to cover this option and add possibility to specify NULL object to be added. These will be skipped as we have with other specifiers. --- src/util/virjson.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/src/util/virjson.c b/src/util/virjson.c index 0dfeaed..f490f2c 100644 --- a/src/util/virjson.c +++ b/src/util/virjson.c @@ -97,8 +97,9 @@ struct _virJSONParser { * * d: double precision floating point number * n: json null value - * a: json array * + * a: json object, must be non-NULL + * A: json object, omitted if NULL * The value corresponds to the selected type. * * Returns -1 on error. 1 on success, if at least one key:pair was valid 0 @@ -229,8 +230,20 @@ virJSONValueObjectCreateVArgs(virJSONValuePtr *obj, va_list args) rc = virJSONValueObjectAppendNull(jargs, key); } break; + case 'A': case 'a': { virJSONValuePtr val = va_arg(args, virJSONValuePtr); + + if (!val) { + if (type == 'A') + continue; + + virReportError(VIR_ERR_INTERNAL_ERROR, + _("argument key '%s' must not have null value"), + key); + goto cleanup; + } + rc = virJSONValueObjectAppend(jargs, key, val); } break; -- 2.1.0
On 10/14/2014 03:29 AM, Peter Krempa wrote:
The JSON structure constructor has an option to add JSON arrays to the constructed object. The description is inaccurate as it can add any json object even a dict. Change the docs to cover this option and add possibility to specify NULL object to be added. These will be skipped as we have with other specifiers. --- src/util/virjson.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)
Seems reasonable and I assume in line with other uses although I could see cause for making two patches... #1 to handle the error of 'a' nothing having 'args' and #2 to allow 'A' to avoid that check. Your call if you want to make that split and how it could be used to resolve some unknown bug...
diff --git a/src/util/virjson.c b/src/util/virjson.c index 0dfeaed..f490f2c 100644 --- a/src/util/virjson.c +++ b/src/util/virjson.c @@ -97,8 +97,9 @@ struct _virJSONParser { * * d: double precision floating point number * n: json null value - * a: json array * + * a: json object, must be non-NULL + * A: json object, omitted if NULL
NIT: there used to be an empty line between "a" and the following comment and that's now removed... Consider adding it back in. I like the spacing between 'n' and 'a'... ACK, John
* The value corresponds to the selected type. * * Returns -1 on error. 1 on success, if at least one key:pair was valid 0 @@ -229,8 +230,20 @@ virJSONValueObjectCreateVArgs(virJSONValuePtr *obj, va_list args) rc = virJSONValueObjectAppendNull(jargs, key); } break;
+ case 'A': case 'a': { virJSONValuePtr val = va_arg(args, virJSONValuePtr); + + if (!val) { + if (type == 'A') + continue; + + virReportError(VIR_ERR_INTERNAL_ERROR, + _("argument key '%s' must not have null value"), + key); + goto cleanup; + } + rc = virJSONValueObjectAppend(jargs, key, val); } break;
To allow live modification of device backends in qemu libvirt needs to be able to hot-add/remove "objects". Add monitor backend functions to allow this. --- src/qemu/qemu_monitor.c | 48 +++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor.h | 8 +++++++ src/qemu/qemu_monitor_json.c | 56 ++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 8 +++++++ 4 files changed, 120 insertions(+) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 5dff9ff..dfcd4ed 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3132,6 +3132,54 @@ int qemuMonitorAddDevice(qemuMonitorPtr mon, return qemuMonitorAddDeviceWithFd(mon, devicestr, -1, NULL); } + +/** + * qemuMonitorAddObject: + * @mon: Pointer to monitor object + * @type: Type of object to add + * @objalias: Alias of the new object + * @props: Optional arguments for the given type. The object is consumed and + * should not be referenced by the caller after this function returs. + * + * Returns 0 on success -1 on error. + */ +int +qemuMonitorAddObject(qemuMonitorPtr mon, + const char *type, + const char *objalias, + virJSONValuePtr props) +{ + VIR_DEBUG("mon=%p type=%s objalias=%s props=%p", + mon, type, objalias, props); + int ret = -1; + + if (mon->json) + ret = qemuMonitorJSONAddObject(mon, type, objalias, props); + else + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("object adding requires JSON monitor")); + + return ret; +} + + +int +qemuMonitorDelObject(qemuMonitorPtr mon, + const char *objalias) +{ + VIR_DEBUG("mon=%p objalias=%s", mon, objalias); + int ret = -1; + + if (mon->json) + ret = qemuMonitorJSONDelObject(mon, objalias); + else + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("object deletion requires JSON monitor")); + + return ret; +} + + int qemuMonitorAddDrive(qemuMonitorPtr mon, const char *drivestr) { diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index fd145a7..750b3dc 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -672,6 +672,14 @@ int qemuMonitorAddDeviceWithFd(qemuMonitorPtr mon, int qemuMonitorDelDevice(qemuMonitorPtr mon, const char *devalias); +int qemuMonitorAddObject(qemuMonitorPtr mon, + const char *type, + const char *objalias, + virJSONValuePtr props); + +int qemuMonitorDelObject(qemuMonitorPtr mon, + const char *objalias); + int qemuMonitorAddDrive(qemuMonitorPtr mon, const char *drivestr); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 2967193..ff1fa45 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -3561,6 +3561,62 @@ int qemuMonitorJSONAddDevice(qemuMonitorPtr mon, } +int qemuMonitorJSONAddObject(qemuMonitorPtr mon, + const char *type, + const char *objalias, + virJSONValuePtr props) +{ + int ret = -1; + virJSONValuePtr cmd; + virJSONValuePtr reply = NULL; + + cmd = qemuMonitorJSONMakeCommand("object-add", + "s:qom-type", type, + "s:id", objalias, + "A:props", props, + NULL); + if (!cmd) + goto cleanup; + + props = NULL; + + ret = qemuMonitorJSONCommand(mon, cmd, &reply); + + if (ret == 0) + ret = qemuMonitorJSONCheckError(cmd, reply); + + cleanup: + virJSONValueFree(cmd); + virJSONValueFree(reply); + virJSONValueFree(props); + return ret; +} + + +int qemuMonitorJSONDelObject(qemuMonitorPtr mon, + const char *objalias) +{ + int ret; + virJSONValuePtr cmd; + virJSONValuePtr reply = NULL; + + cmd = qemuMonitorJSONMakeCommand("object-del", + "s:id", objalias, + NULL); + if (!cmd) + return -1; + + ret = qemuMonitorJSONCommand(mon, cmd, &reply); + + if (ret == 0) + ret = qemuMonitorJSONCheckError(cmd, reply); + + virJSONValueFree(cmd); + virJSONValueFree(reply); + return ret; +} + + int qemuMonitorJSONAddDrive(qemuMonitorPtr mon, const char *drivestr) { diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index c898382..6cdaf18 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -229,6 +229,14 @@ int qemuMonitorJSONAddDevice(qemuMonitorPtr mon, int qemuMonitorJSONDelDevice(qemuMonitorPtr mon, const char *devalias); +int qemuMonitorJSONAddObject(qemuMonitorPtr mon, + const char *type, + const char *objalias, + virJSONValuePtr props); + +int qemuMonitorJSONDelObject(qemuMonitorPtr mon, + const char *objalias); + int qemuMonitorJSONAddDrive(qemuMonitorPtr mon, const char *drivestr); -- 2.1.0
On 10/14/2014 03:29 AM, Peter Krempa wrote:
To allow live modification of device backends in qemu libvirt needs to be able to hot-add/remove "objects". Add monitor backend functions to allow this.
Could add a short list of "known" objects that could make use of this... IOThreads, RNG, etc.
--- src/qemu/qemu_monitor.c | 48 +++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor.h | 8 +++++++ src/qemu/qemu_monitor_json.c | 56 ++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 8 +++++++ 4 files changed, 120 insertions(+)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 5dff9ff..dfcd4ed 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3132,6 +3132,54 @@ int qemuMonitorAddDevice(qemuMonitorPtr mon, return qemuMonitorAddDeviceWithFd(mon, devicestr, -1, NULL); }
+ +/** + * qemuMonitorAddObject: + * @mon: Pointer to monitor object + * @type: Type of object to add
Seems this could have been an 'int' typed value which would have ToString type function to convert into "expected" strings; otherwise, how does one know what to pass and what's valid or not. I'm assuming it's device specific, but there must be "something" that qemu expects. oh...hmm... are you modeling this after qemuMonitorGetObjectProps usage of 'type' and that'll be the callers problem?
+ * @objalias: Alias of the new object + * @props: Optional arguments for the given type. The object is consumed and + * should not be referenced by the caller after this function returs.
s/returs/returns
+ * + * Returns 0 on success -1 on error. + */ +int +qemuMonitorAddObject(qemuMonitorPtr mon, + const char *type, + const char *objalias, + virJSONValuePtr props) +{ + VIR_DEBUG("mon=%p type=%s objalias=%s props=%p", + mon, type, objalias, props); + int ret = -1; + + if (mon->json) + ret = qemuMonitorJSONAddObject(mon, type, objalias, props); + else + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("object adding requires JSON monitor")); + + return ret; +} + + +int +qemuMonitorDelObject(qemuMonitorPtr mon, + const char *objalias) +{ + VIR_DEBUG("mon=%p objalias=%s", mon, objalias); + int ret = -1; + + if (mon->json) + ret = qemuMonitorJSONDelObject(mon, objalias); + else + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("object deletion requires JSON monitor")); + + return ret; +} + + int qemuMonitorAddDrive(qemuMonitorPtr mon, const char *drivestr) { diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index fd145a7..750b3dc 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -672,6 +672,14 @@ int qemuMonitorAddDeviceWithFd(qemuMonitorPtr mon, int qemuMonitorDelDevice(qemuMonitorPtr mon, const char *devalias);
+int qemuMonitorAddObject(qemuMonitorPtr mon, + const char *type, + const char *objalias, + virJSONValuePtr props); + +int qemuMonitorDelObject(qemuMonitorPtr mon, + const char *objalias); + int qemuMonitorAddDrive(qemuMonitorPtr mon, const char *drivestr);
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 2967193..ff1fa45 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -3561,6 +3561,62 @@ int qemuMonitorJSONAddDevice(qemuMonitorPtr mon, }
+int qemuMonitorJSONAddObject(qemuMonitorPtr mon, + const char *type, + const char *objalias, + virJSONValuePtr props) +{ + int ret = -1; + virJSONValuePtr cmd; + virJSONValuePtr reply = NULL; + + cmd = qemuMonitorJSONMakeCommand("object-add", + "s:qom-type", type, + "s:id", objalias, + "A:props", props, + NULL); + if (!cmd) + goto cleanup; +
Like another place in the code, make it clear why... /* @props is part of @cmd now. Avoid double free */
+ props = NULL; + + ret = qemuMonitorJSONCommand(mon, cmd, &reply); + + if (ret == 0) + ret = qemuMonitorJSONCheckError(cmd, reply); + + cleanup: + virJSONValueFree(cmd); + virJSONValueFree(reply); + virJSONValueFree(props); + return ret; +} +
Whether you use an 'int' type for AddObject or not is your call. I wasn't clear what the parameter was or how it should be used/filled in since there is no consumer of this API yet. ACK John
+ +int qemuMonitorJSONDelObject(qemuMonitorPtr mon, + const char *objalias) +{ + int ret; + virJSONValuePtr cmd; + virJSONValuePtr reply = NULL; + + cmd = qemuMonitorJSONMakeCommand("object-del", + "s:id", objalias, + NULL); + if (!cmd) + return -1; + + ret = qemuMonitorJSONCommand(mon, cmd, &reply); + + if (ret == 0) + ret = qemuMonitorJSONCheckError(cmd, reply); + + virJSONValueFree(cmd); + virJSONValueFree(reply); + return ret; +} + + int qemuMonitorJSONAddDrive(qemuMonitorPtr mon, const char *drivestr) { diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index c898382..6cdaf18 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -229,6 +229,14 @@ int qemuMonitorJSONAddDevice(qemuMonitorPtr mon, int qemuMonitorJSONDelDevice(qemuMonitorPtr mon, const char *devalias);
+int qemuMonitorJSONAddObject(qemuMonitorPtr mon, + const char *type, + const char *objalias, + virJSONValuePtr props); + +int qemuMonitorJSONDelObject(qemuMonitorPtr mon, + const char *objalias); + int qemuMonitorJSONAddDrive(qemuMonitorPtr mon, const char *drivestr);
To allow easy implementation of a callback check this patch adds empty post parse callbacks to drivers that were missing them. --- src/bhyve/bhyve_domain.c | 10 ++++++++++ src/parallels/parallels_driver.c | 21 +++++++++++++++++++++ src/phyp/phyp_driver.c | 29 ++++++++++++++++++++++++++++- src/uml/uml_driver.c | 10 ++++++++++ src/vbox/vbox_common.c | 19 +++++++++++++++++++ src/vmware/vmware_driver.c | 24 +++++++++++++++++++++++- src/vmx/vmx.c | 18 ++++++++++++++++++ src/xenapi/xenapi_driver.c | 10 ++++++++++ 8 files changed, 139 insertions(+), 2 deletions(-) diff --git a/src/bhyve/bhyve_domain.c b/src/bhyve/bhyve_domain.c index 7c7bec3..ecb1758 100644 --- a/src/bhyve/bhyve_domain.c +++ b/src/bhyve/bhyve_domain.c @@ -70,6 +70,16 @@ bhyveDomainDefPostParse(virDomainDefPtr def, return 0; } +static int +bhyveDomainDeviceDefPostParse(virDomainDeviceDefPtr dev ATTRIBUTE_UNUSED, + const virDomainDef *def ATTRIBUTE_UNUSED, + virCapsPtr caps ATTRIBUTE_UNUSED, + void *opaque ATTRIBUTE_UNUSED) +{ + return 0; +} + virDomainDefParserConfig virBhyveDriverDomainDefParserConfig = { + .devicesPostParseCallback = bhyveDomainDeviceDefPostParse, .domainPostParseCallback = bhyveDomainDefPostParse, }; diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c index 516a296..04c4bb3 100644 --- a/src/parallels/parallels_driver.c +++ b/src/parallels/parallels_driver.c @@ -915,8 +915,29 @@ parallelsLoadDomains(parallelsConnPtr privconn, const char *domain_name) } +static int +parallelsDomainDefPostParse(virDomainDefPtr def ATTRIBUTE_UNUSED, + virCapsPtr caps ATTRIBUTE_UNUSED, + void *opaque ATTRIBUTE_UNUSED) +{ + return 0; +} + + +static int +parallelsDomainDeviceDefPostParse(virDomainDeviceDefPtr dev ATTRIBUTE_UNUSED, + const virDomainDef *def ATTRIBUTE_UNUSED, + virCapsPtr caps ATTRIBUTE_UNUSED, + void *opaque ATTRIBUTE_UNUSED) +{ + return 0; +} + + virDomainDefParserConfig parallelsDomainDefParserConfig = { .macPrefix = {0x42, 0x1C, 0x00}, + .devicesPostParseCallback = parallelsDomainDeviceDefPostParse, + .domainPostParseCallback = parallelsDomainDefPostParse, }; diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 25f7f2d..6a5a560 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -1054,6 +1054,32 @@ openSSHSession(virConnectPtr conn, virConnectAuthPtr auth, return session; } + +static int +phypDomainDefPostParse(virDomainDefPtr def ATTRIBUTE_UNUSED, + virCapsPtr caps ATTRIBUTE_UNUSED, + void *opaque ATTRIBUTE_UNUSED) +{ + return 0; +} + + +static int +phypDomainDeviceDefPostParse(virDomainDeviceDefPtr dev ATTRIBUTE_UNUSED, + const virDomainDef *def ATTRIBUTE_UNUSED, + virCapsPtr caps ATTRIBUTE_UNUSED, + void *opaque ATTRIBUTE_UNUSED) +{ + return 0; +} + + +virDomainDefParserConfig virPhypDriverDomainDefParserConfig = { + .devicesPostParseCallback = phypDomainDeviceDefPostParse, + .domainPostParseCallback = phypDomainDefPostParse, +}; + + static virDrvOpenStatus phypConnectOpen(virConnectPtr conn, virConnectAuthPtr auth, unsigned int flags) @@ -1131,7 +1157,8 @@ phypConnectOpen(virConnectPtr conn, if ((phyp_driver->caps = phypCapsInit()) == NULL) goto failure; - if (!(phyp_driver->xmlopt = virDomainXMLOptionNew(NULL, NULL, NULL))) + if (!(phyp_driver->xmlopt = virDomainXMLOptionNew(&virPhypDriverDomainDefParserConfig, + NULL, NULL))) goto failure; conn->privateData = phyp_driver; diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 2a40149..7e25861 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -444,8 +444,18 @@ umlDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, } +static int +umlDomainDefPostParse(virDomainDefPtr def ATTRIBUTE_UNUSED, + virCapsPtr caps ATTRIBUTE_UNUSED, + void *opaque ATTRIBUTE_UNUSED) +{ + return 0; +} + + virDomainDefParserConfig umlDriverDomainDefParserConfig = { .devicesPostParseCallback = umlDomainDeviceDefPostParse, + .domainPostParseCallback = umlDomainDefPostParse, }; diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 44270ff..2170dc1 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -314,8 +314,27 @@ static char *vboxGenerateMediumName(PRUint32 storageBus, return name; } +static int +vboxDomainDefPostParse(virDomainDefPtr def ATTRIBUTE_UNUSED, + virCapsPtr caps ATTRIBUTE_UNUSED, + void *opaque ATTRIBUTE_UNUSED) +{ + return 0; +} + +static int +vboxDomainDeviceDefPostParse(virDomainDeviceDefPtr dev ATTRIBUTE_UNUSED, + const virDomainDef *def ATTRIBUTE_UNUSED, + virCapsPtr caps ATTRIBUTE_UNUSED, + void *opaque ATTRIBUTE_UNUSED) +{ + return 0; +} + static virDomainDefParserConfig vboxDomainDefParserConfig = { .macPrefix = { 0x08, 0x00, 0x27 }, + .devicesPostParseCallback = vboxDomainDeviceDefPostParse, + .domainPostParseCallback = vboxDomainDefPostParse, }; static virDomainXMLOptionPtr diff --git a/src/vmware/vmware_driver.c b/src/vmware/vmware_driver.c index 6edc0bc..22ce3a3 100644 --- a/src/vmware/vmware_driver.c +++ b/src/vmware/vmware_driver.c @@ -82,13 +82,35 @@ vmwareDataFreeFunc(void *data) VIR_FREE(dom); } +static int +vmwareDomainDefPostParse(virDomainDefPtr def ATTRIBUTE_UNUSED, + virCapsPtr caps ATTRIBUTE_UNUSED, + void *opaque ATTRIBUTE_UNUSED) +{ + return 0; +} + +static int +vmwareDomainDeviceDefPostParse(virDomainDeviceDefPtr dev ATTRIBUTE_UNUSED, + const virDomainDef *def ATTRIBUTE_UNUSED, + virCapsPtr caps ATTRIBUTE_UNUSED, + void *opaque ATTRIBUTE_UNUSED) +{ + return 0; +} + +virDomainDefParserConfig vmwareDomainDefParserConfig = { + .devicesPostParseCallback = vmwareDomainDeviceDefPostParse, + .domainPostParseCallback = vmwareDomainDefPostParse, +}; + static virDomainXMLOptionPtr vmwareDomainXMLConfigInit(void) { virDomainXMLPrivateDataCallbacks priv = { .alloc = vmwareDataAllocFunc, .free = vmwareDataFreeFunc }; - return virDomainXMLOptionNew(NULL, &priv, NULL); + return virDomainXMLOptionNew(&vmwareDomainDefParserConfig, &priv, NULL); } static virDrvOpenStatus diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index cd6c51e..389a012 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -523,10 +523,28 @@ VIR_ENUM_IMPL(virVMXControllerModelSCSI, VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST, /* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * Helpers */ +static int +vmxDomainDefPostParse(virDomainDefPtr def ATTRIBUTE_UNUSED, + virCapsPtr caps ATTRIBUTE_UNUSED, + void *opaque ATTRIBUTE_UNUSED) +{ + return 0; +} + +static int +vmxDomainDeviceDefPostParse(virDomainDeviceDefPtr dev ATTRIBUTE_UNUSED, + const virDomainDef *def ATTRIBUTE_UNUSED, + virCapsPtr caps ATTRIBUTE_UNUSED, + void *opaque ATTRIBUTE_UNUSED) +{ + return 0; +} virDomainDefParserConfig virVMXDomainDefParserConfig = { .hasWideSCSIBus = true, .macPrefix = {0x00, 0x0c, 0x29}, + .devicesPostParseCallback = vmxDomainDeviceDefPostParse, + .domainPostParseCallback = vmxDomainDefPostParse, }; diff --git a/src/xenapi/xenapi_driver.c b/src/xenapi/xenapi_driver.c index e4fa7cd..a448347 100644 --- a/src/xenapi/xenapi_driver.c +++ b/src/xenapi/xenapi_driver.c @@ -69,8 +69,18 @@ xenapiDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, } +static int +xenapiDomainDefPostParse(virDomainDefPtr def ATTRIBUTE_UNUSED, + virCapsPtr caps ATTRIBUTE_UNUSED, + void *opaque ATTRIBUTE_UNUSED) +{ + return 0; +} + + virDomainDefParserConfig xenapiDomainDefParserConfig = { .devicesPostParseCallback = xenapiDomainDeviceDefPostParse, + .domainPostParseCallback = xenapiDomainDefPostParse, }; -- 2.1.0
On 10/14/2014 03:29 AM, Peter Krempa wrote:
To allow easy implementation of a callback check this patch adds empty post parse callbacks to drivers that were missing them. --- src/bhyve/bhyve_domain.c | 10 ++++++++++ src/parallels/parallels_driver.c | 21 +++++++++++++++++++++ src/phyp/phyp_driver.c | 29 ++++++++++++++++++++++++++++- src/uml/uml_driver.c | 10 ++++++++++ src/vbox/vbox_common.c | 19 +++++++++++++++++++ src/vmware/vmware_driver.c | 24 +++++++++++++++++++++++- src/vmx/vmx.c | 18 ++++++++++++++++++ src/xenapi/xenapi_driver.c | 10 ++++++++++ 8 files changed, 139 insertions(+), 2 deletions(-)
Seems reasonable ACK John
Shove it to the top of the file so that it can be reused earlier. --- src/conf/domain_conf.c | 60 +++++++++++++++++++++++++------------------------- 1 file changed, 30 insertions(+), 30 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0634116..42c0223 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6372,6 +6372,36 @@ virDomainParseScaledValue(const char *xpath, } +/* Parse a memory element located at XPATH within CTXT, and store the + * result into MEM. If REQUIRED, then the value must exist; + * otherwise, the value is optional. The value is in blocks of 1024. + * Return 0 on success, -1 on failure after issuing error. */ +static int +virDomainParseMemory(const char *xpath, xmlXPathContextPtr ctxt, + unsigned long long *mem, bool required) +{ + int ret = -1; + unsigned long long bytes, max; + + /* On 32-bit machines, our bound is 0xffffffff * KiB. On 64-bit + * machines, our bound is off_t (2^63). */ + if (sizeof(unsigned long) < sizeof(long long)) + max = 1024ull * ULONG_MAX; + else + max = LLONG_MAX; + + ret = virDomainParseScaledValue(xpath, ctxt, &bytes, 1024, max, required); + if (ret < 0) + goto cleanup; + + /* Yes, we really do use kibibytes for our internal sizing. */ + *mem = VIR_DIV_UP(bytes, 1024); + ret = 0; + cleanup: + return ret; +} + + static int virDomainControllerModelTypeFromString(const virDomainControllerDef *def, const char *model) @@ -11917,36 +11947,6 @@ virDomainDefMaybeAddInput(virDomainDefPtr def, } -/* Parse a memory element located at XPATH within CTXT, and store the - * result into MEM. If REQUIRED, then the value must exist; - * otherwise, the value is optional. The value is in blocks of 1024. - * Return 0 on success, -1 on failure after issuing error. */ -static int -virDomainParseMemory(const char *xpath, xmlXPathContextPtr ctxt, - unsigned long long *mem, bool required) -{ - int ret = -1; - unsigned long long bytes, max; - - /* On 32-bit machines, our bound is 0xffffffff * KiB. On 64-bit - * machines, our bound is off_t (2^63). */ - if (sizeof(unsigned long) < sizeof(long long)) - max = 1024ull * ULONG_MAX; - else - max = LLONG_MAX; - - ret = virDomainParseScaledValue(xpath, ctxt, &bytes, 1024, max, required); - if (ret < 0) - goto cleanup; - - /* Yes, we really do use kibibytes for our internal sizing. */ - *mem = VIR_DIV_UP(bytes, 1024); - ret = 0; - cleanup: - return ret; -} - - static int virDomainHugepagesParseXML(xmlNodePtr node, xmlXPathContextPtr ctxt, -- 2.1.0
On 10/14/2014 03:29 AM, Peter Krempa wrote:
Shove it to the top of the file so that it can be reused earlier. --- src/conf/domain_conf.c | 60 +++++++++++++++++++++++++------------------------- 1 file changed, 30 insertions(+), 30 deletions(-)
Code motion ACK John
On 10/14/14 12:59, John Ferlan wrote:
On 10/14/2014 03:29 AM, Peter Krempa wrote:
Shove it to the top of the file so that it can be reused earlier. --- src/conf/domain_conf.c | 60 +++++++++++++++++++++++++------------------------- 1 file changed, 30 insertions(+), 30 deletions(-)
Code motion
ACK
John
I've went ahead and pushed the series with the suggested fixes. Peter
participants (2)
-
John Ferlan -
Peter Krempa