[libvirt] [PATCH] storage: Remove redundant refreshPool check
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 --- src/storage/storage_driver.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) 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) -- 2.7.4
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.
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
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, John
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.
John
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
On 06/22/2016 08:29 PM, 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 --- src/storage/storage_driver.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-)
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)
Of course my Coverity checker just pointed out that the subsequent "if (cbdata)" below here is not necessary now since cbdata will always be allocated <sigh>. John
participants (3)
-
Cole Robinson -
John Ferlan -
Martin Kletzander