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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
