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>

On Thu, Jul 26, 2018 at 05:36:28PM +0200, Michal Privoznik wrote:

This way it will be easier to use autofree.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- 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;

Contrary to what I said in the previous patch, I hadn't realized we were expanding a list by 1 in a loop before I looked at this patch. That is very inefficient and we'll keep doing that. Now I'm biased towards tracking the size of the list so that we can do VIR_EXPAND_N(*veths, 0, def->nnets) and then just add new elements, of course that would require tweaking the virStringListAdd more. Erik

On Fri, Jul 27, 2018 at 02:34:41PM +0200, Michal Privoznik wrote:

On 07/27/2018 10:37 AM, Erik Skultety wrote:

...

On Thu, Jul 26, 2018 at 05:36:28PM +0200, Michal Privoznik wrote:

...

This way it will be easier to use autofree.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- 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;

Contrary to what I said in the previous patch, I hadn't realized we were expanding a list by 1 in a loop before I looked at this patch. That is very inefficient and we'll keep doing that. Now I'm biased towards tracking the size of the list so that we can do VIR_EXPAND_N(*veths, 0, def->nnets) and then just add new elements, of course that would require tweaking the virStringListAdd more.

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?

Yes, that's the idea. Originally, I didn't think dropping virStringListAdd would be necessary, but it's clear from the snippet below that it would. Erik

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

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list