On 06/04/13 03:29, John Ferlan wrote:
On 03/25/2013 12:43 PM, Osier Yang wrote:
startPool creates the vHBA if it's not existed yet, stopPool destroys s/it's not exists/it does not exist/
Okay.
the vHBA. Also to support autostart, checkPool will creates the vHBA s/creates/create
Okay.
if it's not existed yet. s/it's not existed/it does not exist
Okay.
--- src/storage/storage_backend_scsi.c | 64 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+)
diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index 258c82e..0bb4e70 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -646,6 +646,36 @@ getAdapterName(virStoragePoolSourceAdapter adapter) }
static int +createVport(virStoragePoolSourceAdapter adapter) +{ + unsigned int parent_host; + + if (adapter.type != VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) + return 0; Since the StopPool has the same check - why put this check in here instead of StartPool?
Sometimes you will want to put checking inside helper to avoid the caller incorrectly use it. Though the checking of this helper is simple enough to let the caller make mistake. But I'd keep it here.
+ + /* This filters either HBA or already created vHBA */ + if (virGetFCHostNameByWWN(NULL, adapter.data.fchost.wwnn, + adapter.data.fchost.wwpn)) + return 0; there's a memory leak here as the called function returns strdup() value. Of course this logic could be backwards too and what you meant was "if (!vir...)" probably changes the return value too...
Added the VIR_FREE.
I hope it goes without saying that the leak exists below too :-)
+ + if (!adapter.data.fchost.parent) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("'parent' for vHBA must be specified")); + return -1; + } + + if (getHostNumber(adapter.data.fchost.parent, &parent_host) < 0) + return -1; + + if (virManageVport(parent_host, adapter.data.fchost.wwnn, + adapter.data.fchost.wwpn, VPORT_CREATE) < 0) + return -1; + + virFileWaitForDevices(); + return 0; +} + +static int virStorageBackendSCSICheckPool(virConnectPtr conn ATTRIBUTE_UNUSED, virStoragePoolObjPtr pool, bool *isActive) @@ -707,10 +737,44 @@ out: return ret; }
+static int +virStorageBackendSCSIStartPool(virConnectPtr conn ATTRIBUTE_UNUSED, + virStoragePoolObjPtr pool) +{ + virStoragePoolSourceAdapter adapter = pool->def->source.adapter; + + return createVport(adapter); +} + +static int +virStorageBackendSCSIStopPool(virConnectPtr conn ATTRIBUTE_UNUSED, + virStoragePoolObjPtr pool) +{ + virStoragePoolSourceAdapter adapter = pool->def->source.adapter; + unsigned int parent_host; + + if (adapter.type != VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) + return 0; + + if (!(virGetFCHostNameByWWN(NULL, adapter.data.fchost.wwnn, + adapter.data.fchost.wwpn))) + return -1; + + if (getHostNumber(adapter.data.fchost.parent, &parent_host) < 0) + return -1; + + if (virManageVport(parent_host, adapter.data.fchost.wwnn, + adapter.data.fchost.wwpn, VPORT_DELETE) < 0) + return -1; + + return 0; "Consistently speaking" there could have been a deleteVport() or the "createVport()" is/was unnecessary.
I added a deleteVport.
+}
virStorageBackend virStorageBackendSCSI = { .type = VIR_STORAGE_POOL_SCSI,
.checkPool = virStorageBackendSCSICheckPool, .refreshPool = virStorageBackendSCSIRefreshPool, + .startPool = virStorageBackendSCSIStartPool, + .stopPool = virStorageBackendSCSIStopPool, };
I think this one needs a refactor and re-review.
The straightforward diff is attached, and I assume you will ACK it.