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@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. > 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 > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list