Re: [libvirt] API Proposal: virDomainBlockPull() (Block device streaming) V4

On 05/24/2011 07:16 AM, Daniel P. Berrange wrote: > 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, We don't even need 'pos' anymore. See below.

> 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() Yeah, despite adding four functions to the API this seems like a natural way to segment it out. >>> 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 ? After talking to Stefan I think we have resolved this issue. We have decided to drop the 'pos' argument to virDomainBlockPull() completely. If pos is a sequential, opaque cursor then there is only ever one valid value that can be passed to qemu at any given time. If qemu has to store the valid value to check against, then we might as well just store the position internally to qemu and save the API user the trouble of managing the iterator. An added benefit is that we now know it is safe to know that the entire disk has been pulled after the last cursor position has finished (regardless of which mode was used).

I will follow-up with V5 of the API...