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

[John Ferlan]

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

> +        virReportSystemError(errno,
> +                             _("Failed to get host number for iSCSI session "
> +                               "with path '%s'"),
> +                             sysfs_path);
> +        goto cleanup;
> +    }
> +
> + cleanup:
>      closedir(sysdir);
> - out:
> -    return retval;
> +    return ret;
>  }
>  
>  static int
> @@ -138,13 +145,8 @@ virStorageBackendISCSIFindLUs(virStoragePoolObjPtr pool,
>                      "/sys/class/iscsi_session/session%s/device", session) < 0)
>          goto cleanup;
>  
> -    if (virStorageBackendISCSIGetHostNumber(sysfs_path, &host) < 0) {
> -        virReportSystemError(errno,
> -                             _("Failed to get host number for iSCSI session "
> -                               "with path '%s'"),
> -                             sysfs_path);
> +    if (virStorageBackendISCSIGetHostNumber(sysfs_path, &host) < 0)
>          goto cleanup;
> -    }
>  
>      if (virStorageBackendSCSIFindLUs(pool, host) < 0)
>          goto cleanup;
>