This function is returning a string (domain XML). Since d3ce7363 when it was first introduced, it was indented incorrectly: static char *qemuMigrationBeginPhase(..) Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/qemu/qemu_migration.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index f7432e8..32c43f3 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2699,14 +2699,14 @@ qemuMigrationCleanup(virDomainObjPtr vm, /* The caller is supposed to lock the vm and start a migration job. */ -static char -*qemuMigrationBeginPhase(virQEMUDriverPtr driver, - virDomainObjPtr vm, - const char *xmlin, - const char *dname, - char **cookieout, - int *cookieoutlen, - unsigned long flags) +static char * +qemuMigrationBeginPhase(virQEMUDriverPtr driver, + virDomainObjPtr vm, + const char *xmlin, + const char *dname, + char **cookieout, + int *cookieoutlen, + unsigned long flags) { char *rv = NULL; qemuMigrationCookiePtr mig = NULL; -- 2.3.6

On 05/26/2015 09:01 AM, Michal Privoznik wrote: format is not initialized here... then only initialized if src->format > 0 Also this should be VIR_STORAGE_FILE_NONE instead of 0 Then used here... My compiler balked... Initializing to NULL worked... ACK with that change. John

From: Pavel Boldin <pboldin(a)mirantis.com&gt; The `virTypedParamsValidate' function now can be instructed to allow multiple entries for some of the keys. For this flag the type with the `VIR_TYPED_PARAM_MULTIPLE' flag. Add unit tests for this new behaviour. Signed-off-by: Pavel Boldin <pboldin(a)mirantis.com&gt; Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/util/virtypedparam.c | 108 +++++++++++++++++++----------- src/util/virtypedparam.h | 10 +++ tests/Makefile.am | 6 ++ tests/virtypedparamtest.c | 167 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 252 insertions(+), 39 deletions(-) create mode 100644 tests/virtypedparamtest.c diff --git a/src/util/virtypedparam.c b/src/util/virtypedparam.c index de2d447..ab32fc7 100644 --- a/src/util/virtypedparam.c +++ b/src/util/virtypedparam.c @@ -47,11 +47,18 @@ VIR_ENUM_IMPL(virTypedParameter, VIR_TYPED_PARAM_LAST, * internal utility functions (those in libvirt_private.syms) may * report errors that the caller will dispatch. */ +static int virTypedParamsSortName(const void *left, const void *right) +{ + const virTypedParameter *param_left = left, *param_right = right; + return strcmp(param_left->field, param_right->field); +} + /* Validate that PARAMS contains only recognized parameter names with - * correct types, and with no duplicates. Pass in as many name/type - * pairs as appropriate, and pass NULL to end the list of accepted - * parameters. Return 0 on success, -1 on failure with error message - * already issued. */ + * correct types, and with no duplicates except for parameters + * specified with VIR_TYPED_PARAM_MULTIPLE flag in type. + * Pass in as many name/type pairs as appropriate, and pass NULL to end + * the list of accepted parameters. Return 0 on success, -1 on failure + * with error message already issued. */ int virTypedParamsValidate(virTypedParameterPtr params, int nparams, ...) { @@ -60,60 +67,83 @@ virTypedParamsValidate(virTypedParameterPtr params, int nparams, ...) size_t i, j; const char *name; int type; + size_t nkeys = 0, nkeysalloc = 0; + virTypedParameterPtr sorted = NULL, keys = NULL; va_start(ap, nparams); - /* Yes, this is quadratic, but since we reject duplicates and - * unknowns, it is constrained by the number of var-args passed - * in, which is expected to be small enough to not be - * noticeable. */ - for (i = 0; i < nparams; i++) { - va_end(ap); - va_start(ap, nparams); + if (VIR_ALLOC_N(sorted, nparams) < 0) + goto cleanup; + /* Here we intentionally don't copy values */ + memcpy(sorted, params, sizeof(*params) * nparams); + qsort(sorted, nparams, sizeof(*sorted), virTypedParamsSortName); + + name = va_arg(ap, const char *); + while (name) { + type = va_arg(ap, int); + if (VIR_RESIZE_N(keys, nkeysalloc, nkeys, 1) < 0) + goto cleanup; + + if (virStrcpyStatic(keys[nkeys].field, name) == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Field name '%s' too long"), name); + goto cleanup; + } + + keys[nkeys].type = type & ~VIR_TYPED_PARAM_MULTIPLE; + /* Value is not used anyway */ + keys[nkeys].value.i = type & VIR_TYPED_PARAM_MULTIPLE; + + nkeys++; name = va_arg(ap, const char *); - while (name) { - type = va_arg(ap, int); - if (STREQ(params[i].field, name)) { - if (params[i].type != type) { - const char *badtype; + } - badtype = virTypedParameterTypeToString(params[i].type); - if (!badtype) - badtype = virTypedParameterTypeToString(0); - virReportError(VIR_ERR_INVALID_ARG, - _("invalid type '%s' for parameter '%s', " - "expected '%s'"), - badtype, params[i].field, - virTypedParameterTypeToString(type)); - } - break; - } - name = va_arg(ap, const char *); - } - if (!name) { - virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, - _("parameter '%s' not supported"), - params[i].field); - goto cleanup; - } - for (j = 0; j < i; j++) { - if (STREQ(params[i].field, params[j].field)) { + qsort(keys, nkeys, sizeof(*keys), virTypedParamsSortName); + + for (i = 0, j = 0; i < nparams && j < nkeys;) { + if (STRNEQ(sorted[i].field, keys[j].field)) { + j++; + } else { + if (i > j && !(keys[j].value.i & VIR_TYPED_PARAM_MULTIPLE)) { virReportError(VIR_ERR_INVALID_ARG, _("parameter '%s' occurs multiple times"), - params[i].field); + sorted[i].field); goto cleanup; } + if (sorted[i].type != keys[j].type) { + const char *badtype; + + badtype = virTypedParameterTypeToString(sorted[i].type); + if (!badtype) + badtype = virTypedParameterTypeToString(0); + virReportError(VIR_ERR_INVALID_ARG, + _("invalid type '%s' for parameter '%s', " + "expected '%s'"), + badtype, sorted[i].field, + virTypedParameterTypeToString(keys[j].type)); + goto cleanup; + } + i++; } } + if (j == nkeys && i != nparams) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, + _("parameter '%s' not supported"), + sorted[i].field); + goto cleanup; + } + ret = 0; cleanup: va_end(ap); + VIR_FREE(sorted); + VIR_FREE(keys); return ret; - } + /* Check if params contains only specified parameter names. Return true if * only specified names are present in params, false if params contains any * unspecified parameter name. */ diff --git a/src/util/virtypedparam.h b/src/util/virtypedparam.h index 0c18504..9f2d08c 100644 --- a/src/util/virtypedparam.h +++ b/src/util/virtypedparam.h @@ -26,6 +26,16 @@ # include "internal.h" # include "virutil.h" +/** + * VIR_TYPED_PARAM_MULTIPLE: + * + * Flag indiciating that the params has multiple occurences of the parameter. + * Only used as a flag for @type argument of the virTypedParamsValidate. + */ +# define VIR_TYPED_PARAM_MULTIPLE (1 << 31) + +verify(!(VIR_TYPED_PARAM_LAST & VIR_TYPED_PARAM_MULTIPLE)); + int virTypedParamsValidate(virTypedParameterPtr params, int nparams, /* const char *name, int type ... */ ...) ATTRIBUTE_SENTINEL ATTRIBUTE_RETURN_CHECK; diff --git a/tests/Makefile.am b/tests/Makefile.am index 8e2dbec..9efb441 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -182,6 +182,7 @@ test_programs = virshtest sockettest \ virhostdevtest \ vircaps2xmltest \ virnetdevtest \ + virtypedparamtest \ $(NULL) if WITH_REMOTE @@ -1225,6 +1226,11 @@ objecteventtest_SOURCES = \ testutils.c testutils.h objecteventtest_LDADD = $(LDADDS) +virtypedparamtest_SOURCES = \ + virtypedparamtest.c testutils.h testutils.c +virtypedparamtest_LDADD = $(LDADDS) + + if WITH_LINUX fchosttest_SOURCES = \ fchosttest.c testutils.h testutils.c diff --git a/tests/virtypedparamtest.c b/tests/virtypedparamtest.c new file mode 100644 index 0000000..95e22a7 --- /dev/null +++ b/tests/virtypedparamtest.c @@ -0,0 +1,167 @@ +/* + * virtypedparamtest.c: Test typed param functions + * + * Copyright (C) 2015 Mirantis, 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 <stdio.h> +#include <virtypedparam.h> + +#include "testutils.h" + +#define VIR_FROM_THIS VIR_FROM_NONE + +typedef struct _TypedParameterTest { + /* Test name for logging */ + const char *name; + /* Flags of the "foobar" parameter check */ + int foobar_flags; + /* Parameters to validate */ + virTypedParameterPtr params; + /* Amount of parameters */ + int nparams; + + /* Expected error code */ + int expected_errcode; + /* Expected error message */ + const char *expected_errmessage; +} TypedParameterTest; + +static int +testTypedParamsValidate(const void *opaque) +{ + int rv; + TypedParameterTest *test = (TypedParameterTest *)opaque; + virErrorPtr errptr; + + rv = virTypedParamsValidate( + test->params, test->nparams, + "foobar", VIR_TYPED_PARAM_STRING | test->foobar_flags, + "foo", VIR_TYPED_PARAM_INT, + "bar", VIR_TYPED_PARAM_UINT, + NULL); + + if (test->expected_errcode) { + errptr = virGetLastError(); + + rv = (errptr == NULL) || ((rv < 0) && + !(errptr->code == test->expected_errcode)); + if (errptr && test->expected_errmessage) { + rv = STRNEQ(test->expected_errmessage, errptr->message); + if (rv) + printf("%s\n", errptr->message); + } + } + + return rv; +} + +#define PARAMS_ARRAY(...) ((virTypedParameter[]){ __VA_ARGS__ }) +#define PARAMS_SIZE(...) ARRAY_CARDINALITY(PARAMS_ARRAY(__VA_ARGS__)) + +#define PARAMS(...) \ + .params = PARAMS_ARRAY(__VA_ARGS__), \ + .nparams = PARAMS_SIZE(__VA_ARGS__), + +static int +testTypedParamsValidator(void) +{ + size_t i; + int rv = 0; + + TypedParameterTest test[] = { + { + .name = "Invalid arg type", + .foobar_flags = 0, + PARAMS({ .field = "foobar", .type = VIR_TYPED_PARAM_INT }) + .expected_errcode = VIR_ERR_INVALID_ARG, + .expected_errmessage = + "invalid argument: invalid type 'int' for parameter " + "'foobar', expected 'string'" + }, + { + .name = "Extra arg", + .foobar_flags = 0, + PARAMS({ .field = "f", .type = VIR_TYPED_PARAM_INT }) + .expected_errcode = VIR_ERR_INVALID_ARG, + .expected_errmessage = + "argument unsupported: parameter 'f' not supported" + }, + { + .name = "Valid parameters", + .foobar_flags = 0, + PARAMS( + { .field = "bar", .type = VIR_TYPED_PARAM_UINT }, + { .field = "foobar", .type = VIR_TYPED_PARAM_STRING }, + { .field = "foo", .type = VIR_TYPED_PARAM_INT } + ) + .expected_errcode = 0, .expected_errmessage = NULL, + }, + { + .name = "Duplicates incorrect", + .foobar_flags = 0, + PARAMS( + { .field = "bar", .type = VIR_TYPED_PARAM_UINT }, + { .field = "foobar", .type = VIR_TYPED_PARAM_STRING }, + { .field = "foobar", .type = VIR_TYPED_PARAM_STRING }, + { .field = "foo", .type = VIR_TYPED_PARAM_INT } + ) + .expected_errcode = VIR_ERR_INVALID_ARG, + .expected_errmessage = + "invalid argument: parameter 'foobar' occurs multiple times" + }, + { + .name = "Duplicates OK for marked", + .foobar_flags = VIR_TYPED_PARAM_MULTIPLE, + PARAMS( + { .field = "bar", .type = VIR_TYPED_PARAM_UINT }, + { .field = "foobar", .type = VIR_TYPED_PARAM_STRING }, + { .field = "foobar", .type = VIR_TYPED_PARAM_STRING }, + { .field = "foo", .type = VIR_TYPED_PARAM_INT } + ) + .expected_errcode = 0, .expected_errmessage = NULL, + }, + { + .name = NULL + } + }; + + for (i = 0; test[i].name; ++i) { + if (virtTestRun(test[i].name, testTypedParamsValidate, &test[i]) < 0) + rv = -1; + } + + return rv; +} + +static int +mymain(void) +{ + int rv = 0; + + if (testTypedParamsValidator() < 0) + rv = -1; + + if (rv < 0) + return EXIT_FAILURE; + return EXIT_SUCCESS; +} + +VIRT_TEST_MAIN(mymain) -- 2.3.6

From: Pavel Boldin <pboldin(a)mirantis.com&gt; Allow multi-value parameters to be build using virTypedParamsAdd* functions by removing check for duplicates. Signed-off-by: Pavel Boldin <pboldin(a)mirantis.com&gt; Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/util/virtypedparam.c | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/src/util/virtypedparam.c b/src/util/virtypedparam.c index ab32fc7..e91ca99 100644 --- a/src/util/virtypedparam.c +++ b/src/util/virtypedparam.c @@ -748,14 +748,6 @@ virTypedParamsGetString(virTypedParameterPtr params, } -#define VIR_TYPED_PARAM_CHECK() \ - do { if (virTypedParamsGet(*params, n, name)) { \ - virReportError(VIR_ERR_INVALID_ARG, \ - _("Parameter '%s' is already set"), name); \ - goto error; \ - } } while (0) - - /** * virTypedParamsAddInt: * @params: pointer to the array of typed parameters @@ -786,7 +778,6 @@ virTypedParamsAddInt(virTypedParameterPtr *params, virResetLastError(); - VIR_TYPED_PARAM_CHECK(); if (VIR_RESIZE_N(*params, max, n, 1) < 0) goto error; *maxparams = max; @@ -834,7 +825,6 @@ virTypedParamsAddUInt(virTypedParameterPtr *params, virResetLastError(); - VIR_TYPED_PARAM_CHECK(); if (VIR_RESIZE_N(*params, max, n, 1) < 0) goto error; *maxparams = max; @@ -882,7 +872,6 @@ virTypedParamsAddLLong(virTypedParameterPtr *params, virResetLastError(); - VIR_TYPED_PARAM_CHECK(); if (VIR_RESIZE_N(*params, max, n, 1) < 0) goto error; *maxparams = max; @@ -930,7 +919,6 @@ virTypedParamsAddULLong(virTypedParameterPtr *params, virResetLastError(); - VIR_TYPED_PARAM_CHECK(); if (VIR_RESIZE_N(*params, max, n, 1) < 0) goto error; *maxparams = max; @@ -978,7 +966,6 @@ virTypedParamsAddDouble(virTypedParameterPtr *params, virResetLastError(); - VIR_TYPED_PARAM_CHECK(); if (VIR_RESIZE_N(*params, max, n, 1) < 0) goto error; *maxparams = max; @@ -1026,7 +1013,6 @@ virTypedParamsAddBoolean(virTypedParameterPtr *params, virResetLastError(); - VIR_TYPED_PARAM_CHECK(); if (VIR_RESIZE_N(*params, max, n, 1) < 0) goto error; *maxparams = max; @@ -1077,7 +1063,6 @@ virTypedParamsAddString(virTypedParameterPtr *params, virResetLastError(); - VIR_TYPED_PARAM_CHECK(); if (VIR_RESIZE_N(*params, max, n, 1) < 0) goto error; *maxparams = max; @@ -1135,7 +1120,6 @@ virTypedParamsAddFromString(virTypedParameterPtr *params, virResetLastError(); - VIR_TYPED_PARAM_CHECK(); if (VIR_RESIZE_N(*params, max, n, 1) < 0) goto error; *maxparams = max; -- 2.3.6

On 05/26/2015 09:01 AM, Michal Privoznik wrote: This will need to change since it misses this release... Extraneous blank line Coverity points out: Event assign_zero: Assigning: "*ret" = "NULL". Coverity points out: "Although "virResizeN" does overwrite "*ret" on some paths, it also contains at least one feasible path which does not overwrite it." [True, but that would assume *ret was already large enough (I believe). On the first pass, there's no way it could return 0.] Coverity points out: Dereferencing null pointer "*ret". Although it seems it's a false positive, if the *ret = NULL; is removed, then the error goes away, but that seems to go against your v2 review of not finding a match to filter and free. Perplexing, but my latest Coverity install has a bunch of false positives with similar code. Another way to avoid the message is a VIR_ALLOC_N(*ret, 1) right after the *ret = NULL (leading me further down the path of false positive, but still annoying). So should we be freeing this since we don't own it and may not have RESIZE'd it? Our only caller currently VIR_FREE()'s on both paths. Looks good in general, but perhaps eblake will have some thoughts on the Coverity issue... John

From: Pavel Boldin <pboldin(a)mirantis.com&gt; The `virTypedParamsAddStringList' function provides interface to add a NULL-terminated array of string values as a multi-value to the params. Signed-off-by: Pavel Boldin <pboldin(a)mirantis.com&gt; Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- include/libvirt/libvirt-host.h | 6 ++++++ src/libvirt_public.syms | 1 + src/util/virtypedparam.c | 36 ++++++++++++++++++++++++++++++++++++ tests/virtypedparamtest.c | 28 ++++++++++++++++++++++++++++ 4 files changed, 71 insertions(+) diff --git a/include/libvirt/libvirt-host.h b/include/libvirt/libvirt-host.h index 6767527..40a2768 100644 --- a/include/libvirt/libvirt-host.h +++ b/include/libvirt/libvirt-host.h @@ -337,6 +337,12 @@ virTypedParamsAddString (virTypedParameterPtr *params, const char *name, const char *value); int +virTypedParamsAddStringList(virTypedParameterPtr *params, + int *nparams, + int *maxparams, + const char *name, + const char **values); +int virTypedParamsAddFromString(virTypedParameterPtr *params, int *nparams, int *maxparams, diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 845a0b8..35e6853 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -715,6 +715,7 @@ LIBVIRT_1.2.16 { virDomainSetUserPassword; virTypedParamsFilter; virTypedParamsPickStrings; + virTypedParamsAddStringList; } LIBVIRT_1.2.15; # .... define new API here using predicted next version number .... diff --git a/src/util/virtypedparam.c b/src/util/virtypedparam.c index d88d4a7..1916f1f 100644 --- a/src/util/virtypedparam.c +++ b/src/util/virtypedparam.c @@ -1197,6 +1197,42 @@ virTypedParamsAddString(virTypedParameterPtr *params, return -1; } +/** + * virTypedParamsAddStringList: + * @params: array of typed parameters + * @nparams: number of parameters in the @params array + * @maxparams: maximum number of parameters that can be stored in @params + * array without allocating more memory + * @name: name of the parameter to store values to + * @values: the values to store into the new parameters + * + * Packs NULL-terminated list of strings @values into @params under the + * key @name. + * + * Returns 0 on success, -1 on error. + */ +int +virTypedParamsAddStringList(virTypedParameterPtr *params, + int *nparams, + int *maxparams, + const char *name, + const char **values) +{ + size_t i; + int rv = -1; + + if (!values) + return 0; + + for (i = 0; values[i]; i++) { + if ((rv = virTypedParamsAddString(params, nparams, maxparams, + name, values[i])) < 0) + break; + } + + return rv; +} + /** * virTypedParamsAddFromString: diff --git a/tests/virtypedparamtest.c b/tests/virtypedparamtest.c index abdddd5..82a3951 100644 --- a/tests/virtypedparamtest.c +++ b/tests/virtypedparamtest.c @@ -126,6 +126,31 @@ testTypedParamsFilter(const void *opaque ATTRIBUTE_UNUSED) } static int +testTypedParamsAddStringList(const void *opaque ATTRIBUTE_UNUSED) +{ + int rv = 0; + virTypedParameterPtr params = NULL; + int nparams = 0, maxparams = 0, i; + + const char *values[] = { + "foo", "bar", "foobar", NULL + }; + + rv = virTypedParamsAddStringList(&params, &nparams, &maxparams, "param", + values); + + for (i = 0; i < nparams; i++) { + if (STRNEQ(params[i].field, "param") || + STRNEQ(params[i].value.s, values[i]) || + params[i].type != VIR_TYPED_PARAM_STRING) + rv = -1; + } + + virTypedParamsFree(params, nparams); + return rv; +} + +static int testTypedParamsPickStrings(const void *opaque ATTRIBUTE_UNUSED) { size_t i, picked; @@ -258,6 +283,9 @@ mymain(void) if (virtTestRun("Picking Strings", testTypedParamsPickStrings, NULL) < 0) rv = -1; + if (virtTestRun("Add string list", testTypedParamsAddStringList, NULL) < 0) + rv = -1; + if (rv < 0) return EXIT_FAILURE; return EXIT_SUCCESS; -- 2.3.6

From: Pavel Boldin <pboldin(a)mirantis.com&gt; Implement a `migrate_disks' parameters for the QEMU driver. This multi- value parameter can be used to explicitly specify what block devices are to be migrated using the NBD server. Tunnelled migration using NBD is to be done. Signed-off-by: Pavel Boldin <pboldin(a)mirantis.com&gt; Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- include/libvirt/libvirt-domain.h | 9 ++ src/qemu/qemu_driver.c | 72 +++++++++--- src/qemu/qemu_migration.c | 245 ++++++++++++++++++++++++++++----------- src/qemu/qemu_migration.h | 24 ++-- 4 files changed, 258 insertions(+), 92 deletions(-) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index d851225..7564c20 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -748,6 +748,15 @@ typedef enum { */ # define VIR_MIGRATE_PARAM_LISTEN_ADDRESS "listen_address" +/** + * VIR_MIGRATE_PARAM_MIGRATE_DISKS: + * + * virDomainMigrate* params multiple field: The multiple values that list + * the block devices to be migrated. At the moment this is only supported + * by the QEMU driver but not for the tunnelled migration. + */ +# define VIR_MIGRATE_PARAM_MIGRATE_DISKS "migrate_disks" + /* Domain migration. */ virDomainPtr virDomainMigrate (virDomainPtr domain, virConnectPtr dconn, unsigned long flags, const char *dname, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index aa0acde..48b818f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12455,7 +12455,7 @@ qemuDomainMigratePrepare2(virConnectPtr dconn, ret = qemuMigrationPrepareDirect(driver, dconn, NULL, 0, NULL, NULL, /* No cookies */ uri_in, uri_out, - &def, origname, NULL, flags); + &def, origname, NULL, 0, NULL, flags); cleanup: VIR_FREE(origname); @@ -12508,7 +12508,7 @@ qemuDomainMigratePerform(virDomainPtr dom, * Consume any cookie we were able to decode though */ ret = qemuMigrationPerform(driver, dom->conn, vm, - NULL, dconnuri, uri, NULL, NULL, + NULL, dconnuri, uri, NULL, NULL, 0, NULL, cookie, cookielen, NULL, NULL, /* No output cookies in v2 */ flags, dname, resource, false); @@ -12585,7 +12585,7 @@ qemuDomainMigrateBegin3(virDomainPtr domain, } return qemuMigrationBegin(domain->conn, vm, xmlin, dname, - cookieout, cookieoutlen, flags); + cookieout, cookieoutlen, 0, NULL, flags); } static char * @@ -12598,11 +12598,14 @@ qemuDomainMigrateBegin3Params(virDomainPtr domain, { const char *xmlin = NULL; const char *dname = NULL; + const char **migrate_disks = NULL; + size_t nmigrate_disks; + char *ret = NULL; virDomainObjPtr vm; virCheckFlags(QEMU_MIGRATION_FLAGS, NULL); if (virTypedParamsValidate(params, nparams, QEMU_MIGRATION_PARAMETERS) < 0) - return NULL; + goto cleanup; if (virTypedParamsGetString(params, nparams, VIR_MIGRATE_PARAM_DEST_XML, @@ -12610,18 +12613,28 @@ qemuDomainMigrateBegin3Params(virDomainPtr domain, virTypedParamsGetString(params, nparams, VIR_MIGRATE_PARAM_DEST_NAME, &dname) < 0) - return NULL; + goto cleanup; + + if (virTypedParamsPickStrings(params, nparams, + VIR_MIGRATE_PARAM_MIGRATE_DISKS, + &migrate_disks, &nmigrate_disks) < 0) + goto cleanup; if (!(vm = qemuDomObjFromDomain(domain))) - return NULL; + goto cleanup; if (virDomainMigrateBegin3ParamsEnsureACL(domain->conn, vm->def) < 0) { virDomainObjEndAPI(&vm); - return NULL; + goto cleanup; } - return qemuMigrationBegin(domain->conn, vm, xmlin, dname, - cookieout, cookieoutlen, flags); + ret = qemuMigrationBegin(domain->conn, vm, xmlin, dname, + cookieout, cookieoutlen, flags, + migrate_disks, nmigrate_disks); + + cleanup: + VIR_FREE(migrate_disks); + return ret; } @@ -12665,7 +12678,7 @@ qemuDomainMigratePrepare3(virConnectPtr dconn, cookiein, cookieinlen, cookieout, cookieoutlen, uri_in, uri_out, - &def, origname, NULL, flags); + &def, origname, NULL, 0, NULL, flags); cleanup: VIR_FREE(origname); @@ -12691,6 +12704,8 @@ qemuDomainMigratePrepare3Params(virConnectPtr dconn, const char *dname = NULL; const char *uri_in = NULL; const char *listenAddress = cfg->migrationAddress; + size_t nmigrate_disks; + const char **migrate_disks = NULL; char *origname = NULL; int ret = -1; @@ -12712,6 +12727,11 @@ qemuDomainMigratePrepare3Params(virConnectPtr dconn, &listenAddress) < 0) goto cleanup; + if (virTypedParamsPickStrings(params, nparams, + VIR_MIGRATE_PARAM_MIGRATE_DISKS, + &migrate_disks, &nmigrate_disks) < 0) + goto cleanup; + if (flags & VIR_MIGRATE_TUNNELLED) { /* this is a logical error; we never should have gotten here with * VIR_MIGRATE_TUNNELLED set @@ -12732,9 +12752,11 @@ qemuDomainMigratePrepare3Params(virConnectPtr dconn, cookiein, cookieinlen, cookieout, cookieoutlen, uri_in, uri_out, - &def, origname, listenAddress, flags); + &def, origname, listenAddress, + nmigrate_disks, migrate_disks, flags); cleanup: + VIR_FREE(migrate_disks); VIR_FREE(origname); virDomainDefFree(def); virObjectUnref(cfg); @@ -12865,7 +12887,7 @@ qemuDomainMigratePerform3(virDomainPtr dom, } return qemuMigrationPerform(driver, dom->conn, vm, xmlin, - dconnuri, uri, NULL, NULL, + dconnuri, uri, NULL, NULL, 0, NULL, cookiein, cookieinlen, cookieout, cookieoutlen, flags, dname, resource, true); @@ -12889,7 +12911,10 @@ qemuDomainMigratePerform3Params(virDomainPtr dom, const char *uri = NULL; const char *graphicsuri = NULL; const char *listenAddress = NULL; + size_t nmigrate_disks; + const char **migrate_disks = NULL; unsigned long long bandwidth = 0; + int ret = -1; virCheckFlags(QEMU_MIGRATION_FLAGS, -1); if (virTypedParamsValidate(params, nparams, QEMU_MIGRATION_PARAMETERS) < 0) @@ -12913,20 +12938,29 @@ qemuDomainMigratePerform3Params(virDomainPtr dom, virTypedParamsGetString(params, nparams, VIR_MIGRATE_PARAM_LISTEN_ADDRESS, &listenAddress) < 0) - return -1; + goto cleanup; + + if (virTypedParamsPickStrings(params, nparams, + VIR_MIGRATE_PARAM_MIGRATE_DISKS, + &migrate_disks, &nmigrate_disks) < 0) + goto cleanup; if (!(vm = qemuDomObjFromDomain(dom))) - return -1; + goto cleanup; if (virDomainMigratePerform3ParamsEnsureACL(dom->conn, vm->def) < 0) { virDomainObjEndAPI(&vm); - return -1; + goto cleanup; } - return qemuMigrationPerform(driver, dom->conn, vm, dom_xml, - dconnuri, uri, graphicsuri, listenAddress, - cookiein, cookieinlen, cookieout, cookieoutlen, - flags, dname, bandwidth, true); + ret = qemuMigrationPerform(driver, dom->conn, vm, dom_xml, + dconnuri, uri, graphicsuri, listenAddress, + nmigrate_disks, migrate_disks, + cookiein, cookieinlen, cookieout, cookieoutlen, + flags, dname, bandwidth, true); + cleanup: + VIR_FREE(migrate_disks); + return ret; } diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 279b43f..5a8057e 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1579,12 +1579,34 @@ qemuMigrationPrecreateDisk(virConnectPtr conn, return ret; } +static bool +qemuMigrateDisk(virDomainDiskDef const *disk, + size_t nmigrate_disks, const char **migrate_disks) +{ + size_t i; + /* Check if the disk alias is in the list */ + if (nmigrate_disks) { + for (i = 0; i < nmigrate_disks; i++) { + if (STREQ(disk->dst, migrate_disks[i])) + return true; + } + return false; + } + + /* Default is to migrate only non-shared non-readonly disks + * with source */ + return !disk->src->shared && !disk->src->readonly && + virDomainDiskGetSource(disk); +} + static int qemuMigrationPrecreateStorage(virConnectPtr conn, virQEMUDriverPtr driver ATTRIBUTE_UNUSED, virDomainObjPtr vm, - qemuMigrationCookieNBDPtr nbd) + qemuMigrationCookieNBDPtr nbd, + size_t nmigrate_disks, + const char **migrate_disks) { int ret = -1; size_t i = 0; @@ -1609,9 +1631,9 @@ qemuMigrationPrecreateStorage(virConnectPtr conn, diskSrcPath = virDomainDiskGetSource(disk); - if (disk->src->shared || disk->src->readonly || + /* Skip disks we don't want to migrate and already existing disks. */ + if (!qemuMigrateDisk(disk, nmigrate_disks, migrate_disks) || (diskSrcPath && virFileExists(diskSrcPath))) { - /* Skip shared, read-only and already existing disks. */ continue; } @@ -1642,7 +1664,9 @@ qemuMigrationPrecreateStorage(virConnectPtr conn, static int qemuMigrationStartNBDServer(virQEMUDriverPtr driver, virDomainObjPtr vm, - const char *listenAddr) + const char *listenAddr, + size_t nmigrate_disks, + const char **migrate_disks) { int ret = -1; qemuDomainObjPrivatePtr priv = vm->privateData; @@ -1653,9 +1677,8 @@ qemuMigrationStartNBDServer(virQEMUDriverPtr driver, for (i = 0; i < vm->def->ndisks; i++) { virDomainDiskDefPtr disk = vm->def->disks[i]; - /* skip shared, RO and source-less disks */ - if (disk->src->shared || disk->src->readonly || - !virDomainDiskGetSource(disk)) + /* check whether disk should be migrated */ + if (!qemuMigrateDisk(disk, nmigrate_disks, migrate_disks)) continue; VIR_FREE(diskAlias); @@ -1914,7 +1937,9 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver, qemuMigrationCookiePtr mig, const char *host, unsigned long speed, - unsigned int *migrate_flags) + unsigned int *migrate_flags, + size_t nmigrate_disks, + const char **migrate_disks) { qemuDomainObjPrivatePtr priv = vm->privateData; int ret = -1; @@ -1946,9 +1971,8 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver, const char *format; int mon_ret; - /* skip shared, RO and source-less disks */ - if (disk->src->shared || disk->src->readonly || - !virDomainDiskGetSource(disk)) + /* check whether disk should be migrated */ + if (!qemuMigrateDisk(disk, nmigrate_disks, migrate_disks)) continue; if (disk->src->format > 0) @@ -2127,7 +2151,9 @@ qemuMigrationIsAllowed(virQEMUDriverPtr driver, virDomainObjPtr vm, } static bool -qemuMigrationIsSafe(virDomainDefPtr def) +qemuMigrationIsSafe(virDomainDefPtr def, + size_t nmigrate_disks, + const char **migrate_disks) { size_t i; @@ -2137,9 +2163,7 @@ qemuMigrationIsSafe(virDomainDefPtr def) /* Our code elsewhere guarantees shared disks are either readonly (in * which case cache mode doesn't matter) or used with cache=none */ - if (src && - !disk->src->shared && - !disk->src->readonly && + if (qemuMigrateDisk(disk, nmigrate_disks, migrate_disks) && disk->cachemode != VIR_DOMAIN_DISK_CACHE_DISABLE) { int rc; @@ -2710,6 +2734,8 @@ qemuMigrationBeginPhase(virQEMUDriverPtr driver, const char *dname, char **cookieout, int *cookieoutlen, + size_t nmigrate_disks, + const char **migrate_disks, unsigned long flags) { char *rv = NULL; @@ -2721,9 +2747,11 @@ qemuMigrationBeginPhase(virQEMUDriverPtr driver, bool abort_on_error = !!(flags & VIR_MIGRATE_ABORT_ON_ERROR); VIR_DEBUG("driver=%p, vm=%p, xmlin=%s, dname=%s," - " cookieout=%p, cookieoutlen=%p, flags=%lx", + " cookieout=%p, cookieoutlen=%p," + " nmigrate_disks=%zu, migrate_disks=%p, flags=%lx", driver, vm, NULLSTR(xmlin), NULLSTR(dname), - cookieout, cookieoutlen, flags); + cookieout, cookieoutlen, nmigrate_disks, + migrate_disks, flags); if (!(caps = virQEMUDriverGetCapabilities(driver, false))) goto cleanup; @@ -2738,17 +2766,54 @@ qemuMigrationBeginPhase(virQEMUDriverPtr driver, if (!qemuMigrationIsAllowed(driver, vm, NULL, true, abort_on_error)) goto cleanup; - if (!(flags & VIR_MIGRATE_UNSAFE) && !qemuMigrationIsSafe(vm->def)) + if (!(flags & VIR_MIGRATE_UNSAFE) && + !qemuMigrationIsSafe(vm->def, nmigrate_disks, migrate_disks)) goto cleanup; - if (flags & (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC) && - virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_MIRROR)) { - /* TODO support NBD for TUNNELLED migration */ - if (flags & VIR_MIGRATE_TUNNELLED) { - VIR_WARN("NBD in tunnelled migration is currently not supported"); - } else { - cookieFlags |= QEMU_MIGRATION_COOKIE_NBD; - priv->nbdPort = 0; + if (flags & (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC)) { + bool has_drive_mirror = virQEMUCapsGet(priv->qemuCaps, + QEMU_CAPS_DRIVE_MIRROR); + + if (nmigrate_disks) { + if (has_drive_mirror) { + size_t i, j; + + /* Check user requested only known disk targets. */ + for (i = 0; i < nmigrate_disks; i++) { + for (j = 0; j < vm->def->ndisks; j++) { + if (STREQ(vm->def->disks[j]->dst, migrate_disks[i])) + break; + } + + if (j == vm->def->ndisks) { + virReportError(VIR_ERR_INVALID_ARG, + _("disk target %s not found"), + migrate_disks[i]); + goto cleanup; + } + } + + if (flags & VIR_MIGRATE_TUNNELLED) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("Selecting disks to migrate is not " + "implemented for tunnelled migration")); + goto cleanup; + } + } else { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("qemu does not support drive-mirror command")); + goto cleanup; + } + } + + if (has_drive_mirror) { + /* TODO support NBD for TUNNELLED migration */ + if (flags & VIR_MIGRATE_TUNNELLED) { + VIR_WARN("NBD in tunnelled migration is currently not supported"); + } else { + cookieFlags |= QEMU_MIGRATION_COOKIE_NBD; + priv->nbdPort = 0; + } } } @@ -2809,7 +2874,9 @@ qemuMigrationBegin(virConnectPtr conn, const char *dname, char **cookieout, int *cookieoutlen, - unsigned long flags) + unsigned long flags, + const char **migrate_disks, + size_t nmigrate_disks) { virQEMUDriverPtr driver = conn->privateData; char *xml = NULL; @@ -2842,7 +2909,7 @@ qemuMigrationBegin(virConnectPtr conn, if (!(xml = qemuMigrationBeginPhase(driver, vm, xmlin, dname, cookieout, cookieoutlen, - flags))) + nmigrate_disks, migrate_disks, flags))) goto endjob; if ((flags & VIR_MIGRATE_CHANGE_PROTECTION)) { @@ -2910,6 +2977,8 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, unsigned short port, bool autoPort, const char *listenAddress, + size_t nmigrate_disks, + const char **migrate_disks, unsigned long flags) { virDomainObjPtr vm = NULL; @@ -3101,7 +3170,8 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, goto cleanup; } - if (qemuMigrationPrecreateStorage(dconn, driver, vm, mig->nbd) < 0) + if (qemuMigrationPrecreateStorage(dconn, driver, vm, mig->nbd, + nmigrate_disks, migrate_disks) < 0) goto cleanup; if (qemuMigrationJobStart(driver, vm, QEMU_ASYNC_JOB_MIGRATION_IN) < 0) @@ -3175,7 +3245,8 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, if (mig->nbd && flags & (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC) && virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NBD_SERVER)) { - if (qemuMigrationStartNBDServer(driver, vm, listenAddress) < 0) { + if (qemuMigrationStartNBDServer(driver, vm, listenAddress, + nmigrate_disks, migrate_disks) < 0) { /* error already reported */ goto endjob; } @@ -3274,7 +3345,7 @@ qemuMigrationPrepareTunnel(virQEMUDriverPtr driver, ret = qemuMigrationPrepareAny(driver, dconn, cookiein, cookieinlen, cookieout, cookieoutlen, def, origname, - st, NULL, 0, false, NULL, flags); + st, NULL, 0, false, NULL, 0, NULL, flags); return ret; } @@ -3314,6 +3385,8 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver, virDomainDefPtr *def, const char *origname, const char *listenAddress, + size_t nmigrate_disks, + const char **migrate_disks, unsigned long flags) { unsigned short port = 0; @@ -3326,10 +3399,12 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver, VIR_DEBUG("driver=%p, dconn=%p, cookiein=%s, cookieinlen=%d, " "cookieout=%p, cookieoutlen=%p, uri_in=%s, uri_out=%p, " - "def=%p, origname=%s, listenAddress=%s, flags=%lx", + "def=%p, origname=%s, listenAddress=%s, " + "nmigrate_disks=%zu, migrate_disks=%p, flags=%lx", driver, dconn, NULLSTR(cookiein), cookieinlen, cookieout, cookieoutlen, NULLSTR(uri_in), uri_out, - *def, origname, NULLSTR(listenAddress), flags); + *def, origname, NULLSTR(listenAddress), + nmigrate_disks, migrate_disks, flags); *uri_out = NULL; @@ -3433,7 +3508,8 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver, ret = qemuMigrationPrepareAny(driver, dconn, cookiein, cookieinlen, cookieout, cookieoutlen, def, origname, NULL, uri ? uri->scheme : "tcp", - port, autoPort, listenAddress, flags); + port, autoPort, listenAddress, + nmigrate_disks, migrate_disks, flags); cleanup: virURIFree(uri); VIR_FREE(hostname); @@ -3905,7 +3981,9 @@ qemuMigrationRun(virQEMUDriverPtr driver, unsigned long resource, qemuMigrationSpecPtr spec, virConnectPtr dconn, - const char *graphicsuri) + const char *graphicsuri, + size_t nmigrate_disks, + const char **migrate_disks) { int ret = -1; unsigned int migrate_flags = QEMU_MONITOR_MIGRATE_BACKGROUND; @@ -3921,11 +3999,12 @@ qemuMigrationRun(virQEMUDriverPtr driver, VIR_DEBUG("driver=%p, vm=%p, cookiein=%s, cookieinlen=%d, " "cookieout=%p, cookieoutlen=%p, flags=%lx, resource=%lu, " - "spec=%p (dest=%d, fwd=%d), dconn=%p, graphicsuri=%s", + "spec=%p (dest=%d, fwd=%d), dconn=%p, graphicsuri=%s, " + "nmigrate_disks=%zu, migrate_disks=%p", driver, vm, NULLSTR(cookiein), cookieinlen, cookieout, cookieoutlen, flags, resource, spec, spec->destType, spec->fwdType, dconn, - NULLSTR(graphicsuri)); + NULLSTR(graphicsuri), nmigrate_disks, migrate_disks); if (flags & VIR_MIGRATE_NON_SHARED_DISK) { migrate_flags |= QEMU_MONITOR_MIGRATE_NON_SHARED_DISK; @@ -3961,7 +4040,9 @@ qemuMigrationRun(virQEMUDriverPtr driver, if (qemuMigrationDriveMirror(driver, vm, mig, spec->dest.host.name, migrate_speed, - &migrate_flags) < 0) { + &migrate_flags, + nmigrate_disks, + migrate_disks) < 0) { goto cleanup; } } else { @@ -4195,7 +4276,9 @@ static int doNativeMigrate(virQEMUDriverPtr driver, unsigned long flags, unsigned long resource, virConnectPtr dconn, - const char *graphicsuri) + const char *graphicsuri, + size_t nmigrate_disks, + const char **migrate_disks) { qemuDomainObjPrivatePtr priv = vm->privateData; virURIPtr uribits = NULL; @@ -4204,10 +4287,10 @@ static int doNativeMigrate(virQEMUDriverPtr driver, VIR_DEBUG("driver=%p, vm=%p, uri=%s, cookiein=%s, cookieinlen=%d, " "cookieout=%p, cookieoutlen=%p, flags=%lx, resource=%lu, " - "graphicsuri=%s", + "graphicsuri=%s, nmigrate_disks=%zu migrate_disks=%p", driver, vm, uri, NULLSTR(cookiein), cookieinlen, cookieout, cookieoutlen, flags, resource, - NULLSTR(graphicsuri)); + NULLSTR(graphicsuri), nmigrate_disks, migrate_disks); if (!(uribits = qemuMigrationParseURI(uri, NULL))) return -1; @@ -4246,7 +4329,7 @@ static int doNativeMigrate(virQEMUDriverPtr driver, ret = qemuMigrationRun(driver, vm, cookiein, cookieinlen, cookieout, cookieoutlen, flags, resource, &spec, dconn, - graphicsuri); + graphicsuri, nmigrate_disks, migrate_disks); if (spec.destType == MIGRATION_DEST_FD) VIR_FORCE_CLOSE(spec.dest.fd.qemu); @@ -4268,7 +4351,9 @@ static int doTunnelMigrate(virQEMUDriverPtr driver, unsigned long flags, unsigned long resource, virConnectPtr dconn, - const char *graphicsuri) + const char *graphicsuri, + size_t nmigrate_disks, + const char **migrate_disks) { qemuDomainObjPrivatePtr priv = vm->privateData; virNetSocketPtr sock = NULL; @@ -4278,10 +4363,10 @@ static int doTunnelMigrate(virQEMUDriverPtr driver, VIR_DEBUG("driver=%p, vm=%p, st=%p, cookiein=%s, cookieinlen=%d, " "cookieout=%p, cookieoutlen=%p, flags=%lx, resource=%lu, " - "graphicsuri=%s", + "graphicsuri=%s, nmigrate_disks=%zu, migrate_disks=%p", driver, vm, st, NULLSTR(cookiein), cookieinlen, cookieout, cookieoutlen, flags, resource, - NULLSTR(graphicsuri)); + NULLSTR(graphicsuri), nmigrate_disks, migrate_disks); if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATE_QEMU_FD) && !virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATE_QEMU_UNIX) && @@ -4334,7 +4419,7 @@ static int doTunnelMigrate(virQEMUDriverPtr driver, ret = qemuMigrationRun(driver, vm, cookiein, cookieinlen, cookieout, cookieoutlen, flags, resource, &spec, dconn, - graphicsuri); + graphicsuri, nmigrate_disks, migrate_disks); cleanup: if (spec.destType == MIGRATION_DEST_FD) { @@ -4444,12 +4529,13 @@ static int doPeer2PeerMigrate2(virQEMUDriverPtr driver, if (flags & VIR_MIGRATE_TUNNELLED) ret = doTunnelMigrate(driver, vm, st, NULL, 0, NULL, NULL, - flags, resource, dconn, NULL); + flags, resource, dconn, + NULL, 0, NULL); else ret = doNativeMigrate(driver, vm, uri_out, cookie, cookielen, NULL, NULL, /* No out cookie with v2 migration */ - flags, resource, dconn, NULL); + flags, resource, dconn, NULL, 0, NULL); /* Perform failed. Make sure Finish doesn't overwrite the error */ if (ret < 0) @@ -4511,6 +4597,8 @@ doPeer2PeerMigrate3(virQEMUDriverPtr driver, const char *uri, const char *graphicsuri, const char *listenAddress, + size_t nmigrate_disks, + const char **migrate_disks, unsigned long long bandwidth, bool useParams, unsigned long flags) @@ -4530,13 +4618,16 @@ doPeer2PeerMigrate3(virQEMUDriverPtr driver, virTypedParameterPtr params = NULL; int nparams = 0; int maxparams = 0; + size_t i; VIR_DEBUG("driver=%p, sconn=%p, dconn=%p, dconnuri=%s, vm=%p, xmlin=%s, " "dname=%s, uri=%s, graphicsuri=%s, listenAddress=%s, " - "bandwidth=%llu, useParams=%d, flags=%lx", + "nmigrate_disks=%zu, migrate_disks=%p, bandwidth=%llu, " + "useParams=%d, flags=%lx", driver, sconn, dconn, NULLSTR(dconnuri), vm, NULLSTR(xmlin), NULLSTR(dname), NULLSTR(uri), NULLSTR(graphicsuri), - NULLSTR(listenAddress), bandwidth, useParams, flags); + NULLSTR(listenAddress), nmigrate_disks, migrate_disks, + bandwidth, useParams, flags); /* Unlike the virDomainMigrateVersion3 counterpart, we don't need * to worry about auto-setting the VIR_MIGRATE_CHANGE_PROTECTION @@ -4544,7 +4635,8 @@ doPeer2PeerMigrate3(virQEMUDriverPtr driver, * a single job. */ dom_xml = qemuMigrationBeginPhase(driver, vm, xmlin, dname, - &cookieout, &cookieoutlen, flags); + &cookieout, &cookieoutlen, + nmigrate_disks, migrate_disks, flags); if (!dom_xml) goto cleanup; @@ -4579,6 +4671,11 @@ doPeer2PeerMigrate3(virQEMUDriverPtr driver, VIR_MIGRATE_PARAM_LISTEN_ADDRESS, listenAddress) < 0) goto cleanup; + for (i = 0; i < nmigrate_disks; i++) + if (virTypedParamsAddString(&params, &nparams, &maxparams, + VIR_MIGRATE_PARAM_MIGRATE_DISKS, + migrate_disks[i]) < 0) + goto cleanup; } if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_PAUSED) @@ -4663,12 +4760,14 @@ doPeer2PeerMigrate3(virQEMUDriverPtr driver, ret = doTunnelMigrate(driver, vm, st, cookiein, cookieinlen, &cookieout, &cookieoutlen, - flags, bandwidth, dconn, graphicsuri); + flags, bandwidth, dconn, graphicsuri, + nmigrate_disks, migrate_disks); } else { ret = doNativeMigrate(driver, vm, uri, cookiein, cookieinlen, &cookieout, &cookieoutlen, - flags, bandwidth, dconn, graphicsuri); + flags, bandwidth, dconn, graphicsuri, + nmigrate_disks, migrate_disks); } /* Perform failed. Make sure Finish doesn't overwrite the error */ @@ -4802,6 +4901,8 @@ static int doPeer2PeerMigrate(virQEMUDriverPtr driver, const char *uri, const char *graphicsuri, const char *listenAddress, + size_t nmigrate_disks, + const char **migrate_disks, unsigned long flags, const char *dname, unsigned long resource, @@ -4815,12 +4916,12 @@ static int doPeer2PeerMigrate(virQEMUDriverPtr driver, virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); bool useParams; - VIR_DEBUG("driver=%p, sconn=%p, vm=%p, xmlin=%s, dconnuri=%s, " - "uri=%s, graphicsuri=%s, listenAddress=%s, flags=%lx, " - "dname=%s, resource=%lu", + VIR_DEBUG("driver=%p, sconn=%p, vm=%p, xmlin=%s, dconnuri=%s, uri=%s, " + "graphicsuri=%s, listenAddress=%s, nmigrate_disks=%zu, " + "migrate_disks=%p, flags=%lx, dname=%s, resource=%lu", driver, sconn, vm, NULLSTR(xmlin), NULLSTR(dconnuri), NULLSTR(uri), NULLSTR(graphicsuri), NULLSTR(listenAddress), - flags, NULLSTR(dname), resource); + nmigrate_disks, migrate_disks, flags, NULLSTR(dname), resource); if (flags & VIR_MIGRATE_TUNNELLED && uri) { virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", @@ -4879,7 +4980,7 @@ static int doPeer2PeerMigrate(virQEMUDriverPtr driver, /* Only xmlin, dname, uri, and bandwidth parameters can be used with * old-style APIs. */ - if (!useParams && (graphicsuri || listenAddress)) { + if (!useParams && (graphicsuri || listenAddress || nmigrate_disks)) { virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", _("Migration APIs with extensible parameters are not " "supported but extended parameters were passed")); @@ -4910,7 +5011,8 @@ static int doPeer2PeerMigrate(virQEMUDriverPtr driver, if (*v3proto) { ret = doPeer2PeerMigrate3(driver, sconn, dconn, dconnuri, vm, xmlin, dname, uri, graphicsuri, listenAddress, - resource, useParams, flags); + nmigrate_disks, migrate_disks, resource, + useParams, flags); } else { ret = doPeer2PeerMigrate2(driver, sconn, dconn, vm, dconnuri, flags, dname, resource); @@ -4944,6 +5046,8 @@ qemuMigrationPerformJob(virQEMUDriverPtr driver, const char *uri, const char *graphicsuri, const char *listenAddress, + size_t nmigrate_disks, + const char **migrate_disks, const char *cookiein, int cookieinlen, char **cookieout, @@ -4971,7 +5075,8 @@ qemuMigrationPerformJob(virQEMUDriverPtr driver, if (!qemuMigrationIsAllowed(driver, vm, NULL, true, abort_on_error)) goto endjob; - if (!(flags & VIR_MIGRATE_UNSAFE) && !qemuMigrationIsSafe(vm->def)) + if (!(flags & VIR_MIGRATE_UNSAFE) && + !qemuMigrationIsSafe(vm->def, nmigrate_disks, migrate_disks)) goto endjob; qemuMigrationStoreDomainState(vm); @@ -4979,12 +5084,13 @@ qemuMigrationPerformJob(virQEMUDriverPtr driver, if ((flags & (VIR_MIGRATE_TUNNELLED | VIR_MIGRATE_PEER2PEER))) { ret = doPeer2PeerMigrate(driver, conn, vm, xmlin, dconnuri, uri, graphicsuri, listenAddress, + nmigrate_disks, migrate_disks, flags, dname, resource, &v3proto); } else { qemuMigrationJobSetPhase(driver, vm, QEMU_MIGRATION_PHASE_PERFORM2); ret = doNativeMigrate(driver, vm, uri, cookiein, cookieinlen, cookieout, cookieoutlen, - flags, resource, NULL, NULL); + flags, resource, NULL, NULL, 0, NULL); } if (ret < 0) goto endjob; @@ -5043,6 +5149,8 @@ qemuMigrationPerformPhase(virQEMUDriverPtr driver, virDomainObjPtr vm, const char *uri, const char *graphicsuri, + size_t nmigrate_disks, + const char **migrate_disks, const char *cookiein, int cookieinlen, char **cookieout, @@ -5067,7 +5175,8 @@ qemuMigrationPerformPhase(virQEMUDriverPtr driver, ret = doNativeMigrate(driver, vm, uri, cookiein, cookieinlen, cookieout, cookieoutlen, - flags, resource, NULL, graphicsuri); + flags, resource, NULL, graphicsuri, + nmigrate_disks, migrate_disks); if (ret < 0) { if (qemuMigrationRestoreDomainState(conn, vm)) { @@ -5108,6 +5217,8 @@ qemuMigrationPerform(virQEMUDriverPtr driver, const char *uri, const char *graphicsuri, const char *listenAddress, + size_t nmigrate_disks, + const char **migrate_disks, const char *cookiein, int cookieinlen, char **cookieout, @@ -5118,13 +5229,14 @@ qemuMigrationPerform(virQEMUDriverPtr driver, bool v3proto) { VIR_DEBUG("driver=%p, conn=%p, vm=%p, xmlin=%s, dconnuri=%s, " - "uri=%s, graphicsuri=%s, listenAddress=%s" + "uri=%s, graphicsuri=%s, listenAddress=%s, " + "nmigrate_disks=%zu, migrate_disks=%p, " "cookiein=%s, cookieinlen=%d, cookieout=%p, cookieoutlen=%p, " "flags=%lx, dname=%s, resource=%lu, v3proto=%d", driver, conn, vm, NULLSTR(xmlin), NULLSTR(dconnuri), NULLSTR(uri), NULLSTR(graphicsuri), NULLSTR(listenAddress), - NULLSTR(cookiein), cookieinlen, cookieout, cookieoutlen, - flags, NULLSTR(dname), resource, v3proto); + nmigrate_disks, migrate_disks, NULLSTR(cookiein), cookieinlen, + cookieout, cookieoutlen, flags, NULLSTR(dname), resource, v3proto); if ((flags & (VIR_MIGRATE_TUNNELLED | VIR_MIGRATE_PEER2PEER))) { if (cookieinlen) { @@ -5135,6 +5247,7 @@ qemuMigrationPerform(virQEMUDriverPtr driver, return qemuMigrationPerformJob(driver, conn, vm, xmlin, dconnuri, uri, graphicsuri, listenAddress, + nmigrate_disks, migrate_disks, cookiein, cookieinlen, cookieout, cookieoutlen, flags, dname, resource, v3proto); @@ -5148,12 +5261,14 @@ qemuMigrationPerform(virQEMUDriverPtr driver, if (v3proto) { return qemuMigrationPerformPhase(driver, conn, vm, uri, graphicsuri, + nmigrate_disks, migrate_disks, cookiein, cookieinlen, cookieout, cookieoutlen, flags, resource); } else { return qemuMigrationPerformJob(driver, conn, vm, xmlin, dconnuri, uri, graphicsuri, listenAddress, + nmigrate_disks, migrate_disks, cookiein, cookieinlen, cookieout, cookieoutlen, flags, dname, resource, v3proto); diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h index 1726455..cc65b66 100644 --- a/src/qemu/qemu_migration.h +++ b/src/qemu/qemu_migration.h @@ -44,13 +44,15 @@ VIR_MIGRATE_RDMA_PIN_ALL) /* All supported migration parameters and their types. */ -# define QEMU_MIGRATION_PARAMETERS \ - VIR_MIGRATE_PARAM_URI, VIR_TYPED_PARAM_STRING, \ - VIR_MIGRATE_PARAM_DEST_NAME, VIR_TYPED_PARAM_STRING, \ - VIR_MIGRATE_PARAM_DEST_XML, VIR_TYPED_PARAM_STRING, \ - VIR_MIGRATE_PARAM_BANDWIDTH, VIR_TYPED_PARAM_ULLONG, \ - VIR_MIGRATE_PARAM_GRAPHICS_URI, VIR_TYPED_PARAM_STRING, \ - VIR_MIGRATE_PARAM_LISTEN_ADDRESS, VIR_TYPED_PARAM_STRING, \ +# define QEMU_MIGRATION_PARAMETERS \ + VIR_MIGRATE_PARAM_URI, VIR_TYPED_PARAM_STRING, \ + VIR_MIGRATE_PARAM_DEST_NAME, VIR_TYPED_PARAM_STRING, \ + VIR_MIGRATE_PARAM_DEST_XML, VIR_TYPED_PARAM_STRING, \ + VIR_MIGRATE_PARAM_BANDWIDTH, VIR_TYPED_PARAM_ULLONG, \ + VIR_MIGRATE_PARAM_GRAPHICS_URI, VIR_TYPED_PARAM_STRING, \ + VIR_MIGRATE_PARAM_LISTEN_ADDRESS, VIR_TYPED_PARAM_STRING, \ + VIR_MIGRATE_PARAM_MIGRATE_DISKS, VIR_TYPED_PARAM_STRING | \ + VIR_TYPED_PARAM_MULTIPLE, \ NULL @@ -99,7 +101,9 @@ char *qemuMigrationBegin(virConnectPtr conn, const char *dname, char **cookieout, int *cookieoutlen, - unsigned long flags); + unsigned long flags, + const char **migrate_disks, + size_t nmigrate_disks); virDomainDefPtr qemuMigrationPrepareDef(virQEMUDriverPtr driver, const char *dom_xml, @@ -128,6 +132,8 @@ int qemuMigrationPrepareDirect(virQEMUDriverPtr driver, virDomainDefPtr *def, const char *origname, const char *listenAddress, + size_t nmigrate_disks, + const char **migrate_disks, unsigned long flags); int qemuMigrationPerform(virQEMUDriverPtr driver, @@ -138,6 +144,8 @@ int qemuMigrationPerform(virQEMUDriverPtr driver, const char *uri, const char *graphicsuri, const char *listenAddress, + size_t nmigrate_disks, + const char **migrate_disks, const char *cookiein, int cookieinlen, char **cookieout, -- 2.3.6

From: Pavel Boldin <pboldin(a)mirantis.com&gt; Add `virsh migrate' option `--migratedisks' that allows CLI user to explicitly specify block devices to migrate. Signed-off-by: Pavel Boldin <pboldin(a)mirantis.com&gt; Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- tools/virsh-domain.c | 23 +++++++++++++++++++++++ tools/virsh.pod | 21 ++++++++++++--------- 2 files changed, 35 insertions(+), 9 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 91a1ca2..41d3829 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -9889,6 +9889,10 @@ static const vshCmdOptDef opts_migrate[] = { .type = VSH_OT_STRING, .help = N_("filename containing updated XML for the target") }, + {.name = "migratedisks", + .type = VSH_OT_STRING, + .help = N_("comma separated list of disks to be migrated") + }, {.name = NULL} }; @@ -9948,6 +9952,25 @@ doMigrate(void *opaque) VIR_MIGRATE_PARAM_DEST_NAME, opt) < 0) goto save_error; + if (vshCommandOptStringReq(ctl, cmd, "migratedisks", &opt) < 0) + goto out; + if (opt) { + char **val = NULL; + + val = virStringSplit(opt, ",", 0); + + if (virTypedParamsAddStringList(&params, + &nparams, + &maxparams, + VIR_MIGRATE_PARAM_MIGRATE_DISKS, + (const char **)val) < 0) { + VIR_FREE(val); + goto save_error; + } + + VIR_FREE(val); + } + if (vshCommandOptStringReq(ctl, cmd, "xml", &opt) < 0) goto out; if (opt) { diff --git a/tools/virsh.pod b/tools/virsh.pod index d588e5a..9275091 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1521,6 +1521,7 @@ to the I<uri> namespace is displayed instead of being modified. [I<--compressed>] [I<--abort-on-error>] [I<--auto-converge>] I<domain> I<desturi> [I<migrateuri>] [I<graphicsuri>] [I<listen-address>] [I<dname>] [I<--timeout> B<seconds>] [I<--xml> B<file>] +[I<--migratedisks> B<disk-list>] Migrate domain to another host. Add I<--live> for live migration; <--p2p> for peer-2-peer migration; I<--direct> for direct migration; or I<--tunnelled> @@ -1536,15 +1537,17 @@ with incremental copy (same base image shared between source and destination). In both cases the disk images have to exist on destination host, the I<--copy-storage-...> options only tell libvirt to transfer data from the images on source host to the images found at the same place on the destination -host. I<--change-protection> enforces that no incompatible configuration -changes will be made to the domain while the migration is underway; this flag -is implicitly enabled when supported by the hypervisor, but can be explicitly -used to reject the migration if the hypervisor lacks change protection -support. I<--verbose> displays the progress of migration. I<--compressed> -activates compression of memory pages that have to be transferred repeatedly -during live migration. I<--abort-on-error> cancels the migration if a soft -error (for example I/O error) happens during the migration. I<--auto-converge> -forces convergence during live migration. +host. By default only non-shared non-readonly images are transfered. Use +I<--migratedisks> to explicitly specify a list of disk targets to transfer. +B<disk-list> is a comma separated list then. I<--change-protection> enforces +that no incompatible configuration changes will be made to the domain while the +migration is underway; this flag is implicitly enabled when supported by the +hypervisor, but can be explicitly used to reject the migration if the +hypervisor lacks change protection support. I<--verbose> displays the progress +of migration. I<--compressed> activates compression of memory pages that have +to be transferred repeatedly during live migration. I<--abort-on-error> cancels +the migration if a soft error (for example I/O error) happens during the +migration. I<--auto-converge> forces convergence during live migration. B<Note>: Individual hypervisors usually do not support all possible types of migration. For example, QEMU does not support direct migration. -- 2.3.6

Michal, Should I fix these or will you do it? Pavel On Fri, May 29, 2015 at 6:37 PM, John Ferlan <jferlan(a)redhat.com&gt; wrote:

From: Michal Privoznik <mprivozn(a)redhat.com&gt; This function is returning a string (domain XML). Since d3ce7363 when it was first introduced, it was indented incorrectly: static char *qemuMigrationBeginPhase(..) Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/qemu/qemu_migration.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 70400f3..6c94052 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2699,14 +2699,14 @@ qemuMigrationCleanup(virDomainObjPtr vm, /* The caller is supposed to lock the vm and start a migration job. */ -static char -*qemuMigrationBeginPhase(virQEMUDriverPtr driver, - virDomainObjPtr vm, - const char *xmlin, - const char *dname, - char **cookieout, - int *cookieoutlen, - unsigned long flags) +static char * +qemuMigrationBeginPhase(virQEMUDriverPtr driver, + virDomainObjPtr vm, + const char *xmlin, + const char *dname, + char **cookieout, + int *cookieoutlen, + unsigned long flags) { char *rv = NULL; qemuMigrationCookiePtr mig = NULL; -- 1.9.1

The `virTypedParamsValidate' function now can be instructed to allow multiple entries for some of the keys. For this flag the type with the `VIR_TYPED_PARAM_MULTIPLE' flag. Add unit tests for this new behaviour. Signed-off-by: Pavel Boldin <pboldin(a)mirantis.com&gt; Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/util/virtypedparam.c | 109 +++++++++++++++++++----------- src/util/virtypedparam.h | 10 +++ tests/Makefile.am | 6 ++ tests/virtypedparamtest.c | 167 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 253 insertions(+), 39 deletions(-) create mode 100644 tests/virtypedparamtest.c diff --git a/src/util/virtypedparam.c b/src/util/virtypedparam.c index de2d447..68620f5 100644 --- a/src/util/virtypedparam.c +++ b/src/util/virtypedparam.c @@ -47,11 +47,19 @@ VIR_ENUM_IMPL(virTypedParameter, VIR_TYPED_PARAM_LAST, * internal utility functions (those in libvirt_private.syms) may * report errors that the caller will dispatch. */ +static int +virTypedParamsSortName(const void *left, const void *right) +{ + const virTypedParameter *param_left = left, *param_right = right; + return strcmp(param_left->field, param_right->field); +} + /* Validate that PARAMS contains only recognized parameter names with - * correct types, and with no duplicates. Pass in as many name/type - * pairs as appropriate, and pass NULL to end the list of accepted - * parameters. Return 0 on success, -1 on failure with error message - * already issued. */ + * correct types, and with no duplicates except for parameters + * specified with VIR_TYPED_PARAM_MULTIPLE flag in type. + * Pass in as many name/type pairs as appropriate, and pass NULL to end + * the list of accepted parameters. Return 0 on success, -1 on failure + * with error message already issued. */ int virTypedParamsValidate(virTypedParameterPtr params, int nparams, ...) { @@ -60,60 +68,83 @@ virTypedParamsValidate(virTypedParameterPtr params, int nparams, ...) size_t i, j; const char *name; int type; + size_t nkeys = 0, nkeysalloc = 0; + virTypedParameterPtr sorted = NULL, keys = NULL; va_start(ap, nparams); - /* Yes, this is quadratic, but since we reject duplicates and - * unknowns, it is constrained by the number of var-args passed - * in, which is expected to be small enough to not be - * noticeable. */ - for (i = 0; i < nparams; i++) { - va_end(ap); - va_start(ap, nparams); + if (VIR_ALLOC_N(sorted, nparams) < 0) + goto cleanup; - name = va_arg(ap, const char *); - while (name) { - type = va_arg(ap, int); - if (STREQ(params[i].field, name)) { - if (params[i].type != type) { - const char *badtype; - - badtype = virTypedParameterTypeToString(params[i].type); - if (!badtype) - badtype = virTypedParameterTypeToString(0); - virReportError(VIR_ERR_INVALID_ARG, - _("invalid type '%s' for parameter '%s', " - "expected '%s'"), - badtype, params[i].field, - virTypedParameterTypeToString(type)); - } - break; - } - name = va_arg(ap, const char *); - } - if (!name) { - virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, - _("parameter '%s' not supported"), - params[i].field); + /* Here we intentionally don't copy values */ + memcpy(sorted, params, sizeof(*params) * nparams); + qsort(sorted, nparams, sizeof(*sorted), virTypedParamsSortName); + + name = va_arg(ap, const char *); + while (name) { + type = va_arg(ap, int); + if (VIR_RESIZE_N(keys, nkeysalloc, nkeys, 1) < 0) + goto cleanup; + + if (virStrcpyStatic(keys[nkeys].field, name) == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Field name '%s' too long"), name); goto cleanup; } - for (j = 0; j < i; j++) { - if (STREQ(params[i].field, params[j].field)) { + + keys[nkeys].type = type & ~VIR_TYPED_PARAM_MULTIPLE; + /* Value is not used anyway */ + keys[nkeys].value.i = type & VIR_TYPED_PARAM_MULTIPLE; + + nkeys++; + name = va_arg(ap, const char *); + } + + qsort(keys, nkeys, sizeof(*keys), virTypedParamsSortName); + + for (i = 0, j = 0; i < nparams && j < nkeys;) { + if (STRNEQ(sorted[i].field, keys[j].field)) { + j++; + } else { + if (i > j && !(keys[j].value.i & VIR_TYPED_PARAM_MULTIPLE)) { virReportError(VIR_ERR_INVALID_ARG, _("parameter '%s' occurs multiple times"), - params[i].field); + sorted[i].field); + goto cleanup; + } + if (sorted[i].type != keys[j].type) { + const char *badtype; + + badtype = virTypedParameterTypeToString(sorted[i].type); + if (!badtype) + badtype = virTypedParameterTypeToString(0); + virReportError(VIR_ERR_INVALID_ARG, + _("invalid type '%s' for parameter '%s', " + "expected '%s'"), + badtype, sorted[i].field, + virTypedParameterTypeToString(keys[j].type)); goto cleanup; } + i++; } } + if (j == nkeys && i != nparams) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, + _("parameter '%s' not supported"), + sorted[i].field); + goto cleanup; + } + ret = 0; cleanup: va_end(ap); + VIR_FREE(sorted); + VIR_FREE(keys); return ret; - } + /* Check if params contains only specified parameter names. Return true if * only specified names are present in params, false if params contains any * unspecified parameter name. */ diff --git a/src/util/virtypedparam.h b/src/util/virtypedparam.h index 0c18504..9f2d08c 100644 --- a/src/util/virtypedparam.h +++ b/src/util/virtypedparam.h @@ -26,6 +26,16 @@ # include "internal.h" # include "virutil.h" +/** + * VIR_TYPED_PARAM_MULTIPLE: + * + * Flag indiciating that the params has multiple occurences of the parameter. + * Only used as a flag for @type argument of the virTypedParamsValidate. + */ +# define VIR_TYPED_PARAM_MULTIPLE (1 << 31) + +verify(!(VIR_TYPED_PARAM_LAST & VIR_TYPED_PARAM_MULTIPLE)); + int virTypedParamsValidate(virTypedParameterPtr params, int nparams, /* const char *name, int type ... */ ...) ATTRIBUTE_SENTINEL ATTRIBUTE_RETURN_CHECK; diff --git a/tests/Makefile.am b/tests/Makefile.am index 8e2dbec..9efb441 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -182,6 +182,7 @@ test_programs = virshtest sockettest \ virhostdevtest \ vircaps2xmltest \ virnetdevtest \ + virtypedparamtest \ $(NULL) if WITH_REMOTE @@ -1225,6 +1226,11 @@ objecteventtest_SOURCES = \ testutils.c testutils.h objecteventtest_LDADD = $(LDADDS) +virtypedparamtest_SOURCES = \ + virtypedparamtest.c testutils.h testutils.c +virtypedparamtest_LDADD = $(LDADDS) + + if WITH_LINUX fchosttest_SOURCES = \ fchosttest.c testutils.h testutils.c diff --git a/tests/virtypedparamtest.c b/tests/virtypedparamtest.c new file mode 100644 index 0000000..95e22a7 --- /dev/null +++ b/tests/virtypedparamtest.c @@ -0,0 +1,167 @@ +/* + * virtypedparamtest.c: Test typed param functions + * + * Copyright (C) 2015 Mirantis, 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 <stdio.h> +#include <virtypedparam.h> + +#include "testutils.h" + +#define VIR_FROM_THIS VIR_FROM_NONE + +typedef struct _TypedParameterTest { + /* Test name for logging */ + const char *name; + /* Flags of the "foobar" parameter check */ + int foobar_flags; + /* Parameters to validate */ + virTypedParameterPtr params; + /* Amount of parameters */ + int nparams; + + /* Expected error code */ + int expected_errcode; + /* Expected error message */ + const char *expected_errmessage; +} TypedParameterTest; + +static int +testTypedParamsValidate(const void *opaque) +{ + int rv; + TypedParameterTest *test = (TypedParameterTest *)opaque; + virErrorPtr errptr; + + rv = virTypedParamsValidate( + test->params, test->nparams, + "foobar", VIR_TYPED_PARAM_STRING | test->foobar_flags, + "foo", VIR_TYPED_PARAM_INT, + "bar", VIR_TYPED_PARAM_UINT, + NULL); + + if (test->expected_errcode) { + errptr = virGetLastError(); + + rv = (errptr == NULL) || ((rv < 0) && + !(errptr->code == test->expected_errcode)); + if (errptr && test->expected_errmessage) { + rv = STRNEQ(test->expected_errmessage, errptr->message); + if (rv) + printf("%s\n", errptr->message); + } + } + + return rv; +} + +#define PARAMS_ARRAY(...) ((virTypedParameter[]){ __VA_ARGS__ }) +#define PARAMS_SIZE(...) ARRAY_CARDINALITY(PARAMS_ARRAY(__VA_ARGS__)) + +#define PARAMS(...) \ + .params = PARAMS_ARRAY(__VA_ARGS__), \ + .nparams = PARAMS_SIZE(__VA_ARGS__), + +static int +testTypedParamsValidator(void) +{ + size_t i; + int rv = 0; + + TypedParameterTest test[] = { + { + .name = "Invalid arg type", + .foobar_flags = 0, + PARAMS({ .field = "foobar", .type = VIR_TYPED_PARAM_INT }) + .expected_errcode = VIR_ERR_INVALID_ARG, + .expected_errmessage = + "invalid argument: invalid type 'int' for parameter " + "'foobar', expected 'string'" + }, + { + .name = "Extra arg", + .foobar_flags = 0, + PARAMS({ .field = "f", .type = VIR_TYPED_PARAM_INT }) + .expected_errcode = VIR_ERR_INVALID_ARG, + .expected_errmessage = + "argument unsupported: parameter 'f' not supported" + }, + { + .name = "Valid parameters", + .foobar_flags = 0, + PARAMS( + { .field = "bar", .type = VIR_TYPED_PARAM_UINT }, + { .field = "foobar", .type = VIR_TYPED_PARAM_STRING }, + { .field = "foo", .type = VIR_TYPED_PARAM_INT } + ) + .expected_errcode = 0, .expected_errmessage = NULL, + }, + { + .name = "Duplicates incorrect", + .foobar_flags = 0, + PARAMS( + { .field = "bar", .type = VIR_TYPED_PARAM_UINT }, + { .field = "foobar", .type = VIR_TYPED_PARAM_STRING }, + { .field = "foobar", .type = VIR_TYPED_PARAM_STRING }, + { .field = "foo", .type = VIR_TYPED_PARAM_INT } + ) + .expected_errcode = VIR_ERR_INVALID_ARG, + .expected_errmessage = + "invalid argument: parameter 'foobar' occurs multiple times" + }, + { + .name = "Duplicates OK for marked", + .foobar_flags = VIR_TYPED_PARAM_MULTIPLE, + PARAMS( + { .field = "bar", .type = VIR_TYPED_PARAM_UINT }, + { .field = "foobar", .type = VIR_TYPED_PARAM_STRING }, + { .field = "foobar", .type = VIR_TYPED_PARAM_STRING }, + { .field = "foo", .type = VIR_TYPED_PARAM_INT } + ) + .expected_errcode = 0, .expected_errmessage = NULL, + }, + { + .name = NULL + } + }; + + for (i = 0; test[i].name; ++i) { + if (virtTestRun(test[i].name, testTypedParamsValidate, &test[i]) < 0) + rv = -1; + } + + return rv; +} + +static int +mymain(void) +{ + int rv = 0; + + if (testTypedParamsValidator() < 0) + rv = -1; + + if (rv < 0) + return EXIT_FAILURE; + return EXIT_SUCCESS; +} + +VIRT_TEST_MAIN(mymain) -- 1.9.1

Allow multi-value parameters to be build using virTypedParamsAdd* functions by removing check for duplicates. Signed-off-by: Pavel Boldin <pboldin(a)mirantis.com&gt; Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/util/virtypedparam.c | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/src/util/virtypedparam.c b/src/util/virtypedparam.c index 68620f5..6f608d6 100644 --- a/src/util/virtypedparam.c +++ b/src/util/virtypedparam.c @@ -749,14 +749,6 @@ virTypedParamsGetString(virTypedParameterPtr params, } -#define VIR_TYPED_PARAM_CHECK() \ - do { if (virTypedParamsGet(*params, n, name)) { \ - virReportError(VIR_ERR_INVALID_ARG, \ - _("Parameter '%s' is already set"), name); \ - goto error; \ - } } while (0) - - /** * virTypedParamsAddInt: * @params: pointer to the array of typed parameters @@ -787,7 +779,6 @@ virTypedParamsAddInt(virTypedParameterPtr *params, virResetLastError(); - VIR_TYPED_PARAM_CHECK(); if (VIR_RESIZE_N(*params, max, n, 1) < 0) goto error; *maxparams = max; @@ -835,7 +826,6 @@ virTypedParamsAddUInt(virTypedParameterPtr *params, virResetLastError(); - VIR_TYPED_PARAM_CHECK(); if (VIR_RESIZE_N(*params, max, n, 1) < 0) goto error; *maxparams = max; @@ -883,7 +873,6 @@ virTypedParamsAddLLong(virTypedParameterPtr *params, virResetLastError(); - VIR_TYPED_PARAM_CHECK(); if (VIR_RESIZE_N(*params, max, n, 1) < 0) goto error; *maxparams = max; @@ -931,7 +920,6 @@ virTypedParamsAddULLong(virTypedParameterPtr *params, virResetLastError(); - VIR_TYPED_PARAM_CHECK(); if (VIR_RESIZE_N(*params, max, n, 1) < 0) goto error; *maxparams = max; @@ -979,7 +967,6 @@ virTypedParamsAddDouble(virTypedParameterPtr *params, virResetLastError(); - VIR_TYPED_PARAM_CHECK(); if (VIR_RESIZE_N(*params, max, n, 1) < 0) goto error; *maxparams = max; @@ -1027,7 +1014,6 @@ virTypedParamsAddBoolean(virTypedParameterPtr *params, virResetLastError(); - VIR_TYPED_PARAM_CHECK(); if (VIR_RESIZE_N(*params, max, n, 1) < 0) goto error; *maxparams = max; @@ -1078,7 +1064,6 @@ virTypedParamsAddString(virTypedParameterPtr *params, virResetLastError(); - VIR_TYPED_PARAM_CHECK(); if (VIR_RESIZE_N(*params, max, n, 1) < 0) goto error; *maxparams = max; @@ -1136,7 +1121,6 @@ virTypedParamsAddFromString(virTypedParameterPtr *params, virResetLastError(); - VIR_TYPED_PARAM_CHECK(); if (VIR_RESIZE_N(*params, max, n, 1) < 0) goto error; *maxparams = max; -- 1.9.1

On 16.06.2015 00:42, Pavel Boldin wrote: I don't think this symbol needs to be exported. Since this is an internal API, it must go into src/libvirt_private.syms too. Once the virTypedParamsFilter symbol is exposed (internally), you will not need this. A loop iterator needs to be size_t. 'syntax-check' would have warn you about this. Michal

The `virTypedParamsAddStringList' function provides interface to add a NULL-terminated array of string values as a multi-value to the params. Signed-off-by: Pavel Boldin <pboldin(a)mirantis.com&gt; Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- include/libvirt/libvirt-host.h | 6 ++++++ src/libvirt_public.syms | 1 + src/util/virtypedparam.c | 36 ++++++++++++++++++++++++++++++++++++ tests/virtypedparamtest.c | 28 ++++++++++++++++++++++++++++ 4 files changed, 71 insertions(+) diff --git a/include/libvirt/libvirt-host.h b/include/libvirt/libvirt-host.h index 8222cfb..55a8e5d 100644 --- a/include/libvirt/libvirt-host.h +++ b/include/libvirt/libvirt-host.h @@ -331,6 +331,12 @@ virTypedParamsAddString (virTypedParameterPtr *params, const char *name, const char *value); int +virTypedParamsAddStringList(virTypedParameterPtr *params, + int *nparams, + int *maxparams, + const char *name, + const char **values); +int virTypedParamsAddFromString(virTypedParameterPtr *params, int *nparams, int *maxparams, diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 0a1feea..ccc7532 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -718,6 +718,7 @@ LIBVIRT_1.2.16 { LIBVIRT_1.3.0 { global: virTypedParamsGetAllStrings; + virTypedParamsAddStringList; } LIBVIRT_1.2.16; # .... define new API here using predicted next version number .... diff --git a/src/util/virtypedparam.c b/src/util/virtypedparam.c index a12006c..ff2c878 100644 --- a/src/util/virtypedparam.c +++ b/src/util/virtypedparam.c @@ -1187,6 +1187,42 @@ virTypedParamsAddString(virTypedParameterPtr *params, return -1; } +/** + * virTypedParamsAddStringList: + * @params: array of typed parameters + * @nparams: number of parameters in the @params array + * @maxparams: maximum number of parameters that can be stored in @params + * array without allocating more memory + * @name: name of the parameter to store values to + * @values: the values to store into the new parameters + * + * Packs NULL-terminated list of strings @values into @params under the + * key @name. + * + * Returns 0 on success, -1 on error. + */ +int +virTypedParamsAddStringList(virTypedParameterPtr *params, + int *nparams, + int *maxparams, + const char *name, + const char **values) +{ + size_t i; + int rv = -1; + + if (!values) + return 0; + + for (i = 0; values[i]; i++) { + if ((rv = virTypedParamsAddString(params, nparams, maxparams, + name, values[i])) < 0) + break; + } + + return rv; +} + /** * virTypedParamsAddFromString: diff --git a/tests/virtypedparamtest.c b/tests/virtypedparamtest.c index 698255a..2869535 100644 --- a/tests/virtypedparamtest.c +++ b/tests/virtypedparamtest.c @@ -126,6 +126,31 @@ testTypedParamsFilter(const void *opaque ATTRIBUTE_UNUSED) } static int +testTypedParamsAddStringList(const void *opaque ATTRIBUTE_UNUSED) +{ + int rv = 0; + virTypedParameterPtr params = NULL; + int nparams = 0, maxparams = 0, i; + + const char *values[] = { + "foo", "bar", "foobar", NULL + }; + + rv = virTypedParamsAddStringList(&params, &nparams, &maxparams, "param", + values); + + for (i = 0; i < nparams; i++) { + if (STRNEQ(params[i].field, "param") || + STRNEQ(params[i].value.s, values[i]) || + params[i].type != VIR_TYPED_PARAM_STRING) + rv = -1; + } + + virTypedParamsFree(params, nparams); + return rv; +} + +static int testTypedParamsGetAllStrings(const void *opaque ATTRIBUTE_UNUSED) { int i, picked; @@ -259,6 +284,9 @@ mymain(void) if (virtTestRun("Get All Strings", testTypedParamsGetAllStrings, NULL) < 0) rv = -1; + if (virtTestRun("Add string list", testTypedParamsAddStringList, NULL) < 0) + rv = -1; + if (rv < 0) return EXIT_FAILURE; return EXIT_SUCCESS; -- 1.9.1

On 16.06.2015 00:42, Pavel Boldin wrote: This is formated suspiciously. I think @flags should be the last argument, like in the rest of the patch where you add @migrate_disks, Michal

On 16.06.2015 00:42, Pavel Boldin wrote: Basically, this is the diff of the all nits I've pointed out: diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index f7373af..6c41e89 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2272,6 +2272,8 @@ virTypedParameterTypeFromString; virTypedParameterTypeToString; virTypedParamsCheck; virTypedParamsCopy; +virTypedParamsFilter; +virTypedParamsGetAllStrings; virTypedParamsReplaceString; virTypedParamsValidate; diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index ccc7532..59d8c12 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -717,7 +717,6 @@ LIBVIRT_1.2.16 { LIBVIRT_1.3.0 { global: - virTypedParamsGetAllStrings; virTypedParamsAddStringList; } LIBVIRT_1.2.16; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d56579b..4c6b530 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12351,7 +12351,7 @@ qemuDomainMigrateBegin3(virDomainPtr domain, } return qemuMigrationBegin(domain->conn, vm, xmlin, dname, - cookieout, cookieoutlen, flags, 0, NULL); + cookieout, cookieoutlen, 0, NULL, flags); } static char * @@ -12381,9 +12381,9 @@ qemuDomainMigrateBegin3Params(virDomainPtr domain, &dname) < 0) goto cleanup; - nmigrate_disks = virTypedParamsGetAllStrings( - params, nparams, VIR_MIGRATE_PARAM_MIGRATE_DISKS, - &migrate_disks); + nmigrate_disks = virTypedParamsGetAllStrings(params, nparams, + VIR_MIGRATE_PARAM_MIGRATE_DISKS, + &migrate_disks); if (nmigrate_disks < 0) goto cleanup; @@ -12397,8 +12397,8 @@ qemuDomainMigrateBegin3Params(virDomainPtr domain, } ret = qemuMigrationBegin(domain->conn, vm, xmlin, dname, - cookieout, cookieoutlen, flags, - nmigrate_disks, migrate_disks); + cookieout, cookieoutlen, + nmigrate_disks, migrate_disks, flags); cleanup: VIR_FREE(migrate_disks); @@ -12495,9 +12495,9 @@ qemuDomainMigratePrepare3Params(virConnectPtr dconn, &listenAddress) < 0) goto cleanup; - nmigrate_disks = virTypedParamsGetAllStrings( - params, nparams, VIR_MIGRATE_PARAM_MIGRATE_DISKS, - &migrate_disks); + nmigrate_disks = virTypedParamsGetAllStrings(params, nparams, + VIR_MIGRATE_PARAM_MIGRATE_DISKS, + &migrate_disks); if (nmigrate_disks < 0) goto cleanup; @@ -12688,7 +12688,7 @@ qemuDomainMigratePerform3Params(virDomainPtr dom, virCheckFlags(QEMU_MIGRATION_FLAGS, -1); if (virTypedParamsValidate(params, nparams, QEMU_MIGRATION_PARAMETERS) < 0) - return -1; + return ret; if (virTypedParamsGetString(params, nparams, VIR_MIGRATE_PARAM_DEST_XML, @@ -12710,9 +12710,9 @@ qemuDomainMigratePerform3Params(virDomainPtr dom, &listenAddress) < 0) goto cleanup; - nmigrate_disks = virTypedParamsGetAllStrings( - params, nparams, VIR_MIGRATE_PARAM_MIGRATE_DISKS, - &migrate_disks); + nmigrate_disks = virTypedParamsGetAllStrings(params, nparams, + VIR_MIGRATE_PARAM_MIGRATE_DISKS, + &migrate_disks); if (nmigrate_disks < 0) goto cleanup; diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 275d416..7fae75b 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1584,6 +1584,7 @@ qemuMigrateDisk(virDomainDiskDef const *disk, size_t nmigrate_disks, const char **migrate_disks) { size_t i; + /* Check if the disk alias is in the list */ if (nmigrate_disks) { for (i = 0; i < nmigrate_disks; i++) { @@ -2871,9 +2872,9 @@ qemuMigrationBegin(virConnectPtr conn, const char *dname, char **cookieout, int *cookieoutlen, - unsigned long flags, size_t nmigrate_disks, - const char **migrate_disks) + const char **migrate_disks, + unsigned long flags) { virQEMUDriverPtr driver = conn->privateData; char *xml = NULL; diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h index 2a942c0..030b32f 100644 --- a/src/qemu/qemu_migration.h +++ b/src/qemu/qemu_migration.h @@ -101,9 +101,9 @@ char *qemuMigrationBegin(virConnectPtr conn, const char *dname, char **cookieout, int *cookieoutlen, - unsigned long flags, size_t nmigrate_disks, - const char **migrate_disks); + const char **migrate_disks, + unsigned long flags); virDomainDefPtr qemuMigrationPrepareDef(virQEMUDriverPtr driver, const char *dom_xml, diff --git a/src/util/virtypedparam.h b/src/util/virtypedparam.h index ac7f3a1..bc84096 100644 --- a/src/util/virtypedparam.h +++ b/src/util/virtypedparam.h @@ -29,7 +29,7 @@ /** * VIR_TYPED_PARAM_MULTIPLE: * - * Flag indiciating that the params has multiple occurences of the parameter. + * Flag indicating that the params has multiple occurrences of the parameter. * Only used as a flag for @type argument of the virTypedParamsValidate. */ # define VIR_TYPED_PARAM_MULTIPLE (1 << 31) diff --git a/tests/Makefile.am b/tests/Makefile.am index 5d10999..77de8c0 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -1236,7 +1236,7 @@ objecteventtest_LDADD = $(LDADDS) virtypedparamtest_SOURCES = \ virtypedparamtest.c testutils.h testutils.c -virtypedparamtest_LDADD = $(LDADDS) ../src/libvirt_util.la $(GNULIB_LIBS) +virtypedparamtest_LDADD = $(LDADDS) if WITH_LINUX diff --git a/tests/virtypedparamtest.c b/tests/virtypedparamtest.c index 2869535..b362b9b 100644 --- a/tests/virtypedparamtest.c +++ b/tests/virtypedparamtest.c @@ -153,7 +153,8 @@ testTypedParamsAddStringList(const void *opaque ATTRIBUTE_UNUSED) static int testTypedParamsGetAllStrings(const void *opaque ATTRIBUTE_UNUSED) { - int i, picked; + size_t i; + int picked; int rv = -1; char l = '1'; const char **strings = NULL; I have it squashed into the corresponding commits. So with this - you have my ACK, although it feels a bit weird to ACK my own patches. Therefore, I'm giving others some time before merging this to express their feelings. Michal