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

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&gt; > --- > 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.

ACK, but please push this after the release so that we can give it a cycle of testing before releasing it.