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
Attachment:
pgpS9x_CvNXBs.pgp
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list