Count the elements in advance rather than using VIR_APPEND_ELEMENT and
ensure that there's a NULL terminator for the string list so it's GStrv
compatible.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 src/storage/storage_backend_iscsi_direct.c | 29 ++++++++--------------
 1 file changed, 11 insertions(+), 18 deletions(-)

diff --git a/src/storage/storage_backend_iscsi_direct.c b/src/storage/storage_backend_iscsi_direct.c
index 263db835ae..2073a6df38 100644
--- a/src/storage/storage_backend_iscsi_direct.c
+++ b/src/storage/storage_backend_iscsi_direct.c
@@ -420,37 +420,30 @@ virISCSIDirectUpdateTargets(struct iscsi_context *iscsi,
                             size_t *ntargets,
                             char ***targets)
 {
-    int ret = -1;
     struct iscsi_discovery_address *addr;
     struct iscsi_discovery_address *tmp_addr;
-    size_t tmp_ntargets = 0;
-    char **tmp_targets = NULL;
+
+    *ntargets = 0;

     if (!(addr = iscsi_discovery_sync(iscsi))) {
         virReportError(VIR_ERR_INTERNAL_ERROR,
                        _("Failed to discover session: %s"),
                        iscsi_get_error(iscsi));
-        return ret;
+        return -1;
     }

-    for (tmp_addr = addr; tmp_addr; tmp_addr = tmp_addr->next) {
-        g_autofree char *target = NULL;
-
-        target = g_strdup(tmp_addr->target_name);
+    for (tmp_addr = addr; tmp_addr; tmp_addr = tmp_addr->next)
+        (*ntargets)++;

-        if (VIR_APPEND_ELEMENT(tmp_targets, tmp_ntargets, target) < 0)
-            goto cleanup;
-    }
+    *targets = g_new0(char *, ntargets + 1);
+    *ntargets = 0;

-    *targets = g_steal_pointer(&tmp_targets);
-    *ntargets = tmp_ntargets;
-    tmp_ntargets = 0;
+    for (tmp_addr = addr; tmp_addr; tmp_addr = tmp_addr->next)
+        *targets[(*ntargets)++] = g_strdup(tmp_addr->target_name);

-    ret = 0;
- cleanup:
     iscsi_free_discovery_data(iscsi, addr);
-    virStringListFreeCount(tmp_targets, tmp_ntargets);
-    return ret;
+
+    return 0;
 }

 static int
-- 
2.31.1

On 6/18/21 3:04 PM, Peter Krempa wrote:
> Count the elements in advance rather than using VIR_APPEND_ELEMENT and
> ensure that there's a NULL terminator for the string list so it's GStrv
> compatible.
> 
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
>  src/storage/storage_backend_iscsi_direct.c | 29 ++++++++--------------
>  1 file changed, 11 insertions(+), 18 deletions(-)
> 
> diff --git a/src/storage/storage_backend_iscsi_direct.c b/src/storage/storage_backend_iscsi_direct.c
> index 263db835ae..2073a6df38 100644
> --- a/src/storage/storage_backend_iscsi_direct.c
> +++ b/src/storage/storage_backend_iscsi_direct.c
> @@ -420,37 +420,30 @@ virISCSIDirectUpdateTargets(struct iscsi_context *iscsi,
>                              size_t *ntargets,
>                              char ***targets)
>  {
> -    int ret = -1;
>      struct iscsi_discovery_address *addr;
>      struct iscsi_discovery_address *tmp_addr;
> -    size_t tmp_ntargets = 0;
> -    char **tmp_targets = NULL;
> +
> +    *ntargets = 0;
> 
>      if (!(addr = iscsi_discovery_sync(iscsi))) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
>                         _("Failed to discover session: %s"),
>                         iscsi_get_error(iscsi));
> -        return ret;
> +        return -1;
>      }
> 
> -    for (tmp_addr = addr; tmp_addr; tmp_addr = tmp_addr->next) {
> -        g_autofree char *target = NULL;
> -
> -        target = g_strdup(tmp_addr->target_name);
> +    for (tmp_addr = addr; tmp_addr; tmp_addr = tmp_addr->next)
> +        (*ntargets)++;
> 
> -        if (VIR_APPEND_ELEMENT(tmp_targets, tmp_ntargets, target) < 0)
> -            goto cleanup;
> -    }
> +    *targets = g_new0(char *, ntargets + 1);

I'm afraid ntargets + 1 will be too big on many systems. Please use:
*ntargets + 1

> +    *ntargets = 0;
> 
> -    *targets = g_steal_pointer(&tmp_targets);
> -    *ntargets = tmp_ntargets;
> -    tmp_ntargets = 0;
> +    for (tmp_addr = addr; tmp_addr; tmp_addr = tmp_addr->next)
> +        *targets[(*ntargets)++] = g_strdup(tmp_addr->target_name);
> 

Re-using *ntargets makes this a bit harder to follow, consider using 'i' and only
filling *ntargets once.

Jano

> -    ret = 0;
> - cleanup:
>      iscsi_free_discovery_data(iscsi, addr);
> -    virStringListFreeCount(tmp_targets, tmp_ntargets);
> -    return ret;
> +
> +    return 0;
>  }
> 
>  static int
>