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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]