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@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 :|