[libvirt] [PATCH 2/5] util: virTypedParamsPick* multikey API
Michal Privoznik
mprivozn at redhat.com
Thu May 14 15:18:36 UTC 2015
On 12.05.2015 14:07, Pavel Boldin wrote:
> Add multikey APIs for virTypedParams*:
>
> * virTypedParamsPick that returns all the parameters with the
> specified name and type.
> * virTypedParamsPickStrings that returns a NULL-terminated `const char**'
> list with all the values for specified name and string type.
>
> Signed-off-by: Pavel Boldin <pboldin at mirantis.com>
> ---
> include/libvirt/libvirt-host.h | 10 +++++
> src/libvirt_public.syms | 6 +++
> src/util/virtypedparam.c | 90 ++++++++++++++++++++++++++++++++++++++++++
> tests/virtypedparamtest.c | 87 ++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 193 insertions(+)
>
> diff --git a/include/libvirt/libvirt-host.h b/include/libvirt/libvirt-host.h
> index a3e8b88..afa730f 100644
> --- a/include/libvirt/libvirt-host.h
> +++ b/include/libvirt/libvirt-host.h
> @@ -256,6 +256,12 @@ virTypedParameterPtr
> virTypedParamsGet (virTypedParameterPtr params,
> int nparams,
> const char *name);
> +virTypedParameterPtr*
s/Ptr*/Ptr */
> +virTypedParamsPick (virTypedParameterPtr params,
> + int nparams,
> + const char *name,
> + int type,
> + size_t *picked);
> int
> virTypedParamsGetInt (virTypedParameterPtr params,
> int nparams,
> @@ -291,6 +297,10 @@ virTypedParamsGetString (virTypedParameterPtr params,
> int nparams,
> const char *name,
> const char **value);
> +const char **
> +virTypedParamsPickStrings(virTypedParameterPtr params,
> + int nparams,
> + const char *name);
> int
> virTypedParamsAddInt (virTypedParameterPtr *params,
> int *nparams,
> diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms
> index ef3d2f0..d886967 100644
> --- a/src/libvirt_public.syms
> +++ b/src/libvirt_public.syms
> @@ -710,4 +710,10 @@ LIBVIRT_1.2.15 {
> virDomainDelIOThread;
> } LIBVIRT_1.2.14;
>
> +LIBVIRT_1.2.16 {
> + global:
> + virTypedParamsPick;
> + virTypedParamsPickStrings;
> +} 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 bd47609..643d10e 100644
> --- a/src/util/virtypedparam.c
> +++ b/src/util/virtypedparam.c
> @@ -480,6 +480,56 @@ virTypedParamsGet(virTypedParameterPtr params,
> }
>
>
> +/**
> + * virTypedParamsPick:
> + * @params: array of typed parameters
> + * @nparams: number of parameters in the @params array
> + * @name: name of the parameter to find
> + * @type: type of the parameter to find
> + * @picked: pointer to a size_t with amount of picked
Maybe "the length of returned array"?
> + *
> + *
> + * Finds typed parameters called @name.
> + *
> + * Returns pointers to the parameters or NULL if there are none in @params.
> + * This function does not raise an error, even when returning NULL.
Generally, it's not okay for a public error to not throw an error.
Returning NULL is a valid case though. However I think we need to
distinguish between no result returned and an error occurring .. [1]
> + * Callee should call VIR_FREE on the returned array.
Don't want to be picky, but s/Callee/Caller/ and /call VIR_FREE
on/free/. And maybe mention that it's only the array that needs to be
freed - items within it are just copied.
> + */
> +virTypedParameterPtr*
> +virTypedParamsPick(virTypedParameterPtr params,
> + int nparams,
> + const char *name,
> + int type,
> + size_t *picked)
> +{
> + size_t i, max = 0;
> + virTypedParameterPtr *values = NULL;
> +
This is a public API. The very first thing it has to do is call
virResetLastError().
> + *picked = 0;
Nah, this should be checked for NULL.
> +
> + if (!params || !name)
> + return NULL;
> +
> + for (i = 0; i < nparams; i++) {
> + if (params[i].type == type && STREQ(params[i].field, name)) {
> + if (VIR_RESIZE_N(values, max, *picked, 1) < 0)
1: .. as a result of OOM. So maybe the function header should look more
like this:
int virTypedParamsFilter(virTypedParameterPtr params,
int nparams,
const char *name,
int type,
virTypedParameterPtr *ret,
size_t *picked);
Also, is the @type needed? I guess we can go with just @name. Picking
all the params that have desired key name.
> + goto error;
> +
> + values[*picked] = ¶ms[i];
> +
> + *picked += 1;
> + }
> + }
> +
> + return values;
> +
> + error:
> + *picked = 0;
> + VIR_FREE(values);
> + return NULL;
> +}
> +
> +
> #define VIR_TYPED_PARAM_CHECK_TYPE(check_type) \
> do { if (param->type != check_type) { \
> virReportError(VIR_ERR_INVALID_ARG, \
> @@ -747,6 +797,46 @@ virTypedParamsGetString(virTypedParameterPtr params,
> }
>
>
> +/**
> + * virTypedParamsPickStrings:
> + * @params: array of typed parameters
> + * @nparams: number of parameters in the @params array
> + * @name: name of the parameter to find
> + *
> + *
> + * Finds typed parameters called @name.
> + *
> + * Returns pointers to the parameters or NULL if there are none in @params.
Returns a string list, which is a NULL terminated array of pointers to
strings.
> + * This function does not raise an error, even when returning NULL.
> + * Callee should call VIR_FREE on the returned array.
> + */
> +const char **
> +virTypedParamsPickStrings(virTypedParameterPtr params,
> + int nparams, const char *name)
> +{
> + const char **values = NULL;
> + size_t i, picked;
> + virTypedParameterPtr *picked_params;
Again, public API, virResetLastError() should go here.
> +
> + picked_params = virTypedParamsPick(params, nparams,
> + name, VIR_TYPED_PARAM_STRING,
> + &picked);
> +
> + if (picked_params == NULL)
> + return NULL;
> +
> + if (VIR_ALLOC_N(values, picked + 1) < 0)
> + goto cleanup;
> +
> + for (i = 0; i < picked; i++)
> + values[i] = picked_params[i]->value.s;
Hm.. Currently we allow virTypedParamsAddString(params, &nparams,
&maxparams, "some key name", NULL); where NULL is the value. Not that it
would be used anywhere, but we allow it. It would break the list here.
So maybe we need to return the length of the list too?
> +
> + cleanup:
> + VIR_FREE(picked_params);
> + return values;
> +}
This function suffers the same problems as the previous one. I guess we
need slightly different function header.
> +
> +
> #define VIR_TYPED_PARAM_CHECK() \
> do { if (virTypedParamsGet(*params, n, name)) { \
> virReportError(VIR_ERR_INVALID_ARG, \
See? This is the check I'm mentioning in 1/5.
> diff --git a/tests/virtypedparamtest.c b/tests/virtypedparamtest.c
> index 27b7e60..287d3f1 100644
> --- a/tests/virtypedparamtest.c
> +++ b/tests/virtypedparamtest.c
> @@ -95,6 +95,87 @@ testTypedParamsValidate(const void *opaque)
> #define PARAM(field_, type_) { .field = field_, .type = type_ }
>
> static int
> +testTypedParamsPick(const void *opaque ATTRIBUTE_UNUSED)
> +{
> + size_t i, picked;
> + 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 *picked_params = NULL;
> +
> +
> + picked_params = virTypedParamsPick(params, ARRAY_CARDINALITY(params),
> + "foo", VIR_TYPED_PARAM_INT, &picked);
> + if (picked != 3)
> + goto cleanup;
> +
> + for (i = 0; i < picked; i++) {
> + if (picked_params[i] != ¶ms[1 + i * 2])
> + goto cleanup;
> + }
> + VIR_FREE(picked_params);
> +
> + picked_params = virTypedParamsPick(params, ARRAY_CARDINALITY(params),
> + "bar", VIR_TYPED_PARAM_UINT, &picked);
> +
> + if (picked != 2)
> + goto cleanup;
> +
> + for (i = 0; i < picked; i++) {
> + if (picked_params[i] != ¶ms[i * 2])
> + goto cleanup;
> + }
> +
> + rv = 0;
> + cleanup:
> + VIR_FREE(picked_params);
> + return rv;
> +}
> +
> +static int
> +testTypedParamsPickStrings(const void *opaque ATTRIBUTE_UNUSED)
> +{
> + size_t i;
> + int rv = -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 = "foo", .type = VIR_TYPED_PARAM_INT },
> + { .field = "bar", .type = VIR_TYPED_PARAM_STRING,
> + .value = { .s = (char*)"bar3"} }
> + };
> +
> + strings = virTypedParamsPickStrings(params,
> + ARRAY_CARDINALITY(params),
> + "bar");
> +
> + for (i = 0; strings[i]; i++) {
> + if (!STREQLEN(strings[i], "bar", 3))
> + goto cleanup;
> + if (strings[i][3] != i + '1')
> + goto cleanup;
> + }
> +
> + rv = 0;
> + cleanup:
> + VIR_FREE(strings);
> + return rv;
> +}
> +
> +static int
> testTypedParamsValidator(void)
> {
> size_t i;
> @@ -169,6 +250,12 @@ mymain(void)
> if (testTypedParamsValidator() < 0)
> rv = -1;
>
> + if (virtTestRun("Picking", testTypedParamsPick, NULL) < 0)
> + rv = -1;
> +
> + if (virtTestRun("Picking Strings", testTypedParamsPickStrings, NULL) < 0)
> + rv = -1;
> +
> if (rv < 0)
> return EXIT_FAILURE;
> return EXIT_SUCCESS;
>
Michal
More information about the libvir-list
mailing list