Re: [libvirt] [PATCH 02/18] Fix error detection in virStorageBackendISCSIGetHostNumber

On 06/21/2016 12:05 PM, Ján Tomko wrote: > In the unlikely case the iSCSI session path exists, but does not > contain an entry starting with "target", we would silently use > an initialized value. > > Rewrite the function to correctly report errors. > --- > src/storage/storage_backend_iscsi.c | 38 +++++++++++++++++++------------------ > 1 file changed, 20 insertions(+), 18 deletions(-) > > diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c > index bccfba3..876c14c 100644 > --- a/src/storage/storage_backend_iscsi.c > +++ b/src/storage/storage_backend_iscsi.c > @@ -90,7 +90,7 @@ static int > virStorageBackendISCSIGetHostNumber(const char *sysfs_path, > uint32_t *host) > { > - int retval = 0; > + int ret = -1; > DIR *sysdir = NULL; > struct dirent *dirent = NULL; > int direrr; > @@ -104,26 +104,33 @@ virStorageBackendISCSIGetHostNumber(const char *sysfs_path, > if (sysdir == NULL) { > virReportSystemError(errno, > _("Failed to opendir path '%s'"), sysfs_path); > - retval = -1; > - goto out; > + goto cleanup; > } > > while ((direrr = virDirRead(sysdir, &dirent, sysfs_path)) > 0) { > if (STREQLEN(dirent->d_name, "target", strlen("target"))) { While we're here and changing, could use STRPREFIX

> - if (sscanf(dirent->d_name, > - "target%u:", host) != 1) { > - VIR_DEBUG("Failed to parse target '%s'", dirent->d_name); > - retval = -1; > - break; > + if (sscanf(dirent->d_name, "target%u:", host) == 1) { > + ret = 0; > + goto cleanup; > + } else { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("Failed to parse target '%s'"), dirent->d_name); > + goto cleanup; > } > } > } > - if (direrr < 0) > - retval = -1; > > + if (direrr == 0) { Should this be <= ? If virDirRead() returns -1, then we don't return with an error...

If virDirRead() returns 0, then we've gone through the entire directory without finding an entry that starts with target - seems that would be a system configuration error and I assume errno would be ENOENT

ACK with obvious adjustments - John