[libvirt] [PATCH v2 3/6] util: virTypedParams{Filter, PickStrings}

[Michal Privoznik]

On 21.05.2015 13:07, Pavel Boldin wrote:
> Add multikey API:
> 
>  * virTypedParamsFilter that returns all the parameters with specified name.
>  * 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       | 107 +++++++++++++++++++++++++++++++++++++++++
>  tests/virtypedparamtest.c      |  96 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 219 insertions(+)
> 
> diff --git a/include/libvirt/libvirt-host.h b/include/libvirt/libvirt-host.h
> index 070550b..36267fc 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,
> @@ -283,6 +288,11 @@ virTypedParamsGetString (virTypedParameterPtr params,
>                           int nparams,
>                           const char *name,
>                           const char **value);
> +const char **
> +virTypedParamsPickStrings(virTypedParameterPtr params,
> +                         int nparams,
> +                         const char *name,
> +                         size_t *picked);
>  int
>  virTypedParamsAddInt    (virTypedParameterPtr *params,
>                           int *nparams,
> diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms
> index ef3d2f0..8fc8c42 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:
> +        virTypedParamsFilter;
> +        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 ec20b74..a3d959e 100644
> --- a/src/util/virtypedparam.c
> +++ b/src/util/virtypedparam.c
> @@ -481,6 +481,58 @@ 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
> + *
> + *
> + * Filters @params retaining only the parameters named @name in the
> + * resulting array @ret. Caller should free the @ret array but no the

s/no/not/

> + * 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, max = 0, n = 0;

s/max/alloc/

> +
> +    virResetLastError();
> +
> +    if (!params || !name || !ret) {
> +        virReportError(VIR_ERR_INVALID_ARG,
> +                       _("Required argument is missing: %s"),
> +                       !params ? "params" : !name ? "name" : "ret");
> +        goto error;

We have a macro for that: virCheckNonNullArgGoto()

> +    }
> +
> +    for (i = 0; i < nparams; i++) {
> +        if (STREQ(params[i].field, name)) {
> +            if (VIR_RESIZE_N(*ret, max, n, 1) < 0)
> +                goto error;
> +
> +            (*ret)[n] = &params[i];
> +
> +            n++;
> +        }
> +    }

So if there's no match, @ret is untouched. This is not cool, consider
the following code:

virTypedParameterPtr *ret;
if (virTypedParamsFilter(.., &ret) < 0) {
  error;
} else {
  success;
  free(ret);
}

The free() may end up freeing a random pointer.

> +
> +    return n;
> +
> + error:
> +    if (ret)
> +        VIR_FREE(*ret);
> +    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 +801,61 @@ virTypedParamsGetString(virTypedParameterPtr params,
>  
>  
>  /**
> + * virTypedParamsPickStrings:
> + * @params: array of typed parameters
> + * @nparams: number of parameters in the @params array
> + * @name: name of the parameter to find
> + * @picked: pointer to the amount of picked strings.
> + *
> + *
> + * Finds typed parameters called @name.
> + *
> + * Returns a string list, which is a NULL terminated array of pointers to
> + * strings. Since a NULL is a valid parameter string value caller can ask
> + * for exact amount of picked strings using @picked argument.

This does not make much sense to me. As even your own test shows, while
you are returning a NULL terminated array, you allow NULLs to occur in
the array too. Therefore seeing a NULL item, one can't be sure if it is
an item or array margin. That's why we have @picked. But since we have
it, why we need NULL terminator?

> + *
> + * Caller should free the returned array but not the items since they are
> + * taken from @params array.
> + */
> +const char **
> +virTypedParamsPickStrings(virTypedParameterPtr params,
> +                          int nparams, const char *name,
> +                          size_t *picked)
> +{
> +    const char **values = NULL;
> +    size_t i, n;
> +    int nfiltered;
> +    virTypedParameterPtr *filtered = NULL;
> +
> +    virResetLastError();
> +
> +    nfiltered = virTypedParamsFilter(params, nparams, name, &filtered);
> +
> +    if (nfiltered <= 0)

@picked should be updated here.

> +        return NULL;
> +
> +    if (VIR_ALLOC_N(values, nfiltered + 1) < 0)
> +        goto error;

So, if allocation fails, NULL is returned. How is this different to case
covered in previous case (i.e. when no such param with @name was found)?
We need to distinguish these two cases.

> +
> +    for (n = 0, i = 0; i < nfiltered; i++) {
> +        if (filtered[i]->type == VIR_TYPED_PARAM_STRING)
> +            values[n++] = filtered[i]->value.s;
> +    }
> +
> +    if (picked)
> +        *picked = n;
> +
> +    VIR_FREE(filtered);
> +    return values;
> +
> + error:
> +    VIR_FREE(filtered);
> +    virDispatchError(NULL);
> +    return NULL;
> +}
> +
> +
> +/**
>   * virTypedParamsAddInt:
>   * @params: pointer to the array of typed parameters
>   * @nparams: number of parameters in the @params array

Michal