| Message ID | 7a64d6c3b0c922b098bcbabfcebe88fcf45c47d6.1466641775.git.crobinso@redhat.com |
|---|---|
| State | Accepted |
| Commit | e808d3f227b8fa461eb7104fd4382ea817152cbb |
| Headers | show |
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 - Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 06/23/2016 10:06 AM, Martin Kletzander wrote: > 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. > Thanks guys, I've pushed this - Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index e2d729f..4b5419d 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2422,20 +2422,18 @@ storageVolUpload(virStorageVolPtr obj, goto cleanup; } - /* If we have a refreshPool, use the callback routine in order to + /* Use the callback routine in order to * refresh the pool after the volume upload stream closes. This way * we make sure the volume and pool data are refreshed without user * interaction and we can just lookup the backend in the callback * routine in order to call the refresh API. */ - if (backend->refreshPool) { - if (VIR_ALLOC(cbdata) < 0 || - VIR_STRDUP(cbdata->pool_name, pool->def->name) < 0) - goto cleanup; - if (vol->target.type == VIR_STORAGE_VOL_PLOOP && - VIR_STRDUP(cbdata->vol_path, vol->target.path) < 0) - goto cleanup; - } + if (VIR_ALLOC(cbdata) < 0 || + VIR_STRDUP(cbdata->pool_name, pool->def->name) < 0) + goto cleanup; + if (vol->target.type == VIR_STORAGE_VOL_PLOOP && + VIR_STRDUP(cbdata->vol_path, vol->target.path) < 0) + goto cleanup; if ((ret = backend->uploadVol(obj->conn, pool, vol, stream, offset, length, flags)) < 0)