[libvirt] [PATCH 1/1] Add trigger of host rescan

[Daniel P. Berrange]

On Mon, Apr 06, 2009 at 11:41:48AM -0400, Dave Allan wrote:
> >> static int
> >>+virStorageBackendSCSIStartPool(virConnectPtr conn ATTRIBUTE_UNUSED,
> >>+                               virStoragePoolObjPtr pool 
> >>ATTRIBUTE_UNUSED)
> >>+{
> >>+    int retval = 0;
> >>+
> >>+    return retval;
> >>+}
> >>+
> >>+
> >>+static int
> >>+virStorageBackendSCSIStopPool(virConnectPtr conn ATTRIBUTE_UNUSED,
> >>+                              virStoragePoolObjPtr pool ATTRIBUTE_UNUSED)
> >>+{
> >>+    int retval = 0;
> >>+
> >>+    return retval;
> >>+}
> >
> >  Is that really better than suggesting the operation is not supported ?
> 
> These two functions are, of course, not required for this patch. 
> They're there because I had started to implement NPIV support and then 
> thought I should submit this patch before the NPIV patch.  I can take 
> them out if you'd like and add them to the subsequent patch where they 
> will have contents.

Yep, prefer to leave them out if they're unrelated to this patch.

> 
> >[...]
> >>+    if (virAsprintf(&path, "/sys/class/scsi_host/host%u/scan", host) < 
> >>0) {
> >>+        virReportOOMError(conn);
> >>+        retval = -1;
> >>+        goto out;
> >>+    }
> >>+
> >>+    VIR_DEBUG(_("Scan trigger path is '%s'"), path);
> >>+
> >>+    fd = open(path, O_WRONLY);
> >>+
> >>+    if (fd < 0) {
> >>+        virReportSystemError(conn, errno,
> >>+                             _("Could not open '%s' to trigger host 
> >>scan"),
> >>+                             path);
> >>+        retval = -1;
> >>+        goto cleanup;
> >>+    }
> >>+
> >>+    if (write(fd,
> >>+              LINUX_SYSFS_SCSI_HOST_SCAN_STRING,
> >>+              sizeof(LINUX_SYSFS_SCSI_HOST_SCAN_STRING)) < 0) {
> >>+
> >>+        virReportSystemError(conn, errno,
> >>+                             _("Write to '%s' to trigger host scan 
> >>failed"),
> >>+                             path);
> >>+        retval = -1;
> >>+        goto cleanup;
> >>+    }
> >>+
> >>+    goto out;
> >
> >  Seems to me that goto should be suppressed, it just generate a leak of 
> >  path
> >On the other hand fd is leaked for sure ... This really need some
> >double-checking ;-)
> 
> Ugh, sorry about that.  Fixed, thanks.
> 
> >>+cleanup:
> >>+    VIR_FREE(path);
> >>+
> >>+out:
> >>+    VIR_DEBUG(_("Rescan of host %d complete"), host);
> >>+    return retval;
> >>+}
> >
> >  Otherwise, sounds fine, as long as this doesn't generate a bus reset.
> 
> It doesn't generate a bus reset.  As I mentioned above, this code is not 
> IO disruptive.  I haven't decided on what I think is the right way to do 
> scans that are IO disruptive.
> 
> I've attached a patch with a fix for the leaks.  It also adds \n to the 
> scan string.
> 
> Dave
> 

> >From 7f2c35b22fbce4ee28d276d870a6eacdfd207c3f Mon Sep 17 00:00:00 2001
> From: David Allan <dallan at redhat.com>
> Date: Mon, 6 Apr 2009 11:16:03 -0400
> Subject: [PATCH 2/2] Fix bugs per DV
> 
> Removed goto causing fd and memory leak.
> 
> Added \n to scan string.
> ---
>  src/storage_backend_scsi.c |    9 +++------
>  1 files changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/src/storage_backend_scsi.c b/src/storage_backend_scsi.c
> index e30b3ce..bb945dc 100644
> --- a/src/storage_backend_scsi.c
> +++ b/src/storage_backend_scsi.c
> @@ -524,7 +524,7 @@ virStorageBackendSCSITriggerRescan(virConnectPtr conn,
>                               _("Could not open '%s' to trigger host scan"),
>                               path);
>          retval = -1;
> -        goto cleanup;
> +        goto free_path;
>      }
>  
>      if (write(fd,
> @@ -535,14 +535,11 @@ virStorageBackendSCSITriggerRescan(virConnectPtr conn,
>                               _("Write to '%s' to trigger host scan failed"),
>                               path);
>          retval = -1;
> -        goto cleanup;
>      }
>  
> -    goto out;
> -
> -cleanup:
> +    close(fd);
> +free_path:
>      VIR_FREE(path);
> -
>  out:
>      VIR_DEBUG(_("Rescan of host %d complete"), host);
>      return retval;

ACK

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|