11:38 a.m.
As indicated in a couple of recent reviews, the new virObjectRWLockable
object and API's I believe needed a few adjustments in order to make
them better. So I present those thoughts in patch format to be hopefully
dicussed and dissected.
Essentially this series, will modify the newly added virObjectLockRead
to return a status and to have that status managed. Since there were only
9 new callers, this was straightforward. None of the callers actually
needed any sort of "divert logic" in order to jump around code that
shouldn't do the Unlock now. Although that could be a problem if ever
changing other more deeply nested type locks. That's someone else's
headache though.
Next, introduce and use virObjectLockWrite to be only for the new
style RWLocks. This too will return status. Only the vir*Remove caller
has a code path that I'm not happy with, but since these type locks
have been processing along anyway without technically ensuring they
have the lock before doing so, well no harm, no foul.
Next, introduce and use virObjectRWUnlock. Rather than overloading the
virObjectUnlock, let's just keep each caller doing it's one thing to
be sure that the calling code is doing the "right thing". In this case,
returning a failure status is just not going to work. We've gone too
far down the path in order for a failure to cause some failure in the
caller. We'll just treat it like virObjectUnlock and if the resource
remains locked, well we'll find out relatively soon as the next time
someone goes to read/write the list they'll be waiting for an unlock
that will never happen because of some coding mistake. But that will
save us from perhaps other dire consequences involving corruption.
Finally, revert the virObjectLock and virObjectUnlock code to just
manage virObjectLockable locks. No worse than what happened before
the RWLock was introduced. Leaving the consumers to handle things
as they would before - which probably ends up in some sort of awful
state leading to memory corruption and a quick death as opposed to
abort()'g in the void() function which has been deemed a don't do
that type operation for libvirt.
The last 2 patches actually have appeared before in various common
object series postings, so they could be a v2 or 3 type logic, but
I see them more related to this than that, so I present them here
again.
John Ferlan (7):
util: Alter virObjectLockRead to return status
util: Introduce and use virObjectLockWrite
util: Only have virObjectLock handle virObjectLockable
util: Introduce virObjectGetRWLockableObj
util: Introduce and use virObjectRWUnlock
util: Create common error path for invalid object
util: Add safety net of checks to ensure valid object
src/conf/virdomainobjlist.c | 81 +++++++++++++---------
src/libvirt_private.syms | 2 +
src/util/virobject.c | 160 +++++++++++++++++++++++++++++++++-----------
src/util/virobject.h | 10 ++-
4 files changed, 181 insertions(+), 72 deletions(-)
--
2.9.4
11:38 a.m.
New subject: [libvirt] [PATCH 1/7] util: Alter virObjectLockRead to return status
Rather than ignore errors, let's have virObjectLockRead check for
the correct usage and issue an error when not properly used so
so that we don't run into situations where the resource we think
we're locking really isn't locked because the void input parameter
wasn't valid.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/virdomainobjlist.c | 27 ++++++++++++++++++---------
src/util/virobject.c | 6 +++++-
src/util/virobject.h | 2 +-
3 files changed, 24 insertions(+), 11 deletions(-)
diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
index 1d027a4..fed4029 100644
--- a/src/conf/virdomainobjlist.c
+++ b/src/conf/virdomainobjlist.c
@@ -118,7 +118,8 @@ virDomainObjListFindByIDInternal(virDomainObjListPtr doms,
bool ref)
{
virDomainObjPtr obj;
- virObjectLockRead(doms);
+ if (virObjectLockRead(doms) < 0)
+ return NULL;
obj = virHashSearch(doms->objs, virDomainObjListSearchID, &id, NULL);
if (ref) {
virObjectRef(obj);
@@ -160,7 +161,8 @@ virDomainObjListFindByUUIDInternal(virDomainObjListPtr doms,
char uuidstr[VIR_UUID_STRING_BUFLEN];
virDomainObjPtr obj;
- virObjectLockRead(doms);
+ if (virObjectLockRead(doms) < 0)
+ return NULL;
virUUIDFormat(uuid, uuidstr);
obj = virHashLookup(doms->objs, uuidstr);
@@ -204,7 +206,8 @@ virDomainObjPtr virDomainObjListFindByName(virDomainObjListPtr doms,
{
virDomainObjPtr obj;
- virObjectLockRead(doms);
+ if (virObjectLockRead(doms) < 0)
+ return NULL;
obj = virHashLookup(doms->objsName, name);
virObjectRef(obj);
virObjectUnlock(doms);
@@ -653,7 +656,8 @@ virDomainObjListNumOfDomains(virDomainObjListPtr doms,
virConnectPtr conn)
{
struct virDomainObjListData data = { filter, conn, active, 0 };
- virObjectLockRead(doms);
+ if (virObjectLockRead(doms) < 0)
+ return -1;
virHashForEach(doms->objs, virDomainObjListCount, &data);
virObjectUnlock(doms);
return data.count;
@@ -697,7 +701,8 @@ virDomainObjListGetActiveIDs(virDomainObjListPtr doms,
{
struct virDomainIDData data = { filter, conn,
0, maxids, ids };
- virObjectLockRead(doms);
+ if (virObjectLockRead(doms) < 0)
+ return -1;
virHashForEach(doms->objs, virDomainObjListCopyActiveIDs, &data);
virObjectUnlock(doms);
return data.numids;
@@ -751,7 +756,8 @@ virDomainObjListGetInactiveNames(virDomainObjListPtr doms,
struct virDomainNameData data = { filter, conn,
0, 0, maxnames, names };
size_t i;
- virObjectLockRead(doms);
+ if (virObjectLockRead(doms) < 0)
+ return -1;
virHashForEach(doms->objs, virDomainObjListCopyInactiveNames, &data);
virObjectUnlock(doms);
if (data.oom) {
@@ -792,7 +798,8 @@ virDomainObjListForEach(virDomainObjListPtr doms,
struct virDomainListIterData data = {
callback, opaque, 0,
};
- virObjectLockRead(doms);
+ if (virObjectLockRead(doms) < 0)
+ return -1;
virHashForEach(doms->objs, virDomainObjListHelper, &data);
virObjectUnlock(doms);
return data.ret;
@@ -925,7 +932,8 @@ virDomainObjListCollect(virDomainObjListPtr domlist,
{
struct virDomainListData data = { NULL, 0 };
- virObjectLockRead(domlist);
+ if (virObjectLockRead(domlist) < 0)
+ return -1;
sa_assert(domlist->objs);
if (VIR_ALLOC_N(data.vms, virHashSize(domlist->objs)) < 0) {
virObjectUnlock(domlist);
@@ -962,7 +970,8 @@ virDomainObjListConvert(virDomainObjListPtr domlist,
*nvms = 0;
*vms = NULL;
- virObjectLockRead(domlist);
+ if (virObjectLockRead(domlist) < 0)
+ return -1;
for (i = 0; i < ndoms; i++) {
virDomainPtr dom = doms[i];
diff --git a/src/util/virobject.c b/src/util/virobject.c
index b1bb378..73de4d3 100644
--- a/src/util/virobject.c
+++ b/src/util/virobject.c
@@ -410,17 +410,21 @@ virObjectLock(void *anyobj)
* The object must be unlocked before releasing this
* reference.
*/
-void
+int
virObjectLockRead(void *anyobj)
{
if (virObjectIsClass(anyobj, virObjectRWLockableClass)) {
virObjectRWLockablePtr obj = anyobj;
virRWLockRead(&obj->lock);
+ return 0;
} else {
virObjectPtr obj = anyobj;
VIR_WARN("Object %p (%s) is not a virObjectRWLockable instance",
anyobj, obj ? obj->klass->name : "(unknown)");
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("unable to obtain rwlock for object=%p"), anyobj);
}
+ return -1;
}
diff --git a/src/util/virobject.h b/src/util/virobject.h
index 5985fad..99910e0 100644
--- a/src/util/virobject.h
+++ b/src/util/virobject.h
@@ -124,7 +124,7 @@ void
virObjectLock(void *lockableobj)
ATTRIBUTE_NONNULL(1);
-void
+int
virObjectLockRead(void *lockableobj)
ATTRIBUTE_NONNULL(1);
--
2.9.4
11:56 a.m.
New subject: [libvirt] [PATCH 1/7] util: Alter virObjectLockRead to return status
On Fri, Jul 28, 2017 at 12:38:55PM -0400, John Ferlan wrote:
Rather than ignore errors, let's have virObjectLockRead check
for
the correct usage and issue an error when not properly used so
so that we don't run into situations where the resource we think
we're locking really isn't locked because the void input parameter
wasn't valid.
I agree with Dan that this doesn't give any benefit. We should rather
consider start using abort() since this is a programming error, not
something that depends on an input from user. It should not happen if
if it does we have serious issues and abort is a best choice.
Pavel
1:26 p.m.
New subject: [libvirt] [PATCH 1/7] util: Alter virObjectLockRead to return status
On 07/28/2017 12:56 PM, Pavel Hrdina wrote:
On Fri, Jul 28, 2017 at 12:38:55PM -0400, John Ferlan wrote:
> Rather than ignore errors, let's have virObjectLockRead check for
> the correct usage and issue an error when not properly used so
> so that we don't run into situations where the resource we think
> we're locking really isn't locked because the void input parameter
> wasn't valid.
I agree with Dan that this doesn't give any benefit. We should rather
consider start using abort() since this is a programming error, not
something that depends on an input from user. It should not happen if
if it does we have serious issues and abort is a best choice.
Pavel
I'm in the minority, but that's fine. I could also change this patch to
be rename virObjectLockRead to be virObjectRWLockRead as suggested later
on too.
John
3:26 a.m.
New subject: [libvirt] [PATCH 1/7] util: Alter virObjectLockRead to return status
On 07/28/2017 08:26 PM, John Ferlan wrote:
On 07/28/2017 12:56 PM, Pavel Hrdina wrote:
> On Fri, Jul 28, 2017 at 12:38:55PM -0400, John Ferlan wrote:
>> Rather than ignore errors, let's have virObjectLockRead check for
>> the correct usage and issue an error when not properly used so
>> so that we don't run into situations where the resource we think
>> we're locking really isn't locked because the void input parameter
>> wasn't valid.
>
> I agree with Dan that this doesn't give any benefit. We should rather
> consider start using abort() since this is a programming error, not
> something that depends on an input from user. It should not happen if
> if it does we have serious issues and abort is a best choice.
>
> Pavel
>
I'm in the minority, but that's fine. I could also change this patch to
be rename virObjectLockRead to be virObjectRWLockRead as suggested later
on too.
Actually, me choosing virObjectLockRead over virObjectRWLockRead was
arbitrary. The reason is that my text editor can offer me completions:
virObjectLock
virObjectLockWrite
virObjectLockRead
BTW: Following your reasoning here, it should have been called
virObjectLockableLock() instead of virObjectLock() ;-)
IOW, I'm failing to see the need for 'RW' in the name you're suggesting.
Michal
5:08 a.m.
New subject: [libvirt] [PATCH 1/7] util: Alter virObjectLockRead to return status
On Fri, Jul 28, 2017 at 12:38:55PM -0400, John Ferlan wrote:
Rather than ignore errors, let's have virObjectLockRead check
for
the correct usage and issue an error when not properly used so
so that we don't run into situations where the resource we think
we're locking really isn't locked because the void input parameter
wasn't valid.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/virdomainobjlist.c | 27 ++++++++++++++++++---------
src/util/virobject.c | 6 +++++-
src/util/virobject.h | 2 +-
3 files changed, 24 insertions(+), 11 deletions(-)
diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
index 1d027a4..fed4029 100644
--- a/src/conf/virdomainobjlist.c
+++ b/src/conf/virdomainobjlist.c
@@ -118,7 +118,8 @@ virDomainObjListFindByIDInternal(virDomainObjListPtr doms,
bool ref)
{
virDomainObjPtr obj;
- virObjectLockRead(doms);
+ if (virObjectLockRead(doms) < 0)
+ return NULL;
obj = virHashSearch(doms->objs, virDomainObjListSearchID, &id, NULL);
if (ref) {
virObjectRef(obj);
@@ -160,7 +161,8 @@ virDomainObjListFindByUUIDInternal(virDomainObjListPtr doms,
char uuidstr[VIR_UUID_STRING_BUFLEN];
virDomainObjPtr obj;
- virObjectLockRead(doms);
+ if (virObjectLockRead(doms) < 0)
+ return NULL;
virUUIDFormat(uuid, uuidstr);
obj = virHashLookup(doms->objs, uuidstr);
@@ -204,7 +206,8 @@ virDomainObjPtr virDomainObjListFindByName(virDomainObjListPtr doms,
{
virDomainObjPtr obj;
- virObjectLockRead(doms);
+ if (virObjectLockRead(doms) < 0)
+ return NULL;
obj = virHashLookup(doms->objsName, name);
virObjectRef(obj);
virObjectUnlock(doms);
@@ -653,7 +656,8 @@ virDomainObjListNumOfDomains(virDomainObjListPtr doms,
virConnectPtr conn)
{
struct virDomainObjListData data = { filter, conn, active, 0 };
- virObjectLockRead(doms);
+ if (virObjectLockRead(doms) < 0)
+ return -1;
virHashForEach(doms->objs, virDomainObjListCount, &data);
virObjectUnlock(doms);
return data.count;
@@ -697,7 +701,8 @@ virDomainObjListGetActiveIDs(virDomainObjListPtr doms,
{
struct virDomainIDData data = { filter, conn,
0, maxids, ids };
- virObjectLockRead(doms);
+ if (virObjectLockRead(doms) < 0)
+ return -1;
virHashForEach(doms->objs, virDomainObjListCopyActiveIDs, &data);
virObjectUnlock(doms);
return data.numids;
@@ -751,7 +756,8 @@ virDomainObjListGetInactiveNames(virDomainObjListPtr doms,
struct virDomainNameData data = { filter, conn,
0, 0, maxnames, names };
size_t i;
- virObjectLockRead(doms);
+ if (virObjectLockRead(doms) < 0)
+ return -1;
virHashForEach(doms->objs, virDomainObjListCopyInactiveNames, &data);
virObjectUnlock(doms);
if (data.oom) {
@@ -792,7 +798,8 @@ virDomainObjListForEach(virDomainObjListPtr doms,
struct virDomainListIterData data = {
callback, opaque, 0,
};
- virObjectLockRead(doms);
+ if (virObjectLockRead(doms) < 0)
+ return -1;
virHashForEach(doms->objs, virDomainObjListHelper, &data);
virObjectUnlock(doms);
return data.ret;
@@ -925,7 +932,8 @@ virDomainObjListCollect(virDomainObjListPtr domlist,
{
struct virDomainListData data = { NULL, 0 };
- virObjectLockRead(domlist);
+ if (virObjectLockRead(domlist) < 0)
+ return -1;
sa_assert(domlist->objs);
if (VIR_ALLOC_N(data.vms, virHashSize(domlist->objs)) < 0) {
virObjectUnlock(domlist);
@@ -962,7 +970,8 @@ virDomainObjListConvert(virDomainObjListPtr domlist,
*nvms = 0;
*vms = NULL;
- virObjectLockRead(domlist);
+ if (virObjectLockRead(domlist) < 0)
+ return -1;
for (i = 0; i < ndoms; i++) {
virDomainPtr dom = doms[i];
diff --git a/src/util/virobject.c b/src/util/virobject.c
index b1bb378..73de4d3 100644
--- a/src/util/virobject.c
+++ b/src/util/virobject.c
@@ -410,17 +410,21 @@ virObjectLock(void *anyobj)
* The object must be unlocked before releasing this
* reference.
*/
-void
+int
I'm NACK on this return value change.
virObjectLockRead(void *anyobj)
{
if (virObjectIsClass(anyobj, virObjectRWLockableClass)) {
virObjectRWLockablePtr obj = anyobj;
virRWLockRead(&obj->lock);
+ return 0;
} else {
virObjectPtr obj = anyobj;
VIR_WARN("Object %p (%s) is not a virObjectRWLockable instance",
anyobj, obj ? obj->klass->name : "(unknown)");
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("unable to obtain rwlock for object=%p"), anyobj);
}
+ return -1;
}
IMHO this should just be simplified to
virObjectLockRead(void *anyobj)
{
virObjectRWLockablePtr obj = anyobj;
virRWLockRead(&obj->lock);
}
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
7:04 a.m.
New subject: [libvirt] [PATCH 1/7] util: Alter virObjectLockRead to return status
[...]
> --- a/src/util/virobject.c
> +++ b/src/util/virobject.c
> @@ -410,17 +410,21 @@ virObjectLock(void *anyobj)
> * The object must be unlocked before releasing this
> * reference.
> */
> -void
> +int
I'm NACK on this return value change.
OK - that's fine.
> virObjectLockRead(void *anyobj)
> {
> if (virObjectIsClass(anyobj, virObjectRWLockableClass)) {
> virObjectRWLockablePtr obj = anyobj;
> virRWLockRead(&obj->lock);
> + return 0;
> } else {
> virObjectPtr obj = anyobj;
> VIR_WARN("Object %p (%s) is not a virObjectRWLockable instance",
> anyobj, obj ? obj->klass->name : "(unknown)");
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("unable to obtain rwlock for object=%p"),
anyobj);
> }
> + return -1;
> }
IMHO this should just be simplified to
virObjectLockRead(void *anyobj)
{
virObjectRWLockablePtr obj = anyobj;
virRWLockRead(&obj->lock);
}
I'm not as sure for this one though, but I do understand the reasoning
especially if @obj == NULL since we'd have a core. Still if @obj is
passed in as a non NULL address, I don't believe we're going to get a
failure - sure pthread_rwlock_{rdlock|wrlock} or pthread_mutex_lock will
fail, but we don't fail on that. Still getting a VIR_WARN somewhere
would hopefully help us debug. It's too bad we couldn't have the extra
checks be for developer builds only and cause the abort then.
In the FWIW department...
Even in the first commit ('b545f65d') for virObject{Lock|Unlock},
rudimentary error checking such as !obj and using an @obj with the right
class was checked and a VIR_WARN issued.
As I was adding a new class for common objects, I made the mistake noted
in patch 7, so it seemed to be a good thing to do to add a few more
checks along the way and perhaps better entrails. Of course doing that
required a multi-step changes... So, commit id '10c2bb2b1' created
virObjectGetLockableObj as a way to combine the existing checks.
The next steps from my v2 weren't NACK'd, but they weren't ACK'd or
discouraged - they just needed some adjustments. The v3 made the
adjustments (along with more patches), but never got any reviews.
With the addition of RWLockable (commit id '77f4593b') those checks got
wiped out in favor of inline code again. I understand why - one API, one
place to check, etc.
If there's going to be 4 API's, then recreating the original check again
is where I was headed, but not it seems like it's not desired even
though checks like that have been around from the start.
We could proceed with no checks, but before posting patches for that I
would like to make sure that's what is really desired given the history
and side effect of doing so.
Tks -
John
11:38 a.m.
New subject: [libvirt] [PATCH 2/7] util: Introduce and use virObjectLockWrite
Instead of making virObjectLock be the entry point for two
different types of locks, let's create a virObjectLockWrite API
which will be able to return status and force the (new) consumers
of the RWLock to make sure the lock is really obtained when the
"right" object type is passed.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/virdomainobjlist.c | 18 ++++++++++++++----
src/libvirt_private.syms | 1 +
src/util/virobject.c | 32 ++++++++++++++++++++++++++++++++
src/util/virobject.h | 4 ++++
4 files changed, 51 insertions(+), 4 deletions(-)
diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
index fed4029..08a51cc 100644
--- a/src/conf/virdomainobjlist.c
+++ b/src/conf/virdomainobjlist.c
@@ -330,7 +330,8 @@ virDomainObjPtr virDomainObjListAdd(virDomainObjListPtr doms,
{
virDomainObjPtr ret;
- virObjectLock(doms);
+ if (virObjectLockWrite(doms) < 0)
+ return NULL;
ret = virDomainObjListAddLocked(doms, def, xmlopt, flags, oldDef);
virObjectUnlock(doms);
return ret;
@@ -352,7 +353,9 @@ void virDomainObjListRemove(virDomainObjListPtr doms,
virObjectRef(dom);
virObjectUnlock(dom);
- virObjectLock(doms);
+ /* We really shouldn't ignore this,
+ * but that ship sailed a long time ago */
+ ignore_value(virObjectLockWrite(doms));
virObjectLock(dom);
virHashRemoveEntry(doms->objs, uuidstr);
virHashRemoveEntry(doms->objsName, dom->def->name);
@@ -397,9 +400,13 @@ virDomainObjListRename(virDomainObjListPtr doms,
* hold a lock on dom but not refcount it. */
virObjectRef(dom);
virObjectUnlock(dom);
- virObjectLock(doms);
+ rc = virObjectLockWrite(doms);
virObjectLock(dom);
virObjectUnref(dom);
+ if (rc < 0) {
+ VIR_FREE(old_name);
+ return ret;
+ }
if (virHashLookup(doms->objsName, new_name) != NULL) {
virReportError(VIR_ERR_OPERATION_INVALID,
@@ -576,7 +583,10 @@ virDomainObjListLoadAllConfigs(virDomainObjListPtr doms,
if ((rc = virDirOpenIfExists(&dir, configDir)) <= 0)
return rc;
- virObjectLock(doms);
+ if (virObjectLockWrite(doms) < 0) {
+ VIR_DIR_CLOSE(dir);
+ return -1;
+ }
while ((ret = virDirRead(dir, &entry, configDir)) > 0) {
virDomainObjPtr dom;
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 37b815c..f1a6510 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2306,6 +2306,7 @@ virObjectListFreeCount;
virObjectLock;
virObjectLockableNew;
virObjectLockRead;
+virObjectLockWrite;
virObjectNew;
virObjectRef;
virObjectRWLockableNew;
diff --git a/src/util/virobject.c b/src/util/virobject.c
index 73de4d3..c48f88c 100644
--- a/src/util/virobject.c
+++ b/src/util/virobject.c
@@ -429,6 +429,38 @@ virObjectLockRead(void *anyobj)
/**
+ * virObjectLockWrite:
+ * @anyobj: any instance of virObjectRWLockable
+ *
+ * Acquire a write lock on @anyobj. The lock must be
+ * released by virObjectUnlock.
+ *
+ * The caller is expected to have acquired a reference
+ * on the object before locking it (eg virObjectRef).
+ * The object must be unlocked before releasing this
+ * reference.
+ *
+ * Returns 0 on success, -1 on failure
+ */
+int
+virObjectLockWrite(void *anyobj)
+{
+ if (virObjectIsClass(anyobj, virObjectRWLockableClass)) {
+ virObjectRWLockablePtr obj = anyobj;
+ virRWLockWrite(&obj->lock);
+ return 0;
+ } else {
+ virObjectPtr obj = anyobj;
+ VIR_WARN("Object %p (%s) is not a virObjectRWLockable instance",
+ anyobj, obj ? obj->klass->name : "(unknown)");
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("unable to obtain rwlock for object=%p"), anyobj);
+ }
+ return -1;
+}
+
+
+/**
* virObjectUnlock:
* @anyobj: any instance of virObjectLockable or virObjectRWLockable
*
diff --git a/src/util/virobject.h b/src/util/virobject.h
index 99910e0..f0d1f97 100644
--- a/src/util/virobject.h
+++ b/src/util/virobject.h
@@ -128,6 +128,10 @@ int
virObjectLockRead(void *lockableobj)
ATTRIBUTE_NONNULL(1);
+int
+virObjectLockWrite(void *lockableobj)
+ ATTRIBUTE_NONNULL(1);
+
void
virObjectUnlock(void *lockableobj)
ATTRIBUTE_NONNULL(1);
--
2.9.4
11:59 a.m.
New subject: [libvirt] [PATCH 2/7] util: Introduce and use virObjectLockWrite
On Fri, Jul 28, 2017 at 12:38:56PM -0400, John Ferlan wrote:
Instead of making virObjectLock be the entry point for two
different types of locks, let's create a virObjectLockWrite API
which will be able to return status and force the (new) consumers
of the RWLock to make sure the lock is really obtained when the
"right" object type is passed.
If you remove the return value for the same reason as in the first patch
Reviewed-by: Pavel Hrdina <phrdina(a)redhat.com>
1:27 p.m.
New subject: [libvirt] [PATCH 2/7] util: Introduce and use virObjectLockWrite
On 07/28/2017 12:59 PM, Pavel Hrdina wrote:
On Fri, Jul 28, 2017 at 12:38:56PM -0400, John Ferlan wrote:
> Instead of making virObjectLock be the entry point for two
> different types of locks, let's create a virObjectLockWrite API
> which will be able to return status and force the (new) consumers
> of the RWLock to make sure the lock is really obtained when the
> "right" object type is passed.
If you remove the return value for the same reason as in the first patch
Reviewed-by: Pavel Hrdina <phrdina(a)redhat.com>
Fine again, minority opinion it seems. Still rename from
virObjectLockWrite to virObjectRWLockWrite is expect, right (or is that
Write)
John
5:10 a.m.
New subject: [libvirt] [PATCH 2/7] util: Introduce and use virObjectLockWrite
On Fri, Jul 28, 2017 at 12:38:56PM -0400, John Ferlan wrote:
Instead of making virObjectLock be the entry point for two
different types of locks, let's create a virObjectLockWrite API
which will be able to return status and force the (new) consumers
of the RWLock to make sure the lock is really obtained when the
"right" object type is passed.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/virdomainobjlist.c | 18 ++++++++++++++----
src/libvirt_private.syms | 1 +
src/util/virobject.c | 32 ++++++++++++++++++++++++++++++++
src/util/virobject.h | 4 ++++
4 files changed, 51 insertions(+), 4 deletions(-)
diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
index fed4029..08a51cc 100644
--- a/src/conf/virdomainobjlist.c
+++ b/src/conf/virdomainobjlist.c
@@ -330,7 +330,8 @@ virDomainObjPtr virDomainObjListAdd(virDomainObjListPtr doms,
{
virDomainObjPtr ret;
- virObjectLock(doms);
+ if (virObjectLockWrite(doms) < 0)
+ return NULL;
ret = virDomainObjListAddLocked(doms, def, xmlopt, flags, oldDef);
virObjectUnlock(doms);
return ret;
@@ -352,7 +353,9 @@ void virDomainObjListRemove(virDomainObjListPtr doms,
virObjectRef(dom);
virObjectUnlock(dom);
- virObjectLock(doms);
+ /* We really shouldn't ignore this,
+ * but that ship sailed a long time ago */
+ ignore_value(virObjectLockWrite(doms));
virObjectLock(dom);
virHashRemoveEntry(doms->objs, uuidstr);
virHashRemoveEntry(doms->objsName, dom->def->name);
@@ -397,9 +400,13 @@ virDomainObjListRename(virDomainObjListPtr doms,
* hold a lock on dom but not refcount it. */
virObjectRef(dom);
virObjectUnlock(dom);
- virObjectLock(doms);
+ rc = virObjectLockWrite(doms);
virObjectLock(dom);
virObjectUnref(dom);
+ if (rc < 0) {
+ VIR_FREE(old_name);
+ return ret;
+ }
if (virHashLookup(doms->objsName, new_name) != NULL) {
virReportError(VIR_ERR_OPERATION_INVALID,
@@ -576,7 +583,10 @@ virDomainObjListLoadAllConfigs(virDomainObjListPtr doms,
if ((rc = virDirOpenIfExists(&dir, configDir)) <= 0)
return rc;
- virObjectLock(doms);
+ if (virObjectLockWrite(doms) < 0) {
+ VIR_DIR_CLOSE(dir);
+ return -1;
+ }
while ((ret = virDirRead(dir, &entry, configDir)) > 0) {
virDomainObjPtr dom;
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 37b815c..f1a6510 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2306,6 +2306,7 @@ virObjectListFreeCount;
virObjectLock;
virObjectLockableNew;
virObjectLockRead;
+virObjectLockWrite;
virObjectNew;
virObjectRef;
virObjectRWLockableNew;
diff --git a/src/util/virobject.c b/src/util/virobject.c
index 73de4d3..c48f88c 100644
--- a/src/util/virobject.c
+++ b/src/util/virobject.c
@@ -429,6 +429,38 @@ virObjectLockRead(void *anyobj)
/**
+ * virObjectLockWrite:
+ * @anyobj: any instance of virObjectRWLockable
+ *
+ * Acquire a write lock on @anyobj. The lock must be
+ * released by virObjectUnlock.
+ *
+ * The caller is expected to have acquired a reference
+ * on the object before locking it (eg virObjectRef).
+ * The object must be unlocked before releasing this
+ * reference.
+ *
+ * Returns 0 on success, -1 on failure
+ */
+int
+virObjectLockWrite(void *anyobj)
+{
+ if (virObjectIsClass(anyobj, virObjectRWLockableClass)) {
+ virObjectRWLockablePtr obj = anyobj;
+ virRWLockWrite(&obj->lock);
+ return 0;
+ } else {
+ virObjectPtr obj = anyobj;
+ VIR_WARN("Object %p (%s) is not a virObjectRWLockable instance",
+ anyobj, obj ? obj->klass->name : "(unknown)");
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("unable to obtain rwlock for object=%p"), anyobj);
+ }
+ return -1;
+}
IMHO this can be simplified
void
virObjectLockWrite(void *anyobj)
{
virObjectRWLockablePtr obj = anyobj;
virRWLockWrite(&obj->lock);
}
because if some caller has passed in a bogus argument (something not an
object, or an object that's already free), then the virObjectIsClass
is just as likley to crash as the simpler code, and IMHO it is preferrable
to get an immediate crash, than to carry on with a warning mesage on
the console and no lock acquired.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
11:38 a.m.
New subject: [libvirt] [PATCH 3/7] util: Only have virObjectLock handle virObjectLockable
Now that virObjectLockWrite exists to handle the virObjectRWLockable
objects, let's restore virObjectLock to only handle virObjectLockable
type locks. There still exists the possibility that the input @anyobj
isn't a valid object and the resource isn't truly locked, but that
also exists before commit id '77f4593b'.
This also restores some logic that commit id '77f4593b' removed
with respect to a common code path that commit id '10c2bb2b' had
introduced as virObjectGetLockableObj.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/util/virobject.c | 37 +++++++++++++++++++++++--------------
1 file changed, 23 insertions(+), 14 deletions(-)
diff --git a/src/util/virobject.c b/src/util/virobject.c
index c48f88c..f9047a1 100644
--- a/src/util/virobject.c
+++ b/src/util/virobject.c
@@ -367,13 +367,28 @@ virObjectRef(void *anyobj)
}
+static virObjectLockablePtr
+virObjectGetLockableObj(void *anyobj)
+{
+ virObjectPtr obj;
+
+ if (virObjectIsClass(anyobj, virObjectLockableClass))
+ return anyobj;
+
+ obj = anyobj;
+ VIR_WARN("Object %p (%s) is not a virObjectLockable instance",
+ anyobj, obj ? obj->klass->name : "(unknown)");
+
+ return NULL;
+}
+
+
/**
* virObjectLock:
* @anyobj: any instance of virObjectLockable or virObjectRWLockable
*
* Acquire a lock on @anyobj. The lock must be released by
- * virObjectUnlock. In case the passed object is instance of
- * virObjectRWLockable a write lock is acquired.
+ * virObjectUnlock.
*
* The caller is expected to have acquired a reference
* on the object before locking it (eg virObjectRef).
@@ -383,18 +398,12 @@ virObjectRef(void *anyobj)
void
virObjectLock(void *anyobj)
{
- if (virObjectIsClass(anyobj, virObjectLockableClass)) {
- virObjectLockablePtr obj = anyobj;
- virMutexLock(&obj->lock);
- } else if (virObjectIsClass(anyobj, virObjectRWLockableClass)) {
- virObjectRWLockablePtr obj = anyobj;
- virRWLockWrite(&obj->lock);
- } else {
- virObjectPtr obj = anyobj;
- VIR_WARN("Object %p (%s) is not a virObjectLockable "
- "nor virObjectRWLockable instance",
- anyobj, obj ? obj->klass->name : "(unknown)");
- }
+ virObjectLockablePtr obj = virObjectGetLockableObj(anyobj);
+
+ if (!obj)
+ return;
+
+ virMutexLock(&obj->lock);
}
--
2.9.4
12:03 p.m.
New subject: [libvirt] [PATCH 3/7] util: Only have virObjectLock handle virObjectLockable
On Fri, Jul 28, 2017 at 12:38:57PM -0400, John Ferlan wrote:
Now that virObjectLockWrite exists to handle the virObjectRWLockable
objects, let's restore virObjectLock to only handle virObjectLockable
type locks. There still exists the possibility that the input @anyobj
isn't a valid object and the resource isn't truly locked, but that
also exists before commit id '77f4593b'.
This also restores some logic that commit id '77f4593b' removed
with respect to a common code path that commit id '10c2bb2b' had
introduced as virObjectGetLockableObj.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/util/virobject.c | 37 +++++++++++++++++++++++--------------
1 file changed, 23 insertions(+), 14 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina(a)redhat.com>
5:12 a.m.
New subject: [libvirt] [PATCH 3/7] util: Only have virObjectLock handle virObjectLockable
On Fri, Jul 28, 2017 at 12:38:57PM -0400, John Ferlan wrote:
Now that virObjectLockWrite exists to handle the virObjectRWLockable
objects, let's restore virObjectLock to only handle virObjectLockable
type locks. There still exists the possibility that the input @anyobj
isn't a valid object and the resource isn't truly locked, but that
also exists before commit id '77f4593b'.
This also restores some logic that commit id '77f4593b' removed
with respect to a common code path that commit id '10c2bb2b' had
introduced as virObjectGetLockableObj.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/util/virobject.c | 37 +++++++++++++++++++++++--------------
1 file changed, 23 insertions(+), 14 deletions(-)
diff --git a/src/util/virobject.c b/src/util/virobject.c
index c48f88c..f9047a1 100644
--- a/src/util/virobject.c
+++ b/src/util/virobject.c
@@ -367,13 +367,28 @@ virObjectRef(void *anyobj)
}
+static virObjectLockablePtr
+virObjectGetLockableObj(void *anyobj)
+{
+ virObjectPtr obj;
+
+ if (virObjectIsClass(anyobj, virObjectLockableClass))
+ return anyobj;
+
+ obj = anyobj;
+ VIR_WARN("Object %p (%s) is not a virObjectLockable instance",
+ anyobj, obj ? obj->klass->name : "(unknown)");
+
+ return NULL;
+}
+
+
/**
* virObjectLock:
* @anyobj: any instance of virObjectLockable or virObjectRWLockable
*
* Acquire a lock on @anyobj. The lock must be released by
- * virObjectUnlock. In case the passed object is instance of
- * virObjectRWLockable a write lock is acquired.
+ * virObjectUnlock.
*
* The caller is expected to have acquired a reference
* on the object before locking it (eg virObjectRef).
@@ -383,18 +398,12 @@ virObjectRef(void *anyobj)
void
virObjectLock(void *anyobj)
{
- if (virObjectIsClass(anyobj, virObjectLockableClass)) {
- virObjectLockablePtr obj = anyobj;
- virMutexLock(&obj->lock);
- } else if (virObjectIsClass(anyobj, virObjectRWLockableClass)) {
- virObjectRWLockablePtr obj = anyobj;
- virRWLockWrite(&obj->lock);
- } else {
- virObjectPtr obj = anyobj;
- VIR_WARN("Object %p (%s) is not a virObjectLockable "
- "nor virObjectRWLockable instance",
- anyobj, obj ? obj->klass->name : "(unknown)");
- }
+ virObjectLockablePtr obj = virObjectGetLockableObj(anyobj);
+
+ if (!obj)
+ return;
+
+ virMutexLock(&obj->lock);
Again I'd prefer to see
virObjectLockablePtr obj = anyobj;
virMutexLock(&obj->lock);
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
11:38 a.m.
New subject: [libvirt] [PATCH 4/7] util: Introduce virObjectGetRWLockableObj
Introduce a helper to handle the error path more cleanly. The same
as virObjectGetLockableObj.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/util/virobject.c | 51 ++++++++++++++++++++++++++++++---------------------
1 file changed, 30 insertions(+), 21 deletions(-)
diff --git a/src/util/virobject.c b/src/util/virobject.c
index f9047a1..0db98c3 100644
--- a/src/util/virobject.c
+++ b/src/util/virobject.c
@@ -383,6 +383,22 @@ virObjectGetLockableObj(void *anyobj)
}
+static virObjectRWLockablePtr
+virObjectGetRWLockableObj(void *anyobj)
+{
+ virObjectPtr obj;
+
+ if (virObjectIsClass(anyobj, virObjectRWLockableClass))
+ return anyobj;
+
+ obj = anyobj;
+ VIR_WARN("Object %p (%s) is not a virObjectRWLockable instance",
+ anyobj, obj ? obj->klass->name : "(unknown)");
+
+ return NULL;
+}
+
+
/**
* virObjectLock:
* @anyobj: any instance of virObjectLockable or virObjectRWLockable
@@ -422,18 +438,13 @@ virObjectLock(void *anyobj)
int
virObjectLockRead(void *anyobj)
{
- if (virObjectIsClass(anyobj, virObjectRWLockableClass)) {
- virObjectRWLockablePtr obj = anyobj;
- virRWLockRead(&obj->lock);
- return 0;
- } else {
- virObjectPtr obj = anyobj;
- VIR_WARN("Object %p (%s) is not a virObjectRWLockable instance",
- anyobj, obj ? obj->klass->name : "(unknown)");
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("unable to obtain rwlock for object=%p"), anyobj);
- }
- return -1;
+ virObjectRWLockablePtr obj = virObjectGetRWLockableObj(anyobj);
+
+ if (!obj)
+ return -1;
+
+ virRWLockRead(&obj->lock);
+ return 0;
}
@@ -454,18 +465,16 @@ virObjectLockRead(void *anyobj)
int
virObjectLockWrite(void *anyobj)
{
- if (virObjectIsClass(anyobj, virObjectRWLockableClass)) {
- virObjectRWLockablePtr obj = anyobj;
- virRWLockWrite(&obj->lock);
- return 0;
- } else {
- virObjectPtr obj = anyobj;
- VIR_WARN("Object %p (%s) is not a virObjectRWLockable instance",
- anyobj, obj ? obj->klass->name : "(unknown)");
+ virObjectRWLockablePtr obj = virObjectGetRWLockableObj(anyobj);
+
+ if (!obj) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("unable to obtain rwlock for object=%p"), anyobj);
+ return -1;
}
- return -1;
+
+ virRWLockWrite(&obj->lock);
+ return 0;
}
--
2.9.4
12:04 p.m.
New subject: [libvirt] [PATCH 4/7] util: Introduce virObjectGetRWLockableObj
On Fri, Jul 28, 2017 at 12:38:58PM -0400, John Ferlan wrote:
Introduce a helper to handle the error path more cleanly. The same
as virObjectGetLockableObj.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/util/virobject.c | 51 ++++++++++++++++++++++++++++++---------------------
1 file changed, 30 insertions(+), 21 deletions(-)
Without the return value as for patch 01 and 02
Reviewed-by: Pavel Hrdina <phrdina(a)redhat.com>
5:14 a.m.
New subject: [libvirt] [PATCH 4/7] util: Introduce virObjectGetRWLockableObj
On Fri, Jul 28, 2017 at 12:38:58PM -0400, John Ferlan wrote:
Introduce a helper to handle the error path more cleanly. The same
as virObjectGetLockableObj.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/util/virobject.c | 51 ++++++++++++++++++++++++++++++---------------------
1 file changed, 30 insertions(+), 21 deletions(-)
diff --git a/src/util/virobject.c b/src/util/virobject.c
index f9047a1..0db98c3 100644
--- a/src/util/virobject.c
+++ b/src/util/virobject.c
@@ -383,6 +383,22 @@ virObjectGetLockableObj(void *anyobj)
}
+static virObjectRWLockablePtr
+virObjectGetRWLockableObj(void *anyobj)
+{
+ virObjectPtr obj;
+
+ if (virObjectIsClass(anyobj, virObjectRWLockableClass))
+ return anyobj;
+
+ obj = anyobj;
+ VIR_WARN("Object %p (%s) is not a virObjectRWLockable instance",
+ anyobj, obj ? obj->klass->name : "(unknown)");
+
+ return NULL;
+}
+
+
/**
* virObjectLock:
* @anyobj: any instance of virObjectLockable or virObjectRWLockable
@@ -422,18 +438,13 @@ virObjectLock(void *anyobj)
int
virObjectLockRead(void *anyobj)
{
- if (virObjectIsClass(anyobj, virObjectRWLockableClass)) {
- virObjectRWLockablePtr obj = anyobj;
- virRWLockRead(&obj->lock);
- return 0;
- } else {
- virObjectPtr obj = anyobj;
- VIR_WARN("Object %p (%s) is not a virObjectRWLockable instance",
- anyobj, obj ? obj->klass->name : "(unknown)");
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("unable to obtain rwlock for object=%p"), anyobj);
- }
- return -1;
+ virObjectRWLockablePtr obj = virObjectGetRWLockableObj(anyobj);
+
+ if (!obj)
+ return -1;
+
+ virRWLockRead(&obj->lock);
+ return 0;
}
@@ -454,18 +465,16 @@ virObjectLockRead(void *anyobj)
int
virObjectLockWrite(void *anyobj)
{
- if (virObjectIsClass(anyobj, virObjectRWLockableClass)) {
- virObjectRWLockablePtr obj = anyobj;
- virRWLockWrite(&obj->lock);
- return 0;
- } else {
- virObjectPtr obj = anyobj;
- VIR_WARN("Object %p (%s) is not a virObjectRWLockable instance",
- anyobj, obj ? obj->klass->name : "(unknown)");
+ virObjectRWLockablePtr obj = virObjectGetRWLockableObj(anyobj);
+
+ if (!obj) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("unable to obtain rwlock for object=%p"), anyobj);
+ return -1;
}
- return -1;
+
+ virRWLockWrite(&obj->lock);
+ return 0;
}
I don't really thing this is a good idea for reasons outlined in the
replies to earlier patches.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
11:38 a.m.
New subject: [libvirt] [PATCH 5/7] util: Introduce and use virObjectRWUnlock
Rather than overload virObjectUnlock as commit id '77f4593b' has
done, create a separate virObjectRWUnlock API that will force the
consumers to make the proper decision regarding unlocking the
RWLock's. This restores the virObjectUnlock code to using the
virObjectGetLockableObj as well.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/virdomainobjlist.c | 36 ++++++++++++++++++------------------
src/libvirt_private.syms | 1 +
src/util/virobject.c | 41 +++++++++++++++++++++++++++--------------
src/util/virobject.h | 4 ++++
4 files changed, 50 insertions(+), 32 deletions(-)
diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
index 08a51cc..85f5434 100644
--- a/src/conf/virdomainobjlist.c
+++ b/src/conf/virdomainobjlist.c
@@ -123,7 +123,7 @@ virDomainObjListFindByIDInternal(virDomainObjListPtr doms,
obj = virHashSearch(doms->objs, virDomainObjListSearchID, &id, NULL);
if (ref) {
virObjectRef(obj);
- virObjectUnlock(doms);
+ virObjectRWUnlock(doms);
}
if (obj) {
virObjectLock(obj);
@@ -135,7 +135,7 @@ virDomainObjListFindByIDInternal(virDomainObjListPtr doms,
}
}
if (!ref)
- virObjectUnlock(doms);
+ virObjectRWUnlock(doms);
return obj;
}
@@ -168,7 +168,7 @@ virDomainObjListFindByUUIDInternal(virDomainObjListPtr doms,
obj = virHashLookup(doms->objs, uuidstr);
if (ref) {
virObjectRef(obj);
- virObjectUnlock(doms);
+ virObjectRWUnlock(doms);
}
if (obj) {
virObjectLock(obj);
@@ -180,7 +180,7 @@ virDomainObjListFindByUUIDInternal(virDomainObjListPtr doms,
}
}
if (!ref)
- virObjectUnlock(doms);
+ virObjectRWUnlock(doms);
return obj;
}
@@ -210,7 +210,7 @@ virDomainObjPtr virDomainObjListFindByName(virDomainObjListPtr doms,
return NULL;
obj = virHashLookup(doms->objsName, name);
virObjectRef(obj);
- virObjectUnlock(doms);
+ virObjectRWUnlock(doms);
if (obj) {
virObjectLock(obj);
if (obj->removing) {
@@ -333,7 +333,7 @@ virDomainObjPtr virDomainObjListAdd(virDomainObjListPtr doms,
if (virObjectLockWrite(doms) < 0)
return NULL;
ret = virDomainObjListAddLocked(doms, def, xmlopt, flags, oldDef);
- virObjectUnlock(doms);
+ virObjectRWUnlock(doms);
return ret;
}
@@ -361,7 +361,7 @@ void virDomainObjListRemove(virDomainObjListPtr doms,
virHashRemoveEntry(doms->objsName, dom->def->name);
virObjectUnlock(dom);
virObjectUnref(dom);
- virObjectUnlock(doms);
+ virObjectRWUnlock(doms);
}
@@ -430,7 +430,7 @@ virDomainObjListRename(virDomainObjListPtr doms,
ret = 0;
cleanup:
- virObjectUnlock(doms);
+ virObjectRWUnlock(doms);
VIR_FREE(old_name);
return ret;
}
@@ -622,7 +622,7 @@ virDomainObjListLoadAllConfigs(virDomainObjListPtr doms,
}
VIR_DIR_CLOSE(dir);
- virObjectUnlock(doms);
+ virObjectRWUnlock(doms);
return ret;
}
@@ -669,7 +669,7 @@ virDomainObjListNumOfDomains(virDomainObjListPtr doms,
if (virObjectLockRead(doms) < 0)
return -1;
virHashForEach(doms->objs, virDomainObjListCount, &data);
- virObjectUnlock(doms);
+ virObjectRWUnlock(doms);
return data.count;
}
@@ -714,7 +714,7 @@ virDomainObjListGetActiveIDs(virDomainObjListPtr doms,
if (virObjectLockRead(doms) < 0)
return -1;
virHashForEach(doms->objs, virDomainObjListCopyActiveIDs, &data);
- virObjectUnlock(doms);
+ virObjectRWUnlock(doms);
return data.numids;
}
@@ -769,7 +769,7 @@ virDomainObjListGetInactiveNames(virDomainObjListPtr doms,
if (virObjectLockRead(doms) < 0)
return -1;
virHashForEach(doms->objs, virDomainObjListCopyInactiveNames, &data);
- virObjectUnlock(doms);
+ virObjectRWUnlock(doms);
if (data.oom) {
for (i = 0; i < data.numnames; i++)
VIR_FREE(data.names[i]);
@@ -811,7 +811,7 @@ virDomainObjListForEach(virDomainObjListPtr doms,
if (virObjectLockRead(doms) < 0)
return -1;
virHashForEach(doms->objs, virDomainObjListHelper, &data);
- virObjectUnlock(doms);
+ virObjectRWUnlock(doms);
return data.ret;
}
@@ -946,12 +946,12 @@ virDomainObjListCollect(virDomainObjListPtr domlist,
return -1;
sa_assert(domlist->objs);
if (VIR_ALLOC_N(data.vms, virHashSize(domlist->objs)) < 0) {
- virObjectUnlock(domlist);
+ virObjectRWUnlock(domlist);
return -1;
}
virHashForEach(domlist->objs, virDomainObjListCollectIterator, &data);
- virObjectUnlock(domlist);
+ virObjectRWUnlock(domlist);
virDomainObjListFilter(&data.vms, &data.nvms, conn, filter, flags);
@@ -991,7 +991,7 @@ virDomainObjListConvert(virDomainObjListPtr domlist,
if (skip_missing)
continue;
- virObjectUnlock(domlist);
+ virObjectRWUnlock(domlist);
virReportError(VIR_ERR_NO_DOMAIN,
_("no domain with matching uuid '%s'
(%s)"),
uuidstr, dom->name);
@@ -1001,12 +1001,12 @@ virDomainObjListConvert(virDomainObjListPtr domlist,
virObjectRef(vm);
if (VIR_APPEND_ELEMENT(*vms, *nvms, vm) < 0) {
- virObjectUnlock(domlist);
+ virObjectRWUnlock(domlist);
virObjectUnref(vm);
goto error;
}
}
- virObjectUnlock(domlist);
+ virObjectRWUnlock(domlist);
sa_assert(*vms);
virDomainObjListFilter(vms, nvms, conn, filter, flags);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index f1a6510..5e6b58c 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2310,6 +2310,7 @@ virObjectLockWrite;
virObjectNew;
virObjectRef;
virObjectRWLockableNew;
+virObjectRWUnlock;
virObjectUnlock;
virObjectUnref;
diff --git a/src/util/virobject.c b/src/util/virobject.c
index 0db98c3..6f786ce 100644
--- a/src/util/virobject.c
+++ b/src/util/virobject.c
@@ -480,26 +480,39 @@ virObjectLockWrite(void *anyobj)
/**
* virObjectUnlock:
- * @anyobj: any instance of virObjectLockable or virObjectRWLockable
+ * @anyobj: any instance of virObjectLockable
*
* Release a lock on @anyobj. The lock must have been acquired by
- * virObjectLock or virObjectLockRead.
+ * virObjectLock.
*/
void
virObjectUnlock(void *anyobj)
{
- if (virObjectIsClass(anyobj, virObjectLockableClass)) {
- virObjectLockablePtr obj = anyobj;
- virMutexUnlock(&obj->lock);
- } else if (virObjectIsClass(anyobj, virObjectRWLockableClass)) {
- virObjectRWLockablePtr obj = anyobj;
- virRWLockUnlock(&obj->lock);
- } else {
- virObjectPtr obj = anyobj;
- VIR_WARN("Object %p (%s) is not a virObjectLockable "
- "nor virObjectRWLockable instance",
- anyobj, obj ? obj->klass->name : "(unknown)");
- }
+ virObjectLockablePtr obj = virObjectGetLockableObj(anyobj);
+
+ if (!obj)
+ return;
+
+ virMutexUnlock(&obj->lock);
+}
+
+
+/**
+ * virObjectRWUnlock:
+ * @anyobj: any instance of virObjectRWLockable
+ *
+ * Release a lock on @anyobj. The lock must have been acquired by
+ * virObjectLockRead or virObjectLockWrite.
+ */
+void
+virObjectRWUnlock(void *anyobj)
+{
+ virObjectRWLockablePtr obj = virObjectGetRWLockableObj(anyobj);
+
+ if (!obj)
+ return;
+
+ virRWLockUnlock(&obj->lock);
}
diff --git a/src/util/virobject.h b/src/util/virobject.h
index f0d1f97..bd0fbb8 100644
--- a/src/util/virobject.h
+++ b/src/util/virobject.h
@@ -137,6 +137,10 @@ virObjectUnlock(void *lockableobj)
ATTRIBUTE_NONNULL(1);
void
+virObjectRWUnlock(void *lockableobj)
+ ATTRIBUTE_NONNULL(1);
+
+void
virObjectListFree(void *list);
void
--
2.9.4
12:07 p.m.
New subject: [libvirt] [PATCH 5/7] util: Introduce and use virObjectRWUnlock
On Fri, Jul 28, 2017 at 12:38:59PM -0400, John Ferlan wrote:
Rather than overload virObjectUnlock as commit id '77f4593b'
has
done, create a separate virObjectRWUnlock API that will force the
consumers to make the proper decision regarding unlocking the
RWLock's. This restores the virObjectUnlock code to using the
virObjectGetLockableObj as well.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/virdomainobjlist.c | 36 ++++++++++++++++++------------------
src/libvirt_private.syms | 1 +
src/util/virobject.c | 41 +++++++++++++++++++++++++++--------------
src/util/virobject.h | 4 ++++
4 files changed, 50 insertions(+), 32 deletions(-)
The virObjectLockRead and virObjectLockWrite should be probably renamed
to virObjectRWLockRead and virObjectRWLockWrite for consistency with the
virObjectRWUnlock.
Reviewed-by: Pavel Hrdina <phrdina(a)redhat.com>
3:26 a.m.
New subject: [libvirt] [PATCH 5/7] util: Introduce and use virObjectRWUnlock
On 07/28/2017 07:07 PM, Pavel Hrdina wrote:
On Fri, Jul 28, 2017 at 12:38:59PM -0400, John Ferlan wrote:
> Rather than overload virObjectUnlock as commit id '77f4593b' has
> done, create a separate virObjectRWUnlock API that will force the
> consumers to make the proper decision regarding unlocking the
> RWLock's. This restores the virObjectUnlock code to using the
> virObjectGetLockableObj as well.
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> src/conf/virdomainobjlist.c | 36 ++++++++++++++++++------------------
> src/libvirt_private.syms | 1 +
> src/util/virobject.c | 41 +++++++++++++++++++++++++++--------------
> src/util/virobject.h | 4 ++++
> 4 files changed, 50 insertions(+), 32 deletions(-)
The virObjectLockRead and virObjectLockWrite should be probably renamed
to virObjectRWLockRead and virObjectRWLockWrite for consistency with the
virObjectRWUnlock.
Reviewed-by: Pavel Hrdina <phrdina(a)redhat.com>
This is where I disagree. While you may dislike lock() polymorphism,
unlock() is used widely in literature for both mutexes and RW locks. And
it makes sense because they are fundamentally the same. I'm not going to
NACK this patch only because I find it impolite.
Michal
11:39 a.m.
New subject: [libvirt] [PATCH 6/7] util: Create common error path for invalid object
If virObjectIsClass fails "internally" to virobject.c, create a
macro to generate the VIR_WARN describing what the problem is.
Also improve the checks and message a bit to indicate which was
the failure - whether the obj was NULL or just not the right class
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/util/virobject.c | 25 +++++++++++++------------
1 file changed, 13 insertions(+), 12 deletions(-)
diff --git a/src/util/virobject.c b/src/util/virobject.c
index 6f786ce..2cf4743 100644
--- a/src/util/virobject.c
+++ b/src/util/virobject.c
@@ -47,6 +47,17 @@ struct _virClass {
virObjectDisposeCallback dispose;
};
+#define VIR_OBJECT_USAGE_PRINT_WARNING(anyobj, objclass) \
+ do { \
+ virObjectPtr obj = anyobj; \
+ if (!obj) \
+ VIR_WARN("Object cannot be NULL"); \
+ else \
+ VIR_WARN("Object %p (%s) is not a %s instance", \
+ anyobj, obj->klass->name, #objclass); \
+ } while (0)
+
+
static virClassPtr virObjectClass;
static virClassPtr virObjectLockableClass;
static virClassPtr virObjectRWLockableClass;
@@ -370,15 +381,10 @@ virObjectRef(void *anyobj)
static virObjectLockablePtr
virObjectGetLockableObj(void *anyobj)
{
- virObjectPtr obj;
-
if (virObjectIsClass(anyobj, virObjectLockableClass))
return anyobj;
- obj = anyobj;
- VIR_WARN("Object %p (%s) is not a virObjectLockable instance",
- anyobj, obj ? obj->klass->name : "(unknown)");
-
+ VIR_OBJECT_USAGE_PRINT_WARNING(anyobj, virObjectLockable);
return NULL;
}
@@ -386,15 +392,10 @@ virObjectGetLockableObj(void *anyobj)
static virObjectRWLockablePtr
virObjectGetRWLockableObj(void *anyobj)
{
- virObjectPtr obj;
-
if (virObjectIsClass(anyobj, virObjectRWLockableClass))
return anyobj;
- obj = anyobj;
- VIR_WARN("Object %p (%s) is not a virObjectRWLockable instance",
- anyobj, obj ? obj->klass->name : "(unknown)");
-
+ VIR_OBJECT_USAGE_PRINT_WARNING(anyobj, virObjectRWLockable);
return NULL;
}
--
2.9.4
12:08 p.m.
New subject: [libvirt] [PATCH 6/7] util: Create common error path for invalid object
On Fri, Jul 28, 2017 at 12:39:00PM -0400, John Ferlan wrote:
If virObjectIsClass fails "internally" to virobject.c,
create a
macro to generate the VIR_WARN describing what the problem is.
Also improve the checks and message a bit to indicate which was
the failure - whether the obj was NULL or just not the right class
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/util/virobject.c | 25 +++++++++++++------------
1 file changed, 13 insertions(+), 12 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina(a)redhat.com>
11:39 a.m.
New subject: [libvirt] [PATCH 7/7] util: Add safety net of checks to ensure valid object
The virObject logic "assumes" that whatever is passed to its API's
would be some sort of virObjectPtr; however, if it is not then some
really bad things can happen.
So far there's been only virObject{Ref|Unref}, virObject{Lock|Unlock},
and virObjectIsClass and the virObject and virObjectLockable class
consumers have been well behaved and code well tested. Soon there will
be more consumers and one such consumer tripped over this during testing
by passing a virHashTablePtr to virObjectIsClass which ends up calling
virClassIsDerivedFrom using "obj->klass", which wasn't really a klass
object causing one of those bad things to happen.
To avoid the future possibility that a non virObject class memory was
passed to some virObject* API, this patch adds two new checks - one
to validate that the object has the 0xCAFExxxx value in obj->->u.s.magic
and the other to ensure obj->u.s.magic doesn't "wrap" some day to
0xCAFF0000 if we ever get that many object classes. The check is also
moved before the name VIR_STRDUP to avoid the extra VIR_FREE that would
be required on the failure path.
It is still left up to the caller to handle the failed API calls just
as it would be if it passed a NULL opaque pointer anyobj.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/util/virobject.c | 26 +++++++++++++++++++-------
1 file changed, 19 insertions(+), 7 deletions(-)
diff --git a/src/util/virobject.c b/src/util/virobject.c
index 2cf4743..dd4c39a 100644
--- a/src/util/virobject.c
+++ b/src/util/virobject.c
@@ -47,14 +47,21 @@ struct _virClass {
virObjectDisposeCallback dispose;
};
+#define VIR_OBJECT_NOTVALID(obj) (!obj || ((obj->u.s.magic & 0xCAFE0000) !=
0xCAFE0000))
+
#define VIR_OBJECT_USAGE_PRINT_WARNING(anyobj, objclass) \
do { \
virObjectPtr obj = anyobj; \
- if (!obj) \
- VIR_WARN("Object cannot be NULL"); \
- else \
+ if (VIR_OBJECT_NOTVALID(obj)) { \
+ if (!obj) \
+ VIR_WARN("Object cannot be NULL"); \
+ else \
+ VIR_WARN("Object %p has a bad magic number %X", \
+ obj, obj->u.s.magic); \
+ } else { \
VIR_WARN("Object %p (%s) is not a %s instance", \
anyobj, obj->klass->name, #objclass); \
+ } \
} while (0)
@@ -177,9 +184,14 @@ virClassNew(virClassPtr parent,
goto error;
klass->parent = parent;
+ klass->magic = virAtomicIntInc(&magicCounter);
+ if (klass->magic > 0xCAFEFFFF) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("too many object classes defined"));
+ goto error;
+ }
if (VIR_STRDUP(klass->name, name) < 0)
goto error;
- klass->magic = virAtomicIntInc(&magicCounter);
klass->objectSize = objectSize;
klass->dispose = dispose;
@@ -331,7 +343,7 @@ virObjectUnref(void *anyobj)
{
virObjectPtr obj = anyobj;
- if (!obj)
+ if (VIR_OBJECT_NOTVALID(obj))
return false;
bool lastRef = virAtomicIntDecAndTest(&obj->u.s.refs);
@@ -370,7 +382,7 @@ virObjectRef(void *anyobj)
{
virObjectPtr obj = anyobj;
- if (!obj)
+ if (VIR_OBJECT_NOTVALID(obj))
return NULL;
virAtomicIntInc(&obj->u.s.refs);
PROBE(OBJECT_REF, "obj=%p", obj);
@@ -532,7 +544,7 @@ virObjectIsClass(void *anyobj,
virClassPtr klass)
{
virObjectPtr obj = anyobj;
- if (!obj)
+ if (VIR_OBJECT_NOTVALID(obj))
return false;
return virClassIsDerivedFrom(obj->klass, klass);
--
2.9.4
12:19 p.m.
New subject: [libvirt] [PATCH 7/7] util: Add safety net of checks to ensure valid object
On Fri, Jul 28, 2017 at 12:39:01PM -0400, John Ferlan wrote:
The virObject logic "assumes" that whatever is passed to
its API's
would be some sort of virObjectPtr; however, if it is not then some
really bad things can happen.
So far there's been only virObject{Ref|Unref}, virObject{Lock|Unlock},
and virObjectIsClass and the virObject and virObjectLockable class
consumers have been well behaved and code well tested. Soon there will
be more consumers and one such consumer tripped over this during testing
by passing a virHashTablePtr to virObjectIsClass which ends up calling
virClassIsDerivedFrom using "obj->klass", which wasn't really a klass
object causing one of those bad things to happen.
To avoid the future possibility that a non virObject class memory was
passed to some virObject* API, this patch adds two new checks - one
to validate that the object has the 0xCAFExxxx value in obj->->u.s.magic
and the other to ensure obj->u.s.magic doesn't "wrap" some day to
0xCAFF0000 if we ever get that many object classes. The check is also
moved before the name VIR_STRDUP to avoid the extra VIR_FREE that would
be required on the failure path.
This doesn't add any safeguard and for most virObject(Ref|Unref) calls
we don't check the return value. This is basically a programming error
as well and we should consider start using abort().
It is still left up to the caller to handle the failed API calls just
as it would be if it passed a NULL opaque pointer anyobj.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/util/virobject.c | 26 +++++++++++++++++++-------
1 file changed, 19 insertions(+), 7 deletions(-)
diff --git a/src/util/virobject.c b/src/util/virobject.c
index 2cf4743..dd4c39a 100644
--- a/src/util/virobject.c
+++ b/src/util/virobject.c
@@ -47,14 +47,21 @@ struct _virClass {
virObjectDisposeCallback dispose;
};
+#define VIR_OBJECT_NOTVALID(obj) (!obj || ((obj->u.s.magic & 0xCAFE0000) !=
0xCAFE0000))
This is not correct, it should be:
((obj->u.s.magic & 0xFFFF0000) != 0xCAFE0000)
+
#define VIR_OBJECT_USAGE_PRINT_WARNING(anyobj, objclass) \
do { \
virObjectPtr obj = anyobj; \
- if (!obj) \
- VIR_WARN("Object cannot be NULL"); \
- else \
+ if (VIR_OBJECT_NOTVALID(obj)) { \
+ if (!obj) \
+ VIR_WARN("Object cannot be NULL"); \
+ else \
+ VIR_WARN("Object %p has a bad magic number %X", \
+ obj, obj->u.s.magic); \
+ } else { \
VIR_WARN("Object %p (%s) is not a %s instance", \
anyobj, obj->klass->name, #objclass); \
+ } \
} while (0)
@@ -177,9 +184,14 @@ virClassNew(virClassPtr parent,
goto error;
klass->parent = parent;
+ klass->magic = virAtomicIntInc(&magicCounter);
+ if (klass->magic > 0xCAFEFFFF) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("too many object classes defined"));
+ goto error;
+ }
This will probably never happen :).
Pavel
1:39 p.m.
New subject: [libvirt] [PATCH 7/7] util: Add safety net of checks to ensure valid object
On 07/28/2017 01:19 PM, Pavel Hrdina wrote:
On Fri, Jul 28, 2017 at 12:39:01PM -0400, John Ferlan wrote:
> The virObject logic "assumes" that whatever is passed to its API's
> would be some sort of virObjectPtr; however, if it is not then some
> really bad things can happen.
>
> So far there's been only virObject{Ref|Unref}, virObject{Lock|Unlock},
> and virObjectIsClass and the virObject and virObjectLockable class
> consumers have been well behaved and code well tested. Soon there will
> be more consumers and one such consumer tripped over this during testing
> by passing a virHashTablePtr to virObjectIsClass which ends up calling
> virClassIsDerivedFrom using "obj->klass", which wasn't really a
klass
> object causing one of those bad things to happen.
>
> To avoid the future possibility that a non virObject class memory was
> passed to some virObject* API, this patch adds two new checks - one
> to validate that the object has the 0xCAFExxxx value in obj->->u.s.magic
> and the other to ensure obj->u.s.magic doesn't "wrap" some day to
> 0xCAFF0000 if we ever get that many object classes. The check is also
> moved before the name VIR_STRDUP to avoid the extra VIR_FREE that would
> be required on the failure path.
This doesn't add any safeguard and for most virObject(Ref|Unref) calls
we don't check the return value. This is basically a programming error
as well and we should consider start using abort().
And yes, this is the programming error - it was awfully stupid, but IIRC
it also didn't "jump out" right away what the problem is/was. It was far
worse for *Ref/Unref, but lock was interesting too insomuch as I
wouldn't get the lock so perhaps two threads would make a change at the
same time and we may never know unless we check lock status.
>
> It is still left up to the caller to handle the failed API calls just
> as it would be if it passed a NULL opaque pointer anyobj.
And of course this was my escape clause because of void Lock's.
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> src/util/virobject.c | 26 +++++++++++++++++++-------
> 1 file changed, 19 insertions(+), 7 deletions(-)
>
> diff --git a/src/util/virobject.c b/src/util/virobject.c
> index 2cf4743..dd4c39a 100644
> --- a/src/util/virobject.c
> +++ b/src/util/virobject.c
> @@ -47,14 +47,21 @@ struct _virClass {
> virObjectDisposeCallback dispose;
> };
>
> +#define VIR_OBJECT_NOTVALID(obj) (!obj || ((obj->u.s.magic & 0xCAFE0000) !=
0xCAFE0000))
This is not correct, it should be:
((obj->u.s.magic & 0xFFFF0000) != 0xCAFE0000)
Oh right and I see it was a straight cut-n-paste from Dan's reply too:
https://www.redhat.com/archives/libvir-list/2017-May/msg01211.html
> +
> #define VIR_OBJECT_USAGE_PRINT_WARNING(anyobj, objclass) \
> do { \
> virObjectPtr obj = anyobj; \
> - if (!obj) \
> - VIR_WARN("Object cannot be NULL");
\
> - else \
> + if (VIR_OBJECT_NOTVALID(obj)) { \
> + if (!obj) \
> + VIR_WARN("Object cannot be NULL");
\
> + else \
> + VIR_WARN("Object %p has a bad magic number %X",
\
> + obj, obj->u.s.magic); \
> + } else { \
> VIR_WARN("Object %p (%s) is not a %s instance",
\
> anyobj, obj->klass->name, #objclass); \
> + } \
> } while (0)
>
>
> @@ -177,9 +184,14 @@ virClassNew(virClassPtr parent,
> goto error;
>
> klass->parent = parent;
> + klass->magic = virAtomicIntInc(&magicCounter);
> + if (klass->magic > 0xCAFEFFFF) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("too many object classes defined"));
> + goto error;
> + }
This will probably never happen :).
Pavel
Agreed, but if it does happen then I'd be the last to touch right... So
I'd get the blame. In any case, another one of those Dan suggestions:
https://www.redhat.com/archives/libvir-list/2017-May/msg01221.html
If this is not desired - that's fine I can drop it. Cannot say I didn't
try though.
Thanks for the quick review! With a freeze on at least this may give
time for others to chime in with thoughts as well.
John
5:19 a.m.
New subject: [libvirt] [PATCH 7/7] util: Add safety net of checks to ensure valid object
On Fri, Jul 28, 2017 at 12:39:01PM -0400, John Ferlan wrote:
The virObject logic "assumes" that whatever is passed to
its API's
would be some sort of virObjectPtr; however, if it is not then some
really bad things can happen.
So far there's been only virObject{Ref|Unref}, virObject{Lock|Unlock},
and virObjectIsClass and the virObject and virObjectLockable class
consumers have been well behaved and code well tested. Soon there will
be more consumers and one such consumer tripped over this during testing
by passing a virHashTablePtr to virObjectIsClass which ends up calling
virClassIsDerivedFrom using "obj->klass", which wasn't really a klass
object causing one of those bad things to happen.
To avoid the future possibility that a non virObject class memory was
passed to some virObject* API, this patch adds two new checks - one
to validate that the object has the 0xCAFExxxx value in obj->->u.s.magic
and the other to ensure obj->u.s.magic doesn't "wrap" some day to
0xCAFF0000 if we ever get that many object classes. The check is also
moved before the name VIR_STRDUP to avoid the extra VIR_FREE that would
be required on the failure path.
It is still left up to the caller to handle the failed API calls just
as it would be if it passed a NULL opaque pointer anyobj.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/util/virobject.c | 26 +++++++++++++++++++-------
1 file changed, 19 insertions(+), 7 deletions(-)
diff --git a/src/util/virobject.c b/src/util/virobject.c
index 2cf4743..dd4c39a 100644
--- a/src/util/virobject.c
+++ b/src/util/virobject.c
@@ -47,14 +47,21 @@ struct _virClass {
virObjectDisposeCallback dispose;
};
+#define VIR_OBJECT_NOTVALID(obj) (!obj || ((obj->u.s.magic & 0xCAFE0000) !=
0xCAFE0000))
+
#define VIR_OBJECT_USAGE_PRINT_WARNING(anyobj, objclass) \
do { \
virObjectPtr obj = anyobj; \
- if (!obj) \
- VIR_WARN("Object cannot be NULL"); \
- else \
+ if (VIR_OBJECT_NOTVALID(obj)) { \
+ if (!obj) \
+ VIR_WARN("Object cannot be NULL"); \
+ else \
+ VIR_WARN("Object %p has a bad magic number %X", \
+ obj, obj->u.s.magic); \
+ } else { \
VIR_WARN("Object %p (%s) is not a %s instance", \
anyobj, obj->klass->name, #objclass); \
+ } \
} while (0)
@@ -177,9 +184,14 @@ virClassNew(virClassPtr parent,
goto error;
klass->parent = parent;
+ klass->magic = virAtomicIntInc(&magicCounter);
+ if (klass->magic > 0xCAFEFFFF) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("too many object classes defined"));
+ goto error;
+ }
if (VIR_STRDUP(klass->name, name) < 0)
goto error;
- klass->magic = virAtomicIntInc(&magicCounter);
klass->objectSize = objectSize;
klass->dispose = dispose;
@@ -331,7 +343,7 @@ virObjectUnref(void *anyobj)
{
virObjectPtr obj = anyobj;
- if (!obj)
+ if (VIR_OBJECT_NOTVALID(obj))
return false;
bool lastRef = virAtomicIntDecAndTest(&obj->u.s.refs);
@@ -370,7 +382,7 @@ virObjectRef(void *anyobj)
{
virObjectPtr obj = anyobj;
- if (!obj)
+ if (VIR_OBJECT_NOTVALID(obj))
return NULL;
virAtomicIntInc(&obj->u.s.refs);
PROBE(OBJECT_REF, "obj=%p", obj);
@@ -532,7 +544,7 @@ virObjectIsClass(void *anyobj,
virClassPtr klass)
{
virObjectPtr obj = anyobj;
- if (!obj)
+ if (VIR_OBJECT_NOTVALID(obj))
return false;
I really don't think these changes are a positive move.
If you have code that is passing in something that is not a valid object,
then silently doing nothing in virObjectRef / virObjectIsClass is not
going to make the code any more correct. In fact you're turning something
that could be an immediate crash (and thus easy to diagnose) into
something that could be silent bad behaviour, which is much harder to
diagnose, or cause a crash much further away from the original root bug.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
7:11 a.m.
New subject: [libvirt] [PATCH 7/7] util: Add safety net of checks to ensure valid object
[...]
I really don't think these changes are a positive move.
If you have code that is passing in something that is not a valid object,
then silently doing nothing in virObjectRef / virObjectIsClass is not
going to make the code any more correct. In fact you're turning something
that could be an immediate crash (and thus easy to diagnose) into
something that could be silent bad behaviour, which is much harder to
diagnose, or cause a crash much further away from the original root bug.
Consider the longevity of the patch being on list - I cannot remember
all the details, although the commit message does help a bit. I do
recall looking at the code and thinking incorrect usage could lead down
the road of bad things.
For Ref/Unref all that has been checked is !obj and we Incr/Decr a
location. For Lock/Unlock as described in my 1/7 response class checking
is/was added.
Adding in object validity for Ref/Unref at least avoids rogue corruption
which is really hard to diagnose in favor of leaving an entrail. Even
without the additional check, the @obj someone may have thought they
were incr/decr the refcnt isn't going to happen.
Still, it just seems better in the event that we don't have a real @obj
to at least message that in the hopes that someone trips across it.
Similarly for Lock/Unlock same thing.
IMO: The patches aren't making it harder to diagnose a problem - they're
helping and they're not altering the value of some field for a valid
@obj address.
But if that's not desired, then fine - at least attempt was made and the
feedback has been provided.
Tks -
John
2671
days inactive
2674
days old
26 comments
4 participants
participants (4)
-
Daniel P. Berrange -
John Ferlan -
Michal Privoznik -
Pavel Hrdina