Re: [PATCH v3 14/18] conf: Convert virStoragePoolSourceAdapter to virStorageAdapter

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 03/12/2017 05:53 PM, Laine Stump wrote:
> On 03/10/2017 04:10 PM, John Ferlan wrote:
>> Move the virStoragePoolSourceAdapter from storage_conf.h and rename
>> to virStorageAdapter.
>>
>> Continue with code realignment for brevity and flow.
>>
>> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
>> ---
>>  src/conf/storage_adapter_conf.c    | 71 ++++++++++++++++++--------------------
>>  src/conf/storage_adapter_conf.h    | 51 ++++++++++++++++++++++++---
>>  src/conf/storage_conf.c            | 32 ++++++++---------
>>  src/conf/storage_conf.h            | 44 ++---------------------
>>  src/libvirt_private.syms           |  2 --
>>  src/phyp/phyp_driver.c             |  3 +-
>>  src/storage/storage_backend_scsi.c | 18 +++++-----
>>  src/test/test_driver.c             |  5 ++-
>>  8 files changed, 109 insertions(+), 117 deletions(-)
>>
>> diff --git a/src/conf/storage_adapter_conf.c b/src/conf/storage_adapter_conf.c
>> index 6efe5ae..53c07c7 100644
>> --- a/src/conf/storage_adapter_conf.c
>> +++ b/src/conf/storage_adapter_conf.c
>> @@ -19,7 +19,7 @@

[...]

>>  int
>> -virStorageAdapterParseValidate(virStoragePoolDefPtr ret)
>> +virStorageAdapterParseValidate(virStorageAdapterPtr adapter)
>>  {
>> -    if (!ret->source.adapter.type) {
>> +    if (!adapter->type) {
>>          virReportError(VIR_ERR_XML_ERROR, "%s",
>>                         _("missing storage pool source adapter"));
>>          return -1;
>>      }
>>  
>> -    if (ret->source.adapter.type ==
>> -        VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST)
>> -        return virStorageAdapterFCHostParseValidate(&ret->source.adapter.data.fchost);
>> +    if (adapter->type == VIR_STORAGE_ADAPTER_TYPE_FC_HOST)
>> +        return virStorageAdapterFCHostParseValidate(&adapter->data.fchost);
>>  
>> -    if (ret->source.adapter.type ==
>> -        VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST)
>> -        return virStorageAdapterSCSIHostParseValidate(&ret->source.adapter.data.scsi_host);
>> +    if (adapter->type == VIR_STORAGE_ADAPTER_TYPE_SCSI_HOST)
>> +        return virStorageAdapterSCSIHostParseValidate(&adapter->data.scsi_host);
>>  
>>      return 0;
>>  }
>> @@ -285,13 +280,13 @@ virStorageAdapterFCHostFormat(virBufferPtr buf,
>>                                virStorageAdapterFCHostPtr fchost)
>>  {
>>      virBufferEscapeString(buf, " parent='%s'", fchost->parent);
>> -    if (fchost->managed)
>> -        virBufferAsprintf(buf, " managed='%s'",
>> -                          virTristateBoolTypeToString(fchost->managed));
>>      virBufferEscapeString(buf, " parent_wwnn='%s'", fchost->parent_wwnn);
>>      virBufferEscapeString(buf, " parent_wwpn='%s'", fchost->parent_wwpn);
>>      virBufferEscapeString(buf, " parent_fabric_wwn='%s'",
>>                            fchost->parent_fabric_wwn);
>> +    if (fchost->managed != VIR_TRISTATE_BOOL_ABSENT)
>> +        virBufferAsprintf(buf, " managed='%s'",
>> +                          virTristateBoolTypeToString(fchost->managed));
> 
> No test cases that are tripped up by this change in order? (Not saying there need to be, just wondering...)
> 

No - the managed would probably only appear as "managed='no'", although
managed='yes' is the de facto default and essentially determines whether
or not to destroy the vHBA when the pool is destroyed.

I can move it if really desired/required - I guess I saw it as something
"after" defining parent, parent_wwnn/wwpn, or parent_fabric_wwn - when
adding those I should have put them after parent.  Yeah, I know could
have been a separate patch.

>>  
>>      virBufferAsprintf(buf, " wwnn='%s' wwpn='%s'/>\n",
>>                        fchost->wwnn, fchost->wwpn);
>> @@ -322,14 +317,14 @@ virStorageAdapterSCSIHostFormat(virBufferPtr buf,
>>  

[...]

>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index 6a2bdf2..8a9e71b 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -883,8 +883,6 @@ virStoragePoolObjSaveDef;
>>  virStoragePoolObjUnlock;
>>  virStoragePoolSaveConfig;
>>  virStoragePoolSaveState;
>> -virStoragePoolSourceAdapterTypeFromString;
>> -virStoragePoolSourceAdapterTypeToString;
> 
> These are no longer used globally? (Just making sure)
> 

Nope - replaced by virStorageAdapterType{From|To}String which are only
local to storage_adapter_conf.c... That's the VIR_ENUM_IMPL change.

> 
>>  virStoragePoolSourceClear;
>>  virStoragePoolSourceDeviceClear;
>>  virStoragePoolSourceFindDuplicate;

[...]

>> diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c
>> index 77a51ff..ff17409 100644
>> --- a/src/storage/storage_backend_scsi.c
>> +++ b/src/storage/storage_backend_scsi.c
>> @@ -176,12 +176,12 @@ virStoragePoolFCRefreshThread(void *opaque)
>>  }
>>  
>>  static char *
>> -getAdapterName(virStoragePoolSourceAdapterPtr adapter)
>> +getAdapterName(virStorageAdapterPtr adapter)
>>  {
>>      char *name = NULL;
>>      char *parentaddr = NULL;
>>  
>> -    if (adapter->type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) {
>> +    if (adapter->type == VIR_STORAGE_ADAPTER_TYPE_SCSI_HOST) {
>>          virStorageAdapterSCSIHostPtr scsi_host = &adapter->data.scsi_host;
>>  
>>          if (scsi_host->has_parent) {
>> @@ -197,7 +197,9 @@ getAdapterName(virStoragePoolSourceAdapterPtr adapter)
>>          } else {
>>              ignore_value(VIR_STRDUP(name, scsi_host->name));
>>          }
>> -    } else if (adapter->type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) {
>> +    }
>> +
>> +    if (adapter->type == VIR_STORAGE_ADAPTER_TYPE_FC_HOST) {
> 
> If you're just getting rid of the "} else" in order to shorten the line, then I'd say leave it in...
> 

OK

> 
>>          virStorageAdapterFCHostPtr fchost = &adapter->data.fchost;
>>  
>>          if (!(name = virVHBAGetHostByWWN(NULL, fchost->wwnn, fchost->wwpn))) {
>> @@ -451,7 +453,7 @@ virStorageBackendSCSICheckPool(virStoragePoolObjPtr pool,
>>           * the adapter based on might be not created yet.
>>           */
>>          if (pool->def->source.adapter.type ==
>> -            VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) {
>> +            VIR_STORAGE_ADAPTER_TYPE_FC_HOST) {
>>              virResetLastError();
>>              return 0;
>>          } else {
>> @@ -505,24 +507,24 @@ virStorageBackendSCSIRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED,
>>      return ret;
>>  }
>>  

[...]

> 
> 
> ACK.
> 

Before I go off and push all this - just want to double check if you
desire to see the adjustments in a v4 series or do you feel comfortable
enough with a "summary"?

1. The virStorageAdapterPtr and friends had no changes to their names.

2. The function/helper names are now:

virStorageAdapterClearFCHost
virStorageAdapterClear
virStorageAdapterParseXMLFCHost
virStorageAdapterParseXMLSCSIHost
virStorageAdapterParseXMLLegacy  **
virStorageAdapterParseXML
virStorageAdapterValidateFCHost
virStorageAdapterValidateSCSIHost
virStorageAdapterValidate
virStorageAdapterFormatFCHost
virStorageAdapterFormatSCSIHost
virStorageAdapterFormat

** No such thing as a short comment ;-)

3. In patch1 rather than the single if statement - there are now two.
The end result is the following check:

    if ((fchost->parent_wwnn && !fchost->parent_wwpn)) {
        virReportError(VIR_ERR_XML_ERROR,
                       _("when providing parent_wwnn='%s', the "
                         "parent_wwpn must also be provided"),
                       fchost->parent_wwnn);
        return -1;
    }

    if (!fchost->parent_wwnn && fchost->parent_wwpn) {
        virReportError(VIR_ERR_XML_ERROR,
                       _("when providing parent_wwpn='%s', the "
                         "parent_wwnn must also be provided"),
                       fchost->parent_wwpn);
        return -1;
    }

which I'll replicate in a separately posted patch for node_device_conf

4. Using virPCIDeviceAddressPtr in getSCSIHostNumber and getAdapterName.
Ironically the "original" series I had passed along the
virStorageAdapterSCSIHostPtr, but since it's been decreed that a
src/util function cannot include a src/conf header, I had to back that off.


Tks -

John

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux