Re: [libvirt] [PATCH 6/7] storage: Add startPool and stopPool for scsi backend

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/

> the vHBA. Also to support autostart, checkPool will creates the vHBA s/creates/create

> if it's not existed yet. s/it's not existed/it does not exist

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

> + > + /* 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...

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.

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