[libvirt] [PATCH 07/15] storage: Use VIR_AUTOFREE for storage backends

[Erik Skultety]

On Fri, Feb 08, 2019 at 10:06:07AM -0500, John Ferlan wrote:
> [...]
>
> >> diff --git a/src/storage/storage_backend_iscsi_direct.c b/src/storage/storage_backend_iscsi_direct.c
> >> index 82fa4d7a25..6458b0f835 100644
> >> --- a/src/storage/storage_backend_iscsi_direct.c
> >> +++ b/src/storage/storage_backend_iscsi_direct.c
> >> @@ -421,15 +421,14 @@ virISCSIDirectUpdateTargets(struct iscsi_context *iscsi,
> >>      }
> >>
> >>      for (tmp_addr = addr; tmp_addr; tmp_addr = tmp_addr->next) {
> >> -        char *target = NULL;
> >> +        VIR_AUTOFREE(char *) target = NULL;
> >>
> >>          if (VIR_STRDUP(target, tmp_addr->target_name) < 0)
> >>              goto cleanup;
> >>
> >> -        if (VIR_APPEND_ELEMENT(tmp_targets, tmp_ntargets, target) < 0) {
> >> -            VIR_FREE(target);
> >> +        if (VIR_APPEND_ELEMENT(tmp_targets, tmp_ntargets, target) < 0)
> >>              goto cleanup;
> >> -        }
> >> +        target = NULL;
> >
> > VIR_APPEND_ELEMENT clears the source, so ^this should not be needed.
> >
> > [snip]
> >
>
> Right - just rote habit I guess...
>
> >> -    dev->path = pvname;
> >> +    VIR_STEAL_PTR(dev->path, pvname);
> >
> > This VIR_STEAL_PTR stuff should come separe as mentioned in previous reviews.
> >
>
> For this one I disagree...  similar to the @vgname, previously we
> "failed" through the error: label and returned -1, but with the AUTO
> stuff added, we now cannot leave @vgname and @pvname as NULL.

Why does it need to stay? The outcome (same patch in v2) after this patch is
that vgname is NULL so it's "leaked" within thisSource->name and -1 is returned
on error, I don't see how that's different if you only do the VIR_STEAL_PTR
stuff in a separate patch, you still return -1 and vgname will be NULL even
before it hits the error label;

Erik