On 10/14/2014 03:29 AM, Peter Krempa wrote: 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

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

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

--- 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/14 12:46, John Ferlan wrote: ... I've also added an ATTRIBUTE_SENTINEL to the first function. Peter

On 10/14/2014 03:29 AM, Peter Krempa wrote: 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... 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

On 10/14/2014 03:29 AM, Peter Krempa wrote: Could add a short list of "known" objects that could make use of this... IOThreads, RNG, etc. 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? s/returs/returns Like another place in the code, make it clear why... /* @props is part of @cmd now. Avoid double free */ 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