On 07/29/14 16:05, John Ferlan wrote: > > > 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? AFAIK that is refreshed before retrieval as running VMs with storage in the pool may change that anyways. > > 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(). Hmm by using the thread as suggested by Dan you still may use refreshPool even if it does a ton of stuff. > > 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 >
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list