[libvirt] [PATCH 1/3] util: Rework virStringListAdd

[Erik Skultety]

On Thu, Jul 26, 2018 at 05:36:27PM +0200, Michal Privoznik wrote:
> So every caller does the same: they use virStringListAdd() to add

^This sounds a bit like there's a handful of them ;).

> new item into the list and then free the old copy to replace it
> with new list. It's not very memory effective, nor environmental
> friendly.
>
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/util/virmacmap.c  |  8 ++------
>  src/util/virstring.c  | 34 ++++++++++++++--------------------
>  src/util/virstring.h  |  4 ++--
>  tests/virstringtest.c |  6 +-----
>  4 files changed, 19 insertions(+), 33 deletions(-)
>
> diff --git a/src/util/virmacmap.c b/src/util/virmacmap.c
> index 88ca9b3f36..c7b700fa05 100644
> --- a/src/util/virmacmap.c
> +++ b/src/util/virmacmap.c
> @@ -90,7 +90,6 @@ virMacMapAddLocked(virMacMapPtr mgr,
>  {
>      int ret = -1;
>      char **macsList = NULL;
> -    char **newMacsList = NULL;
>
>      if ((macsList = virHashLookup(mgr->macs, domain)) &&
>          virStringListHasString((const char**) macsList, mac)) {
> @@ -98,15 +97,12 @@ virMacMapAddLocked(virMacMapPtr mgr,
>          goto cleanup;
>      }
>
> -    if (!(newMacsList = virStringListAdd((const char **) macsList, mac)) ||
> -        virHashUpdateEntry(mgr->macs, domain, newMacsList) < 0)
> +    if (virStringListAdd(&macsList, mac) < 0 ||
> +        virHashUpdateEntry(mgr->macs, domain, macsList) < 0)
>          goto cleanup;
> -    newMacsList = NULL;
> -    virStringListFree(macsList);
>
>      ret = 0;
>   cleanup:
> -    virStringListFree(newMacsList);
>      return ret;
>  }
>
> diff --git a/src/util/virstring.c b/src/util/virstring.c
> index 93fda69d7f..59f57a97b8 100644
> --- a/src/util/virstring.c
> +++ b/src/util/virstring.c
> @@ -175,32 +175,26 @@ char *virStringListJoin(const char **strings,
>   * @strings: a NULL-terminated array of strings
>   * @item: string to add
>   *
> - * Creates new strings list with all strings duplicated and @item
> - * at the end of the list. Callers is responsible for freeing
> - * both @strings and returned list.
> + * Appends @item into string list @strings. If *@strings is not
> + * allocated yet new string list is created.
> + *
> + * Returns: 0 on success,
> + *         -1 otherwise
>   */
> -char **
> -virStringListAdd(const char **strings,
> +int
> +virStringListAdd(char ***strings,
>                   const char *item)
>  {
> -    char **ret = NULL;
> -    size_t i = virStringListLength(strings);
> +    size_t i = virStringListLength((const char **) *strings);
>
> -    if (VIR_ALLOC_N(ret, i + 2) < 0)
> -        goto error;

This scales linearly, but given the number of "every" caller of this and the
projected frequency of usage, I don't really care about N sentinels.
You could consider VIR_EXPAND_N to get rid of the explicit sentinel assignment
below, your call.

> +    if (VIR_REALLOC_N(*strings, i + 2) < 0)
> +        return -1;
>
> -    for (i = 0; strings && strings[i]; i++) {
> -        if (VIR_STRDUP(ret[i], strings[i]) < 0)
> -            goto error;
> -    }
> +    (*strings)[i + 1] = NULL;
> +    if (VIR_STRDUP((*strings)[i], item) < 0)
> +        return -1;
>
> -    if (VIR_STRDUP(ret[i], item) < 0)
> -        goto error;
> -
> -    return ret;
> - error:
> -    virStringListFree(ret);
> -    return NULL;
> +    return 0;

Reviewed-by: Erik Skultety <eskultet at redhat.com>