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
