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

[Osier Yang]

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.

-------------- next part --------------
diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c
index fb7aef1..d72c153 100644
--- a/src/storage/storage_backend_scsi.c
+++ b/src/storage/storage_backend_scsi.c
@@ -648,14 +648,17 @@ static int
 createVport(virStoragePoolSourceAdapter adapter)
 {
     unsigned int parent_host;
+    char *name = NULL;
 
     if (adapter.type != VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST)
         return 0;
 
     /* This filters either HBA or already created vHBA */
-    if (virGetFCHostNameByWWN(NULL, adapter.data.fchost.wwnn,
-                              adapter.data.fchost.wwpn))
+    if ((name = virGetFCHostNameByWWN(NULL, adapter.data.fchost.wwnn,
+                                      adapter.data.fchost.wwpn))) {
+        VIR_FREE(name);
         return 0;
+    }
 
     if (!adapter.data.fchost.parent) {
         virReportError(VIR_ERR_XML_ERROR, "%s",
@@ -675,6 +678,29 @@ createVport(virStoragePoolSourceAdapter adapter)
 }
 
 static int
+deleteVport(virStoragePoolSourceAdapter 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;
+}
+
+
+static int
 virStorageBackendSCSICheckPool(virConnectPtr conn ATTRIBUTE_UNUSED,
                                virStoragePoolObjPtr pool,
                                bool *isActive)
@@ -741,7 +767,6 @@ virStorageBackendSCSIStartPool(virConnectPtr conn ATTRIBUTE_UNUSED,
                                virStoragePoolObjPtr pool)
 {
     virStoragePoolSourceAdapter adapter = pool->def->source.adapter;
-
     return createVport(adapter);
 }
 
@@ -750,23 +775,7 @@ 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;
+    return deleteVport(adapter);
 }
 
 virStorageBackend virStorageBackendSCSI = {