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

On 05/21/2014 08:02 AM, Roman Bogorodskiy wrote: >>>> 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? I trust Daniel's judgment here, since he wrote virObjectPtr and virObjectUnlock. Cleaning up the DomainObjListLookup function to grab a reference will have a big ripple effect, so it probably doesn't need to be done now (that is, you don't necessarily have to be the one doing the cleanup he proposes); but I guess that means for the present that we are still stuck with the current design pattern of checking for NULL ourself before calling virObjectUnlock.