Re: [PATCH v2 10/32] storage: Use VIR_AUTOPTR(virString)

[Ján Tomko <jtomko@xxxxxxxxxx>]

On Fri, Feb 08, 2019 at 01:37:04PM -0500, John Ferlan wrote:
> Let's make use of the auto __cleanup capabilities cleaning up any
> now unnecessary goto paths.
>
> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
> Reviewed-by: Erik Skultety <eskultet@xxxxxxxxxx>
> ---
> src/storage/storage_backend_iscsi_direct.c |  3 +-
> src/storage/storage_backend_sheepdog.c     |  6 +--
> src/storage/storage_backend_zfs.c          | 15 ++----
> src/util/virstoragefile.c                  | 58 ++++++++--------------
> 4 files changed, 28 insertions(+), 54 deletions(-)
>
> diff --git a/src/storage/storage_backend_iscsi_direct.c b/src/storage/storage_backend_iscsi_direct.c
> index 42060dd758..cf48c29cde 100644
> --- a/src/storage/storage_backend_iscsi_direct.c
> +++ b/src/storage/storage_backend_iscsi_direct.c
> @@ -411,7 +411,7 @@ virISCSIDirectUpdateTargets(struct iscsi_context *iscsi,
>     struct iscsi_discovery_address *addr;
>     struct iscsi_discovery_address *tmp_addr;
>     size_t tmp_ntargets = 0;
> -    char **tmp_targets = NULL;
> +    VIR_AUTOPTR(virString) tmp_targets = NULL;
>
>     if (!(addr = iscsi_discovery_sync(iscsi))) {
>         virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -439,7 +439,6 @@ virISCSIDirectUpdateTargets(struct iscsi_context *iscsi,
>     ret = 0;
>  cleanup:
>     iscsi_free_discovery_data(iscsi, addr);
> -    virStringListFreeCount(tmp_targets, tmp_ntargets);
>     return ret;
> }

virstring.h says:
VIR_DEFINE_AUTOPTR_FUNC(virString, virStringListFree);

Using virStringListFree instead of virStringListFreeCount might access
elements outside of the array, so this hunk should be dropped.

> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index 1eb1ede59d..fc26c2f22e 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -1594,11 +1594,10 @@ virStorageFileParseBackingStoreStr(const char *str,
>                                    char **target,
>                                    unsigned int *chainIndex)
> {
> -    char **strings = NULL;
>     size_t nstrings;
>     unsigned int idx = 0;
>     char *suffix;
> -    int ret = -1;
> +    VIR_AUTOPTR(virString) strings = NULL;
>
>     *chainIndex = 0;
>
> @@ -1608,19 +1607,15 @@ virStorageFileParseBackingStoreStr(const char *str,
>     if (nstrings == 2) {
>         if (virStrToLong_uip(strings[1], &suffix, 10, &idx) < 0 ||
>             STRNEQ(suffix, "]"))
> -            goto cleanup;
> +            return -1;
>     }
>
>     if (target &&
>         VIR_STRDUP(*target, strings[0]) < 0)
> -        goto cleanup;
> +        return -1;
>
>     *chainIndex = idx;
> -    ret = 0;
> -
> - cleanup:
> -    virStringListFreeCount(strings, nstrings);
> -    return ret;
> +    return 0;
> }

It's OK here, virStringSplitCount returns a NULL-terminated array of
strings.

>

Reviewed-by: Ján Tomko <jtomko@xxxxxxxxxx>

Jano

Attachment: signature.asc
Description: PGP signature