[libvirt] [REPOST PATCHv2 2/6] conf: Convert virStoragePoolSourceAdapter to virStorageAdapter

[John Ferlan]

On 03/09/2017 10:35 AM, Ján Tomko wrote:
> On Sat, Mar 04, 2017 at 10:52:08AM -0500, John Ferlan wrote:
>> Introduce src/conf/storage_adapter_conf.{c,h}. Separate out the <adapter>
>> parsing/formatting and management into a separate conf module.
>>
>> Move the virStoragePoolSourceAdapter from storage_conf.h and rename
>> to virStorageAdapter. This includes creating 'scsi_host' and 'fchost'
>> specific structs as virStorageAdapterSCSIHost and virStorageAdapterFCHost
>> which allows code using the structs to use the typedefs rather than
>> needing to take the lengthy union paths.
>>
>> Modify code in the various places which formerly used the pool structure
>> to use the new model. This includes some changes that will use the Ptr
>> rather than just the struct (try to shorten the number of times one
>> has to type adapter.data.fchost or adapter.data.scsi_host as well as
>> [pool->]def->source.adapter.
> 
> This patch would be much easier to read if it did not do so many things
> at once. One patch moving the code (with renaming the functions to match
> the new location) and other patches changing the lengthy unions and
> arguments would look much nicer.
> 

<sigh> OK... this was one of those that I just go going and wasn't
careful to stop at particular steps.  Things compounded fairly quickly.

In any case, in a be careful for what you wish type comment - I've done
a split of patch2 and now there's 12 patches to replace it.  Of course
that leads me down the path of well there's too many patches to review
so it'll languish on list... Oh well, it's a small price to pay to make
progress I guess.

>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>> po/POTFILES.in                     |   1 +
>> src/Makefile.am                    |   1 +
>> src/conf/storage_adapter_conf.c    | 291 ++++++++++++++++++++++++++++++++
>> src/conf/storage_adapter_conf.h    |  89 ++++++++++
>> src/conf/storage_conf.c            | 333
>> +++++++++----------------------------
>> src/conf/storage_conf.h            |  35 +---
>> src/libvirt_private.syms           |  16 +-
>> src/phyp/phyp_driver.c             |   3 +-
>> src/storage/storage_backend_scsi.c | 129 +++++++-------
>> src/test/test_driver.c             |   5 +-
>> 10 files changed, 538 insertions(+), 365 deletions(-)
>> create mode 100644 src/conf/storage_adapter_conf.c
>> create mode 100644 src/conf/storage_adapter_conf.h
>>
>> diff --git a/po/POTFILES.in b/po/POTFILES.in
>> index 7c7f530..0320918 100644
>> --- a/po/POTFILES.in
>> +++ b/po/POTFILES.in
>> @@ -40,6 +40,7 @@ src/conf/object_event.c
>> src/conf/secret_conf.c
>> src/conf/snapshot_conf.c
>> src/conf/storage_conf.c
>> +src/conf/storage_adapter_conf.c
>> src/conf/virchrdev.c
>> src/conf/virdomainobjlist.c
>> src/conf/virnodedeviceobj.c
>> diff --git a/src/Makefile.am b/src/Makefile.am
>> index 7d42eac..74bcab3 100644
>> +void
>> +virStorageAdapterVHBAClear(virStorageAdapterFCHostPtr fchost)
>> +{
>> +    VIR_FREE(fchost->parent);
>> +    VIR_FREE(fchost->parent_wwnn);
>> +    VIR_FREE(fchost->parent_wwpn);
>> +    VIR_FREE(fchost->parent_fabric_wwn);
>> +    VIR_FREE(fchost->wwnn);
>> +    VIR_FREE(fchost->wwpn);
>> +}
>> +
>> +
> 
> 
>> +void
>> +virStorageAdapterClear(virStorageAdapterPtr adapter)
>> +{
> 
> The split of AdapterClear into two function is not present in the
> original file.
> 
>> +    if (adapter->type == VIR_STORAGE_ADAPTER_TYPE_FC_HOST)
>> +        virStorageAdapterVHBAClear(&adapter->data.fchost);
>> +
>> +    if (adapter->type == VIR_STORAGE_ADAPTER_TYPE_SCSI_HOST)
>> +        VIR_FREE(adapter->data.scsi_host.name);
>> +}
>> +
>> +
>> +int
>> +virStorageAdapterVHBAParseValidate(virStorageAdapterFCHostPtr fchost)
>> +{
>> +    if (!fchost->wwnn || !fchost->wwpn) {
>> +        virReportError(VIR_ERR_XML_ERROR, "%s",
>> +                       _("'wwnn' and 'wwpn' must be specified for "
>> +                         "a VHBA adapter"));
>> +        return -1;
>> +    }
>> +
>> +    if (!virValidateWWN(fchost->wwnn) || !virValidateWWN(fchost->wwpn))
>> +        return -1;
>> +
>> +    if (fchost->parent_wwnn && !virValidateWWN(fchost->parent_wwnn))
>> +        return -1;
>> +
>> +    if (fchost->parent_wwpn && !virValidateWWN(fchost->parent_wwpn))
>> +        return -1;
>> +
> 
>> +    if (fchost->parent_fabric_wwn &&
>> +        !virValidateWWN(fchost->parent_fabric_wwn))
>> +        return -1;
> 
> Validation of parent_fabric_wwn also seems new.
> 


Hmm... True...  Neither was parent_wwnn/parent_wwpn validation.  See
what I mean about compounding things!  It certainly became more obvious
the deeper I went.

In any case, I split this change out into a separate patch that goes
before all the patches that will perform the step by step split.  I also
added one more from a bz that I have in which a tester only supplied one
of parent_wwnn/parent_wwpn...

Just putting the finishing touches on all of them before posting...


Tks -

John
>> +
>> +    return 0;
>> +}
>> +
>> +
> 
> Jan