On 12.05.2015 14:07, Pavel Boldin wrote:
> 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@xxxxxxxxxxxx>
> ---
> include/libvirt/libvirt-host.h | 8 ++
> src/util/virtypedparam.c | 107 ++++++++++++++++---------
> tests/Makefile.am | 6 ++
> tests/virtypedparamtest.c | 177 +++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 259 insertions(+), 39 deletions(-)
> create mode 100644 tests/virtypedparamtest.c
>
> diff --git a/include/libvirt/libvirt-host.h b/include/libvirt/libvirt-host.h
> index 953366b..a3e8b88 100644
> --- a/include/libvirt/libvirt-host.h
> +++ b/include/libvirt/libvirt-host.h
> @@ -180,6 +180,14 @@ typedef enum {
> } virTypedParameterType;
>
> /**
> + * 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)
> +
> +/**
I think we should not expose this flag. libvirt-host.h is a public
header file, and the flag is/should be private. I think virtypedparam.h
is more appropriate.
> * virTypedParameterFlags:
> *
> * Flags related to libvirt APIs that use virTypedParameter.
> diff --git a/src/util/virtypedparam.c b/src/util/virtypedparam.c
> index de2d447..bd47609 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);
> +}
> +
We don't like function names starting with an underscore.
> /* 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,82 @@ virTypedParamsValidate(virTypedParameterPtr params, int nparams, ...)
> size_t i, j;
> const char *name;
> int type;
> + size_t nkeys = 0, nkeysmax = 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 intentially don't copy values */
intentionally
> + 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, nkeysmax, 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));
missing 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/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..27b7e60
> --- /dev/null
> +++ b/tests/virtypedparamtest.c
> @@ -0,0 +1,177 @@
> +/*
> + * 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 TEST(testname) { \
> + .name = testname,
> +
> +#define ENDTEST },
> +
> +#define FOOBAR_SINGLE .foobar_flags = 0,
> +#define FOOBAR_MULTIPLE .foobar_flags = VIR_TYPED_PARAM_MULTIPLE,
> +
> +#define EXPECTED_OK .expected_errcode = 0, .expected_errmessage = NULL,
> +#define EXPECTED_ERROR(code, msg) .expected_errcode = code, \
> + .expected_errmessage = msg,
> +
> +#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__),
> +
> +#define PARAM(field_, type_) { .field = field_, .type = type_ }
> +
> +static int
> +testTypedParamsValidator(void)
> +{
> + size_t i;
> + int rv = 0;
> +
> + TypedParameterTest test[] = {
> + TEST("Invalid arg type")
> + FOOBAR_SINGLE
> + PARAMS(PARAM("foobar", VIR_TYPED_PARAM_INT))
> + EXPECTED_ERROR(
> + VIR_ERR_INVALID_ARG,
> + "invalid argument: invalid type 'int' for parameter "
> + "'foobar', expected 'string'"
> + )
> + ENDTEST
> + TEST("Extra arg")
> + FOOBAR_SINGLE
> + PARAMS(PARAM("f", VIR_TYPED_PARAM_INT))
> + EXPECTED_ERROR(
> + VIR_ERR_INVALID_ARG,
> + "argument unsupported: parameter 'f' not supported"
> + )
> + ENDTEST
> + TEST("Valid parameters")
> + FOOBAR_SINGLE
> + PARAMS(
> + PARAM("bar", VIR_TYPED_PARAM_UINT),
> + PARAM("foobar", VIR_TYPED_PARAM_STRING),
> + PARAM("foo", VIR_TYPED_PARAM_INT)
> + )
> + EXPECTED_OK
> + ENDTEST
> + TEST("Duplicates incorrect")
> + FOOBAR_SINGLE
> + PARAMS(
> + PARAM("bar", VIR_TYPED_PARAM_UINT),
> + PARAM("foobar", VIR_TYPED_PARAM_STRING),
> + PARAM("foobar", VIR_TYPED_PARAM_STRING),
> + PARAM("foo", VIR_TYPED_PARAM_INT)
> + )
> + EXPECTED_ERROR(
> + VIR_ERR_INVALID_ARG,
> + "invalid argument: parameter 'foobar' occurs multiple times"
> + )
> + ENDTEST
> + TEST("Duplicates OK for marked")
> + FOOBAR_MULTIPLE
> + PARAMS(
> + PARAM("bar", VIR_TYPED_PARAM_UINT),
> + PARAM("foobar", VIR_TYPED_PARAM_STRING),
> + PARAM("foobar", VIR_TYPED_PARAM_STRING),
> + PARAM("foo", VIR_TYPED_PARAM_INT)
> + )
> + EXPECTED_OK
> + },
> + TEST(NULL) ENDTEST
> + };
I must say this looks like a plain English programming to me. It's not
that I would hate English, but I think it would be more readable if it
was C. I'm not saying you must drop all the macros, but ENDTEST?
Although kudos for the idea.
> +
> + 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)
>
Hm. How are users supposed to create multi value array of typed
parameters? Usually, they create it by calling virTypedParamsAdd* which
among other things call VIR_TYPED_PARAM_CHECK(), which checks whether
there already exists a key with given name. I *think* this check can be
removed from all the callers and we can rely just on the server
validating the array. On the other hand - there's not much that a client
can know which keys are allowed to occur multiple times and which not.
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list