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@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@redhat.com>