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

[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 <pkrempa at 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
>