[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On 05/20/2017 01:18 PM, John Ferlan wrote:
> 
> 
> 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 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.

The advantage to me is that we get rid of the magical constant. I can
immediately see what is "2" supposed to mean. But I agree that the test
driver is not our display case. So after all, I don't care that much. I
just thought it would be nice to have it.

Michal


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]