[PATCH 0/4] virscsihost: Couple of cleanups and fixes
*** BLURB HERE *** Michal Prívozník (4): virscsihost: use g_autofree more virSCSIHostFindByPCI: Decrease scope of some variables virscsihost: Drop needless labels virSCSIHostFindByPCI: Fix link detection src/util/virscsihost.c | 48 ++++++++++++++---------------------------- 1 file changed, 16 insertions(+), 32 deletions(-) -- 2.39.2
Remove some obvious uses of VIR_FREE() in favor of automatic cleanup. This also means, that some variables affected are brought into the inner most block, so that automatic cleanup is effective. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virscsihost.c | 24 ++++++------------------ 1 file changed, 6 insertions(+), 18 deletions(-) diff --git a/src/util/virscsihost.c b/src/util/virscsihost.c index 969cdd9f79..54ba9ddb9b 100644 --- a/src/util/virscsihost.c +++ b/src/util/virscsihost.c @@ -95,18 +95,19 @@ virSCSIHostFindByPCI(const char *sysfs_prefix, const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_SCSI_HOST_PATH; struct dirent *entry = NULL; g_autoptr(DIR) dir = NULL; - char *host_link = NULL; - char *host_path = NULL; char *p = NULL; char *ret = NULL; - char *buf = NULL; - char *unique_path = NULL; unsigned int read_unique_id; if (virDirOpen(&dir, prefix) < 0) return NULL; while (virDirRead(dir, &entry, prefix) > 0) { + g_autofree char *host_link = NULL; + g_autofree char *host_path = NULL; + g_autofree char *unique_path = NULL; + g_autofree char *buf = NULL; + if (!virFileIsLink(entry->d_name)) continue; @@ -116,17 +117,12 @@ virSCSIHostFindByPCI(const char *sysfs_prefix, goto cleanup; if (!strstr(host_path, parentaddr)) { - VIR_FREE(host_link); - VIR_FREE(host_path); continue; } - VIR_FREE(host_link); - VIR_FREE(host_path); unique_path = g_strdup_printf("%s/%s/unique_id", prefix, entry->d_name); if (!virFileExists(unique_path)) { - VIR_FREE(unique_path); continue; } @@ -139,10 +135,7 @@ virSCSIHostFindByPCI(const char *sysfs_prefix, if (virStrToLong_ui(buf, NULL, 10, &read_unique_id) < 0) goto cleanup; - VIR_FREE(buf); - if (read_unique_id != unique_id) { - VIR_FREE(unique_path); continue; } @@ -151,10 +144,6 @@ virSCSIHostFindByPCI(const char *sysfs_prefix, } cleanup: - VIR_FREE(unique_path); - VIR_FREE(host_link); - VIR_FREE(host_path); - VIR_FREE(buf); return ret; } @@ -226,7 +215,7 @@ virSCSIHostGetNameByParentaddr(unsigned int domain, unsigned int unique_id) { char *name = NULL; - char *parentaddr = NULL; + g_autofree char *parentaddr = NULL; parentaddr = g_strdup_printf("%04x:%02x:%02x.%01x", domain, bus, slot, function); @@ -239,7 +228,6 @@ virSCSIHostGetNameByParentaddr(unsigned int domain, } cleanup: - VIR_FREE(parentaddr); return name; } -- 2.39.2
Inside of virSCSIHostFindByPCI() there're some variables that are used from a while() loop exclusively. Bring their declaration into the loop. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virscsihost.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/util/virscsihost.c b/src/util/virscsihost.c index 54ba9ddb9b..c816b21f64 100644 --- a/src/util/virscsihost.c +++ b/src/util/virscsihost.c @@ -95,9 +95,7 @@ virSCSIHostFindByPCI(const char *sysfs_prefix, const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_SCSI_HOST_PATH; struct dirent *entry = NULL; g_autoptr(DIR) dir = NULL; - char *p = NULL; char *ret = NULL; - unsigned int read_unique_id; if (virDirOpen(&dir, prefix) < 0) return NULL; @@ -107,6 +105,8 @@ virSCSIHostFindByPCI(const char *sysfs_prefix, g_autofree char *host_path = NULL; g_autofree char *unique_path = NULL; g_autofree char *buf = NULL; + char *p = NULL; + unsigned int read_unique_id; if (!virFileIsLink(entry->d_name)) continue; -- 2.39.2
After previous cleanups, we're left with a couple of needless labels, that contain nothing but a return statement. Drop those. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virscsihost.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/src/util/virscsihost.c b/src/util/virscsihost.c index c816b21f64..014b96452c 100644 --- a/src/util/virscsihost.c +++ b/src/util/virscsihost.c @@ -95,7 +95,6 @@ virSCSIHostFindByPCI(const char *sysfs_prefix, const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_SCSI_HOST_PATH; struct dirent *entry = NULL; g_autoptr(DIR) dir = NULL; - char *ret = NULL; if (virDirOpen(&dir, prefix) < 0) return NULL; @@ -114,7 +113,7 @@ virSCSIHostFindByPCI(const char *sysfs_prefix, host_link = g_strdup_printf("%s/%s", prefix, entry->d_name); if (virFileResolveLink(host_link, &host_path) < 0) - goto cleanup; + return NULL; if (!strstr(host_path, parentaddr)) { continue; @@ -127,24 +126,22 @@ virSCSIHostFindByPCI(const char *sysfs_prefix, } if (virFileReadAll(unique_path, 1024, &buf) < 0) - goto cleanup; + return NULL; if ((p = strchr(buf, '\n'))) *p = '\0'; if (virStrToLong_ui(buf, NULL, 10, &read_unique_id) < 0) - goto cleanup; + return NULL; if (read_unique_id != unique_id) { continue; } - ret = g_strdup(entry->d_name); - break; + return g_strdup(entry->d_name); } - cleanup: - return ret; + return NULL; } @@ -224,10 +221,9 @@ virSCSIHostGetNameByParentaddr(unsigned int domain, _("Failed to find scsi_host using PCI '%s' " "and unique_id='%u'"), parentaddr, unique_id); - goto cleanup; + return NULL; } - cleanup: return name; } -- 2.39.2
Inside of virSCSIHostFindByPCI() there's a loop which iterates of entries of "/sys/class/scsi_host" directory trying to identify all symlinks (which then point to a SCSI device, but that's not important right now). But the way virFileIsLink() is called can never return a truthful reply - because it's called over dent->d_name instead of full path. Fix this by moving the virFileIsLink() call and passing constructed path into it. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virscsihost.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/util/virscsihost.c b/src/util/virscsihost.c index 014b96452c..32d7f2312f 100644 --- a/src/util/virscsihost.c +++ b/src/util/virscsihost.c @@ -107,11 +107,11 @@ virSCSIHostFindByPCI(const char *sysfs_prefix, char *p = NULL; unsigned int read_unique_id; - if (!virFileIsLink(entry->d_name)) - continue; - host_link = g_strdup_printf("%s/%s", prefix, entry->d_name); + if (!virFileIsLink(host_link)) + continue; + if (virFileResolveLink(host_link, &host_path) < 0) return NULL; -- 2.39.2
On a Wednesday in 2023, Michal Privoznik wrote:
Inside of virSCSIHostFindByPCI() there's a loop which iterates of entries of "/sys/class/scsi_host" directory trying to identify all symlinks (which then point to a SCSI device, but that's not important right now). But the way virFileIsLink() is called can never return a truthful reply - because it's called over dent->d_name instead of full path. Fix this by moving the virFileIsLink() call and passing constructed path into it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virscsihost.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/util/virscsihost.c b/src/util/virscsihost.c index 014b96452c..32d7f2312f 100644 --- a/src/util/virscsihost.c +++ b/src/util/virscsihost.c @@ -107,11 +107,11 @@ virSCSIHostFindByPCI(const char *sysfs_prefix, char *p = NULL; unsigned int read_unique_id;
- if (!virFileIsLink(entry->d_name)) - continue; - host_link = g_strdup_printf("%s/%s", prefix, entry->d_name);
+ if (!virFileIsLink(host_link)) + continue; +
Functionally speaking, this is just a micro-optimization. If you left it out completely, the ResloveLink would copy the path and we would move on to the next entry when strstr fails to find the parent address. In theory. In practice, /sys/class/scsi_host/ probably contains nothing else but symlinks. Feel free to drop the virFileIsLink call completely. Jano
if (virFileResolveLink(host_link, &host_path) < 0) return NULL;
-- 2.39.2
On a Wednesday in 2023, Michal Privoznik wrote:
*** BLURB HERE ***
Michal Prívozník (4): virscsihost: use g_autofree more virSCSIHostFindByPCI: Decrease scope of some variables virscsihost: Drop needless labels virSCSIHostFindByPCI: Fix link detection
src/util/virscsihost.c | 48 ++++++++++++++---------------------------- 1 file changed, 16 insertions(+), 32 deletions(-)
Regardless of what you choose in 4/4: Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Michal Privoznik