So every caller does the same: they use virStringListAdd() to add 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(a)redhat.com&gt; --- 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; + 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; } diff --git a/src/util/virstring.h b/src/util/virstring.h index 125fd4eede..a2133ab7ce 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -44,8 +44,8 @@ char *virStringListJoin(const char **strings, const char *delim) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); -char **virStringListAdd(const char **strings, - const char *item); +int virStringListAdd(char ***strings, + const char *item); void virStringListRemove(char ***strings, const char *item); diff --git a/tests/virstringtest.c b/tests/virstringtest.c index 1230aba5b7..1a1e6364d1 100644 --- a/tests/virstringtest.c +++ b/tests/virstringtest.c @@ -179,12 +179,8 @@ static int testAdd(const void *args) size_t i; for (i = 0; data->tokens[i]; i++) { - char **tmp = virStringListAdd((const char **)list, data->tokens[i]); - if (!tmp) + if (virStringListAdd(&list, data->tokens[i]) < 0) goto cleanup; - virStringListFree(list); - list = tmp; - tmp = NULL; } if (!list && -- 2.16.4

This way it will be easier to use autofree. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/lxc/lxc_process.c | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index d021a890f7..3ac39d598c 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -514,8 +514,7 @@ static int virLXCProcessSetupNamespaces(virConnectPtr conn, * virLXCProcessSetupInterfaces: * @conn: pointer to connection * @def: pointer to virtual machine structure - * @nveths: number of interfaces - * @veths: interface names + * @veths: string list of interface names * * Sets up the container interfaces by creating the veth device pairs and * attaching the parent end to the appropriate bridge. The container end @@ -525,7 +524,6 @@ static int virLXCProcessSetupNamespaces(virConnectPtr conn, */ static int virLXCProcessSetupInterfaces(virConnectPtr conn, virDomainDefPtr def, - size_t *nveths, char ***veths) { int ret = -1; @@ -534,6 +532,8 @@ static int virLXCProcessSetupInterfaces(virConnectPtr conn, virDomainNetDefPtr net; virDomainNetType type; + *veths = NULL; + for (i = 0; i < def->nnets; i++) { char *veth = NULL; virNetDevBandwidthPtr actualBandwidth; @@ -549,9 +549,6 @@ static int virLXCProcessSetupInterfaces(virConnectPtr conn, if (virDomainNetAllocateActualDevice(def, net) < 0) goto cleanup; - if (VIR_EXPAND_N(*veths, *nveths, 1) < 0) - goto cleanup; - type = virDomainNetGetActualType(net); switch (type) { case VIR_DOMAIN_NET_TYPE_NETWORK: @@ -604,7 +601,8 @@ static int virLXCProcessSetupInterfaces(virConnectPtr conn, } } - (*veths)[(*nveths)-1] = veth; + if (virStringListAdd(veths, veth) < 0) + goto cleanup; if (VIR_STRDUP(def->nets[i]->ifname_guest_actual, veth) < 0) goto cleanup; @@ -902,7 +900,6 @@ int virLXCProcessStop(virLXCDriverPtr driver, static virCommandPtr virLXCProcessBuildControllerCmd(virLXCDriverPtr driver, virDomainObjPtr vm, - int nveths, char **veths, int *ttyFDs, size_t nttyFDs, @@ -987,7 +984,7 @@ virLXCProcessBuildControllerCmd(virLXCDriverPtr driver, virCommandAddArg(cmd, "--handshake"); virCommandAddArgFormat(cmd, "%d", handshakefd); - for (i = 0; i < nveths; i++) + for (i = 0; veths && veths[i]; i++) virCommandAddArgList(cmd, "--veth", veths[i], NULL); virCommandPassFD(cmd, handshakefd, 0); @@ -1184,7 +1181,6 @@ int virLXCProcessStart(virConnectPtr conn, size_t i; char *logfile = NULL; int logfd = -1; - size_t nveths = 0; char **veths = NULL; int handshakefds[2] = { -1, -1 }; off_t pos = -1; @@ -1355,7 +1351,7 @@ int virLXCProcessStart(virConnectPtr conn, } VIR_DEBUG("Setting up Interfaces"); - if (virLXCProcessSetupInterfaces(conn, vm->def, &nveths, &veths) < 0) + if (virLXCProcessSetupInterfaces(conn, vm->def, &veths) < 0) goto cleanup; VIR_DEBUG("Setting up namespaces if any"); @@ -1379,7 +1375,7 @@ int virLXCProcessStart(virConnectPtr conn, if (!(cmd = virLXCProcessBuildControllerCmd(driver, vm, - nveths, veths, + veths, ttyFDs, nttyFDs, nsInheritFDs, files, nfiles, @@ -1559,9 +1555,7 @@ int virLXCProcessStart(virConnectPtr conn, virLXCProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED); } virCommandFree(cmd); - for (i = 0; i < nveths; i++) - VIR_FREE(veths[i]); - VIR_FREE(veths); + virStringListFree(veths); for (i = 0; i < nttyFDs; i++) VIR_FORCE_CLOSE(ttyFDs[i]); VIR_FREE(ttyFDs); -- 2.16.4

On 07/27/2018 10:37 AM, Erik Skultety wrote: Hold on. I'm not quite sure I follow. Are you suggesting to pre-allocate the string list and ditch virStringListAdd completely? Something like this? diff --git i/src/lxc/lxc_process.c w/src/lxc/lxc_process.c index 6eef17d1ce..33c806630b 100644 --- i/src/lxc/lxc_process.c +++ w/src/lxc/lxc_process.c @@ -532,7 +532,8 @@ static int virLXCProcessSetupInterfaces(virConnectPtr conn, virDomainNetDefPtr net; virDomainNetType type; - *veths = NULL; + if (VIR_ALLOC_N(*veths, def->nnets + 1) < 0) + return -1; for (i = 0; i < def->nnets; i++) { char *veth = NULL; @@ -601,8 +602,7 @@ static int virLXCProcessSetupInterfaces(virConnectPtr conn, } } - if (virStringListAdd(veths, veth) < 0) - goto cleanup; + (*veths)[i] = veth; if (VIR_STRDUP(def->nets[i]->ifname_guest_actual, veth) < 0) goto cleanup; Michal

On Fri, Jul 27, 2018 at 05:02:36PM +0200, Erik Skultety wrote: Something I forgot: Reviewed-by: Erik Skultety <eskultet(a)redhat.com&gt;