On Wed, Nov 30, 2016 at 11:25:55AM +0100, Pino Toscano wrote:
> 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.
>
I'm not sure to what use would such an information serve. The way the patch
suggest removes the necessity for yet another call just to determine the length
of the new list. In python, sure, that would be IMHO the correct reasoning as
the intended use of the objects and its methods but since the caller is indeed
responsible for freeing both of the lists, returning the size of the new list,
stripped down from the non-desired elements, is a valuable information for
free. Additionally, I'd expect the caller to also know how many elements
they're trying to remove.
> > +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);
>
I think in this case you might want to look at this as a black-box, you know
what the function should return and that should be enough, therefore returning
0 should be an indicator that there either is a some rubbish assigned by the
function or your variable wasn't touched at all, but you cannot tell, the
function does not state explicitly what it does with it, you only know that you
have to call free() unless there was an error returned by the function.
> 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.
Agreed.
Erik
>
> Thanks,
> --
> Pino Toscano
> --
> libvir-list mailing list
> libvir-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
