Re: [PATCH] storage: Refresh storage pool after upload

[Peter Krempa <pkrempa@xxxxxxxxxx>]

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@xxxxxxxxxx>
> ---
>  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.

> +    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

Attachment: signature.asc
Description: OpenPGP digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list