On 05/26/2015 09:01 AM, Michal Privoznik wrote:
> From: Pavel Boldin <pboldin@xxxxxxxxxxxx>
>
> Add multikey API:
>
> * virTypedParamsFilter that returns all the parameters with specified name.
> * virTypedParamsPickStrings that returns a list with all the values for
> specified name and string type.
>
> Signed-off-by: Pavel Boldin <pboldin@xxxxxxxxxxxx>
> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> ---
> include/libvirt/libvirt-host.h | 11 ++++
> src/libvirt_public.syms | 2 +
> src/util/virtypedparam.c | 113 +++++++++++++++++++++++++++++++++++++++++
> tests/virtypedparamtest.c | 99 ++++++++++++++++++++++++++++++++++++
> 4 files changed, 225 insertions(+)
>
> diff --git a/include/libvirt/libvirt-host.h b/include/libvirt/libvirt-host.h
> index 070550b..6767527 100644
> --- a/include/libvirt/libvirt-host.h
> +++ b/include/libvirt/libvirt-host.h
> @@ -249,6 +249,11 @@ virTypedParamsGet (virTypedParameterPtr params,
> int nparams,
> const char *name);
> int
> +virTypedParamsFilter (virTypedParameterPtr params,
> + int nparams,
> + const char *name,
> + virTypedParameterPtr **ret);
> +int
> virTypedParamsGetInt (virTypedParameterPtr params,
> int nparams,
> const char *name,
> @@ -284,6 +289,12 @@ virTypedParamsGetString (virTypedParameterPtr params,
> const char *name,
> const char **value);
> int
> +virTypedParamsPickStrings(virTypedParameterPtr params,
> + int nparams,
> + const char *name,
> + const char ***values,
> + size_t *picked);
> +int
> virTypedParamsAddInt (virTypedParameterPtr *params,
> int *nparams,
> int *maxparams,
> diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms
> index 716dd2f..845a0b8 100644
> --- a/src/libvirt_public.syms
> +++ b/src/libvirt_public.syms
> @@ -713,6 +713,8 @@ LIBVIRT_1.2.15 {
> LIBVIRT_1.2.16 {
> global:
> virDomainSetUserPassword;
> + virTypedParamsFilter;
> + virTypedParamsPickStrings;
> } LIBVIRT_1.2.15;
This will need to change since it misses this release...
>
> # .... define new API here using predicted next version number ....
> diff --git a/src/util/virtypedparam.c b/src/util/virtypedparam.c
> index e91ca99..d88d4a7 100644
> --- a/src/util/virtypedparam.c
> +++ b/src/util/virtypedparam.c
> @@ -481,6 +481,57 @@ virTypedParamsGet(virTypedParameterPtr params,
> }
>
>
> +/**
> + * virTypedParamsFilter:
> + * @params: array of typed parameters
> + * @nparams: number of parameters in the @params array
> + * @name: name of the parameter to find
> + * @ret: pointer to the returned array
> + *
> + *
Extraneous blank line
> + * Filters @params retaining only the parameters named @name in the
> + * resulting array @ret. Caller should free the @ret array but not
> + * the items since they are pointing to the @params elements.
> + *
> + * Returns amount of elements in @ret on success, -1 on error.
> + */
> +int
> +virTypedParamsFilter(virTypedParameterPtr params,
> + int nparams,
> + const char *name,
> + virTypedParameterPtr **ret)
> +{
> + size_t i, alloc = 0, n = 0;
> +
> + virResetLastError();
> +
> + virCheckNonNullArgGoto(params, error);
> + virCheckNonNullArgGoto(name, error);
> + virCheckNonNullArgGoto(ret, error);
> +
> + *ret = NULL;
Coverity points out:
Event assign_zero: Assigning: "*ret" = "NULL".
> +
> + for (i = 0; i < nparams; i++) {
> + if (STREQ(params[i].field, name)) {
> + if (VIR_RESIZE_N(*ret, alloc, n, 1) < 0)
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.]
> + goto error;
> +
> + (*ret)[n] = ¶ms[i];
Coverity points out:
Dereferencing null pointer "*ret".
> +
> + n++;
> + }
> + }
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).
> +
> + return n;
> +
> + error:
> + if (ret)
> + VIR_FREE(*ret);
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
> + virDispatchError(NULL);
> + return -1;
> +}
> +
> +
> #define VIR_TYPED_PARAM_CHECK_TYPE(check_type) \
> do { if (param->type != check_type) { \
> virReportError(VIR_ERR_INVALID_ARG, \
> @@ -749,6 +800,68 @@ virTypedParamsGetString(virTypedParameterPtr params,
>
>
> /**
> + * virTypedParamsPickStrings:
> + * @params: array of typed parameters
> + * @nparams: number of parameters in the @params array
> + * @name: name of the parameter to find
> + * @values: array of returned values
> + * @picked: pointer to the amount of picked strings.
> + *
> + * Finds all parameters with desired @name within @params and
> + * store their values into @values. The @values array is self
> + * allocated and its length is stored into @picked. When no
> + * longer needed, caller should free the returned array, but not
> + * the items since they are taken from @params array.
> + *
> + * Returns 0 on success, -1 otherwise.
> + */
> +int
> +virTypedParamsPickStrings(virTypedParameterPtr params,
> + int nparams,
> + const char *name,
> + const char ***values,
> + size_t *picked)
> +{
> + size_t i, n;
> + int nfiltered;
> + virTypedParameterPtr *filtered = NULL;
> +
> + virResetLastError();
> +
> + virCheckNonNullArgGoto(values, error);
> + virCheckNonNullArgGoto(picked, error);
> +
> + nfiltered = virTypedParamsFilter(params, nparams, name, &filtered);
> +
> + if (nfiltered < 0)
> + goto error;
> +
> + *values = NULL;
> +
> + if (nfiltered &&
> + VIR_ALLOC_N(*values, nfiltered) < 0)
> + goto error;
> +
> + for (n = 0, i = 0; i < nfiltered; i++) {
> + if (filtered[i]->type == VIR_TYPED_PARAM_STRING)
> + (*values)[n++] = filtered[i]->value.s;
> + }
> +
> + *picked = n;
> +
> + VIR_FREE(filtered);
> + return 0;
> +
> + error:
> + if (values)
> + VIR_FREE(*values);
> + VIR_FREE(filtered);
> + virDispatchError(NULL);
> + return -1;
> +}
> +
> +
> +/**
> * virTypedParamsAddInt:
> * @params: pointer to the array of typed parameters
> * @nparams: number of parameters in the @params array
> diff --git a/tests/virtypedparamtest.c b/tests/virtypedparamtest.c
> index 95e22a7..abdddd5 100644
> --- a/tests/virtypedparamtest.c
> +++ b/tests/virtypedparamtest.c
> @@ -81,6 +81,99 @@ testTypedParamsValidate(const void *opaque)
> .nparams = PARAMS_SIZE(__VA_ARGS__),
>
> static int
> +testTypedParamsFilter(const void *opaque ATTRIBUTE_UNUSED)
> +{
> + size_t i, nfiltered;
> + int rv = -1;
> + virTypedParameter params[] = {
> + { .field = "bar", .type = VIR_TYPED_PARAM_UINT },
> + { .field = "foo", .type = VIR_TYPED_PARAM_INT },
> + { .field = "bar", .type = VIR_TYPED_PARAM_UINT },
> + { .field = "foo", .type = VIR_TYPED_PARAM_INT },
> + { .field = "foobar", .type = VIR_TYPED_PARAM_STRING },
> + { .field = "foo", .type = VIR_TYPED_PARAM_INT }
> + };
> + virTypedParameterPtr *filtered = NULL;
> +
> +
> + nfiltered = virTypedParamsFilter(params, ARRAY_CARDINALITY(params),
> + "foo", &filtered);
> + if (nfiltered != 3)
> + goto cleanup;
> +
> + for (i = 0; i < nfiltered; i++) {
> + if (filtered[i] != ¶ms[1 + i * 2])
> + goto cleanup;
> + }
> + VIR_FREE(filtered);
> + filtered = NULL;
> +
> + nfiltered = virTypedParamsFilter(params, ARRAY_CARDINALITY(params),
> + "bar", &filtered);
> +
> + if (nfiltered != 2)
> + goto cleanup;
> +
> + for (i = 0; i < nfiltered; i++) {
> + if (filtered[i] != ¶ms[i * 2])
> + goto cleanup;
> + }
> +
> + rv = 0;
> + cleanup:
> + VIR_FREE(filtered);
> + return rv;
> +}
> +
> +static int
> +testTypedParamsPickStrings(const void *opaque ATTRIBUTE_UNUSED)
> +{
> + size_t i, picked;
> + int rv = -1;
> + char l = '1';
> + const char **strings = NULL;
> +
> + virTypedParameter params[] = {
> + { .field = "bar", .type = VIR_TYPED_PARAM_STRING,
> + .value = { .s = (char*)"bar1"} },
> + { .field = "foo", .type = VIR_TYPED_PARAM_INT },
> + { .field = "bar", .type = VIR_TYPED_PARAM_STRING,
> + .value = { .s = (char*)"bar2"} },
> + { .field = "foo", .type = VIR_TYPED_PARAM_INT },
> + { .field = "foobar", .type = VIR_TYPED_PARAM_STRING },
> + { .field = "bar", .type = VIR_TYPED_PARAM_STRING,
> + .value = { .s = NULL } },
> + { .field = "foo", .type = VIR_TYPED_PARAM_INT },
> + { .field = "bar", .type = VIR_TYPED_PARAM_STRING,
> + .value = { .s = (char*)"bar3"} }
> + };
> +
> + if (virTypedParamsPickStrings(params,
> + ARRAY_CARDINALITY(params),
> + "bar",
> + &strings,
> + &picked) < 0)
> + goto cleanup;
> +
> + for (i = 0; i < picked; i++) {
> + if (i == 2) {
> + if (strings[i] != NULL)
> + goto cleanup;
> + continue;
> + }
> + if (!STREQLEN(strings[i], "bar", 3))
> + goto cleanup;
> + if (strings[i][3] != l++)
> + goto cleanup;
> + }
> +
> + rv = 0;
> + cleanup:
> + VIR_FREE(strings);
> + return rv;
> +}
> +
> +static int
> testTypedParamsValidator(void)
> {
> size_t i;
> @@ -159,6 +252,12 @@ mymain(void)
> if (testTypedParamsValidator() < 0)
> rv = -1;
>
> + if (virtTestRun("Filtering", testTypedParamsFilter, NULL) < 0)
> + rv = -1;
> +
> + if (virtTestRun("Picking Strings", testTypedParamsPickStrings, NULL) < 0)
> + rv = -1;
> +
> if (rv < 0)
> return EXIT_FAILURE;
> return EXIT_SUCCESS;
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list