9:05 a.m.
On 07/29/2014 09:28 AM, Peter Krempa wrote:
On 07/28/14 15:24, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1072653
>
> Upon successful upload of a volume, the target volume and storage pool
> were not updated to reflect any changes as a result of the upload. Make
> use of the existing stream close callback mechanism to force a backend
> pool refresh to occur once the stream closes.
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> src/libvirt.c | 8 +++++
> src/libvirt_private.syms | 1 +
> src/storage/storage_driver.c | 78 ++++++++++++++++++++++++++++++++++++++++++++
> tools/virsh.pod | 3 ++
> 4 files changed, 90 insertions(+)
>
> diff --git a/src/libvirt.c b/src/libvirt.c
> index 143d319..992e4f2 100644
> --- a/src/libvirt.c
> +++ b/src/libvirt.c
> @@ -13960,6 +13960,14 @@ virStorageVolDownload(virStorageVolPtr vol,
> * detect any errors. The results will be unpredictable if
> * another active stream is writing to the storage volume.
> *
> + * When the data stream is closed whether the upload is successful
> + * or not the target storage pool will be refreshed to reflect pool
> + * and volume changes as a result of the upload. Depending on
> + * the target volume storage backend and the source stream type
> + * for a successful upload, the target volume may take on the
> + * characteristics from the source stream such as format type,
> + * capacity, and allocation.
> + *
> * Returns 0, or -1 upon error.
> */
> int
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index b1fb7c9..7ef68a1 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -825,6 +825,7 @@ virFDStreamCreateFile;
> virFDStreamOpen;
> virFDStreamOpenFile;
> virFDStreamOpenPTY;
> +virFDStreamSetInternalCloseCb;
>
>
> # libvirt_internal.h
> diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
> index efbe5ff..5fd5514 100644
> --- a/src/storage/storage_driver.c
> +++ b/src/storage/storage_driver.c
> @@ -59,6 +59,12 @@ static virStorageDriverStatePtr driverState;
>
> static int storageStateCleanup(void);
>
> +typedef struct _virStorageVolStreamInfo virStorageVolStreamInfo;
> +typedef virStorageVolStreamInfo *virStorageVolStreamInfoPtr;
> +struct _virStorageVolStreamInfo {
> + char *pool_name;
> +};
> +
> static void storageDriverLock(virStorageDriverStatePtr driver)
> {
> virMutexLock(&driver->lock);
> @@ -1956,6 +1962,52 @@ storageVolDownload(virStorageVolPtr obj,
> }
>
>
> +/**
> + * Frees opaque data provided for the stream closing callback
> + *
> + * @opaque Data to be freed.
> + */
> +static void virStorageVolFDStreamCloseCbFree(void *opaque)
> +{
> + virStorageVolStreamInfoPtr cbdata = opaque;
> +
> + VIR_FREE(cbdata->pool_name);
> + VIR_FREE(cbdata);
> +}
> +
> +/**
> + * Callback being called if a FDstream is closed. Frees device entries
> + * from data structures and removes lockfiles.
> + *
> + * @st Pointer to stream being closed.
> + * @opaque Domain's device information structure.
> + */
> +static void virStorageVolFDStreamCloseCb(virStreamPtr st ATTRIBUTE_UNUSED,
> + void *opaque)
> +{
> +
> + virStorageVolStreamInfoPtr cbdata = opaque;
> + virStoragePoolObjPtr pool = NULL;
> + virStorageBackendPtr backend;
> +
> + storageDriverLock(driverState);
I'm a bit concerned about locking the storage driver here. This code
will be called by the stream close callback which is handled from the
event loop. While I don't currently see any problem, this instance could
bite us in the future.
> + if (!(pool = virStoragePoolObjFindByName(&driverState->pools,
> + cbdata->pool_name)))
> + goto cleanup;
> +
> + if (!(backend = virStorageBackendForType(pool->def->type)))
> + goto cleanup;
> +
> + virStoragePoolObjClearVols(pool);
Hmmm, having a single volume update func would be nice here.
Right, but a change to the volume could mean the pool data is off
(allocation, capacity), right? or is that calculated "on the fly" as a
result of each volume?
It's possible to pass the volume name along through here as well and I
did so for my initial iterations; however, refreshVol() is implemented
in far less storage pools than refreshPool().
John
> + if (backend->refreshPool(NULL, pool) < 0)
> + VIR_DEBUG("Failed to refresh storage pool");
> +
> + cleanup:
> + if (pool)
> + virStoragePoolObjUnlock(pool);
> + storageDriverUnlock(driverState);
> +}
> +
> static int
> storageVolUpload(virStorageVolPtr obj,
> virStreamPtr stream,
ACK, but please push this after the release so that we can give it a
cycle of testing before releasing it.
Peter