[libvirt] [PATCH 00/12] memory hotplug: Preliminary fixes and cleanups

This series was split out from my memory hotplug series that is not quite ready yet. The aim of this series is to refactor the code that creates commandline for the memory-backend-* qemu object so that it can be later reused for hotplug via monitor. Additionally this series also fixes a bug in NUMA memory initialisation where qemu doesn't support a combination of old and new approach. Peter Krempa (12): conf: numatune: Extract code for requesting memory nodeset from formatting test: utils: Add helpers for automatic numbering of test cases util: json: make value object creator universal by supporting adding util: json: Add functions to convert JSON arrays from/to virBitmaps util: json: add helper to iterate JSON object key=value pairs qemu: command: Add helper to format -object strings from JSON representation qemu: Extract code to setup memory backing objects qemu: command: Shuffle around formating of alias for memory backend objs qemu: command: Unify values for boolean values when formating memory backends qemu: command: Switch to bytes when formatting size for memory backends qemu: command: Refactor NUMA backend object formatting to use JSON objs qemu: command: Don't combine old and modern NUMA node creation src/conf/numatune_conf.c | 33 +- src/conf/numatune_conf.h | 5 + src/libvirt_private.syms | 6 + src/qemu/qemu_command.c | 547 +++++++++++++++------ src/qemu/qemu_command.h | 4 + src/util/virjson.c | 220 ++++++++- src/util/virjson.h | 17 + tests/Makefile.am | 13 +- tests/qemucommandutiltest.c | 118 +++++ .../qemuxml2argv-hugepages-pages.args | 20 +- .../qemuxml2argv-hugepages-pages2.args | 8 +- .../qemuxml2argv-hugepages-pages3.args | 7 +- .../qemuxml2argv-hugepages-shared.args | 20 +- .../qemuxml2argv-numatune-memnode-no-memory.args | 6 +- .../qemuxml2argv-numatune-memnode.args | 8 +- tests/testutils.c | 46 ++ tests/testutils.h | 3 + 17 files changed, 871 insertions(+), 210 deletions(-) create mode 100644 tests/qemucommandutiltest.c -- 2.2.2

Extract the logic to determine which nodeset has to be used for a domain from the formatting step so that it can be reused separately when the nodeset is used in a different way. --- src/conf/numatune_conf.c | 33 ++++++++++++++++++++++++++------- src/conf/numatune_conf.h | 5 +++++ src/libvirt_private.syms | 1 + 3 files changed, 32 insertions(+), 7 deletions(-) diff --git a/src/conf/numatune_conf.c b/src/conf/numatune_conf.c index ad928e0..16610ed 100644 --- a/src/conf/numatune_conf.c +++ b/src/conf/numatune_conf.c @@ -400,13 +400,14 @@ virDomainNumatuneFormatNodeset(virDomainNumatunePtr numatune, cellid)); } + int -virDomainNumatuneMaybeFormatNodeset(virDomainNumatunePtr numatune, - virBitmapPtr auto_nodeset, - char **mask, - int cellid) +virDomainNumatuneMaybeGetNodeset(virDomainNumatunePtr numatune, + virBitmapPtr auto_nodeset, + virBitmapPtr *retNodeset, + int cellid) { - *mask = NULL; + *retNodeset = NULL; if (!numatune) return 0; @@ -424,8 +425,26 @@ virDomainNumatuneMaybeFormatNodeset(virDomainNumatunePtr numatune, return -1; } - *mask = virDomainNumatuneFormatNodeset(numatune, auto_nodeset, cellid); - if (!*mask) + *retNodeset = virDomainNumatuneGetNodeset(numatune, auto_nodeset, cellid); + + return 0; +} + + +int +virDomainNumatuneMaybeFormatNodeset(virDomainNumatunePtr numatune, + virBitmapPtr auto_nodeset, + char **mask, + int cellid) +{ + virBitmapPtr nodeset; + + if (virDomainNumatuneMaybeGetNodeset(numatune, auto_nodeset, &nodeset, + cellid) < 0) + return -1; + + if (nodeset && + !(*mask = virBitmapFormat(nodeset))) return -1; return 0; diff --git a/src/conf/numatune_conf.h b/src/conf/numatune_conf.h index 7ca7f97..28c4ce2 100644 --- a/src/conf/numatune_conf.h +++ b/src/conf/numatune_conf.h @@ -70,6 +70,11 @@ virBitmapPtr virDomainNumatuneGetNodeset(virDomainNumatunePtr numatune, virBitmapPtr auto_nodeset, int cellid); +int virDomainNumatuneMaybeGetNodeset(virDomainNumatunePtr numatune, + virBitmapPtr auto_nodeset, + virBitmapPtr *retNodeset, + int cellid); + /* * Formatters */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 75a6d83..2bbce03 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -625,6 +625,7 @@ virDomainNumatuneGetNodeset; virDomainNumatuneHasPerNodeBinding; virDomainNumatuneHasPlacementAuto; virDomainNumatuneMaybeFormatNodeset; +virDomainNumatuneMaybeGetNodeset; virDomainNumatuneMemModeTypeFromString; virDomainNumatuneMemModeTypeToString; virDomainNumatuneNodesetIsAvailable; -- 2.2.2

Adding or reordering test cases is usually a pain due to static test case names that are then passed to virtTestRun(). To ease the numbering of test cases, this patch adds two simple helpers that generate the test names according to the order they are run. The test name can be configured via the reset function. This will allow us to freely add test cases in mid of test groups without the need to re-number the rest of test cases. --- tests/testutils.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ tests/testutils.h | 3 +++ 2 files changed, 49 insertions(+) diff --git a/tests/testutils.c b/tests/testutils.c index 9a79f98..c7d2615 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -986,3 +986,49 @@ virDomainXMLOptionPtr virTestGenericDomainXMLConfInit(void) &virTestGenericPrivateDataCallbacks, NULL); } + + +static int virtTestCounter; +static char virtTestCounterStr[128]; +static char *virtTestCounterPrefixEndOffset; + + +/** + * virtTestCounterReset: + * @prefix: name of the test group + * + * Resets the counter and sets up the test group name to use with + * virtTestCounterNext(). This function is not thread safe. + */ +void +virtTestCounterReset(const char *prefix) +{ + virtTestCounter = 0; + + ignore_value(virStrcpyStatic(virtTestCounterStr, prefix)); + virtTestCounterPrefixEndOffset = strchrnul(virtTestCounterStr, '\0'); +} + + +/** + * virtTestCounterNext: + * + * This function is designed to ease test creation and reordering by adding + * a way to do automagic test case numbring. + * + * Returns string consisting of test name prefix configured via + * virtTestCounterReset() and a number that increments in every call of this + * function. This function is not thread safe. + */ +const char +*virtTestCounterNext(void) +{ + size_t len = ARRAY_CARDINALITY(virtTestCounterStr); + + /* calculate length of the rest of the string */ + len -= (virtTestCounterPrefixEndOffset - virtTestCounterStr); + + snprintf(virtTestCounterPrefixEndOffset, len, "%d", ++virtTestCounter); + + return virtTestCounterStr; +} diff --git a/tests/testutils.h b/tests/testutils.h index d78818d..155b30f 100644 --- a/tests/testutils.h +++ b/tests/testutils.h @@ -82,6 +82,9 @@ char *virtTestLogContentAndReset(void); void virtTestQuiesceLibvirtErrors(bool always); +void virtTestCounterReset(const char *prefix); +const char *virtTestCounterNext(void); + int virtTestMain(int argc, char **argv, int (*func)(void)); -- 2.2.2

I don't see this being used anywhere in this set of patches... I assume you have some other upcoming patch series that will use it... NITs On 01/28/2015 05:30 AM, Peter Krempa wrote:
Adding or reordering test cases is usually a pain due to static test case names that are then passed to virtTestRun(). To ease the numbering of test cases, this patch adds two simple helpers that generate the test names according to the order they are run. The test name can be configured via the reset function.
This will allow us to freely add test cases in mid of test groups
s/in mid of/in the middle of/
without the need to re-number the rest of test cases. --- tests/testutils.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ tests/testutils.h | 3 +++ 2 files changed, 49 insertions(+)
diff --git a/tests/testutils.c b/tests/testutils.c index 9a79f98..c7d2615 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -986,3 +986,49 @@ virDomainXMLOptionPtr virTestGenericDomainXMLConfInit(void) &virTestGenericPrivateDataCallbacks, NULL); } + + +static int virtTestCounter; +static char virtTestCounterStr[128]; +static char *virtTestCounterPrefixEndOffset; + + +/** + * virtTestCounterReset: + * @prefix: name of the test group + * + * Resets the counter and sets up the test group name to use with + * virtTestCounterNext(). This function is not thread safe. + */ +void +virtTestCounterReset(const char *prefix) +{ + virtTestCounter = 0;
Not that it'd happen, but if the prefix was larger than 128 characters... ;-)
+ + ignore_value(virStrcpyStatic(virtTestCounterStr, prefix)); + virtTestCounterPrefixEndOffset = strchrnul(virtTestCounterStr, '\0'); +} + + +/** + * virtTestCounterNext: + * + * This function is designed to ease test creation and reordering by adding + * a way to do automagic test case numbring.
s/numbring/numbering
+ * + * Returns string consisting of test name prefix configured via + * virtTestCounterReset() and a number that increments in every call of this + * function. This function is not thread safe. + */ +const char +*virtTestCounterNext(void) +{ + size_t len = ARRAY_CARDINALITY(virtTestCounterStr); + + /* calculate length of the rest of the string */ + len -= (virtTestCounterPrefixEndOffset - virtTestCounterStr); + + snprintf(virtTestCounterPrefixEndOffset, len, "%d", ++virtTestCounter); + + return virtTestCounterStr; +} diff --git a/tests/testutils.h b/tests/testutils.h index d78818d..155b30f 100644 --- a/tests/testutils.h +++ b/tests/testutils.h @@ -82,6 +82,9 @@ char *virtTestLogContentAndReset(void);
void virtTestQuiesceLibvirtErrors(bool always);
+void virtTestCounterReset(const char *prefix); +const char *virtTestCounterNext(void); + int virtTestMain(int argc, char **argv, int (*func)(void));

On 01/29/2015 02:23 PM, John Ferlan wrote:
I don't see this being used anywhere in this set of patches... I assume you have some other upcoming patch series that will use it...
NM - my cscope autorebuild failed <sigh>... I found the usage in 6/12 John

On Thu, Jan 29, 2015 at 14:23:52 -0500, John Ferlan wrote:
I don't see this being used anywhere in this set of patches... I assume you have some other upcoming patch series that will use it...
NITs
On 01/28/2015 05:30 AM, Peter Krempa wrote:
Adding or reordering test cases is usually a pain due to static test case names that are then passed to virtTestRun(). To ease the numbering of test cases, this patch adds two simple helpers that generate the test names according to the order they are run. The test name can be configured via the reset function.
This will allow us to freely add test cases in mid of test groups
s/in mid of/in the middle of/
without the need to re-number the rest of test cases. --- tests/testutils.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ tests/testutils.h | 3 +++ 2 files changed, 49 insertions(+)
diff --git a/tests/testutils.c b/tests/testutils.c index 9a79f98..c7d2615 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -986,3 +986,49 @@ virDomainXMLOptionPtr virTestGenericDomainXMLConfInit(void) &virTestGenericPrivateDataCallbacks, NULL); } + + +static int virtTestCounter; +static char virtTestCounterStr[128]; +static char *virtTestCounterPrefixEndOffset; + + +/** + * virtTestCounterReset: + * @prefix: name of the test group + * + * Resets the counter and sets up the test group name to use with + * virtTestCounterNext(). This function is not thread safe. + */ +void +virtTestCounterReset(const char *prefix) +{ + virtTestCounter = 0;
Not that it'd happen, but if the prefix was larger than 128 characters... ;-)
I can document it if it would help. I wanted to avoid memory allocation in the testsuite so that this code can't introduce a failure. A worst case scenario here should be that the test names will be truncated which will make it less obvious in case of a failure. In such case we can always bump the size of the string.
Peter

To allow constructing of value objects stepwise explode the helper into separate steps and allow appending into existing value objects. --- src/libvirt_private.syms | 2 ++ src/util/virjson.c | 74 +++++++++++++++++++++++++++++++----------------- src/util/virjson.h | 5 ++++ 3 files changed, 55 insertions(+), 26 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 2bbce03..cc74e35 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1533,6 +1533,8 @@ virJSONValueNewNumberUlong; virJSONValueNewObject; virJSONValueNewString; virJSONValueNewStringLen; +virJSONValueObjectAdd; +virJSONValueObjectAddVArgs; virJSONValueObjectAppend; virJSONValueObjectAppendBoolean; virJSONValueObjectAppendNull; diff --git a/src/util/virjson.c b/src/util/virjson.c index 9f2e1cf..9eb1bff 100644 --- a/src/util/virjson.c +++ b/src/util/virjson.c @@ -64,12 +64,11 @@ struct _virJSONParser { /** - * virJSONValueObjectCreateVArgs: - * @obj: returns the created JSON object - * @...: a key-value argument pairs, terminated by NULL + * virJSONValueObjectAddVArgs: + * @obj: JSON object to add the values to + * @args: a key-value argument pairs, terminated by NULL * - * Creates a JSON value object filled with key-value pairs supplied as variable - * argument list. + * Adds the key-value pairs supplied as variable argument list to @obj. * * Keys look like s:name the first letter is a type code: * Explanation of type codes: @@ -104,22 +103,17 @@ struct _virJSONParser { * 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). + * in case of no error but nothing was filled. */ int -virJSONValueObjectCreateVArgs(virJSONValuePtr *obj, va_list args) +virJSONValueObjectAddVArgs(virJSONValuePtr obj, + va_list args) { - virJSONValuePtr jargs = NULL; char type; char *key; int ret = -1; int rc; - *obj = NULL; - - if (!(jargs = virJSONValueNewObject())) - goto cleanup; - while ((key = va_arg(args, char *)) != NULL) { if (strlen(key) < 3) { @@ -146,7 +140,7 @@ virJSONValueObjectCreateVArgs(virJSONValuePtr *obj, va_list args) key); goto cleanup; } - rc = virJSONValueObjectAppendString(jargs, key, val); + rc = virJSONValueObjectAppendString(obj, key, val); } break; case 'z': @@ -165,7 +159,7 @@ virJSONValueObjectCreateVArgs(virJSONValuePtr *obj, va_list args) if (!val && (type == 'z' || type == 'y')) continue; - rc = virJSONValueObjectAppendNumberInt(jargs, key, val); + rc = virJSONValueObjectAppendNumberInt(obj, key, val); } break; case 'p': @@ -175,7 +169,7 @@ virJSONValueObjectCreateVArgs(virJSONValuePtr *obj, va_list args) if (!val && type == 'p') continue; - rc = virJSONValueObjectAppendNumberUint(jargs, key, val); + rc = virJSONValueObjectAppendNumberUint(obj, key, val); } break; case 'Z': @@ -194,7 +188,7 @@ virJSONValueObjectCreateVArgs(virJSONValuePtr *obj, va_list args) if (!val && (type == 'Z' || type == 'Y')) continue; - rc = virJSONValueObjectAppendNumberLong(jargs, key, val); + rc = virJSONValueObjectAppendNumberLong(obj, key, val); } break; case 'P': @@ -209,12 +203,12 @@ virJSONValueObjectCreateVArgs(virJSONValuePtr *obj, va_list args) if (!val && type == 'P') continue; - rc = virJSONValueObjectAppendNumberLong(jargs, key, val); + rc = virJSONValueObjectAppendNumberLong(obj, key, val); } break; case 'd': { double val = va_arg(args, double); - rc = virJSONValueObjectAppendNumberDouble(jargs, key, val); + rc = virJSONValueObjectAppendNumberDouble(obj, key, val); } break; case 'B': @@ -224,11 +218,11 @@ virJSONValueObjectCreateVArgs(virJSONValuePtr *obj, va_list args) if (!val && type == 'B') continue; - rc = virJSONValueObjectAppendBoolean(jargs, key, val); + rc = virJSONValueObjectAppendBoolean(obj, key, val); } break; case 'n': { - rc = virJSONValueObjectAppendNull(jargs, key); + rc = virJSONValueObjectAppendNull(obj, key); } break; case 'A': @@ -245,7 +239,7 @@ virJSONValueObjectCreateVArgs(virJSONValuePtr *obj, va_list args) goto cleanup; } - rc = virJSONValueObjectAppend(jargs, key, val); + rc = virJSONValueObjectAppend(obj, key, val); } break; default: @@ -259,17 +253,45 @@ virJSONValueObjectCreateVArgs(virJSONValuePtr *obj, va_list args) } /* verify that we added at least one key-value pair */ - if (virJSONValueObjectKeysNumber(jargs) == 0) { + if (virJSONValueObjectKeysNumber(obj) == 0) { ret = 0; goto cleanup; } - *obj = jargs; - jargs = NULL; ret = 1; cleanup: - virJSONValueFree(jargs); + return ret; +} + + +int +virJSONValueObjectAdd(virJSONValuePtr obj, ...) +{ + va_list args; + int ret; + + va_start(args, obj); + ret = virJSONValueObjectAddVArgs(obj, args); + va_end(args); + + return ret; +} + + +int +virJSONValueObjectCreateVArgs(virJSONValuePtr *obj, + va_list args) +{ + int ret; + + if (!(*obj = virJSONValueNewObject())) + return -1; + + /* free the object on error, or if no value objects were added */ + if ((ret = virJSONValueObjectAddVArgs(*obj, args)) <= 0) + VIR_FREE(*obj); + return ret; } diff --git a/src/util/virjson.h b/src/util/virjson.h index c0aefba..fc05ad9 100644 --- a/src/util/virjson.h +++ b/src/util/virjson.h @@ -85,6 +85,11 @@ int virJSONValueObjectCreate(virJSONValuePtr *obj, ...) ATTRIBUTE_NONNULL(1) ATTRIBUTE_SENTINEL; int virJSONValueObjectCreateVArgs(virJSONValuePtr *obj, va_list args) ATTRIBUTE_NONNULL(1); +int virJSONValueObjectAdd(virJSONValuePtr obj, ...) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_SENTINEL; +int virJSONValueObjectAddVArgs(virJSONValuePtr obj, va_list args) + ATTRIBUTE_NONNULL(1); + virJSONValuePtr virJSONValueNewString(const char *data); virJSONValuePtr virJSONValueNewStringLen(const char *data, size_t length); -- 2.2.2

On 28.01.2015 11:30, Peter Krempa wrote:
To allow constructing of value objects stepwise explode the helper into separate steps and allow appending into existing value objects. --- src/libvirt_private.syms | 2 ++ src/util/virjson.c | 74 +++++++++++++++++++++++++++++++----------------- src/util/virjson.h | 5 ++++ 3 files changed, 55 insertions(+), 26 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 2bbce03..cc74e35 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1533,6 +1533,8 @@ virJSONValueNewNumberUlong; virJSONValueNewObject; virJSONValueNewString; virJSONValueNewStringLen; +virJSONValueObjectAdd; +virJSONValueObjectAddVArgs; virJSONValueObjectAppend; virJSONValueObjectAppendBoolean; virJSONValueObjectAppendNull; diff --git a/src/util/virjson.c b/src/util/virjson.c index 9f2e1cf..9eb1bff 100644 --- a/src/util/virjson.c +++ b/src/util/virjson.c @@ -64,12 +64,11 @@ struct _virJSONParser {
/** - * virJSONValueObjectCreateVArgs: - * @obj: returns the created JSON object - * @...: a key-value argument pairs, terminated by NULL + * virJSONValueObjectAddVArgs: + * @obj: JSON object to add the values to + * @args: a key-value argument pairs, terminated by NULL * - * Creates a JSON value object filled with key-value pairs supplied as variable - * argument list. + * Adds the key-value pairs supplied as variable argument list to @obj. * * Keys look like s:name the first letter is a type code: * Explanation of type codes: @@ -104,22 +103,17 @@ struct _virJSONParser { * 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). + * in case of no error but nothing was filled. */ int -virJSONValueObjectCreateVArgs(virJSONValuePtr *obj, va_list args) +virJSONValueObjectAddVArgs(virJSONValuePtr obj, + va_list args) { - virJSONValuePtr jargs = NULL; char type; char *key; int ret = -1; int rc;
- *obj = NULL; - - if (!(jargs = virJSONValueNewObject())) - goto cleanup; - while ((key = va_arg(args, char *)) != NULL) {
if (strlen(key) < 3) { @@ -146,7 +140,7 @@ virJSONValueObjectCreateVArgs(virJSONValuePtr *obj, va_list args) key); goto cleanup; } - rc = virJSONValueObjectAppendString(jargs, key, val); + rc = virJSONValueObjectAppendString(obj, key, val); } break;
case 'z': @@ -165,7 +159,7 @@ virJSONValueObjectCreateVArgs(virJSONValuePtr *obj, va_list args) if (!val && (type == 'z' || type == 'y')) continue;
- rc = virJSONValueObjectAppendNumberInt(jargs, key, val); + rc = virJSONValueObjectAppendNumberInt(obj, key, val); } break;
case 'p': @@ -175,7 +169,7 @@ virJSONValueObjectCreateVArgs(virJSONValuePtr *obj, va_list args) if (!val && type == 'p') continue;
- rc = virJSONValueObjectAppendNumberUint(jargs, key, val); + rc = virJSONValueObjectAppendNumberUint(obj, key, val); } break;
case 'Z': @@ -194,7 +188,7 @@ virJSONValueObjectCreateVArgs(virJSONValuePtr *obj, va_list args) if (!val && (type == 'Z' || type == 'Y')) continue;
- rc = virJSONValueObjectAppendNumberLong(jargs, key, val); + rc = virJSONValueObjectAppendNumberLong(obj, key, val); } break;
case 'P': @@ -209,12 +203,12 @@ virJSONValueObjectCreateVArgs(virJSONValuePtr *obj, va_list args) if (!val && type == 'P') continue;
- rc = virJSONValueObjectAppendNumberLong(jargs, key, val); + rc = virJSONValueObjectAppendNumberLong(obj, key, val); } break;
case 'd': { double val = va_arg(args, double); - rc = virJSONValueObjectAppendNumberDouble(jargs, key, val); + rc = virJSONValueObjectAppendNumberDouble(obj, key, val); } break;
case 'B': @@ -224,11 +218,11 @@ virJSONValueObjectCreateVArgs(virJSONValuePtr *obj, va_list args) if (!val && type == 'B') continue;
- rc = virJSONValueObjectAppendBoolean(jargs, key, val); + rc = virJSONValueObjectAppendBoolean(obj, key, val); } break;
case 'n': { - rc = virJSONValueObjectAppendNull(jargs, key); + rc = virJSONValueObjectAppendNull(obj, key); } break;
case 'A': @@ -245,7 +239,7 @@ virJSONValueObjectCreateVArgs(virJSONValuePtr *obj, va_list args) goto cleanup; }
- rc = virJSONValueObjectAppend(jargs, key, val); + rc = virJSONValueObjectAppend(obj, key, val); } break;
default: @@ -259,17 +253,45 @@ virJSONValueObjectCreateVArgs(virJSONValuePtr *obj, va_list args) }
/* verify that we added at least one key-value pair */ - if (virJSONValueObjectKeysNumber(jargs) == 0) { + if (virJSONValueObjectKeysNumber(obj) == 0) { ret = 0; goto cleanup; }
- *obj = jargs; - jargs = NULL; ret = 1;
cleanup: - virJSONValueFree(jargs); + return ret; +} + + +int +virJSONValueObjectAdd(virJSONValuePtr obj, ...) +{ + va_list args; + int ret; + + va_start(args, obj); + ret = virJSONValueObjectAddVArgs(obj, args); + va_end(args); + + return ret; +} + + +int +virJSONValueObjectCreateVArgs(virJSONValuePtr *obj, + va_list args) +{ + int ret; + + if (!(*obj = virJSONValueNewObject())) + return -1; + + /* free the object on error, or if no value objects were added */ + if ((ret = virJSONValueObjectAddVArgs(*obj, args)) <= 0) + VIR_FREE(*obj); +
So if I do something like: virJSONValueObjectCreate(&obj, "s:key1", "val1", "j:key2", -1337, NULL); the virJSONValueObjectCreateVArgs() will allocate the object, and virJSONValueObjectAddVArgs() starts adding objects into it. The first pair will succeed (as long as there's enough memory). The second will, however, fail, as 'j' format requires nonnegative integer. So we VIR_FREE() the object constructed so far. Leaving us with a memleak - the "val1" has been strcpy-ied into obj. s/VIR_FREE/virJSONValueFree(); *obj = NULL;/
return ret; }
diff --git a/src/util/virjson.h b/src/util/virjson.h index c0aefba..fc05ad9 100644 --- a/src/util/virjson.h +++ b/src/util/virjson.h @@ -85,6 +85,11 @@ int virJSONValueObjectCreate(virJSONValuePtr *obj, ...) ATTRIBUTE_NONNULL(1) ATTRIBUTE_SENTINEL; int virJSONValueObjectCreateVArgs(virJSONValuePtr *obj, va_list args) ATTRIBUTE_NONNULL(1); +int virJSONValueObjectAdd(virJSONValuePtr obj, ...) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_SENTINEL; +int virJSONValueObjectAddVArgs(virJSONValuePtr obj, va_list args) + ATTRIBUTE_NONNULL(1); +
virJSONValuePtr virJSONValueNewString(const char *data); virJSONValuePtr virJSONValueNewStringLen(const char *data, size_t length);
Michal

To be able to easily represent nodesets and other data stored in virBitmaps in libvirt, this patch introduces a set of helpers that allow to convert the bitmap to and from JSON value objects. --- src/libvirt_private.syms | 2 + src/util/virjson.c | 113 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virjson.h | 4 ++ 3 files changed, 119 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index cc74e35..70c81a8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1514,6 +1514,7 @@ virJSONValueArrayGet; virJSONValueArraySize; virJSONValueFree; virJSONValueFromString; +virJSONValueGetArrayAsBitmap; virJSONValueGetBoolean; virJSONValueGetNumberDouble; virJSONValueGetNumberInt; @@ -1523,6 +1524,7 @@ virJSONValueGetNumberUlong; virJSONValueGetString; virJSONValueIsNull; virJSONValueNewArray; +virJSONValueNewArrayFromBitmap; virJSONValueNewBoolean; virJSONValueNewNull; virJSONValueNewNumberDouble; diff --git a/src/util/virjson.c b/src/util/virjson.c index 9eb1bff..3e00650 100644 --- a/src/util/virjson.c +++ b/src/util/virjson.c @@ -99,6 +99,8 @@ struct _virJSONParser { * * a: json object, must be non-NULL * A: json object, omitted if NULL + * m: a bitmap represented as a JSON array, must be non-NULL + * M: a bitmap represented as a JSON array, omitted if NULL * * The value corresponds to the selected type. * @@ -242,6 +244,28 @@ virJSONValueObjectAddVArgs(virJSONValuePtr obj, rc = virJSONValueObjectAppend(obj, key, val); } break; + case 'M': + case 'm': { + virBitmapPtr map = va_arg(args, virBitmapPtr); + virJSONValuePtr jsonMap; + + if (!map) { + if (type == 'M') + continue; + + virReportError(VIR_ERR_INTERNAL_ERROR, + _("argument key '%s' must not have null value"), + key); + goto cleanup; + } + + if (!(jsonMap = virJSONValueNewArrayFromBitmap(map))) + goto cleanup; + + if ((rc = virJSONValueObjectAppend(obj, key, jsonMap)) < 0) + virJSONValueFree(jsonMap); + } break; + default: virReportError(VIR_ERR_INTERNAL_ERROR, _("unsupported data type '%c' for arg '%s'"), type, key - 2); @@ -941,6 +965,95 @@ virJSONValueGetBoolean(virJSONValuePtr val, } +/** + * virJSONValueGetArrayAsBitmap: + * @val: JSON array to convert to bitmap + * @bitmap: New bitmap is allocated filled and returned via this argument + * + * Attempts a conversion of a JSON array to a bitmap. The members of the array + * must be non-negative integers for the conversion to succeed. This function + * does not report libvirt errors (except for out-of-memory) so that it can be + * used to probe that the array can be represented as a bitmap. + * + * Returns 0 on success and fills @bitmap; -1 on error and @bitmap is set to + * NULL. + */ +int +virJSONValueGetArrayAsBitmap(const virJSONValue *val, + virBitmapPtr *bitmap) +{ + int ret = -1; + virJSONValuePtr elem; + size_t i; + unsigned long long *elems = NULL; + unsigned long long maxelem = 0; + + *bitmap = NULL; + + if (val->type != VIR_JSON_TYPE_ARRAY) + return -1; + + if (VIR_ALLOC_N(elems, val->data.array.nvalues) < 0) + return -1; + + /* first pass converts array members to numbers and finds the maximum */ + for (i = 0; i < val->data.array.nvalues; i++) { + elem = val->data.array.values[i]; + + if (elem->type != VIR_JSON_TYPE_NUMBER || + virStrToLong_ullp(elem->data.number, NULL, 10, &elems[i]) < 0) + goto cleanup; + + if (elems[i] > maxelem) + maxelem = elems[i]; + } + + if (!(*bitmap = virBitmapNew(maxelem + 1))) + goto cleanup; + + /* second pass sets the correct bits in the map */ + for (i = 0; i < val->data.array.nvalues; i++) + ignore_value(virBitmapSetBit(*bitmap, elems[i])); + + ret = 0; + + cleanup: + VIR_FREE(elems); + + return ret; +} + + +virJSONValuePtr +virJSONValueNewArrayFromBitmap(virBitmapPtr bitmap) +{ + virJSONValuePtr ret; + ssize_t pos = -1; + + if (!(ret = virJSONValueNewArray())) + return NULL; + + if (!bitmap) + return ret; + + while ((pos = virBitmapNextSetBit(bitmap, pos)) > -1) { + virJSONValuePtr newelem; + + if (!(newelem = virJSONValueNewNumberLong(pos)) || + virJSONValueArrayAppend(ret, newelem) < 0) { + virJSONValueFree(newelem); + goto error; + } + } + + return ret; + + error: + virJSONValueFree(ret); + return NULL; +} + + int virJSONValueIsNull(virJSONValuePtr val) { diff --git a/src/util/virjson.h b/src/util/virjson.h index fc05ad9..57010b0 100644 --- a/src/util/virjson.h +++ b/src/util/virjson.h @@ -25,6 +25,7 @@ # define __VIR_JSON_H_ # include "internal.h" +# include "virbitmap.h" # include <stdarg.h> @@ -102,6 +103,7 @@ virJSONValuePtr virJSONValueNewBoolean(int boolean); virJSONValuePtr virJSONValueNewNull(void); virJSONValuePtr virJSONValueNewArray(void); virJSONValuePtr virJSONValueNewObject(void); +virJSONValuePtr virJSONValueNewArrayFromBitmap(virBitmapPtr bitmap); int virJSONValueObjectAppend(virJSONValuePtr object, const char *key, virJSONValuePtr value); int virJSONValueArrayAppend(virJSONValuePtr object, virJSONValuePtr value); @@ -125,6 +127,8 @@ int virJSONValueGetNumberLong(virJSONValuePtr object, long long *value); int virJSONValueGetNumberUlong(virJSONValuePtr object, unsigned long long *value); int virJSONValueGetNumberDouble(virJSONValuePtr object, double *value); int virJSONValueGetBoolean(virJSONValuePtr object, bool *value); +int virJSONValueGetArrayAsBitmap(const virJSONValue *val, virBitmapPtr *bitmap) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); int virJSONValueIsNull(virJSONValuePtr object); const char *virJSONValueObjectGetString(virJSONValuePtr object, const char *key); -- 2.2.2

On 28.01.2015 11:30, Peter Krempa wrote:
To be able to easily represent nodesets and other data stored in virBitmaps in libvirt, this patch introduces a set of helpers that allow to convert the bitmap to and from JSON value objects. --- src/libvirt_private.syms | 2 + src/util/virjson.c | 113 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virjson.h | 4 ++ 3 files changed, 119 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index cc74e35..70c81a8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1514,6 +1514,7 @@ virJSONValueArrayGet; virJSONValueArraySize; virJSONValueFree; virJSONValueFromString; +virJSONValueGetArrayAsBitmap; virJSONValueGetBoolean; virJSONValueGetNumberDouble; virJSONValueGetNumberInt; @@ -1523,6 +1524,7 @@ virJSONValueGetNumberUlong; virJSONValueGetString; virJSONValueIsNull; virJSONValueNewArray; +virJSONValueNewArrayFromBitmap; virJSONValueNewBoolean; virJSONValueNewNull; virJSONValueNewNumberDouble; diff --git a/src/util/virjson.c b/src/util/virjson.c index 9eb1bff..3e00650 100644 --- a/src/util/virjson.c +++ b/src/util/virjson.c @@ -99,6 +99,8 @@ struct _virJSONParser { * * a: json object, must be non-NULL * A: json object, omitted if NULL + * m: a bitmap represented as a JSON array, must be non-NULL + * M: a bitmap represented as a JSON array, omitted if NULL * * The value corresponds to the selected type. * @@ -242,6 +244,28 @@ virJSONValueObjectAddVArgs(virJSONValuePtr obj, rc = virJSONValueObjectAppend(obj, key, val); } break;
+ case 'M': + case 'm': { + virBitmapPtr map = va_arg(args, virBitmapPtr); + virJSONValuePtr jsonMap; + + if (!map) { + if (type == 'M') + continue; + + virReportError(VIR_ERR_INTERNAL_ERROR, + _("argument key '%s' must not have null value"), + key); + goto cleanup; + } + + if (!(jsonMap = virJSONValueNewArrayFromBitmap(map))) + goto cleanup; + + if ((rc = virJSONValueObjectAppend(obj, key, jsonMap)) < 0) + virJSONValueFree(jsonMap); + } break; + default: virReportError(VIR_ERR_INTERNAL_ERROR, _("unsupported data type '%c' for arg '%s'"), type, key - 2); @@ -941,6 +965,95 @@ virJSONValueGetBoolean(virJSONValuePtr val, }
+/** + * virJSONValueGetArrayAsBitmap: + * @val: JSON array to convert to bitmap + * @bitmap: New bitmap is allocated filled and returned via this argument + * + * Attempts a conversion of a JSON array to a bitmap. The members of the array + * must be non-negative integers for the conversion to succeed. This function + * does not report libvirt errors (except for out-of-memory) so that it can be + * used to probe that the array can be represented as a bitmap. + * + * Returns 0 on success and fills @bitmap; -1 on error and @bitmap is set to + * NULL. + */ +int +virJSONValueGetArrayAsBitmap(const virJSONValue *val, + virBitmapPtr *bitmap) +{ + int ret = -1; + virJSONValuePtr elem; + size_t i; + unsigned long long *elems = NULL; + unsigned long long maxelem = 0; + + *bitmap = NULL; + + if (val->type != VIR_JSON_TYPE_ARRAY) + return -1; + + if (VIR_ALLOC_N(elems, val->data.array.nvalues) < 0) + return -1;
This reports an error in case of failure ...
+ + /* first pass converts array members to numbers and finds the maximum */ + for (i = 0; i < val->data.array.nvalues; i++) { + elem = val->data.array.values[i]; + + if (elem->type != VIR_JSON_TYPE_NUMBER ||
while this does not. I'd rather make this report an error on all failures.
+ virStrToLong_ullp(elem->data.number, NULL, 10, &elems[i]) < 0) + goto cleanup; + + if (elems[i] > maxelem) + maxelem = elems[i]; + } + + if (!(*bitmap = virBitmapNew(maxelem + 1))) + goto cleanup; + + /* second pass sets the correct bits in the map */ + for (i = 0; i < val->data.array.nvalues; i++) + ignore_value(virBitmapSetBit(*bitmap, elems[i])); + + ret = 0; + + cleanup: + VIR_FREE(elems); + + return ret; +}
Michal

This helper easies iterating all key=value pairs stored in a JSON object. Usually we pick only certain known keys from a JSON object, but this will allow to walk complete objects and have the callback act on those. --- src/libvirt_private.syms | 1 + src/util/virjson.c | 33 +++++++++++++++++++++++++++++++++ src/util/virjson.h | 8 ++++++++ 3 files changed, 42 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 70c81a8..196b837 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1548,6 +1548,7 @@ virJSONValueObjectAppendNumberUlong; virJSONValueObjectAppendString; virJSONValueObjectCreate; virJSONValueObjectCreateVArgs; +virJSONValueObjectForeachKeyValue; virJSONValueObjectGet; virJSONValueObjectGetBoolean; virJSONValueObjectGetKey; diff --git a/src/util/virjson.c b/src/util/virjson.c index 3e00650..ec1778d 100644 --- a/src/util/virjson.c +++ b/src/util/virjson.c @@ -1198,6 +1198,39 @@ virJSONValueObjectIsNull(virJSONValuePtr object, } +/** + * virJSONValueObjectForeachKeyValue: + * @object: JSON object to iterate + * @cb: callback to call on key-value pairs contained in the object + * @opaque: generic data for the callback + * + * Iterates all key=value pairs in @object. Iteration breaks if @cb returns + * negative value. + * + * Returns 0 if all elements were iterated, -2 if @cb returned negative value + * during iteration and -1 on generic errors. + */ +int +virJSONValueObjectForeachKeyValue(virJSONValuePtr object, + virJSONValueObjectIteratorFunc cb, + void *opaque) +{ + size_t i; + + if (object->type != VIR_JSON_TYPE_OBJECT) + return -1; + + for (i = 0; i < object->data.object.npairs; i++) { + virJSONObjectPairPtr elem = object->data.object.pairs + i; + + if (cb(elem->key, elem->value, opaque) < 0) + return -2; + } + + return 0; +} + + #if WITH_YAJL static int virJSONParserInsertValue(virJSONParserPtr parser, diff --git a/src/util/virjson.h b/src/util/virjson.h index 57010b0..9bb7461 100644 --- a/src/util/virjson.h +++ b/src/util/virjson.h @@ -157,4 +157,12 @@ virJSONValuePtr virJSONValueFromString(const char *jsonstring); char *virJSONValueToString(virJSONValuePtr object, bool pretty); +typedef int (*virJSONValueObjectIteratorFunc)(const char *key, + const virJSONValue *value, + void *opaque); + +int virJSONValueObjectForeachKeyValue(virJSONValuePtr object, + virJSONValueObjectIteratorFunc cb, + void *opaque); + #endif /* __VIR_JSON_H_ */ -- 2.2.2

On 01/28/2015 03:30 AM, Peter Krempa wrote:
This helper easies iterating all key=value pairs stored in a JSON
s/easies/eases/
object. Usually we pick only certain known keys from a JSON object, but this will allow to walk complete objects and have the callback act on those. --- src/libvirt_private.syms | 1 + src/util/virjson.c | 33 +++++++++++++++++++++++++++++++++ src/util/virjson.h | 8 ++++++++ 3 files changed, 42 insertions(+)
Sounds useful. ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

Unlike -device, qemu uses a JSON object to add backend "objects" via the monitor rather than the string that would be passed on the commandline. To be able to reuse code parts that configure backends for various devices, this patch adds a helper that will allow generating the command line representations from the JSON property object. --- src/qemu/qemu_command.c | 111 +++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_command.h | 4 ++ tests/Makefile.am | 13 ++++- tests/qemucommandutiltest.c | 118 ++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 244 insertions(+), 2 deletions(-) create mode 100644 tests/qemucommandutiltest.c diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 100deed..6f298ac 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -416,6 +416,117 @@ qemuDomainSupportsNetdev(virDomainDefPtr def, return virQEMUCapsGet(qemuCaps, QEMU_CAPS_NETDEV); } + +static int +qemuBuildObjectCommandLinePropsInternal(const char *key, + const virJSONValue *value, + virBufferPtr buf, + bool nested) +{ + virJSONValuePtr elem; + virBitmapPtr bitmap = NULL; + ssize_t pos = -1; + ssize_t end; + size_t i; + + switch ((virJSONType) value->type) { + case VIR_JSON_TYPE_STRING: + virBufferAsprintf(buf, ",%s=%s", key, value->data.string); + break; + + case VIR_JSON_TYPE_NUMBER: + virBufferAsprintf(buf, ",%s=%s", key, value->data.number); + break; + + case VIR_JSON_TYPE_BOOLEAN: + if (value->data.boolean) + virBufferAsprintf(buf, ",%s=yes", key); + else + virBufferAsprintf(buf, ",%s=no", key); + + break; + + case VIR_JSON_TYPE_ARRAY: + if (nested) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("nested -object property arrays are not supported")); + return -1; + } + + if (virJSONValueGetArrayAsBitmap(value, &bitmap) == 0) { + while ((pos = virBitmapNextSetBit(bitmap, pos)) > -1) { + if ((end = virBitmapNextClearBit(bitmap, pos)) < 0) + end = virBitmapLastSetBit(bitmap) + 1; + + if (end - 1 > pos) { + virBufferAsprintf(buf, ",%s=%zd-%zd", key, pos, end - 1); + pos = end; + } else { + virBufferAsprintf(buf, ",%s=%zd", key, pos); + } + } + } else { + /* fallback, treat the array as a non-bitmap, adding the key + * for each member */ + for (i = 0; i < virJSONValueArraySize(value); i++) { + elem = virJSONValueArrayGet((virJSONValuePtr)value, i); + + /* recurse to avoid duplicating code */ + if (qemuBuildObjectCommandLinePropsInternal(key, elem, buf, + true) < 0) + return -1; + } + } + break; + + case VIR_JSON_TYPE_OBJECT: + case VIR_JSON_TYPE_NULL: + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("NULL and OBJECT JSON types can't be converted to " + "commandline string")); + return -1; + } + + virBitmapFree(bitmap); + return 0; +} + + +static int +qemuBuildObjectCommandLineProps(const char *key, + const virJSONValue *value, + void *opaque) +{ + return qemuBuildObjectCommandLinePropsInternal(key, value, opaque, false); +} + + +char * +qemuBuildObjectCommandlineFromJSON(const char *type, + const char *alias, + virJSONValuePtr props) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *ret = NULL; + + virBufferAsprintf(&buf, "%s,id=%s", type, alias); + + if (virJSONValueObjectForeachKeyValue(props, + qemuBuildObjectCommandLineProps, + &buf) < 0) + goto cleanup; + + if (virBufferCheckError(&buf) < 0) + goto cleanup; + + ret = virBufferContentAndReset(&buf); + + cleanup: + virBufferFreeAndReset(&buf); + return ret; +} + + /** * qemuOpenVhostNet: * @def: domain definition diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 8eaf1e4..ae36bd8 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -69,6 +69,10 @@ struct _qemuBuildCommandLineCallbacks { extern qemuBuildCommandLineCallbacks buildCommandLineCallbacks; +char *qemuBuildObjectCommandlineFromJSON(const char *type, + const char *alias, + virJSONValuePtr props); + virCommandPtr qemuBuildCommandLine(virConnectPtr conn, virQEMUDriverPtr driver, virDomainDefPtr def, diff --git a/tests/Makefile.am b/tests/Makefile.am index 1d838a5..938270c 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -237,7 +237,8 @@ if WITH_QEMU test_programs += qemuxml2argvtest qemuxml2xmltest qemuxmlnstest \ qemuargv2xmltest qemuhelptest domainsnapshotxml2xmltest \ qemumonitortest qemumonitorjsontest qemuhotplugtest \ - qemuagenttest qemucapabilitiestest qemucaps2xmltest + qemuagenttest qemucapabilitiestest qemucaps2xmltest \ + qemucommandutiltest endif WITH_QEMU if WITH_LXC @@ -586,6 +587,14 @@ qemucapabilitiestest_SOURCES = \ qemucapabilitiestest_LDADD = libqemumonitortestutils.la \ $(qemu_LDADDS) $(LDADDS) +qemucommandutiltest_SOURCES = \ + qemucommandutiltest.c \ + testutils.c testutils.h \ + testutilsqemu.c testutilsqemu.h \ + $(NULL) +qemucommandutiltest_LDADD = libqemumonitortestutils.la \ + $(qemu_LDADDS) $(LDADDS) + qemucaps2xmltest_SOURCES = \ qemucaps2xmltest.c \ testutils.c testutils.h \ @@ -616,7 +625,7 @@ EXTRA_DIST += qemuxml2argvtest.c qemuxml2xmltest.c qemuargv2xmltest.c \ qemumonitortest.c testutilsqemu.c testutilsqemu.h \ qemumonitorjsontest.c qemuhotplugtest.c \ qemuagenttest.c qemucapabilitiestest.c \ - qemucaps2xmltest.c \ + qemucaps2xmltest.c qemucommandutiltest.c \ $(QEMUMONITORTESTUTILS_SOURCES) endif ! WITH_QEMU diff --git a/tests/qemucommandutiltest.c b/tests/qemucommandutiltest.c new file mode 100644 index 0000000..8c52f02 --- /dev/null +++ b/tests/qemucommandutiltest.c @@ -0,0 +1,118 @@ +/* + * Copyright (C) 2015 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/>. + * + */ + +#include <config.h> + +#include "qemu/qemu_command.h" +#include "util/virjson.h" +#include "testutils.h" +#include "testutilsqemu.h" + +#define VIR_FROM_THIS VIR_FROM_NONE + +typedef struct +{ + const char *props; + const char *expectprops; +} testQemuCommandBuildObjectFromJSONData; + +static int +testQemuCommandBuildObjectFromJSON(const void *opaque) +{ + const testQemuCommandBuildObjectFromJSONData *data = opaque; + virJSONValuePtr val = NULL; + char *expect = NULL; + char *result = NULL; + int ret = -1; + + if (!(val = virJSONValueFromString(data->props))) { + fprintf(stderr, "Failed to parse JSON string '%s'", data->props); + return -1; + } + + if (virAsprintf(&expect, "testobject,id=testalias%s%s", + data->expectprops ? "," : "", + data->expectprops ? data->expectprops : "") < 0) + return -1; + + result = qemuBuildObjectCommandlineFromJSON("testobject", + "testalias", + val); + + if (STRNEQ_NULLABLE(expect, result)) { + fprintf(stderr, "\nFailed to create object string. " + "\nExpected:\n'%s'\nGot:\n'%s'", + NULLSTR(expect), NULLSTR(result)); + goto cleanup; + } + + ret = 0; + cleanup: + virJSONValueFree(val); + VIR_FREE(result); + return ret; +} + +static int +mymain(void) +{ + int ret = 0; + testQemuCommandBuildObjectFromJSONData data1; + +#if !WITH_YAJL + fputs("libvirt not compiled with yajl, skipping this test\n", stderr); + return EXIT_AM_SKIP; +#endif + + virtTestCounterReset("testQemuCommandBuildObjectFromJSON"); + +#define DO_TEST_COMMAND_OBJECT_FROM_JSON(PROPS, EXPECT) \ + do { \ + data1.props = PROPS; \ + data1.expectprops = EXPECT; \ + if (virtTestRun(virtTestCounterNext(), \ + testQemuCommandBuildObjectFromJSON, \ + &data1) < 0) \ + ret = -1; \ + } while (0) + + DO_TEST_COMMAND_OBJECT_FROM_JSON("{}", NULL); + DO_TEST_COMMAND_OBJECT_FROM_JSON("{\"string\":\"qwer\"}", "string=qwer"); + DO_TEST_COMMAND_OBJECT_FROM_JSON("{\"number\":1234}", "number=1234"); + DO_TEST_COMMAND_OBJECT_FROM_JSON("{\"boolean\":true}", "boolean=yes"); + DO_TEST_COMMAND_OBJECT_FROM_JSON("{\"boolean\":false}", "boolean=no"); + DO_TEST_COMMAND_OBJECT_FROM_JSON("{\"bitmap\":[]}", NULL); + DO_TEST_COMMAND_OBJECT_FROM_JSON("{\"bitmap\":[0]}", "bitmap=0"); + DO_TEST_COMMAND_OBJECT_FROM_JSON("{\"bitmap\":[1,3,5]}", + "bitmap=1,bitmap=3,bitmap=5"); + DO_TEST_COMMAND_OBJECT_FROM_JSON("{\"bitmap\":[0,1,2,3]}", "bitmap=0-3"); + DO_TEST_COMMAND_OBJECT_FROM_JSON("{\"bitmap\":[1,2,3,5]}", + "bitmap=1-3,bitmap=5"); + DO_TEST_COMMAND_OBJECT_FROM_JSON("{\"bitmap\":[1,2,3,5,7,8,9]}", + "bitmap=1-3,bitmap=5,bitmap=7-9"); + DO_TEST_COMMAND_OBJECT_FROM_JSON("{\"array\":[\"bleah\",\"qwerty\",1]}", + "array=bleah,array=qwerty,array=1"); + DO_TEST_COMMAND_OBJECT_FROM_JSON("{\"boolean\":true,\"hyphen-name\":1234,\"some_string\":\"bleah\"}", + "boolean=yes,hyphen-name=1234,some_string=bleah"); + + return ret; + +} + +VIRT_TEST_MAIN(mymain) -- 2.2.2

On 28.01.2015 11:30, Peter Krempa wrote:
Unlike -device, qemu uses a JSON object to add backend "objects" via the monitor rather than the string that would be passed on the commandline.
To be able to reuse code parts that configure backends for various devices, this patch adds a helper that will allow generating the command line representations from the JSON property object. --- src/qemu/qemu_command.c | 111 +++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_command.h | 4 ++ tests/Makefile.am | 13 ++++- tests/qemucommandutiltest.c | 118 ++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 244 insertions(+), 2 deletions(-) create mode 100644 tests/qemucommandutiltest.c
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 100deed..6f298ac 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -416,6 +416,117 @@ qemuDomainSupportsNetdev(virDomainDefPtr def, return virQEMUCapsGet(qemuCaps, QEMU_CAPS_NETDEV); }
+ +static int +qemuBuildObjectCommandLinePropsInternal(const char *key, + const virJSONValue *value, + virBufferPtr buf, + bool nested) +{ + virJSONValuePtr elem; + virBitmapPtr bitmap = NULL; + ssize_t pos = -1; + ssize_t end; + size_t i; + + switch ((virJSONType) value->type) { + case VIR_JSON_TYPE_STRING: + virBufferAsprintf(buf, ",%s=%s", key, value->data.string); + break; + + case VIR_JSON_TYPE_NUMBER: + virBufferAsprintf(buf, ",%s=%s", key, value->data.number);
So a number is a string? Me goes check the struct, and you're right. I don't even ...
+ break; + + case VIR_JSON_TYPE_BOOLEAN: + if (value->data.boolean) + virBufferAsprintf(buf, ",%s=yes", key); + else + virBufferAsprintf(buf, ",%s=no", key); + + break; + + case VIR_JSON_TYPE_ARRAY: + if (nested) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("nested -object property arrays are not supported")); + return -1; + } + + if (virJSONValueGetArrayAsBitmap(value, &bitmap) == 0) { + while ((pos = virBitmapNextSetBit(bitmap, pos)) > -1) { + if ((end = virBitmapNextClearBit(bitmap, pos)) < 0) + end = virBitmapLastSetBit(bitmap) + 1; + + if (end - 1 > pos) { + virBufferAsprintf(buf, ",%s=%zd-%zd", key, pos, end - 1); + pos = end; + } else { + virBufferAsprintf(buf, ",%s=%zd", key, pos); + } + } + } else { + /* fallback, treat the array as a non-bitmap, adding the key + * for each member */ + for (i = 0; i < virJSONValueArraySize(value); i++) { + elem = virJSONValueArrayGet((virJSONValuePtr)value, i); + + /* recurse to avoid duplicating code */ + if (qemuBuildObjectCommandLinePropsInternal(key, elem, buf, + true) < 0) + return -1; + } + } + break; + + case VIR_JSON_TYPE_OBJECT: + case VIR_JSON_TYPE_NULL: + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("NULL and OBJECT JSON types can't be converted to " + "commandline string")); + return -1; + } + + virBitmapFree(bitmap); + return 0; +} +
Michal

Extract the memory backend device code into a separate function so that it can be later easily refactored and reused. Few small changes for future reusability, namely: - new (currently unused) parameter for user specified page size - size of the memory is specified in kibibytes, divided up in the function - new (currently unused) parameter for user specifed source nodeset - option to enforce capability check --- src/qemu/qemu_command.c | 382 +++++++++++++++++++++++++++++++----------------- 1 file changed, 250 insertions(+), 132 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 6f298ac..c65f187 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4502,6 +4502,243 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef, } +/** + * qemuBuildMemoryBackendStr: + * @size: size of the memory device in kibibytes + * @pagesize: size of the requested memory page in KiB, 0 for default + * @guestNode: NUMA node in the guest that the memory object will be attached to + * @hostNodes: map of host nodes to alloc the memory in, NULL for default + * @autoNodeset: fallback nodeset in case of automatic numa placement + * @def: domain definition object + * @qemuCaps: qemu capabilities object + * @cfg: qemu driver config object + * @aliasPrefix: prefix string of the alias (to allow for multiple frontents) + * @id: index of the device (to construct the alias) + * @backendStr: returns the object string + * + * Formats the configuration string for the memory device backend according + * to the configuration. @pagesize and @hostNodes can be used to override the + * default source configuration, both are optional. + * + * Returns 0 on success, 1 if only the implicit memory-device-ram with no + * other configuration was used (to detect legacy configurations). Returns + * -1 in case of an error. + */ +static int +qemuBuildMemoryBackendStr(unsigned long long size, + unsigned long long pagesize, + int guestNode, + virBitmapPtr userNodeset, + virBitmapPtr autoNodeset, + virDomainDefPtr def, + virQEMUCapsPtr qemuCaps, + virQEMUDriverConfigPtr cfg, + const char *aliasPrefix, + size_t id, + char **backendStr, + bool force) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + virDomainHugePagePtr master_hugepage = NULL; + virDomainHugePagePtr hugepage = NULL; + virDomainNumatuneMemMode mode; + const long system_page_size = sysconf(_SC_PAGESIZE) / 1024; + virMemAccess memAccess = def->cpu->cells[guestNode].memAccess; + size_t i; + char *mem_path = NULL; + char *nodemask = NULL; + char *tmpmask = NULL, *next = NULL; + int ret = -1; + + *backendStr = NULL; + + mode = virDomainNumatuneGetMode(def->numatune, guestNode); + + if (pagesize == 0 || pagesize != system_page_size) { + /* Find the huge page size we want to use */ + for (i = 0; i < def->mem.nhugepages; i++) { + bool thisHugepage = false; + + hugepage = &def->mem.hugepages[i]; + + if (!hugepage->nodemask) { + master_hugepage = hugepage; + continue; + } + + if (virBitmapGetBit(hugepage->nodemask, guestNode, + &thisHugepage) < 0) { + /* Ignore this error. It's not an error after all. Well, + * the nodemask for this <page/> can contain lower NUMA + * nodes than we are querying in here. */ + continue; + } + + if (thisHugepage) { + /* Hooray, we've found the page size */ + break; + } + } + + if (i == def->mem.nhugepages) { + /* We have not found specific huge page to be used with this + * NUMA node. Use the generic setting then (<page/> without any + * @nodemask) if possible. */ + hugepage = master_hugepage; + } + + if (hugepage) + pagesize = hugepage->size; + + if (hugepage && hugepage->size == system_page_size) { + /* However, if user specified to use "huge" page + * of regular system page size, it's as if they + * hasn't specified any huge pages at all. */ + hugepage = NULL; + } + } + + if (hugepage) { + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_FILE)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("this qemu doesn't support hugepage memory backing")); + goto cleanup; + } + + /* Now lets see, if the huge page we want to use is even mounted + * and ready to use */ + for (i = 0; i < cfg->nhugetlbfs; i++) { + if (cfg->hugetlbfs[i].size == hugepage->size) + break; + } + + if (i == cfg->nhugetlbfs) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to find any usable hugetlbfs mount for %llu KiB"), + pagesize); + goto cleanup; + } + + VIR_FREE(mem_path); + if (!(mem_path = qemuGetHugepagePath(&cfg->hugetlbfs[i]))) + goto cleanup; + + virBufferAsprintf(&buf, "memory-backend-file,prealloc=yes,mem-path=%s", + mem_path); + + switch (memAccess) { + case VIR_MEM_ACCESS_SHARED: + virBufferAddLit(&buf, ",share=on"); + break; + + case VIR_MEM_ACCESS_PRIVATE: + virBufferAddLit(&buf, ",share=off"); + break; + + case VIR_MEM_ACCESS_DEFAULT: + case VIR_MEM_ACCESS_LAST: + break; + } + } else { + if (memAccess) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Shared memory mapping is supported " + "only with hugepages")); + goto cleanup; + } + + virBufferAddLit(&buf, "memory-backend-ram"); + } + + virBufferAsprintf(&buf, ",size=%lluM,id=%s%zu", size / 1024, + aliasPrefix, id); + + if (userNodeset) { + if (!(nodemask = virBitmapFormat(userNodeset))) + goto cleanup; + } else { + if (virDomainNumatuneMaybeFormatNodeset(def->numatune, autoNodeset, + &nodemask, guestNode) < 0) + goto cleanup; + } + + if (nodemask) { + if (strchr(nodemask, ',') && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_NUMA)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("disjoint NUMA node ranges are not supported " + "with this QEMU")); + goto cleanup; + } + + for (tmpmask = nodemask; tmpmask; tmpmask = next) { + if ((next = strchr(tmpmask, ','))) + *(next++) = '\0'; + virBufferAddLit(&buf, ",host-nodes="); + virBufferAdd(&buf, tmpmask, -1); + } + + virBufferAsprintf(&buf, ",policy=%s", qemuNumaPolicyTypeToString(mode)); + } + + if (virBufferCheckError(&buf) < 0) + goto cleanup; + + *backendStr = virBufferContentAndReset(&buf); + + if (!hugepage) { + if ((nodemask || force) && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_RAM)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("this qemu doesn't support the " + "memory-backend-ram object")); + goto cleanup; + } + + /* report back that using the new backend is not necessary to achieve + * the desired configuration */ + if (!nodemask) { + ret = 1; + goto cleanup; + } + } + + ret = 0; + + cleanup: + virBufferFreeAndReset(&buf); + VIR_FREE(nodemask); + VIR_FREE(mem_path); + + return ret; +} + + +static int +qemuBuildMemoryCellBackendStr(virDomainDefPtr def, + virQEMUCapsPtr qemuCaps, + virQEMUDriverConfigPtr cfg, + size_t cell, + virBitmapPtr auto_nodeset, + char **backendStr) +{ + int ret; + + ret = qemuBuildMemoryBackendStr(def->cpu->cells[cell].mem, 0, cell, + NULL, auto_nodeset, + def, qemuCaps, cfg, + "ram-node", cell, + backendStr, false); + + if (ret == 1) { + VIR_FREE(*backendStr); + return 0; + } + + return ret; +} + + char * qemuBuildNicStr(virDomainNetDefPtr net, const char *prefix, @@ -6796,14 +7033,12 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg, virDomainDefPtr def, virCommandPtr cmd, virQEMUCapsPtr qemuCaps, - virBitmapPtr nodeset) + virBitmapPtr auto_nodeset) { - size_t i, j; + size_t i; virBuffer buf = VIR_BUFFER_INITIALIZER; - virDomainHugePagePtr master_hugepage = NULL; char *cpumask = NULL, *tmpmask = NULL, *next = NULL; - char *nodemask = NULL; - char *mem_path = NULL; + char *backendStr = NULL; int ret = -1; const long system_page_size = sysconf(_SC_PAGESIZE) / 1024; @@ -6825,7 +7060,7 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg, goto cleanup; } - if (!virDomainNumatuneNodesetIsAvailable(def->numatune, nodeset)) + if (!virDomainNumatuneNodesetIsAvailable(def->numatune, auto_nodeset)) goto cleanup; for (i = 0; i < def->mem.nhugepages; i++) { @@ -6853,13 +7088,11 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg, } for (i = 0; i < def->cpu->ncells; i++) { - virDomainHugePagePtr hugepage = NULL; unsigned long long cellmem = VIR_DIV_UP(def->cpu->cells[i].mem, 1024); def->cpu->cells[i].mem = cellmem * 1024; - virMemAccess memAccess = def->cpu->cells[i].memAccess; VIR_FREE(cpumask); - VIR_FREE(nodemask); + VIR_FREE(backendStr); if (!(cpumask = virBitmapFormat(def->cpu->cells[i].cpumask))) goto cleanup; @@ -6872,133 +7105,19 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg, goto cleanup; } + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_RAM) || virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_FILE)) { - virDomainNumatuneMemMode mode; - const char *policy = NULL; - - mode = virDomainNumatuneGetMode(def->numatune, i); - policy = qemuNumaPolicyTypeToString(mode); - - /* Find the huge page size we want to use */ - for (j = 0; j < def->mem.nhugepages; j++) { - bool thisHugepage = false; - - hugepage = &def->mem.hugepages[j]; - - if (!hugepage->nodemask) { - master_hugepage = hugepage; - continue; - } - - if (virBitmapGetBit(hugepage->nodemask, i, &thisHugepage) < 0) { - /* Ignore this error. It's not an error after all. Well, - * the nodemask for this <page/> can contain lower NUMA - * nodes than we are querying in here. */ - continue; - } - - if (thisHugepage) { - /* Hooray, we've found the page size */ - break; - } - } - - if (j == def->mem.nhugepages) { - /* We have not found specific huge page to be used with this - * NUMA node. Use the generic setting then (<page/> without any - * @nodemask) if possible. */ - hugepage = master_hugepage; - } - - if (hugepage && hugepage->size == system_page_size) { - /* However, if user specified to use "huge" page - * of regular system page size, it's as if they - * hasn't specified any huge pages at all. */ - hugepage = NULL; - } - - if (hugepage) { - /* Now lets see, if the huge page we want to use is even mounted - * and ready to use */ - - for (j = 0; j < cfg->nhugetlbfs; j++) { - if (cfg->hugetlbfs[j].size == hugepage->size) - break; - } - - if (j == cfg->nhugetlbfs) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unable to find any usable hugetlbfs mount for %llu KiB"), - hugepage->size); - goto cleanup; - } - - VIR_FREE(mem_path); - if (!(mem_path = qemuGetHugepagePath(&cfg->hugetlbfs[j]))) - goto cleanup; - - virBufferAsprintf(&buf, - "memory-backend-file,prealloc=yes,mem-path=%s", - mem_path); - - switch (memAccess) { - case VIR_MEM_ACCESS_SHARED: - virBufferAddLit(&buf, ",share=on"); - break; - - case VIR_MEM_ACCESS_PRIVATE: - virBufferAddLit(&buf, ",share=off"); - break; - - case VIR_MEM_ACCESS_DEFAULT: - case VIR_MEM_ACCESS_LAST: - break; - } - - } else { - if (memAccess) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Shared memory mapping is supported " - "only with hugepages")); - goto cleanup; - } - virBufferAddLit(&buf, "memory-backend-ram"); - } - - virBufferAsprintf(&buf, ",size=%lluM,id=ram-node%zu", cellmem, i); - - if (virDomainNumatuneMaybeFormatNodeset(def->numatune, nodeset, - &nodemask, i) < 0) + if (qemuBuildMemoryCellBackendStr(def, qemuCaps, cfg, i, + auto_nodeset, &backendStr) < 0) goto cleanup; - if (nodemask) { - if (strchr(nodemask, ',') && - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_NUMA)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("disjoint NUMA node ranges are not supported " - "with this QEMU")); - goto cleanup; - } - - for (tmpmask = nodemask; tmpmask; tmpmask = next) { - if ((next = strchr(tmpmask, ','))) - *(next++) = '\0'; - virBufferAddLit(&buf, ",host-nodes="); - virBufferAdd(&buf, tmpmask, -1); - } - - virBufferAsprintf(&buf, ",policy=%s", policy); - } - - if (hugepage || nodemask) { + if (backendStr) { virCommandAddArg(cmd, "-object"); - virCommandAddArgBuffer(cmd, &buf); - } else { - virBufferFreeAndReset(&buf); + virCommandAddArg(cmd, backendStr); } } else { - if (memAccess) { + if (def->cpu->cells[i].memAccess) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Shared memory mapping is not supported " "with this QEMU")); @@ -7016,7 +7135,7 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg, virBufferAdd(&buf, tmpmask, -1); } - if (hugepage || nodemask) + if (backendStr) virBufferAsprintf(&buf, ",memdev=ram-node%zu", i); else virBufferAsprintf(&buf, ",mem=%llu", cellmem); @@ -7027,8 +7146,7 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg, cleanup: VIR_FREE(cpumask); - VIR_FREE(nodemask); - VIR_FREE(mem_path); + VIR_FREE(backendStr); virBufferFreeAndReset(&buf); return ret; } -- 2.2.2

Move the alias as the second formated argument and tweak the tests so that a future refactor that will change the order doesn't break tests. --- src/qemu/qemu_command.c | 9 ++++----- .../qemuxml2argvdata/qemuxml2argv-hugepages-pages.args | 16 ++++++++-------- .../qemuxml2argv-hugepages-pages2.args | 8 ++++---- .../qemuxml2argv-hugepages-pages3.args | 4 ++-- .../qemuxml2argv-hugepages-shared.args | 18 ++++++++++-------- .../qemuxml2argv-numatune-memnode-no-memory.args | 2 +- .../qemuxml2argv-numatune-memnode.args | 6 +++--- 7 files changed, 32 insertions(+), 31 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index c65f187..2d89c2d 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4623,8 +4623,8 @@ qemuBuildMemoryBackendStr(unsigned long long size, if (!(mem_path = qemuGetHugepagePath(&cfg->hugetlbfs[i]))) goto cleanup; - virBufferAsprintf(&buf, "memory-backend-file,prealloc=yes,mem-path=%s", - mem_path); + virBufferAsprintf(&buf, "memory-backend-file,id=%s%zu", aliasPrefix, id); + virBufferAsprintf(&buf, ",prealloc=yes,mem-path=%s", mem_path); switch (memAccess) { case VIR_MEM_ACCESS_SHARED: @@ -4647,11 +4647,10 @@ qemuBuildMemoryBackendStr(unsigned long long size, goto cleanup; } - virBufferAddLit(&buf, "memory-backend-ram"); + virBufferAsprintf(&buf, "memory-backend-ram,id=%s%zu", aliasPrefix, id); } - virBufferAsprintf(&buf, ",size=%lluM,id=%s%zu", size / 1024, - aliasPrefix, id); + virBufferAsprintf(&buf, ",size=%lluM", size / 1024); if (userNodeset) { if (!(nodemask = virBitmapFormat(userNodeset))) diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages.args b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages.args index 042683a..b954fbc 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages.args @@ -1,16 +1,16 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ /usr/bin/qemu -S -M pc -m 4096 -smp 4 \ --object memory-backend-file,prealloc=yes,mem-path=/dev/hugepages1G/libvirt/qemu,\ -size=1024M,id=ram-node0,host-nodes=0-3,policy=bind \ +-object memory-backend-file,id=ram-node0,prealloc=yes,\ +mem-path=/dev/hugepages1G/libvirt/qemu,size=1024M,host-nodes=0-3,policy=bind \ -numa node,nodeid=0,cpus=0,memdev=ram-node0 \ --object memory-backend-file,prealloc=yes,mem-path=/dev/hugepages2M/libvirt/qemu,\ -size=1024M,id=ram-node1,host-nodes=0-3,policy=bind \ +-object memory-backend-file,id=ram-node1,prealloc=yes,\ +mem-path=/dev/hugepages2M/libvirt/qemu,size=1024M,host-nodes=0-3,policy=bind \ -numa node,nodeid=1,cpus=1,memdev=ram-node1 \ --object memory-backend-file,prealloc=yes,mem-path=/dev/hugepages1G/libvirt/qemu,\ -size=1024M,id=ram-node2,host-nodes=0-3,policy=bind \ +-object memory-backend-file,id=ram-node2,prealloc=yes,\ +mem-path=/dev/hugepages1G/libvirt/qemu,size=1024M,host-nodes=0-3,policy=bind \ -numa node,nodeid=2,cpus=2,memdev=ram-node2 \ --object memory-backend-file,prealloc=yes,mem-path=/dev/hugepages1G/libvirt/qemu,\ -size=1024M,id=ram-node3,host-nodes=3,policy=bind \ +-object memory-backend-file,id=ram-node3,prealloc=yes,\ +mem-path=/dev/hugepages1G/libvirt/qemu,size=1024M,host-nodes=3,policy=bind \ -numa node,nodeid=3,cpus=3,memdev=ram-node3 \ -nographic -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb \ -hda /dev/HostVG/QEMUGuest1 -net none -serial none -parallel none diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages2.args b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages2.args index 9211bc6..90ab776 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages2.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages2.args @@ -1,10 +1,10 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ /usr/bin/qemu -S -M pc -m 1024 -smp 2 \ --object memory-backend-file,prealloc=yes,\ -mem-path=/dev/hugepages2M/libvirt/qemu,size=256M,id=ram-node0 \ +-object memory-backend-file,id=ram-node0,prealloc=yes,\ +mem-path=/dev/hugepages2M/libvirt/qemu,size=256M \ -numa node,nodeid=0,cpus=0,memdev=ram-node0 \ --object memory-backend-file,prealloc=yes,\ -mem-path=/dev/hugepages2M/libvirt/qemu,size=768M,id=ram-node1 \ +-object memory-backend-file,id=ram-node1,prealloc=yes,\ +mem-path=/dev/hugepages2M/libvirt/qemu,size=768M \ -numa node,nodeid=1,cpus=1,memdev=ram-node1 \ -nographic -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ -usb -hda /dev/HostVG/QEMUGuest1 -net none -serial none -parallel none diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages3.args b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages3.args index f81947e..b932520 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages3.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages3.args @@ -1,8 +1,8 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ /usr/bin/qemu -S -M pc -m 1024 -smp 2 \ -numa node,nodeid=0,cpus=0,mem=256 \ --object memory-backend-file,prealloc=yes,\ -mem-path=/dev/hugepages1G/libvirt/qemu,size=768M,id=ram-node1 \ +-object memory-backend-file,id=ram-node1,prealloc=yes,\ +mem-path=/dev/hugepages1G/libvirt/qemu,size=768M \ -numa node,nodeid=1,cpus=1,memdev=ram-node1 \ -nographic -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb \ -hda /dev/HostVG/QEMUGuest1 -net none -serial none -parallel none diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-shared.args b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-shared.args index a7c7d92..b51de1b 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-shared.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-shared.args @@ -1,16 +1,18 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ /usr/bin/qemu -S -M pc -m 4096 -smp 4 \ --object memory-backend-file,prealloc=yes,mem-path=/dev/hugepages1G/libvirt/qemu,\ -size=1024M,id=ram-node0,host-nodes=0-3,policy=bind \ +-object memory-backend-file,id=ram-node0,prealloc=yes,\ +mem-path=/dev/hugepages1G/libvirt/qemu,size=1024M,host-nodes=0-3,policy=bind \ -numa node,nodeid=0,cpus=0,memdev=ram-node0 \ --object memory-backend-file,prealloc=yes,mem-path=/dev/hugepages2M/libvirt/qemu,\ -share=on,size=1024M,id=ram-node1,host-nodes=0-3,policy=bind \ +-object memory-backend-file,id=ram-node1,prealloc=yes,\ +mem-path=/dev/hugepages2M/libvirt/qemu,share=on,size=1024M,host-nodes=0-3,\ +policy=bind \ -numa node,nodeid=1,cpus=1,memdev=ram-node1 \ --object memory-backend-file,prealloc=yes,mem-path=/dev/hugepages1G/libvirt/qemu,\ -share=off,size=1024M,id=ram-node2,host-nodes=0-3,policy=bind \ +-object memory-backend-file,id=ram-node2,prealloc=yes,\ +mem-path=/dev/hugepages1G/libvirt/qemu,share=off,size=1024M,host-nodes=0-3,\ +policy=bind \ -numa node,nodeid=2,cpus=2,memdev=ram-node2 \ --object memory-backend-file,prealloc=yes,mem-path=/dev/hugepages1G/libvirt/qemu,\ -size=1024M,id=ram-node3,host-nodes=3,policy=bind \ +-object memory-backend-file,id=ram-node3,prealloc=yes,\ +mem-path=/dev/hugepages1G/libvirt/qemu,size=1024M,host-nodes=3,policy=bind \ -numa node,nodeid=3,cpus=3,memdev=ram-node3 \ -nographic -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb \ -hda /dev/HostVG/QEMUGuest1 -net none -serial none -parallel none diff --git a/tests/qemuxml2argvdata/qemuxml2argv-numatune-memnode-no-memory.args b/tests/qemuxml2argvdata/qemuxml2argv-numatune-memnode-no-memory.args index 2addf97..4f1f2aa 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-numatune-memnode-no-memory.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-numatune-memnode-no-memory.args @@ -1,6 +1,6 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ /usr/bin/kvm -S -M pc -m 64 -smp 2 \ --object memory-backend-ram,size=32M,id=ram-node0,host-nodes=3,policy=preferred \ +-object memory-backend-ram,id=ram-node0,size=32M,host-nodes=3,policy=preferred \ -numa node,nodeid=0,cpus=0,memdev=ram-node0 \ -numa node,nodeid=1,cpus=1,mem=32 \ -nographic -monitor unix:/tmp/test-monitor,server,nowait \ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-numatune-memnode.args b/tests/qemuxml2argvdata/qemuxml2argv-numatune-memnode.args index e4beb98..18007d5 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-numatune-memnode.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-numatune-memnode.args @@ -1,10 +1,10 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ /usr/bin/kvm -S -M pc -m 24104 -smp 32 \ --object memory-backend-ram,size=20M,id=ram-node0,host-nodes=3,policy=preferred \ +-object memory-backend-ram,id=ram-node0,size=20M,host-nodes=3,policy=preferred \ -numa node,nodeid=0,cpus=0,memdev=ram-node0 \ --object memory-backend-ram,size=645M,id=ram-node1,host-nodes=0-7,policy=bind \ +-object memory-backend-ram,id=ram-node1,size=645M,host-nodes=0-7,policy=bind \ -numa node,nodeid=1,cpus=1-27,cpus=29,memdev=ram-node1 \ --object memory-backend-ram,size=23440M,id=ram-node2,\ +-object memory-backend-ram,id=ram-node2,size=23440M,\ host-nodes=1-2,host-nodes=5,host-nodes=7,policy=bind \ -numa node,nodeid=2,cpus=28,cpus=30-31,memdev=ram-node2 \ -nographic -monitor unix:/tmp/test-monitor,server,nowait \ -- 2.2.2

QEMU's qapi visitor code allows yes/on/y for true and no/off/n for false value of boolean properities. Unify the used style so that we can generate it later and fix test cases. --- src/qemu/qemu_command.c | 4 ++-- tests/qemuxml2argvdata/qemuxml2argv-hugepages-shared.args | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 2d89c2d..9e0b178 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4628,11 +4628,11 @@ qemuBuildMemoryBackendStr(unsigned long long size, switch (memAccess) { case VIR_MEM_ACCESS_SHARED: - virBufferAddLit(&buf, ",share=on"); + virBufferAddLit(&buf, ",share=yes"); break; case VIR_MEM_ACCESS_PRIVATE: - virBufferAddLit(&buf, ",share=off"); + virBufferAddLit(&buf, ",share=no"); break; case VIR_MEM_ACCESS_DEFAULT: diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-shared.args b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-shared.args index b51de1b..27f476f 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-shared.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-shared.args @@ -4,11 +4,11 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ mem-path=/dev/hugepages1G/libvirt/qemu,size=1024M,host-nodes=0-3,policy=bind \ -numa node,nodeid=0,cpus=0,memdev=ram-node0 \ -object memory-backend-file,id=ram-node1,prealloc=yes,\ -mem-path=/dev/hugepages2M/libvirt/qemu,share=on,size=1024M,host-nodes=0-3,\ +mem-path=/dev/hugepages2M/libvirt/qemu,share=yes,size=1024M,host-nodes=0-3,\ policy=bind \ -numa node,nodeid=1,cpus=1,memdev=ram-node1 \ -object memory-backend-file,id=ram-node2,prealloc=yes,\ -mem-path=/dev/hugepages1G/libvirt/qemu,share=off,size=1024M,host-nodes=0-3,\ +mem-path=/dev/hugepages1G/libvirt/qemu,share=no,size=1024M,host-nodes=0-3,\ policy=bind \ -numa node,nodeid=2,cpus=2,memdev=ram-node2 \ -object memory-backend-file,id=ram-node3,prealloc=yes,\ -- 2.2.2

QEMU's command line visitor as well as the JSON interface take bytes by default for memory object sizes. Convert mebibytes to bytes so that we can later refactor the existing code for hotplug purposes. --- src/qemu/qemu_command.c | 2 +- tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages.args | 12 ++++++++---- tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages2.args | 4 ++-- tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages3.args | 2 +- tests/qemuxml2argvdata/qemuxml2argv-hugepages-shared.args | 14 ++++++++------ .../qemuxml2argv-numatune-memnode-no-memory.args | 3 ++- tests/qemuxml2argvdata/qemuxml2argv-numatune-memnode.args | 8 +++++--- 7 files changed, 27 insertions(+), 18 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 9e0b178..0c343b6 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4650,7 +4650,7 @@ qemuBuildMemoryBackendStr(unsigned long long size, virBufferAsprintf(&buf, "memory-backend-ram,id=%s%zu", aliasPrefix, id); } - virBufferAsprintf(&buf, ",size=%lluM", size / 1024); + virBufferAsprintf(&buf, ",size=%llu", size * 1024); if (userNodeset) { if (!(nodemask = virBitmapFormat(userNodeset))) diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages.args b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages.args index b954fbc..46ec751 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages.args @@ -1,16 +1,20 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ /usr/bin/qemu -S -M pc -m 4096 -smp 4 \ -object memory-backend-file,id=ram-node0,prealloc=yes,\ -mem-path=/dev/hugepages1G/libvirt/qemu,size=1024M,host-nodes=0-3,policy=bind \ +mem-path=/dev/hugepages1G/libvirt/qemu,size=1073741824,host-nodes=0-3,\ +policy=bind \ -numa node,nodeid=0,cpus=0,memdev=ram-node0 \ -object memory-backend-file,id=ram-node1,prealloc=yes,\ -mem-path=/dev/hugepages2M/libvirt/qemu,size=1024M,host-nodes=0-3,policy=bind \ +mem-path=/dev/hugepages2M/libvirt/qemu,size=1073741824,host-nodes=0-3,\ +policy=bind \ -numa node,nodeid=1,cpus=1,memdev=ram-node1 \ -object memory-backend-file,id=ram-node2,prealloc=yes,\ -mem-path=/dev/hugepages1G/libvirt/qemu,size=1024M,host-nodes=0-3,policy=bind \ +mem-path=/dev/hugepages1G/libvirt/qemu,size=1073741824,host-nodes=0-3,\ +policy=bind \ -numa node,nodeid=2,cpus=2,memdev=ram-node2 \ -object memory-backend-file,id=ram-node3,prealloc=yes,\ -mem-path=/dev/hugepages1G/libvirt/qemu,size=1024M,host-nodes=3,policy=bind \ +mem-path=/dev/hugepages1G/libvirt/qemu,size=1073741824,host-nodes=3,\ +policy=bind \ -numa node,nodeid=3,cpus=3,memdev=ram-node3 \ -nographic -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb \ -hda /dev/HostVG/QEMUGuest1 -net none -serial none -parallel none diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages2.args b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages2.args index 90ab776..0488800 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages2.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages2.args @@ -1,10 +1,10 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ /usr/bin/qemu -S -M pc -m 1024 -smp 2 \ -object memory-backend-file,id=ram-node0,prealloc=yes,\ -mem-path=/dev/hugepages2M/libvirt/qemu,size=256M \ +mem-path=/dev/hugepages2M/libvirt/qemu,size=268435456 \ -numa node,nodeid=0,cpus=0,memdev=ram-node0 \ -object memory-backend-file,id=ram-node1,prealloc=yes,\ -mem-path=/dev/hugepages2M/libvirt/qemu,size=768M \ +mem-path=/dev/hugepages2M/libvirt/qemu,size=805306368 \ -numa node,nodeid=1,cpus=1,memdev=ram-node1 \ -nographic -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ -usb -hda /dev/HostVG/QEMUGuest1 -net none -serial none -parallel none diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages3.args b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages3.args index b932520..3bca26c 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages3.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages3.args @@ -2,7 +2,7 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ /usr/bin/qemu -S -M pc -m 1024 -smp 2 \ -numa node,nodeid=0,cpus=0,mem=256 \ -object memory-backend-file,id=ram-node1,prealloc=yes,\ -mem-path=/dev/hugepages1G/libvirt/qemu,size=768M \ +mem-path=/dev/hugepages1G/libvirt/qemu,size=805306368 \ -numa node,nodeid=1,cpus=1,memdev=ram-node1 \ -nographic -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb \ -hda /dev/HostVG/QEMUGuest1 -net none -serial none -parallel none diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-shared.args b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-shared.args index 27f476f..a6e4d95 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-shared.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-shared.args @@ -1,18 +1,20 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ /usr/bin/qemu -S -M pc -m 4096 -smp 4 \ -object memory-backend-file,id=ram-node0,prealloc=yes,\ -mem-path=/dev/hugepages1G/libvirt/qemu,size=1024M,host-nodes=0-3,policy=bind \ +mem-path=/dev/hugepages1G/libvirt/qemu,size=1073741824,host-nodes=0-3,\ +policy=bind \ -numa node,nodeid=0,cpus=0,memdev=ram-node0 \ -object memory-backend-file,id=ram-node1,prealloc=yes,\ -mem-path=/dev/hugepages2M/libvirt/qemu,share=yes,size=1024M,host-nodes=0-3,\ -policy=bind \ +mem-path=/dev/hugepages2M/libvirt/qemu,share=yes,size=1073741824,\ +host-nodes=0-3,policy=bind \ -numa node,nodeid=1,cpus=1,memdev=ram-node1 \ -object memory-backend-file,id=ram-node2,prealloc=yes,\ -mem-path=/dev/hugepages1G/libvirt/qemu,share=no,size=1024M,host-nodes=0-3,\ -policy=bind \ +mem-path=/dev/hugepages1G/libvirt/qemu,share=no,size=1073741824,\ +host-nodes=0-3,policy=bind \ -numa node,nodeid=2,cpus=2,memdev=ram-node2 \ -object memory-backend-file,id=ram-node3,prealloc=yes,\ -mem-path=/dev/hugepages1G/libvirt/qemu,size=1024M,host-nodes=3,policy=bind \ +mem-path=/dev/hugepages1G/libvirt/qemu,size=1073741824,host-nodes=3,\ +policy=bind \ -numa node,nodeid=3,cpus=3,memdev=ram-node3 \ -nographic -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb \ -hda /dev/HostVG/QEMUGuest1 -net none -serial none -parallel none diff --git a/tests/qemuxml2argvdata/qemuxml2argv-numatune-memnode-no-memory.args b/tests/qemuxml2argvdata/qemuxml2argv-numatune-memnode-no-memory.args index 4f1f2aa..f0e35b7 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-numatune-memnode-no-memory.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-numatune-memnode-no-memory.args @@ -1,6 +1,7 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ /usr/bin/kvm -S -M pc -m 64 -smp 2 \ --object memory-backend-ram,id=ram-node0,size=32M,host-nodes=3,policy=preferred \ +-object memory-backend-ram,id=ram-node0,size=33554432,host-nodes=3,\ +policy=preferred \ -numa node,nodeid=0,cpus=0,memdev=ram-node0 \ -numa node,nodeid=1,cpus=1,mem=32 \ -nographic -monitor unix:/tmp/test-monitor,server,nowait \ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-numatune-memnode.args b/tests/qemuxml2argvdata/qemuxml2argv-numatune-memnode.args index 18007d5..513d657 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-numatune-memnode.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-numatune-memnode.args @@ -1,10 +1,12 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ /usr/bin/kvm -S -M pc -m 24104 -smp 32 \ --object memory-backend-ram,id=ram-node0,size=20M,host-nodes=3,policy=preferred \ +-object memory-backend-ram,id=ram-node0,size=20971520,host-nodes=3,\ +policy=preferred \ -numa node,nodeid=0,cpus=0,memdev=ram-node0 \ --object memory-backend-ram,id=ram-node1,size=645M,host-nodes=0-7,policy=bind \ +-object memory-backend-ram,id=ram-node1,size=676331520,host-nodes=0-7,\ +policy=bind \ -numa node,nodeid=1,cpus=1-27,cpus=29,memdev=ram-node1 \ --object memory-backend-ram,id=ram-node2,size=23440M,\ +-object memory-backend-ram,id=ram-node2,size=24578621440,\ host-nodes=1-2,host-nodes=5,host-nodes=7,policy=bind \ -numa node,nodeid=2,cpus=28,cpus=30-31,memdev=ram-node2 \ -nographic -monitor unix:/tmp/test-monitor,server,nowait \ -- 2.2.2

With the new JSON to argv formatter we are now able to represent the memory backend definitions in the JSON object format that is reusable for monitor use (hotplug) and then convert it into the shell string. This will avoid having two separate instances of the same code that would create the different formats. Previous refactors now allow to make this step without changes to the test suite. --- src/qemu/qemu_command.c | 109 +++++++++++++++++++++++++++--------------------- 1 file changed, 62 insertions(+), 47 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 0c343b6..bb58a09 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4533,12 +4533,10 @@ qemuBuildMemoryBackendStr(unsigned long long size, virDomainDefPtr def, virQEMUCapsPtr qemuCaps, virQEMUDriverConfigPtr cfg, - const char *aliasPrefix, - size_t id, - char **backendStr, + const char **backendType, + virJSONValuePtr *backendProps, bool force) { - virBuffer buf = VIR_BUFFER_INITIALIZER; virDomainHugePagePtr master_hugepage = NULL; virDomainHugePagePtr hugepage = NULL; virDomainNumatuneMemMode mode; @@ -4546,11 +4544,15 @@ qemuBuildMemoryBackendStr(unsigned long long size, virMemAccess memAccess = def->cpu->cells[guestNode].memAccess; size_t i; char *mem_path = NULL; - char *nodemask = NULL; - char *tmpmask = NULL, *next = NULL; + virBitmapPtr nodemask = NULL; int ret = -1; + virJSONValuePtr props = NULL; - *backendStr = NULL; + *backendProps = NULL; + *backendType = NULL; + + if (!(props = virJSONValueNewObject())) + return -1; mode = virDomainNumatuneGetMode(def->numatune, guestNode); @@ -4623,16 +4625,23 @@ qemuBuildMemoryBackendStr(unsigned long long size, if (!(mem_path = qemuGetHugepagePath(&cfg->hugetlbfs[i]))) goto cleanup; - virBufferAsprintf(&buf, "memory-backend-file,id=%s%zu", aliasPrefix, id); - virBufferAsprintf(&buf, ",prealloc=yes,mem-path=%s", mem_path); + *backendType = "memory-backend-file"; + + if (virJSONValueObjectAdd(props, + "b:prealloc", true, + "s:mem-path", mem_path, + NULL) < 0) + goto cleanup; switch (memAccess) { case VIR_MEM_ACCESS_SHARED: - virBufferAddLit(&buf, ",share=yes"); + if (virJSONValueObjectAdd(props, "b:share", true, NULL) < 0) + goto cleanup; break; case VIR_MEM_ACCESS_PRIVATE: - virBufferAddLit(&buf, ",share=no"); + if (virJSONValueObjectAdd(props, "b:share", false, NULL) < 0) + goto cleanup; break; case VIR_MEM_ACCESS_DEFAULT: @@ -4647,43 +4656,30 @@ qemuBuildMemoryBackendStr(unsigned long long size, goto cleanup; } - virBufferAsprintf(&buf, "memory-backend-ram,id=%s%zu", aliasPrefix, id); + *backendType = "memory-backend-ram"; } - virBufferAsprintf(&buf, ",size=%llu", size * 1024); + if (virJSONValueObjectAdd(props, "U:size", size * 1024, NULL) < 0) + goto cleanup; if (userNodeset) { - if (!(nodemask = virBitmapFormat(userNodeset))) - goto cleanup; + nodemask = userNodeset; } else { - if (virDomainNumatuneMaybeFormatNodeset(def->numatune, autoNodeset, - &nodemask, guestNode) < 0) + if (virDomainNumatuneMaybeGetNodeset(def->numatune, autoNodeset, + &nodemask, guestNode) < 0) goto cleanup; } if (nodemask) { - if (strchr(nodemask, ',') && - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_NUMA)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("disjoint NUMA node ranges are not supported " - "with this QEMU")); + if (virJSONValueObjectAdd(props, + "m:host-nodes", nodemask, + "S:policy", qemuNumaPolicyTypeToString(mode), + NULL) < 0) goto cleanup; - } - - for (tmpmask = nodemask; tmpmask; tmpmask = next) { - if ((next = strchr(tmpmask, ','))) - *(next++) = '\0'; - virBufferAddLit(&buf, ",host-nodes="); - virBufferAdd(&buf, tmpmask, -1); - } - - virBufferAsprintf(&buf, ",policy=%s", qemuNumaPolicyTypeToString(mode)); } - if (virBufferCheckError(&buf) < 0) - goto cleanup; - - *backendStr = virBufferContentAndReset(&buf); + *backendProps = props; + props = NULL; if (!hugepage) { if ((nodemask || force) && @@ -4705,8 +4701,7 @@ qemuBuildMemoryBackendStr(unsigned long long size, ret = 0; cleanup: - virBufferFreeAndReset(&buf); - VIR_FREE(nodemask); + virJSONValueFree(props); VIR_FREE(mem_path); return ret; @@ -4721,19 +4716,39 @@ qemuBuildMemoryCellBackendStr(virDomainDefPtr def, virBitmapPtr auto_nodeset, char **backendStr) { - int ret; + virJSONValuePtr props = NULL; + char *alias = NULL; + const char *backendType; + int ret = -1; + int rc; - ret = qemuBuildMemoryBackendStr(def->cpu->cells[cell].mem, 0, cell, - NULL, auto_nodeset, - def, qemuCaps, cfg, - "ram-node", cell, - backendStr, false); + *backendStr = NULL; - if (ret == 1) { - VIR_FREE(*backendStr); - return 0; + if (virAsprintf(&alias, "ram-node%zu", cell) < 0) + goto cleanup; + + if ((rc = qemuBuildMemoryBackendStr(def->cpu->cells[cell].mem, 0, cell, + NULL, auto_nodeset, + def, qemuCaps, cfg, + &backendType, &props, false)) < 0) + goto cleanup; + + if (rc == 1) { + ret = 0; + goto cleanup; } + if (!(*backendStr = qemuBuildObjectCommandlineFromJSON(backendType, + alias, + props)) < 0) + goto cleanup; + + ret = 0; + + cleanup: + VIR_FREE(alias); + virJSONValueFree(props); + return ret; } -- 2.2.2

On 28.01.2015 11:30, Peter Krempa wrote:
With the new JSON to argv formatter we are now able to represent the memory backend definitions in the JSON object format that is reusable for monitor use (hotplug) and then convert it into the shell string. This will avoid having two separate instances of the same code that would create the different formats.
Previous refactors now allow to make this step without changes to the test suite. --- src/qemu/qemu_command.c | 109 +++++++++++++++++++++++++++--------------------- 1 file changed, 62 insertions(+), 47 deletions(-)
So we will construct a JSON object just to produce a command line? Well, I am afraid there's no better way if want to use the same code to generate the monitor commands one day. Michal

On 01/28/2015 05:30 AM, Peter Krempa wrote:
With the new JSON to argv formatter we are now able to represent the memory backend definitions in the JSON object format that is reusable for monitor use (hotplug) and then convert it into the shell string. This will avoid having two separate instances of the same code that would create the different formats.
Previous refactors now allow to make this step without changes to the test suite. --- src/qemu/qemu_command.c | 109 +++++++++++++++++++++++++++--------------------- 1 file changed, 62 insertions(+), 47 deletions(-)
Ran changes through my Coverity checker... Issue found with this change...
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 0c343b6..bb58a09 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4533,12 +4533,10 @@ qemuBuildMemoryBackendStr(unsigned long long size, virDomainDefPtr def, virQEMUCapsPtr qemuCaps, virQEMUDriverConfigPtr cfg, - const char *aliasPrefix, - size_t id, - char **backendStr, + const char **backendType, + virJSONValuePtr *backendProps, bool force) { - virBuffer buf = VIR_BUFFER_INITIALIZER; virDomainHugePagePtr master_hugepage = NULL; virDomainHugePagePtr hugepage = NULL; virDomainNumatuneMemMode mode; @@ -4546,11 +4544,15 @@ qemuBuildMemoryBackendStr(unsigned long long size, virMemAccess memAccess = def->cpu->cells[guestNode].memAccess; size_t i; char *mem_path = NULL; - char *nodemask = NULL; - char *tmpmask = NULL, *next = NULL; + virBitmapPtr nodemask = NULL; int ret = -1; + virJSONValuePtr props = NULL;
- *backendStr = NULL; + *backendProps = NULL; + *backendType = NULL; + + if (!(props = virJSONValueNewObject())) + return -1;
mode = virDomainNumatuneGetMode(def->numatune, guestNode);
@@ -4623,16 +4625,23 @@ qemuBuildMemoryBackendStr(unsigned long long size, if (!(mem_path = qemuGetHugepagePath(&cfg->hugetlbfs[i]))) goto cleanup;
- virBufferAsprintf(&buf, "memory-backend-file,id=%s%zu", aliasPrefix, id); - virBufferAsprintf(&buf, ",prealloc=yes,mem-path=%s", mem_path); + *backendType = "memory-backend-file"; + + if (virJSONValueObjectAdd(props, + "b:prealloc", true, + "s:mem-path", mem_path, + NULL) < 0) + goto cleanup;
switch (memAccess) { case VIR_MEM_ACCESS_SHARED: - virBufferAddLit(&buf, ",share=yes"); + if (virJSONValueObjectAdd(props, "b:share", true, NULL) < 0) + goto cleanup; break;
case VIR_MEM_ACCESS_PRIVATE: - virBufferAddLit(&buf, ",share=no"); + if (virJSONValueObjectAdd(props, "b:share", false, NULL) < 0) + goto cleanup; break;
case VIR_MEM_ACCESS_DEFAULT: @@ -4647,43 +4656,30 @@ qemuBuildMemoryBackendStr(unsigned long long size, goto cleanup; }
- virBufferAsprintf(&buf, "memory-backend-ram,id=%s%zu", aliasPrefix, id); + *backendType = "memory-backend-ram"; }
- virBufferAsprintf(&buf, ",size=%llu", size * 1024); + if (virJSONValueObjectAdd(props, "U:size", size * 1024, NULL) < 0) + goto cleanup;
if (userNodeset) { - if (!(nodemask = virBitmapFormat(userNodeset))) - goto cleanup; + nodemask = userNodeset; } else { - if (virDomainNumatuneMaybeFormatNodeset(def->numatune, autoNodeset, - &nodemask, guestNode) < 0) + if (virDomainNumatuneMaybeGetNodeset(def->numatune, autoNodeset, + &nodemask, guestNode) < 0) goto cleanup; }
if (nodemask) { - if (strchr(nodemask, ',') && - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_NUMA)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("disjoint NUMA node ranges are not supported " - "with this QEMU")); + if (virJSONValueObjectAdd(props, + "m:host-nodes", nodemask, + "S:policy", qemuNumaPolicyTypeToString(mode), + NULL) < 0) goto cleanup; - } - - for (tmpmask = nodemask; tmpmask; tmpmask = next) { - if ((next = strchr(tmpmask, ','))) - *(next++) = '\0'; - virBufferAddLit(&buf, ",host-nodes="); - virBufferAdd(&buf, tmpmask, -1); - } - - virBufferAsprintf(&buf, ",policy=%s", qemuNumaPolicyTypeToString(mode)); }
- if (virBufferCheckError(&buf) < 0) - goto cleanup; - - *backendStr = virBufferContentAndReset(&buf); + *backendProps = props; + props = NULL;
if (!hugepage) { if ((nodemask || force) && @@ -4705,8 +4701,7 @@ qemuBuildMemoryBackendStr(unsigned long long size, ret = 0;
cleanup: - virBufferFreeAndReset(&buf); - VIR_FREE(nodemask); + virJSONValueFree(props); VIR_FREE(mem_path);
return ret; @@ -4721,19 +4716,39 @@ qemuBuildMemoryCellBackendStr(virDomainDefPtr def, virBitmapPtr auto_nodeset, char **backendStr) { - int ret; + virJSONValuePtr props = NULL; + char *alias = NULL; + const char *backendType; + int ret = -1; + int rc;
- ret = qemuBuildMemoryBackendStr(def->cpu->cells[cell].mem, 0, cell, - NULL, auto_nodeset, - def, qemuCaps, cfg, - "ram-node", cell, - backendStr, false); + *backendStr = NULL;
- if (ret == 1) { - VIR_FREE(*backendStr); - return 0; + if (virAsprintf(&alias, "ram-node%zu", cell) < 0) + goto cleanup; + + if ((rc = qemuBuildMemoryBackendStr(def->cpu->cells[cell].mem, 0, cell, + NULL, auto_nodeset, + def, qemuCaps, cfg, + &backendType, &props, false)) < 0) + goto cleanup; + + if (rc == 1) { + ret = 0; + goto cleanup; }
Coverity Error: CONSTANT_EXPRESSION_RESULT 4735 (1) Event result_independent_of_operands: "!(*backendStr = qemuBuildObjectCommandlineFromJSON(backendType, alias, props)) < 0" is always false regardless of the values of its operands. This occurs as the logical operand of if. 4736 if (!(*backendStr = qemuBuildObjectCommandlineFromJSON(backendType, 4737 alias, 4738 props)) < 0) ^^^^ Looks like the " < 0" is unnecessary And of course it cglames the goto cleanup is DEADCODE because of that.
+ if (!(*backendStr = qemuBuildObjectCommandlineFromJSON(backendType, + alias, + props)) < 0) + goto cleanup; + + ret = 0; + + cleanup: + VIR_FREE(alias); + virJSONValueFree(props); + return ret; }

Change done by commit f309db1f4d51009bad0d32e12efc75530b66836b wrongly assumes that qemu can start with a combination of NUMA nodes specified with the "memdev" option and the appropriate backends, and the legacy way by specifying only "mem" as a size argument. QEMU rejects such commandline though: $ /usr/bin/qemu-system-x86_64 -S -M pc -m 1024 -smp 2 \ -numa node,nodeid=0,cpus=0,mem=256 \ -object memory-backend-ram,id=ram-node1,size=12345 \ -numa node,nodeid=1,cpus=1,memdev=ram-node1 qemu-system-x86_64: -numa node,nodeid=1,cpus=1,memdev=ram-node1: qemu: memdev option must be specified for either all or no nodes To fix this issue we need to check if any of the nodes requires the new definition with the backend and if so, then all other nodes have to use it too. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1182467 --- src/qemu/qemu_command.c | 72 ++++++++++++---------- .../qemuxml2argv-hugepages-pages3.args | 3 +- .../qemuxml2argv-numatune-memnode-no-memory.args | 3 +- 3 files changed, 45 insertions(+), 33 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index bb58a09..8089e4e 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4733,17 +4733,12 @@ qemuBuildMemoryCellBackendStr(virDomainDefPtr def, &backendType, &props, false)) < 0) goto cleanup; - if (rc == 1) { - ret = 0; - goto cleanup; - } - if (!(*backendStr = qemuBuildObjectCommandlineFromJSON(backendType, alias, props)) < 0) goto cleanup; - ret = 0; + ret = rc; cleanup: VIR_FREE(alias); @@ -7052,7 +7047,9 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg, size_t i; virBuffer buf = VIR_BUFFER_INITIALIZER; char *cpumask = NULL, *tmpmask = NULL, *next = NULL; - char *backendStr = NULL; + char **nodeBackends = NULL; + bool needBackend = false; + int rc; int ret = -1; const long system_page_size = sysconf(_SC_PAGESIZE) / 1024; @@ -7101,35 +7098,24 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg, } } + if (VIR_ALLOC_N(nodeBackends, def->cpu->ncells) < 0) + goto cleanup; + + /* using of -numa memdev= cannot be combined with -numa mem=, thus we + * need to check which approach to use */ for (i = 0; i < def->cpu->ncells; i++) { unsigned long long cellmem = VIR_DIV_UP(def->cpu->cells[i].mem, 1024); def->cpu->cells[i].mem = cellmem * 1024; - VIR_FREE(cpumask); - VIR_FREE(backendStr); - - if (!(cpumask = virBitmapFormat(def->cpu->cells[i].cpumask))) - goto cleanup; - - if (strchr(cpumask, ',') && - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_NUMA)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("disjoint NUMA cpu ranges are not supported " - "with this QEMU")); - goto cleanup; - } - - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_RAM) || virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_FILE)) { - if (qemuBuildMemoryCellBackendStr(def, qemuCaps, cfg, i, - auto_nodeset, &backendStr) < 0) + if ((rc = qemuBuildMemoryCellBackendStr(def, qemuCaps, cfg, i, + auto_nodeset, + &nodeBackends[i])) < 0) goto cleanup; - if (backendStr) { - virCommandAddArg(cmd, "-object"); - virCommandAddArg(cmd, backendStr); - } + if (rc == 0) + needBackend = true; } else { if (def->cpu->cells[i].memAccess) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -7138,6 +7124,23 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg, goto cleanup; } } + } + + for (i = 0; i < def->cpu->ncells; i++) { + VIR_FREE(cpumask); + if (!(cpumask = virBitmapFormat(def->cpu->cells[i].cpumask))) + goto cleanup; + + if (strchr(cpumask, ',') && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_NUMA)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("disjoint NUMA cpu ranges are not supported " + "with this QEMU")); + goto cleanup; + } + + if (needBackend) + virCommandAddArgList(cmd, "-object", nodeBackends[i], NULL); virCommandAddArg(cmd, "-numa"); virBufferAsprintf(&buf, "node,nodeid=%zu", i); @@ -7149,10 +7152,10 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg, virBufferAdd(&buf, tmpmask, -1); } - if (backendStr) + if (needBackend) virBufferAsprintf(&buf, ",memdev=ram-node%zu", i); else - virBufferAsprintf(&buf, ",mem=%llu", cellmem); + virBufferAsprintf(&buf, ",mem=%llu", def->cpu->cells[i].mem / 1024); virCommandAddArgBuffer(cmd, &buf); } @@ -7160,7 +7163,14 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg, cleanup: VIR_FREE(cpumask); - VIR_FREE(backendStr); + + if (nodeBackends) { + for (i = 0; i < def->cpu->ncells; i++) + VIR_FREE(nodeBackends[i]); + + VIR_FREE(nodeBackends); + } + virBufferFreeAndReset(&buf); return ret; } diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages3.args b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages3.args index 3bca26c..bf37a69 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages3.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages3.args @@ -1,6 +1,7 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ /usr/bin/qemu -S -M pc -m 1024 -smp 2 \ --numa node,nodeid=0,cpus=0,mem=256 \ +-object memory-backend-ram,id=ram-node0,size=268435456 \ +-numa node,nodeid=0,cpus=0,memdev=ram-node0 \ -object memory-backend-file,id=ram-node1,prealloc=yes,\ mem-path=/dev/hugepages1G/libvirt/qemu,size=805306368 \ -numa node,nodeid=1,cpus=1,memdev=ram-node1 \ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-numatune-memnode-no-memory.args b/tests/qemuxml2argvdata/qemuxml2argv-numatune-memnode-no-memory.args index f0e35b7..f9a35ac 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-numatune-memnode-no-memory.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-numatune-memnode-no-memory.args @@ -3,6 +3,7 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ -object memory-backend-ram,id=ram-node0,size=33554432,host-nodes=3,\ policy=preferred \ -numa node,nodeid=0,cpus=0,memdev=ram-node0 \ --numa node,nodeid=1,cpus=1,mem=32 \ +-object memory-backend-ram,id=ram-node1,size=33554432 \ +-numa node,nodeid=1,cpus=1,memdev=ram-node1 \ -nographic -monitor unix:/tmp/test-monitor,server,nowait \ -no-acpi -boot c -usb -net none -serial none -parallel none -- 2.2.2
participants (4)
-
Eric Blake
-
John Ferlan
-
Michal Privoznik
-
Peter Krempa