On Thu, Jun 23, 2016 at 08:26:06AM -0400, John Ferlan wrote:


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


I'll send a separate patch for that.

-            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...


virDirRead does report errors.

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


Actually, errno would probably be 0. I'll fix the error in v2.

Jan

ACK with obvious adjustments -

John

--
libvir-list mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/libvir-list

Reply via email to