Re: [libvirt] [PATCH v2 12/32] storage: Use VIR_AUTOFREE for storage backends

On 2/11/19 7:52 AM, Ján Tomko wrote: > On Fri, Feb 08, 2019 at 01:37:06PM -0500, John Ferlan wrote: >> Let's make use of the auto __cleanup capabilities. This also allows >> for the cleanup of some goto paths. >> >> Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; >> --- >> src/storage/storage_backend.c              |  9 +-- >> src/storage/storage_backend_disk.c         | 62 ++++++----------- >> src/storage/storage_backend_fs.c           | 17 ++--- >> src/storage/storage_backend_gluster.c      | 30 +++----- >> src/storage/storage_backend_iscsi.c        | 73 +++++++------------- >> src/storage/storage_backend_iscsi_direct.c | 36 ++++------ >> src/storage/storage_backend_logical.c      | 35 +++------- >> src/storage/storage_backend_mpath.c        | 18 ++--- >> src/storage/storage_backend_rbd.c          | 35 +++------- >> src/storage/storage_backend_scsi.c         | 79 ++++++++-------------- >> src/storage/storage_backend_sheepdog.c     | 27 +++----- >> src/storage/storage_backend_vstorage.c     | 25 +++---- >> src/storage/storage_backend_zfs.c          | 15 ++-- >> src/storage/storage_file_gluster.c         | 16 ++--- >> 14 files changed, 158 insertions(+), 319 deletions(-) >> [...] >> diff --git a/src/storage/storage_backend_scsi.c >> b/src/storage/storage_backend_scsi.c >> index 14f01f9ec0..7460349c81 100644 >> --- a/src/storage/storage_backend_scsi.c >> +++ b/src/storage/storage_backend_scsi.c >> @@ -56,16 +56,14 @@ static int >> virStorageBackendSCSITriggerRescan(uint32_t host) >> { >>     int fd = -1; >> -    int retval = 0; >> -    char *path; >> +    int retval = -1; > > This inverts the logic of the function > >> +    VIR_AUTOFREE(char *) path = NULL; >> >>     VIR_DEBUG("Triggering rescan of host %d", host); >> >>     if (virAsprintf(&path, "%s/host%u/scan", >> -                    LINUX_SYSFS_SCSI_HOST_PREFIX, host) < 0) { >> -        retval = -1; >> -        goto out; >> -    } >> +                    LINUX_SYSFS_SCSI_HOST_PREFIX, host) < 0) >> +        return -1; >> >>     VIR_DEBUG("Scan trigger path is '%s'", path); >> >> @@ -75,8 +73,7 @@ virStorageBackendSCSITriggerRescan(uint32_t host) >>         virReportSystemError(errno, >>                              _("Could not open '%s' to trigger host >> scan"), >>                              path); > >> -        retval = -1; >> -        goto free_path; >> +        goto cleanup; > > Unrelated rename. (There's no jump to 'cleanup' with fd != -1) > >>     } >> >>     if (safewrite(fd, >> @@ -86,13 +83,12 @@ virStorageBackendSCSITriggerRescan(uint32_t host) >>         virReportSystemError(errno, >>                              _("Write to '%s' to trigger host scan >> failed"), >>                              path); >> -        retval = -1; > > Before, this returned -1, now it will return 0. > >>     } >> >> +    retval = 0; >> + >> + cleanup: >>     VIR_FORCE_CLOSE(fd); >> - free_path: >> -    VIR_FREE(path); >> - out: >>     VIR_DEBUG("Rescan of host %d complete", host); >>     return retval; >> } > So two pre-patches are attached with any luck...

John

From 46e3b6fe94d68220c52e23e359d5b43a3f5cfbba Mon Sep 17 00:00:00 2001 From: John Ferlan <jferlan(a)redhat.com&gt; Date: Mon, 11 Feb 2019 21:48:53 -0500 Subject: [PATCH 2/2] storage: Invert retval logic in virStorageBackendSCSITriggerRescan Rather than initialize to 0 and change to -1 on error, let's do the normal operation of initializing to -1 and set to 0 on success. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/storage/storage_backend_scsi.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index 85a177865f..591bcb04e2 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -56,16 +56,14 @@ static int virStorageBackendSCSITriggerRescan(uint32_t host) { int fd = -1; - int retval = 0; + int retval = -1; char *path = NULL; VIR_DEBUG("Triggering rescan of host %d", host); if (virAsprintf(&path, "%s/host%u/scan", - LINUX_SYSFS_SCSI_HOST_PREFIX, host) < 0) { - retval = -1; + LINUX_SYSFS_SCSI_HOST_PREFIX, host) < 0) goto cleanup; - } VIR_DEBUG("Scan trigger path is '%s'", path); @@ -75,7 +73,6 @@ virStorageBackendSCSITriggerRescan(uint32_t host) virReportSystemError(errno, _("Could not open '%s' to trigger host scan"), path); - retval = -1; goto cleanup; } @@ -85,9 +82,11 @@ virStorageBackendSCSITriggerRescan(uint32_t host) virReportSystemError(errno, _("Write to '%s' to trigger host scan failed"), path); - retval = -1; + goto cleanup; } + retval = 0; + cleanup: VIR_FORCE_CLOSE(fd); VIR_FREE(path); -- 2.20.1

From e7e2aa6a7fb00a1207e9a5a52596fb4ec3ffce8b Mon Sep 17 00:00:00 2001 From: John Ferlan <jferlan(a)redhat.com&gt; Date: Mon, 11 Feb 2019 21:46:28 -0500 Subject: [PATCH 1/2] src: Fix label logic in virStorageBackendSCSITriggerRescan Let's initialize @path to NULL, then rather than use two labels free_path and out labels, let's use the cleanup: label to call VIR_FREE(path); and VIR_FORCE_CLOSE(fd); Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/storage/storage_backend_scsi.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)