[libvirt] [PATCH] bhyve: fix virObjectUnlock() usage

[Roman Bogorodskiy]

  Daniel P. Berrange wrote:

> On Mon, May 19, 2014 at 11:12:05AM -0600, Eric Blake wrote:
> > On 05/19/2014 10:57 AM, Roman Bogorodskiy wrote:
> > >   Eric Blake wrote:
> > > 
> > >> On 05/17/2014 01:44 PM, Roman Bogorodskiy wrote:
> > >>> In a number of places in the bhyve driver, virObjectUnlock()
> > >>> is called with an arg without check if the arg is non-NULL, which
> > >>> could result in passing NULL value and a warning like:
> > >>>
> > >>> virObjectUnlock:340 : Object 0x0 ((unknown)) is not a virObjectLockable instance
> > >>
> > >> Doesn't this instead argue that we should fix virObjectUnlock to
> > >> gracefully handle a NULL parameter, rather than making every caller uglier?
> > > 
> > > Calling it with NULL seems to be harmless anyway and the only problem
> > > is this annoying warning.
> > > 
> > > So, what you propose is checking explicitly for NULL in
> > > virObjectUnlock before we do virObjectIsClass(), and if the passed
> > > argument is NULL indeed, just return, without logging anything about
> > > that?
> > 
> > Yes, since we have other virObject code that special cases NULL (for
> > example, virObjectUnref).
> 
> IMHO passing NULL into the locking APIs is a coding error we shouldn't
> try to paper over by ignoring.
> 
> Ultimately I think the real flaw is the way we obtain the virDomainPtr
> pointers in the first place. ie the virDomainObjListLookup functions
> don't acquire a reference on the object they return. So the caller has
> to worry about the object being released behind their back, hence all
> our logic which has to set 'vm = NULL' in various places where it might
> have been deleted.
> 
> IOW, I'd much rather we looked at changing our design here so that we
> didn't have so much NULL vm pointers in the first place.

Eric, could you please comment on that?

Roman Bogorodskiy
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140521/e017c9de/attachment-0001.sig>