[libvirt] [PATCH v3] interfaces: Convert virInterfaceObjList to virObjectRWLockable

John Ferlan jferlan at redhat.com
Thu Oct 19 14:48:56 UTC 2017



On 10/19/2017 10:07 AM, Erik Skultety wrote:
> On Fri, Oct 13, 2017 at 07:45:01AM -0400, John Ferlan wrote:
>> Rather than a forward linked list, let's use the ObjectRWLockable object
>> in order to manage the data.
>>
>> Requires numerous changes from List to Object management similar to
>> many other drivers/vir*obj.c modules
>>
> 
> This patch should be split in two. One that converts it to RWLockable, the
> other using a hash table instead of a linked list.
> 
> [...]

Personally, I don't recall during various conversions I've done to having:

    virObjectLockable parent;

    size_t count;
    virInterfaceObjPtr *objs;

first, then converting to count/objs to (a) hash table(s). Although
looking through history I see Michal had the networkobj code take a long
winding path to get there, so it's not impossible - just IMO pointless.

I think whenever I've converted from count/obs to a hash table with a
'real object', that the locking was done a the same time.

As I recall, usually it's been converting from :

    size_t count;
    virInterfaceObjPtr *objs;

to:

    virObjectLockable parent;

    /* name string -> virInterfaceObj  mapping
     * for O(1), lockless lookup-by-name */
    virHashTable *objsName;

then more recently the conversion from virObjectLockable to
virObjectRWLockable

Since this patch is taking @objs and converting to a hash table - it's
avoiding the Lockable and going straight to RWLockable.

>>
>> @@ -253,9 +334,11 @@ virInterfaceObjListAssignDef(virInterfaceObjListPtr interfaces,
>>  {
>>      virInterfaceObjPtr obj;
>>
>> -    if ((obj = virInterfaceObjListFindByName(interfaces, def->name))) {
>> +    virObjectRWLockWrite(interfaces);
>> +    if ((obj = virInterfaceObjListFindByNameLocked(interfaces, def->name))) {
>>          virInterfaceDefFree(obj->def);
>>          obj->def = def;
>> +        virObjectRWUnlock(interfaces);
>>
>>          return obj;
>>      }
>> @@ -263,13 +346,19 @@ virInterfaceObjListAssignDef(virInterfaceObjListPtr interfaces,
>>      if (!(obj = virInterfaceObjNew()))
>>          return NULL;
>>
>> -    if (VIR_APPEND_ELEMENT_COPY(interfaces->objs,
>> -                                interfaces->count, obj) < 0) {
>> -        virInterfaceObjEndAPI(&obj);
>> -        return NULL;
>> -    }
>> +    if (virHashAddEntry(interfaces->objsName, def->name, obj) < 0)
>> +        goto error;
>> +    virObjectRef(obj);
>> +
>>      obj->def = def;
>> -    return virObjectRef(obj);
>> +    virObjectRWUnlock(interfaces);
>> +
>> +    return obj;
>> +
>> + error:
>> +    virInterfaceObjEndAPI(&obj);
>> +    virObjectRWUnlock(interfaces);
>> +    return NULL;
>>  }
> 
> ^This could be tweaked even more:
> 

Sure, but in doing so eagle eyes now spots a problem in his own code...


> diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c
> index cf3626def..941893fc5 100644
> --- a/src/conf/virinterfaceobj.c
> +++ b/src/conf/virinterfaceobj.c
> @@ -337,19 +337,15 @@ virInterfaceObjListAssignDef(virInterfaceObjListPtr interfaces,
>      virObjectRWLockWrite(interfaces);
>      if ((obj = virInterfaceObjListFindByNameLocked(interfaces, def->name))) {
>          virInterfaceDefFree(obj->def);
> -        obj->def = def;
> -        virObjectRWUnlock(interfaces);
> +    } else {
> +        if (!(obj = virInterfaceObjNew()))
> +            return NULL;

Leaving with RWLock, <sigh>

> 
> -        return obj;
> +        if (virHashAddEntry(interfaces->objsName, def->name, obj) < 0)
> +            goto error;
> +        virObjectRef(obj);
>      }
> 
> -    if (!(obj = virInterfaceObjNew()))
> -        return NULL;
> -
> -    if (virHashAddEntry(interfaces->objsName, def->name, obj) < 0)
> -        goto error;
> -    virObjectRef(obj);
> -
>      obj->def = def;
>      virObjectRWUnlock(interfaces);
> 
>>
>>
>> @@ -277,20 +366,37 @@ void
>>  virInterfaceObjListRemove(virInterfaceObjListPtr interfaces,
>>                            virInterfaceObjPtr obj)
>>  {
>> -    size_t i;
> 
> if (!obj)
>     return;
> 

OK - I think at some point in time I figured it wasn't possible, but
adding it doesn't hurt. It's there in my branch.

> I didn't bother to check whether it could happen (it's used in test driver only
> anyway), but just in case there's an early cleanup exit path like it was in my
> recent nodedev code which was missing this very check too which would have made
> it fail miserably in such scenario.
> 
> With the patch split in 2 introducing 2 distinct changes + the NULL check:
> Reviewed-by: Erik Skultety <eskultet at redhat.com>

Hopefully you reconsider the desire for 2 patches...

> 
> 
> PS: I'm also sad, that we have two backends here just like we have in nodedev
> with one of them being essentially useless (just like in nodedev) we have this
> 'conf' generic code (just like in nodedev), yet in this case it's only used in
> the test driver. I'd very much appreciate if those backends could be adjusted
> in a way where we could make use of these functions. I can also imagine a
> cooperation of the udev backend with the nodedev driver where we have an active
> connection to the monitor, thus reacting to all events realtime, instead of
> defining a bunch of filters and then letting udev re-enumerate the list of
> interfaces each and every time (and by saying that I'm also aware that udev is
> actually the useless backend here).
> 
> Erik
> 

Yeah - the driver code here is quite different/strange and could
possibly use a bit more convergence. I feel too battered and bruised
over this convergence right now though ;-)....  Besides the differences
between the two really kind of are a bit scary w/r/t needing to call
some sort of netcf* or udev* interface to get the answers.

John




More information about the libvir-list mailing list