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