Re: [PATCH 3/4] virISCSIDirectUpdateTargets: Rework to simplify cleanup and return GStrv

2021-06-18 Thread Jano Tomko
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 
> ---
>  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(_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
> 



[PATCH 3/4] virISCSIDirectUpdateTargets: Rework to simplify cleanup and return GStrv

2021-06-18 Thread Peter Krempa
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 
---
 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(_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