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

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

>[...] >>+ 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(a)redhat.com&gt; 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;