1:56 p.m.
On 04/07/2011 03:41 AM, Hu Tao wrote:
>> @@ -1010,6 +1007,11 @@ static virDomainObjPtr
virDomainObjNew(virCapsPtr caps)
>> return NULL;
>> }
>>
>> + if (virObjectInit(&domain->obj, doDomainObjFree)) {
>> + VIR_FREE(domain);
>> + return NULL;
>
> Hmm. virObjectInit used VIR_ERROR, which logs, but doesn't call into
> virtError. By returning NULL here, a user will get the dreaded "Unknown
> error" message since we didn't hook into the virterror machinery.
> Should virObjectInit instead be using virReportErrorHelper? Or is it
How to use virReportErrorHelper in this case?
> considered a coding bug to ever have virObjectInit fail in the first
> place, so we don't really have to worry about that.
Sorry for my bad English, I don't understand this sentence.
Basically, I was envisioning:
# define virObjectReportError(code, ...) \
virReportErrorHelper(NULL, VIR_FROM_NONE, code, __FILE__, \
__FUNCTION__, __LINE__, __VA_ARGS__)
and calling virObjectReportError() instead of VIR_ERROR() for any
failure inside of virobject.c. VIR_ERROR only hooks up to the log file,
but virReportErrorHelper _also_ hooks up to the remote procedure call
(which means that if the error happens due to someone making an API
call, they get the message back rather than just a cryptic "Unknown error").
But if we can guarantee that the only possible errors are due to coding
mistakes (and in reality, that's true), then I'd almost go so far as to
do make virObjectInit return void instead of int (callers need not check
for failure), and inside virObjectInit do:
if (obj->free) {
VIR_ERROR ("Coding bug encountered, aborting");
abort ();
}
Gnulib already has uses of abort() in places that can only be reached by
pure coding bugs, even though libvirt.git does not currently have
anything like that.
On the other hand, there's a difference between errors in
virObjectInit() (which are only possible due to severe coding bugs and
easy to audit that they will never hit without ever risking an abort()
escaping into released code) vs. errors in virObjectUnref() (if we've
unreferenced an object too many times, it's hard to tell which place in
the code was the culprit, so the bug may escape in to a release and a
library should avoid abort() at all costs in released code). At any
rate, any time we hit a reference counting bug, it is in our best
interest to do something loud and obvious that a bug occurred, to make
it easier to fix the bug, rather than trying to proceed with invalid data.
> The only potential drawback to that is that if you use the wrong
> function, the referencing doesn't happen. Or maybe even make
> virHashCreateFull take a bool parameter of whether the data must be a
> virObjectPtr, so you don't have to wrap virHashAddObjEntry (and since
> virHashRemoveEntry doesn't really have any way to create a
> virHashRemoveObjEntry wrapper, but it would need to be in on the game of
> automatic reference count manipulations any time we know the table
> hashes only virObjects as data).
Would it be better to have two types of hashtable, one hashes only
virObjects, the other hashes data except virObjects? this can minimize
the impact brought by the change of hashtable to existing code.
Proving something is a virObject can be done via type-safe wrapper
functions, but only for functions that take a data argument in the first
place (virHashRemoveEntry does not). But in the opposite direction, how
can you prove something is not a virObject? I don't think you can
forbid that (nor do you necessarily want to; it might make sense to hash
void* which happens to be virObject but where the container does not
plan on owning the object). I think the best you can do is adding
helper methods that operate only if you request and promise to only use
virObject as the data, and adding a bool flag to virHashCreateFull seems
the least invasive way to do it.
If a dom is added into a hashtable, then it is not safe to modify
its
uuid since it is the key of dom in hashtable. And we have to make some
rules about when the uuid can be modified(say, hold the driver lock
first when dom is in hashtable).
Yes, for a given object, especially if that object is hashed by a subset
of itself, then the data being used as a hash key must not be modified.
And we don't modify domain uuid's after creation (if we need a new
uuid, we are creating a new domain rather than modifying an existing
one). But without documenting which fields are constant and can be
accessed without obtaining a lock and which must not be modified after
the constructor, compared to the remaining fields that must only be read
or written while holding the lock, is on a case-by-case basis per struct
that inherits from virObject.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org