Re: [libvirt] [PATCH 4/8] virstring: Introduce virStringListRemove
On 30.11.2016 11:25, Pino Toscano wrote:
On Wednesday, 30 November 2016 10:59:31 CET Michal Privoznik wrote:
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> 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.
Since this function is used at exactly one point in our source code I've
tailored it to my needs. Moreover, it's internal function so we can
change it later.
> +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);
Should it? All our StringList APIs threat { NULL } and NULL as the same
empty string list.
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.
Ah, good point. Will fix that.
Michal