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@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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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@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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[...]
>> --- 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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.