On Mon, May 23, 2011 at 5:56 PM, Adam Litke <agl(a)us.ibm.com&gt; wrote: > /** >  * virDomainBlockPull: >  * @dom: pointer to domain object >  * @path: Fully-qualified filename of disk >  * @cursor: pointer to a virDomainBlockPullCursor, or NULL >  * @flags: One of virDomainBlockPullFlags, or zero >  * >  * Populate a disk image with data from its backing image.  Once all data from >  * its backing image has been pulled, the disk no longer depends on a backing >  * image. >  * >  * If VIR_DOMAIN_BLOCK_PULL_CONTINUOUS is specified, the entire device will be >  * streamed in the background.  Otherwise, the value stored in @cursor will be >  * used to stream an increment. >  * >  * Returns -1 in case of failure, 0 when successful.  On success and when @flags >  * does not contain VIR_DOMAIN_BLOCK_PULL_CONTINUOUS, the iterator @cursor will >  * be updated to the proper value for use in a subsequent call. >  */ > int                 virDomainBlockPull(virDomainPtr dom, >                                       const char *path, >                                       virDomainBlockPullCursor *cursor, >                                       unsigned int flags); If this function is used without VIR_DOMAIN_BLOCK_PULL_CONTINUOUS then the "end" value is unknown. Therefore it is not possible to calculate streaming progress. Perhaps instead of cursor we need a virDomainBlockStreamInfoPtr info argument?

> NOTE: Qemu will emit an asynchronous event (VIR_DOMAIN_BLOCK_PULL_COMPLETED) > after any operation that eliminates the dependency on the backing file.  This > could be a virDomainBlockPull() without VIR_DOMAIN_BLOCK_PULL_CONTINUOUS and > will allow libvirt to manage backing file security regardless of the pull mode > used. Currently QEMU does not change the backing file when incremental streaming is used (instead of continuous). This is because it is hard to know whether or not the entire file has finished streaming - it's an operation that requires traversing all block allocation metadata to check that the backing file is no longer necessary.

On Tue, May 24, 2011 at 01:00:04PM +0100, Stefan Hajnoczi wrote: > On Mon, May 23, 2011 at 5:56 PM, Adam Litke <agl(a)us.ibm.com&gt; wrote: >> /** >> * virDomainBlockPull: >> * @dom: pointer to domain object >> * @path: Fully-qualified filename of disk >> * @cursor: pointer to a virDomainBlockPullCursor, or NULL >> * @flags: One of virDomainBlockPullFlags, or zero >> * >> * Populate a disk image with data from its backing image. Once all data from >> * its backing image has been pulled, the disk no longer depends on a backing >> * image. >> * >> * If VIR_DOMAIN_BLOCK_PULL_CONTINUOUS is specified, the entire device will be >> * streamed in the background. Otherwise, the value stored in @cursor will be >> * used to stream an increment. >> * >> * Returns -1 in case of failure, 0 when successful. On success and when @flags >> * does not contain VIR_DOMAIN_BLOCK_PULL_CONTINUOUS, the iterator @cursor will >> * be updated to the proper value for use in a subsequent call. >> */ >> int virDomainBlockPull(virDomainPtr dom, >> const char *path, >> virDomainBlockPullCursor *cursor, >> unsigned int flags); > > If this function is used without VIR_DOMAIN_BLOCK_PULL_CONTINUOUS then > the "end" value is unknown. Therefore it is not possible to calculate > streaming progress. Perhaps instead of cursor we need a > virDomainBlockStreamInfoPtr info argument? It almost feels like we should just not overload semantics using flags and have a separate APIs: Incremental, just iterate on: int virDomainBlockPull(virDomainPtr dom, const char *path, virDomainBlockPullInfoPtr *pos,

unsigned int flags); Continuous, invoke once: int virDomainBlockPullAll(virDomainPtr dom, const char *path, unsigned int flags); ...and wait for the async event notification for completion, or optionally poll on virDomainGetBlockPullInfo, or use BlockPullAbort()

>> NOTE: Qemu will emit an asynchronous event (VIR_DOMAIN_BLOCK_PULL_COMPLETED) >> after any operation that eliminates the dependency on the backing file. This >> could be a virDomainBlockPull() without VIR_DOMAIN_BLOCK_PULL_CONTINUOUS and >> will allow libvirt to manage backing file security regardless of the pull mode >> used. > > Currently QEMU does not change the backing file when incremental > streaming is used (instead of continuous). This is because it is hard > to know whether or not the entire file has finished streaming - it's > an operation that requires traversing all block allocation metadata to > check that the backing file is no longer necessary. Having different semantics for what happens at the end of streaming for incremental vs continuous is a really bad idea. I assume this limitation will be fixed in QEMU before streaming is finally merged ?

Here is version 4 of the BlockStream API (now called BlockPull). Hopefully this version addresses all remaining concerns and I can begin to work on the code. Does everyone approve of the new name 'virDomainBlockPull'? Changes since V3: - Renamed the API to 'Block Pull' to emphasize the effect of the API over its method of operation. - Changed virDomainGetBlockPullInfo() to accept a path argument and to return information about that path alone. - Added a new virDomainEvent to report final status when using CONTINUOUS mode. /* * Block Pull API */ typedef enum { /* * If set, virDomainBlockPull() will operate on the entire device in the * background. The status can be checked and the operation aborted by * using the functions virDomainBlockPullInfo() and * virDomainBlockPullAbort(). */ VIR_DOMAIN_BLOCK_PULL_CONTINUOUS = 1, } virDomainBlockPullFlags; /* An iterator for initiating and monitoring block pull operations */ typedef unsigned long long virDomainBlockPullCursor; typedef struct _virDomainBlockPullInfo virDomainBlockPullInfo; struct _virDomainBlockPullInfo { /* * The following fields provide an indication of block pull progress. @cur * indicates the current position and will be between 0 and @end. @end is * the final cursor position for this operation and represents completion. * To approximate progress, divide @cur by @end. */ virDomainBlockPullCursor cur; virDomainBlockPullCursor end; }; typedef virDomainBlockPullInfo *virDomainBlockPullInfoPtr; /** * virDomainBlockPull: * @dom: pointer to domain object * @path: Fully-qualified filename of disk * @cursor: pointer to a virDomainBlockPullCursor, or NULL * @flags: One of virDomainBlockPullFlags, or zero * * Populate a disk image with data from its backing image. Once all data from * its backing image has been pulled, the disk no longer depends on a backing * image. * * If VIR_DOMAIN_BLOCK_PULL_CONTINUOUS is specified, the entire device will be * streamed in the background. Otherwise, the value stored in @cursor will be * used to stream an increment. * * Returns -1 in case of failure, 0 when successful. On success and when @flags * does not contain VIR_DOMAIN_BLOCK_PULL_CONTINUOUS, the iterator @cursor will * be updated to the proper value for use in a subsequent call. */ int virDomainBlockPull(virDomainPtr dom, const char *path, virDomainBlockPullCursor *cursor, unsigned int flags); /** * virDomainBlockPullAbort: * @dom: pointer to domain object * @path: fully-qualified filename of disk * @flags: currently unused, for future extension * * Cancel a pull operation that has been previously started by a call to * virDomainBlockPull() with the VIR_DOMAIN_BLOCK_PULL_CONTINUOUS flag. * * Returns -1 in case of failure, 0 when successful. */ int virDomainBlockPullAbort(virDomainPtr dom, const char *path, unsigned int flags); /** * virDomainGetBlockPullInfo: * @dom: pointer to domain object * @path: fully-qualified filename of disk * @info: pointer to a virDomainBlockPullInfo structure * @flags: currently unused, for future extension * * Request progress information on a block pull operation that has been started * with the VIR_DOMAIN_BLOCK_PULL_CONTINUOUS flag set. If an operation is * active for the given parameters, @info will be updated with the current * progress. * * Returns -1 in case of failure, 0 when successful. */ int virDomainGetBlockPullInfo(virDomainPtr dom, const char *path, virDomainBlockStreamInfoPtr info, unsigned int flags); The following new asynchronous event will be made available for subscription: VIR_DOMAIN_EVENT_ID_BLOCK_PULL = 7, typedef enum { VIR_DOMAIN_BLOCK_PULL_COMPLETED, VIR_DOMAIN_BLOCK_PULL_FAILED, } virConnectDomainEventBlockPullStatus; typedef void (*virConnectDomainEventBlockPullCallback(virConnectPtr conn, virDomainPtr dom, const char *path, int status); Upon receipt of this event and when the status field indicates success, libvirt will revoke access to the backing file which is no longer needed by the domain. NOTE: Qemu will emit an asynchronous event (VIR_DOMAIN_BLOCK_PULL_COMPLETED) after any operation that eliminates the dependency on the backing file. This could be a virDomainBlockPull() without VIR_DOMAIN_BLOCK_PULL_CONTINUOUS and will allow libvirt to manage backing file security regardless of the pull mode used.

On 05/24/2011 07:19 AM, Daniel P. Berrange wrote: > On Mon, May 23, 2011 at 11:56:03AM -0500, Adam Litke wrote: >> Here is version 4 of the BlockStream API (now called BlockPull). Hopefully this >> version addresses all remaining concerns and I can begin to work on the code. >> Does everyone approve of the new name 'virDomainBlockPull'? >> >> >> Changes since V3: >> - Renamed the API to 'Block Pull' to emphasize the effect of the API over its >> method of operation. >> - Changed virDomainGetBlockPullInfo() to accept a path argument and to return >> information about that path alone. >> - Added a new virDomainEvent to report final status when using CONTINUOUS mode. >> >> /* >> * Block Pull API >> */ >> typedef enum { >> /* >> * If set, virDomainBlockPull() will operate on the entire device in the >> * background. The status can be checked and the operation aborted by >> * using the functions virDomainBlockPullInfo() and >> * virDomainBlockPullAbort(). >> */ >> VIR_DOMAIN_BLOCK_PULL_CONTINUOUS = 1, >> } virDomainBlockPullFlags; >> >> /* An iterator for initiating and monitoring block pull operations */ >> typedef unsigned long long virDomainBlockPullCursor; >> >> typedef struct _virDomainBlockPullInfo virDomainBlockPullInfo; >> struct _virDomainBlockPullInfo { >> /* >> * The following fields provide an indication of block pull progress. @cur >> * indicates the current position and will be between 0 and @end. @end is >> * the final cursor position for this operation and represents completion. >> * To approximate progress, divide @cur by @end. >> */ >> virDomainBlockPullCursor cur; >> virDomainBlockPullCursor end; >> }; >> typedef virDomainBlockPullInfo *virDomainBlockPullInfoPtr; >> >> /** >> * virDomainBlockPull: >> * @dom: pointer to domain object >> * @path: Fully-qualified filename of disk >> * @cursor: pointer to a virDomainBlockPullCursor, or NULL >> * @flags: One of virDomainBlockPullFlags, or zero >> * >> * Populate a disk image with data from its backing image. Once all data from >> * its backing image has been pulled, the disk no longer depends on a backing >> * image. >> * >> * If VIR_DOMAIN_BLOCK_PULL_CONTINUOUS is specified, the entire device will be >> * streamed in the background. Otherwise, the value stored in @cursor will be >> * used to stream an increment. >> * >> * Returns -1 in case of failure, 0 when successful. On success and when @flags >> * does not contain VIR_DOMAIN_BLOCK_PULL_CONTINUOUS, the iterator @cursor will >> * be updated to the proper value for use in a subsequent call. >> */ >> int virDomainBlockPull(virDomainPtr dom, >> const char *path, >> virDomainBlockPullCursor *cursor, >> unsigned int flags); >> >> /** >> * virDomainBlockPullAbort: >> * @dom: pointer to domain object >> * @path: fully-qualified filename of disk >> * @flags: currently unused, for future extension >> * >> * Cancel a pull operation that has been previously started by a call to >> * virDomainBlockPull() with the VIR_DOMAIN_BLOCK_PULL_CONTINUOUS flag. >> * >> * Returns -1 in case of failure, 0 when successful. >> */ >> int virDomainBlockPullAbort(virDomainPtr dom, >> const char *path, >> unsigned int flags); >> >> /** >> * virDomainGetBlockPullInfo: >> * @dom: pointer to domain object >> * @path: fully-qualified filename of disk >> * @info: pointer to a virDomainBlockPullInfo structure >> * @flags: currently unused, for future extension >> * >> * Request progress information on a block pull operation that has been started >> * with the VIR_DOMAIN_BLOCK_PULL_CONTINUOUS flag set. If an operation is >> * active for the given parameters, @info will be updated with the current >> * progress. >> * >> * Returns -1 in case of failure, 0 when successful. >> */ >> int virDomainGetBlockPullInfo(virDomainPtr dom, >> const char *path, >> virDomainBlockStreamInfoPtr info, >> unsigned int flags); >> >> >> The following new asynchronous event will be made available for subscription: >> >> VIR_DOMAIN_EVENT_ID_BLOCK_PULL = 7, >> >> typedef enum { >> VIR_DOMAIN_BLOCK_PULL_COMPLETED, >> VIR_DOMAIN_BLOCK_PULL_FAILED, >> } virConnectDomainEventBlockPullStatus; >> >> typedef void (*virConnectDomainEventBlockPullCallback(virConnectPtr conn, >> virDomainPtr dom, >> const char *path, >> int status); >> >> Upon receipt of this event and when the status field indicates success, libvirt >> will revoke access to the backing file which is no longer needed by the domain. >> >> NOTE: Qemu will emit an asynchronous event (VIR_DOMAIN_BLOCK_PULL_COMPLETED) >> after any operation that eliminates the dependency on the backing file. This >> could be a virDomainBlockPull() without VIR_DOMAIN_BLOCK_PULL_CONTINUOUS and >> will allow libvirt to manage backing file security regardless of the pull mode >> used. > > I assume it will also emit VIR_DOMAIN_BLOCK_PULL_FAILED when a continuous > streaming operation aborts, because in that case you obviously still have > the backing file present so PULL_COMPLETED wouldn't be issued ? I don't think abort is the same as failed in this case. An abort is triggered by the API user whereas a failure is not. If a user issues an abort, they should know to stop waiting around for PULL_COMPLETED. A failure is unexpected and thus absolutely needs an event to be raised.