[libvirt] [PATCH 7/8] interface: Clean up virInterfaceObjListFindByMACString

[John Ferlan]

On 05/19/2017 11:29 AM, Michal Privoznik wrote:
> On 04/26/2017 12:36 AM, John Ferlan wrote:
>> Alter the algorithm to return a list of matching names rather than a
>> list of match virInterfaceObjPtr which are then just dereferenced
>> extracting the def->name and def->mac. Since the def->mac would be
>> the same as the passed @mac, just return a list of names and as long
>> as there's only one, extract the [0] entry from the passed list.
>> Also alter the error message on failure to include the mac that wasn't
>> found.
>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>>  src/conf/virinterfaceobj.c | 23 ++++++++++++++---------
>>  src/conf/virinterfaceobj.h |  2 +-
>>  src/test/test_driver.c     | 16 ++++++++--------
>>  3 files changed, 23 insertions(+), 18 deletions(-)
>>
>> diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c
>> index 3aeeebd..1cc5c92 100644
>> --- a/src/conf/virinterfaceobj.c
>> +++ b/src/conf/virinterfaceobj.c
>> @@ -108,11 +108,11 @@ virInterfaceObjListNew(void)
>>  int
>>  virInterfaceObjListFindByMACString(virInterfaceObjListPtr interfaces,
>>                                     const char *mac,
>> -                                   virInterfaceObjPtr *matches,
>> +                                   char **const matches,
>>                                     int maxmatches)
>>  {
>>      size_t i;
>> -    unsigned int matchct = 0;
>> +    int matchct = 0;
>>  
>>      for (i = 0; i < interfaces->count; i++) {
>>          virInterfaceObjPtr obj = interfaces->objs[i];
>> @@ -121,18 +121,23 @@ virInterfaceObjListFindByMACString(virInterfaceObjListPtr interfaces,
>>          virInterfaceObjLock(obj);
>>          def = obj->def;
>>          if (STRCASEEQ(def->mac, mac)) {
>> -            matchct++;
>> -            if (matchct <= maxmatches) {
>> -                matches[matchct - 1] = obj;
>> -                /* keep the lock if we're returning object to caller */
>> -                /* it is the caller's responsibility to unlock *all* matches */
>> -                continue;
>> +            if (matchct < maxmatches) {
>> +                if (VIR_STRDUP(matches[matchct], def->name) < 0) {
>> +                    virInterfaceObjUnlock(obj);
>> +                    goto error;
>> +                }
>> +                matchct++;
>>              }
>>          }
>>          virInterfaceObjUnlock(obj);
>> -
>>      }
>>      return matchct;
>> +
>> + error:
>> +    while (--matchct >= 0)
>> +        VIR_FREE(matches[matchct]);
>> +
>> +    return -1;
>>  }
>>  
>>  
>> diff --git a/src/conf/virinterfaceobj.h b/src/conf/virinterfaceobj.h
>> index f1bcab9..3934e63 100644
>> --- a/src/conf/virinterfaceobj.h
>> +++ b/src/conf/virinterfaceobj.h
>> @@ -44,7 +44,7 @@ virInterfaceObjListNew(void);
>>  int
>>  virInterfaceObjListFindByMACString(virInterfaceObjListPtr interfaces,
>>                                     const char *mac,
>> -                                   virInterfaceObjPtr *matches,
>> +                                   char **const matches,
>>                                     int maxmatches);
>>  
>>  virInterfaceObjPtr
>> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
>> index 89a705c..ac16f4f 100644
>> --- a/src/test/test_driver.c
>> +++ b/src/test/test_driver.c
>> @@ -3728,17 +3728,18 @@ testInterfaceLookupByMACString(virConnectPtr conn,
>>                                 const char *mac)
>>  {
>>      testDriverPtr privconn = conn->privateData;
>> -    virInterfaceObjPtr obj;
>> -    virInterfaceDefPtr def;
>>      int ifacect;
>> +    char *ifacenames[] = { NULL, NULL };
>>      virInterfacePtr ret = NULL;
>>  
>>      testDriverLock(privconn);
>> -    ifacect = virInterfaceObjListFindByMACString(privconn->ifaces, mac, &obj, 1);
>> +    ifacect = virInterfaceObjListFindByMACString(privconn->ifaces, mac,
>> +                                                 ifacenames, 2);
> 
> ARRAY_CARDINALITY()
> 

Don't really see the advantage. All this does is try to ensure the
provided MAC is "unique" by collecting all interfaces with it defined.
Once it reaches 2, FindByMACString won't collect any more since it
reached maxmatches.

John

>>      testDriverUnlock(privconn);
>>  
>>      if (ifacect == 0) {
>> -        virReportError(VIR_ERR_NO_INTERFACE, NULL);
>> +        virReportError(VIR_ERR_NO_INTERFACE,
>> +                       _("no interface with matching mac '%s'"), mac);
>>          goto cleanup;
>>      }
>>  
>> @@ -3747,12 +3748,11 @@ testInterfaceLookupByMACString(virConnectPtr conn,
>>          goto cleanup;
>>      }
>>  
>> -    def = virInterfaceObjGetDef(obj);
>> -    ret = virGetInterface(conn, def->name, def->mac);
>> +    ret = virGetInterface(conn, ifacenames[0], mac);
>>  
>>   cleanup:
>> -    if (obj)
>> -        virInterfaceObjUnlock(obj);
>> +    VIR_FREE(ifacenames[0]);
>> +    VIR_FREE(ifacenames[1]);
> 
> And this can be probably dynamic then:
> 
> for (i = 0; i < ARRAY_CARDINALITY(ifacenames); i++)
>     VIR_FREE(ifacenames[i]);
> 
>>      return ret;
>>  }
>>  
>>
> 
> Michal
>