[libvirt] [PATCH 1/9] storage: Refactor iSCSI Source matching

[John Ferlan]

On 04/13/2015 05:27 AM, Peter Krempa wrote:
> On Thu, Apr 02, 2015 at 13:39:38 -0400, John Ferlan wrote:
>> Create a separate iSCSI Source matching subroutine. Makes the calling
>> code a bit cleaner as well as sets up for future patches which need to
>> do better source hosts[0].name processing/checking
>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>>  src/conf/storage_conf.c | 35 ++++++++++++++++++++++++-----------
>>  1 file changed, 24 insertions(+), 11 deletions(-)
>>
>> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
>> index 8b1898b..4a38416 100644
>> --- a/src/conf/storage_conf.c
>> +++ b/src/conf/storage_conf.c
>> @@ -2291,6 +2291,28 @@ matchSCSIAdapterParent(virStoragePoolObjPtr pool,
>>  }
>>  
>>  
>> +static bool
>> +matchISCSISource(virStoragePoolObjPtr matchpool,
> 
> Please use the virStorageConf... prefix for the new function.
> 

How about "virStoragePoolSourceISCSIMatch"


>> +                 virStoragePoolDefPtr def)
>> +{
>> +    if (matchpool->def->source.nhost == 1 && def->source.nhost == 1) {
>> +        if (STREQ(matchpool->def->source.hosts[0].name,
>> +                  def->source.hosts[0].name)) {
>> +            if ((matchpool->def->source.initiator.iqn) &&
>> +                (def->source.initiator.iqn)) {
>> +                if (STREQ(matchpool->def->source.initiator.iqn,
>> +                          def->source.initiator.iqn))
>> +                    return true;
>> +                else
>> +                    return false;
> 
> Um, how about return STREQ(... ?

myopia, but in the long run it won't matter as I agree with your view to
merge patches 1-3 (something I probably thought along the way but didn't
type as I was trying to show the transformation for at least the review)

John
> 
>> +            }
>> +            return true;
>> +        }
>> +    }
>> +    return false;
>> +}
>> +
>> +
>>  int
>>  virStoragePoolSourceFindDuplicate(virConnectPtr conn,
>>                                    virStoragePoolObjListPtr pools,
>> @@ -2390,17 +2412,8 @@ virStoragePoolSourceFindDuplicate(virConnectPtr conn,
>>          case VIR_STORAGE_POOL_ISCSI:
>>              matchpool = virStoragePoolSourceFindDuplicateDevices(pool, def);
>>              if (matchpool) {
>> -                if (matchpool->def->source.nhost == 1 && def->source.nhost == 1) {
>> -                    if (STREQ(matchpool->def->source.hosts[0].name, def->source.hosts[0].name)) {
>> -                        if ((matchpool->def->source.initiator.iqn) && (def->source.initiator.iqn)) {
>> -                            if (STREQ(matchpool->def->source.initiator.iqn, def->source.initiator.iqn))
>> -                                break;
>> -                            matchpool = NULL;
>> -                        }
>> -                        break;
>> -                    }
>> -                }
>> -                matchpool = NULL;
>> +                if (!matchISCSISource(matchpool, def))
>> +                    matchpool = NULL;
>>              }
>>              break;
>>          case VIR_STORAGE_POOL_FS:
> 
> ACK if you rename the function and remove the redundant if.
> 
> Peter
>