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

[Erik Skultety]

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.

[...]
>
> @@ -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:

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;

-        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;

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>


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