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