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

2018-07-27 Thread 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 
> ---
>  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(, 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 

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

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

2018-07-26 Thread Michal Privoznik 
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 
---
 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(, 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(, data->tokens[i]) < 0)
 goto cleanup;
-virStringListFree(list);
-list = tmp;
-tmp = NULL;
 }
 
 if (!list &&
-- 
2.16.4

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