8:33 a.m.
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(a)redhat.com>
> ---
> 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