Re: [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