Re: [libvirt] [PATCH v3 9/9] interface: Convert virInterfaceObj to use virObjectLockable

[John Ferlan]

On 05/30/2017 08:13 AM, Bjoern Walk wrote:
> John Ferlan  [2017-05-30, 06:43AM -0400]:
>> [...]
>> void
>> -virInterfaceObjFree(virInterfaceObjPtr obj)
>> +virInterfaceObjEndAPI(virInterfaceObjPtr *obj)
> 
> Naming is hard, and I don't have any better suggestion. Just wanted to
> say that the name is, maybe, improvable :)
> 

It's a common nomenclature for libvirt... virDomainObjEndAPI,
virNetworkObjEndAPI, and virSecretObjEndAPI.

>> {
>> -if (!obj)
>> +if (!*obj)
>> return;
>>
>> -virInterfaceDefFree(obj->def);
>> -virMutexDestroy(>lock);
>> -VIR_FREE(obj);
>> +virObjectUnlock(*obj);
>> +virObjectUnref(*obj);
>> +*obj = NULL;
> 
> I'm a bit conflicted if this is strictly necessary. In what situation
> would this bite a consumer if we don't NULL obj? At least in this patch
> I can't find any.
> 

Although perhaps true - it's the common way this happens for other
vir*ObjEndAPI source code. Since it's theoretically possible an Unref
could cause the object to be Disposed (if refcnt == 0), setting *obj =
NULL at least "ensures" the caller misusing the object would be a NULL
deref rather than referencing something it no longer owned.

>> }
>>
>>
>> @@ -140,18 +150,18 @@
>> virInterfaceObjListFindByMACString(virInterfaceObjListPtr interfaces,
>> virInterfaceObjPtr obj = interfaces->objs[i];
>> virInterfaceDefPtr def;
>>
>> -virInterfaceObjLock(obj);
>> +virObjectLock(obj);
>> def = obj->def;
>> if (STRCASEEQ(def->mac, mac)) {
>> if (matchct < maxmatches) {
>> if (VIR_STRDUP(matches[matchct], def->name) < 0) {
>> -virInterfaceObjUnlock(obj);
>> +virObjectUnlock(obj);
>> goto error;
>> }
>> matchct++;
>> }
>> }
>> -virInterfaceObjUnlock(obj);
>> +virObjectUnlock(obj);
>> }
>> return matchct;
>>
>> @@ -173,11 +183,11 @@
>> virInterfaceObjListFindByName(virInterfaceObjListPtr interfaces,
>> virInterfaceObjPtr obj = interfaces->objs[i];
>> virInterfaceDefPtr def;
>>
>> -virInterfaceObjLock(obj);
>> +virObjectLock(obj);
>> def = obj->def;
>> if (STREQ(def->name, name))
>> -return obj;
>> -virInterfaceObjUnlock(obj);
>> +return virObjectRef(obj);
>> +virObjectUnlock(obj);
>> }
>>
>> return NULL;
>> @@ -190,7 +200,7 @@ virInterfaceObjListFree(virInterfaceObjListPtr
>> interfaces)
>> size_t i;
>>
>> for (i = 0; i < interfaces->count; i++)
>> -virInterfaceObjFree(interfaces->objs[i]);
>> +virObjectUnref(interfaces->objs[i]);
>> VIR_FREE(interfaces->objs);
>> VIR_FREE(interfaces);
>> }
>> @@ -227,7 +237,7 @@ virInterfaceObjListClone(virInterfaceObjListPtr
>> interfaces)
>> VIR_FREE(xml);
>> if (!(obj = virInterfaceObjListAssignDef(dest, backup)))
>> goto error;
>> -virInterfaceObjUnlock(obj); /* locked by
>> virInterfaceObjListAssignDef */
>> +virInterfaceObjEndAPI();
>> }
>>
>> return dest;
>> @@ -256,13 +266,12 @@
>> virInterfaceObjListAssignDef(virInterfaceObjListPtr interfaces,
>>
>> if (VIR_APPEND_ELEMENT_COPY(interfaces->objs,
>> interfaces->count, obj) < 0) {
>> -virInterfaceObjUnlock(obj);
>> -virInterfaceObjFree(obj);
>> +virInterfaceObjEndAPI();
>> return NULL;
>> }
>>
>> obj->def = def;
>> -return obj;
>> +return virObjectRef(obj);
>>
>> }
>>
>> @@ -273,17 +282,17 @@ virInterfaceObjListRemove(virInterfaceObjListPtr
>> interfaces,
>> {
>> size_t i;
>>
>> -virInterfaceObjUnlock(obj);
>> +virObjectUnlock(obj);
>> for (i = 0; i < interfaces->count; i++) {
>> -virInterfaceObjLock(interfaces->objs[i]);
>> +virObjectLock(interfaces->objs[i]);
>> if (interfaces->objs[i] == obj) {
>> -virInterfaceObjUnlock(interfaces->objs[i]);
>> -virInterfaceObjFree(interfaces->objs[i]);
>> +virObjectUnlock(interfaces->objs[i]);
>> +virObjectUnref(interfaces->objs[i]);
> 
> Here you could use virInterfaceObjEndAPI if the nulling would be
> dropped. Small advantage, I know.

Understood, I suppose this could also have taken the
"virInterfaceObjEndAPI();" option...

Still eventually this is changing from a forward linked list removal to
a removal from a hash table.

For the sake of my local branches, I'd prefer to keep as is, although if
there's a strong desire for something different I'm not opposed to
adjusting.

Thanks for taking a look at the last two!

John

>>
>> VIR_DELETE_ELEMENT(interfaces->objs, i, interfaces->count);
>> break;
>> }
>> -virInterfaceObjUnlock(interfaces->objs[i]);
>> +virObjectUnlock(interfaces->objs[i]);
>> }
>> }
>>
>> @@ -297,10 +306,10 @@
>> 

Re: [libvirt] [PATCH v3 9/9] interface: Convert virInterfaceObj to use virObjectLockable

[Bjoern Walk]

John Ferlan  [2017-05-30, 06:43AM -0400]:

[...]
void
-virInterfaceObjFree(virInterfaceObjPtr obj)
+virInterfaceObjEndAPI(virInterfaceObjPtr *obj)


Naming is hard, and I don't have any better suggestion. Just wanted to
say that the name is, maybe, improvable :)


{
-if (!obj)
+if (!*obj)
return;

-virInterfaceDefFree(obj->def);
-virMutexDestroy(>lock);
-VIR_FREE(obj);
+virObjectUnlock(*obj);
+virObjectUnref(*obj);
+*obj = NULL;


I'm a bit conflicted if this is strictly necessary. In what situation
would this bite a consumer if we don't NULL obj? At least in this patch
I can't find any.


}


@@ -140,18 +150,18 @@ virInterfaceObjListFindByMACString(virInterfaceObjListPtr 
interfaces,
virInterfaceObjPtr obj = interfaces->objs[i];
virInterfaceDefPtr def;

-virInterfaceObjLock(obj);
+virObjectLock(obj);
def = obj->def;
if (STRCASEEQ(def->mac, mac)) {
if (matchct < maxmatches) {
if (VIR_STRDUP(matches[matchct], def->name) < 0) {
-virInterfaceObjUnlock(obj);
+virObjectUnlock(obj);
goto error;
}
matchct++;
}
}
-virInterfaceObjUnlock(obj);
+virObjectUnlock(obj);
}
return matchct;

@@ -173,11 +183,11 @@ virInterfaceObjListFindByName(virInterfaceObjListPtr 
interfaces,
virInterfaceObjPtr obj = interfaces->objs[i];
virInterfaceDefPtr def;

-virInterfaceObjLock(obj);
+virObjectLock(obj);
def = obj->def;
if (STREQ(def->name, name))
-return obj;
-virInterfaceObjUnlock(obj);
+return virObjectRef(obj);
+virObjectUnlock(obj);
}

return NULL;
@@ -190,7 +200,7 @@ virInterfaceObjListFree(virInterfaceObjListPtr interfaces)
size_t i;

for (i = 0; i < interfaces->count; i++)
-virInterfaceObjFree(interfaces->objs[i]);
+virObjectUnref(interfaces->objs[i]);
VIR_FREE(interfaces->objs);
VIR_FREE(interfaces);
}
@@ -227,7 +237,7 @@ virInterfaceObjListClone(virInterfaceObjListPtr interfaces)
VIR_FREE(xml);
if (!(obj = virInterfaceObjListAssignDef(dest, backup)))
goto error;
-virInterfaceObjUnlock(obj); /* locked by virInterfaceObjListAssignDef 
*/
+virInterfaceObjEndAPI();
}

return dest;
@@ -256,13 +266,12 @@ virInterfaceObjListAssignDef(virInterfaceObjListPtr 
interfaces,

if (VIR_APPEND_ELEMENT_COPY(interfaces->objs,
interfaces->count, obj) < 0) {
-virInterfaceObjUnlock(obj);
-virInterfaceObjFree(obj);
+virInterfaceObjEndAPI();
return NULL;
}

obj->def = def;
-return obj;
+return virObjectRef(obj);

}

@@ -273,17 +282,17 @@ virInterfaceObjListRemove(virInterfaceObjListPtr 
interfaces,
{
size_t i;

-virInterfaceObjUnlock(obj);
+virObjectUnlock(obj);
for (i = 0; i < interfaces->count; i++) {
-virInterfaceObjLock(interfaces->objs[i]);
+virObjectLock(interfaces->objs[i]);
if (interfaces->objs[i] == obj) {
-virInterfaceObjUnlock(interfaces->objs[i]);
-virInterfaceObjFree(interfaces->objs[i]);
+virObjectUnlock(interfaces->objs[i]);
+virObjectUnref(interfaces->objs[i]);


Here you could use virInterfaceObjEndAPI if the nulling would be
dropped. Small advantage, I know.


VIR_DELETE_ELEMENT(interfaces->objs, i, interfaces->count);
break;
}
-virInterfaceObjUnlock(interfaces->objs[i]);
+virObjectUnlock(interfaces->objs[i]);
}
}

@@ -297,10 +306,10 @@ virInterfaceObjListNumOfInterfaces(virInterfaceObjListPtr 
interfaces,

for (i = 0; (i < interfaces->count); i++) {
virInterfaceObjPtr obj = interfaces->objs[i];
-virInterfaceObjLock(obj);
+virObjectLock(obj);
if (wantActive == virInterfaceObjIsActive(obj))
ninterfaces++;
-virInterfaceObjUnlock(obj);
+virObjectUnlock(obj);
}

return ninterfaces;
@@ -320,16 +329,16 @@ virInterfaceObjListGetNames(virInterfaceObjListPtr 
interfaces,
virInterfaceObjPtr obj = interfaces->objs[i];
virInterfaceDefPtr def;

-virInterfaceObjLock(obj);
+virObjectLock(obj);
def = obj->def;
if (wantActive == virInterfaceObjIsActive(obj)) {
if (VIR_STRDUP(names[nnames], def->name) < 0) {
-virInterfaceObjUnlock(obj);
+virObjectUnlock(obj);
goto failure;
}
nnames++;
}
-virInterfaceObjUnlock(obj);
+virObjectUnlock(obj);
}

return nnames;
diff --git a/src/conf/virinterfaceobj.h b/src/conf/virinterfaceobj.h
index 3934e63..2b9e1b2 100644
--- a/src/conf/virinterfaceobj.h
+++ b/src/conf/virinterfaceobj.h
@@ -28,6 +28,9 @@ typedef 

[libvirt] [PATCH v3 9/9] interface: Convert virInterfaceObj to use virObjectLockable

[John Ferlan]

Now that we have a bit more control, let's convert our object into
a lockable object and let that magic handle the create and lock/unlock.

This commit also introduces virInterfaceObjEndAPI in order to handle the
lock unlock and object unref in one call for consumers returning a NULL
obj upon return. This removes the need for virInterfaceObj{Lock|Unlock}
external API's.

Signed-off-by: John Ferlan 
---
 po/POTFILES.in |   1 -
 src/conf/virinterfaceobj.c | 105 -
 src/conf/virinterfaceobj.h |   9 ++--
 src/libvirt_private.syms   |   3 +-
 src/test/test_driver.c |  15 +++
 5 files changed, 68 insertions(+), 65 deletions(-)

diff --git a/po/POTFILES.in b/po/POTFILES.in
index 142b4d3..923d647 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -44,7 +44,6 @@ src/conf/storage_adapter_conf.c
 src/conf/storage_conf.c
 src/conf/virchrdev.c
 src/conf/virdomainobjlist.c
-src/conf/virinterfaceobj.c
 src/conf/virnetworkobj.c
 src/conf/virnodedeviceobj.c
 src/conf/virnwfilterobj.c
diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c
index 1e3f25c..51c3c82 100644
--- a/src/conf/virinterfaceobj.c
+++ b/src/conf/virinterfaceobj.c
@@ -33,7 +33,7 @@
 VIR_LOG_INIT("conf.virinterfaceobj");
 
 struct _virInterfaceObj {
-virMutex lock;
+virObjectLockable parent;
 
 bool active;   /* true if interface is active (up) */
 virInterfaceDefPtr def; /* The interface definition */
@@ -46,50 +46,60 @@ struct _virInterfaceObjList {
 
 /* virInterfaceObj manipulation */
 
-static virInterfaceObjPtr
-virInterfaceObjNew(void)
-{
-virInterfaceObjPtr obj;
+static virClassPtr virInterfaceObjClass;
+static void virInterfaceObjDispose(void *obj);
 
-if (VIR_ALLOC(obj) < 0)
-return NULL;
+static int
+virInterfaceObjOnceInit(void)
+{
+if (!(virInterfaceObjClass = virClassNew(virClassForObjectLockable(),
+ "virInterfaceObj",
+ sizeof(virInterfaceObj),
+ virInterfaceObjDispose)))
+return -1;
 
-if (virMutexInit(>lock) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   "%s", _("cannot initialize mutex"));
-VIR_FREE(obj);
-return NULL;
-}
+return 0;
+}
 
-virInterfaceObjLock(obj);
 
-return obj;
-}
+VIR_ONCE_GLOBAL_INIT(virInterfaceObj)
 
 
-void
-virInterfaceObjLock(virInterfaceObjPtr obj)
+static void
+virInterfaceObjDispose(void *opaque)
 {
-virMutexLock(>lock);
+virInterfaceObjPtr obj = opaque;
+
+virInterfaceDefFree(obj->def);
 }
 
 
-void
-virInterfaceObjUnlock(virInterfaceObjPtr obj)
+static virInterfaceObjPtr
+virInterfaceObjNew(void)
 {
-virMutexUnlock(>lock);
+virInterfaceObjPtr obj;
+
+if (virInterfaceObjInitialize() < 0)
+return NULL;
+
+if (!(obj = virObjectLockableNew(virInterfaceObjClass)))
+return NULL;
+
+virObjectLock(obj);
+
+return obj;
 }
 
 
 void
-virInterfaceObjFree(virInterfaceObjPtr obj)
+virInterfaceObjEndAPI(virInterfaceObjPtr *obj)
 {
-if (!obj)
+if (!*obj)
 return;
 
-virInterfaceDefFree(obj->def);
-virMutexDestroy(>lock);
-VIR_FREE(obj);
+virObjectUnlock(*obj);
+virObjectUnref(*obj);
+*obj = NULL;
 }
 
 
@@ -140,18 +150,18 @@ virInterfaceObjListFindByMACString(virInterfaceObjListPtr 
interfaces,
 virInterfaceObjPtr obj = interfaces->objs[i];
 virInterfaceDefPtr def;
 
-virInterfaceObjLock(obj);
+virObjectLock(obj);
 def = obj->def;
 if (STRCASEEQ(def->mac, mac)) {
 if (matchct < maxmatches) {
 if (VIR_STRDUP(matches[matchct], def->name) < 0) {
-virInterfaceObjUnlock(obj);
+virObjectUnlock(obj);
 goto error;
 }
 matchct++;
 }
 }
-virInterfaceObjUnlock(obj);
+virObjectUnlock(obj);
 }
 return matchct;
 
@@ -173,11 +183,11 @@ virInterfaceObjListFindByName(virInterfaceObjListPtr 
interfaces,
 virInterfaceObjPtr obj = interfaces->objs[i];
 virInterfaceDefPtr def;
 
-virInterfaceObjLock(obj);
+virObjectLock(obj);
 def = obj->def;
 if (STREQ(def->name, name))
-return obj;
-virInterfaceObjUnlock(obj);
+return virObjectRef(obj);
+virObjectUnlock(obj);
 }
 
 return NULL;
@@ -190,7 +200,7 @@ virInterfaceObjListFree(virInterfaceObjListPtr interfaces)
 size_t i;
 
 for (i = 0; i < interfaces->count; i++)
-virInterfaceObjFree(interfaces->objs[i]);
+virObjectUnref(interfaces->objs[i]);
 VIR_FREE(interfaces->objs);
 VIR_FREE(interfaces);
 }
@@ -227,7 +237,7 @@ virInterfaceObjListClone(virInterfaceObjListPtr interfaces)
 VIR_FREE(xml);
 if (!(obj =