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

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.