Re: [libvirt] [PATCH] storage: Remove redundant refreshPool check

On Thu, Jun 23, 2016 at 09:20:19AM -0400, John Ferlan wrote: > > > On 06/23/2016 09:03 AM, Cole Robinson wrote: >> On 06/23/2016 03:32 AM, Martin Kletzander wrote: >>> On Wed, Jun 22, 2016 at 08:29:35PM -0400, Cole Robinson wrote: >>>> Every driver provides a refreshPool impl, and many other critical >>>> places in the code unconditionally call it without checking if >>>> it exists, so this check is pointless >>> >>> I'm not entirely sure about it, but it'd be nicer if we actually checked >>> that it's non-NULL. Just to future-proof the code in case someone adds >>> another backend. >> >> Please check the other storage_driver.c code... every 'startPool' invocation >> is followed by an uncondtional refreshPool call. If a driver is added without >> a refreshPool impl, it will crash libvirtd from any avenue that the pool can >> be started, so to support a driver like that will need much more work. This is >> the one place in the code that checks for backend->refreshPool > > > Hmm.. this check was caused by commit id '4a85bf3e2' where IIRC I was > probably being really paranoid. > > Digging a bit more finds commit id '318ea3cb77' which seems to indicate > refreshPool *must* be supplied. > > So ACK to the change, > Fair enough, sorry for the noise, ACK.