Re: [PATCH 4/8] virstring: Introduce virStringListRemove

[Pino Toscano <ptoscano@xxxxxxxxxx>]

On Wednesday, 30 November 2016 10:59:31 CET Michal Privoznik wrote:
> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> ---
>  src/libvirt_private.syms |  1 +
>  src/util/virstring.c     | 50 ++++++++++++++++++++++++++++++++++++++++++++++++
>  src/util/virstring.h     |  3 +++
>  tests/virstringtest.c    | 38 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 92 insertions(+)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index bd46e5f..b1f42f2 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2465,6 +2465,7 @@ virStringListGetFirstWithPrefix;
>  virStringListHasString;
>  virStringListJoin;
>  virStringListLength;
> +virStringListRemove;
>  virStringReplace;
>  virStringSearch;
>  virStringSortCompare;
> diff --git a/src/util/virstring.c b/src/util/virstring.c
> index d2fb543..7d2c4c7 100644
> --- a/src/util/virstring.c
> +++ b/src/util/virstring.c
> @@ -206,6 +206,56 @@ virStringListAdd(const char **strings,
>  
>  
>  /**
> + * virStringListRemove:
> + * @strings: a NULL-terminated array of strings
> + * @newStrings: new NULL-terminated array of strings
> + * @item: string to remove
> + *
> + * Creates new strings list with all strings duplicated except
> + * for every occurrence of @item. Callers is responsible for
> + * freeing both @strings and returned list.
> + *
> + * Returns the number of items in the new list (excluding NULL
> + * anchor), -1 on error.

Wouldn't it be better to return the number of items removed? After all,
if the size of the new list is needed, virStringListLength can be used.

> +int
> +virStringListRemove(const char **strings,
> +                    char ***newStrings,
> +                    const char *item)
> +{
> +    char **ret = NULL;
> +    size_t i, j = 0;
> +
> +    for (i = 0; strings && strings[i]; i++) {
> +        if (STRNEQ(strings[i], item))
> +            j++;
> +    }
> +
> +    if (!j) {
> +        *newStrings = NULL;
> +        return 0;
> +    }

Shouldn't this produce an empty list instead? I.e. I'd expect that:

  char **elems = { "foo", NULL };
  char **newStrings;
  int count = virStringListRemove(elems, &newStrings, "foo");
  assert(count == 0);
  assert(newStrings != NULL);
  assert(newStrings[0] == NULL);

Ditto when trying to remove anything from an empty list.

The exception would be when strings == NULL, so virStringListRemove
should just directly set *newStrings to NULL and return 0.

> +static int testRemove(const void *args)
> +{
> +    const struct testSplitData *data = args;
> +    char **list = NULL;
> +    size_t ntokens;
> +    size_t i;
> +    int ret = -1;
> +
> +    if (!(list = virStringSplitCount(data->string, data->delim,
> +                                     data->max_tokens, &ntokens))) {
> +        VIR_DEBUG("Got no tokens at all");
> +        return -1;
> +    }
> +
> +    for (i = 0; data->tokens[i]; i++) {
> +        char **tmp;
> +        if (virStringListRemove((const char **) list,
> +                                &tmp, data->tokens[i]) < 0)

IMHO the test should also check the return value is what is expected.

Thanks,
-- 
Pino Toscano

Attachment: signature.asc
Description: This is a digitally signed message part.

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list