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

On Wed, May 18, 2011 at 02:08:12PM -0500, Adam Litke wrote: > Version 2 of the block device streaming API diverged > significantly from the original API provided by Qemu. > In an effort to provide an API that is designed to work > well for the specific task of streaming data to a local > device from its backing store I am proposing this: > version 3 of the block device streaming API. Can you clarify what happens upon completion of streaming ? Is the backing store going to automagically go away, or will there need to be a separate API to explicitly remove the (now unncessary) backing store ?

I'm still not really a fan of the name 'Stream' in the APIs because it doesn't give any indication of what the effect of the API is. 'Streaming' refers to the manner in which the API operates, rather what its purpose/goal is. If the backing store is automagically removed, is this better described as a 'Rebase' command/operation ?

> Some noteworthy changes from earlier versions: > > The offset and length parameters to virDomainStream > have been replaced by an iterator type. Manual > specification of ranges is outside of the scope of > the API and does not add utility. Instead, an iterator > is used to perform a series of individual operations in > an efficient manner without exposing device format > particulars. > > I have added a new call virDomainBlockStreamAbort() > that can be used to cancel an active stream operation. > This is an improvement over the use of flags to override > the meaning of virDomainBlockStream(). > > Block devices are specified by the fully-qualified path > to the source file. This information is easier for API > users to provide than the block device alias. > > All API functions return an int. Rather than returning > an unsigned long long, virDomainBlockStream() updates > the the iterator (which is passed in by reference) in > place. > > ----- API V3 ----- > > /* > * Disk Streaming > */ > typedef enum { > /* > * If set, virDomainBlockStream() will begin streaming the entire device > * continuously. The status can be checked and the operation aborted by using > * the functions virDomainGetBlockStreamInfo() and virDomainBlockStreamAbort(). > */ > VIR_DOMAIN_BLOCK_STREAM_CONTINUOUS = 1, > } virDomainStreamDiskFlags; > > #define VIR_DOMAIN_BLOCK_STREAM_PATH_MAX 1024 > > /* An iterator for initiating and monitoring block streaming operations */ > typedef unsigned long long virDomainBlockStreamCursor; > > typedef struct _virDomainBlockStreamInfo virDomainBlockStreamInfo; > struct _virDomainBlockStreamInfo { > char path[VIR_DOMAIN_BLOCK_STREAM_PATH_MAX]; We really shouldn't be having hardcoded path lengths in the public API. IMHO this field should be dropped and the StreamInfo API should have the path passed in

> /* > * The following fields provide an indication of streaming 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. > */ > virDomainBlockStreamCursor cur; > virDomainBlockStreamCursor end; > }; > typedef virDomainBlockStreamInfo *virDomainBlockStreamInfoPtr; > > /** > * virDomainBlockStream: > * @dom: pointer to domain object > * @path: Fully-qualified filename of disk > * @cursor: pointer to a virDomainBlockStreamCursor, or NULL > * @flags: One of virDomainStreamDiskFlags, or zero > * > * Initate a streaming operation on the named block device associated with the > * given domain. If VIR_DOMAIN_BLOCK_STREAM_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_STREAM_CONTINUOUS, the iterator @cursor will > * be updated to the proper value for use in a subsequent call. > */ > int virDomainBlockStream(virDomainPtr dom, > const char *path, > virDomainBlockStreamCursor *cursor, > unsigned int flags); > > /** > * virDomainBlockStreamAbort: > * @dom: pointer to domain object > * @path: fully-qualified filename of disk > * @flags: currently unused, for future extension > * > * Cancel a disk streaming operation that has been previously started by a call to > * virDomainBlockStream() with the VIR_DOMAIN_BLOCK_STREAM_CONTINUOUS flag. > * > * Returns -1 in case of failure, 0 when successful. > */ > int virDomainBlockStreamAbort(virDomainPtr dom, > const char *path, > unsigned int flags); > > /** > * virDomainGetBlockStreamInfo: > * @dom: pointer to domain object > * @info: pointer to an array of virDomainBlockStreamInfo objects > * @nr_results: Number of results requested > * @flags: currently unused, for future extension > * > * Request information on active block streaming operations for the given domain. > * The caller must supply an allocated array of virDomainBlockStreamInfo objects > * Information about up to @nr_results active streams will be stored in @info. > * > * Returns -1 in case of failure, or the number of results that were stored. > */ > int virDomainGetBlockStreamInfo(virDomainPtr dom, > virDomainBlockStreamInfoPtr info, > unsigned int nr_results, > unsigned int flags); This really needs to be: int virDomainGetBlockStreamInfo(virDomainPtr dom, const char *path, virDomainBlockStreamInfoPtr info, unsigned int flags); If the apps want info on > 1 path, then call it multiple times with each path.

In the VIR_DOMAIN_BLOCK_STREAM_CONTINUOUS case, I think it is desirable to have an async event emitted when the streaming is completed, so we don't force apps to poll on the GetBlockStreamInfo if they only care about completion. The event would also allow apps to determine if the background stream job failed, which isn't something that's exposed in any of these APis. eg, in virDomainEventID enum, add VIR_DOMAIN_EVENT_ID_BLOCK_STREAM = 7, typedef enum { VIR_DOMAIN_BLOCK_STREAM_COMPLETED, VIR_DOMAIN_BLOCK_STREAM_FAILED, } virConnectDomainEventlockStreamStatus; typedef void (*virConnectDomainEventBlockStreamCallback(virConnectPtr conn, virDomainPtr dom, const char *path, int status); Internally this would have to be wired upto an event QEMU emits. If completion automatically removes the backing store from the block device, then we need the async event from QEMU regardless, since we'll need to perform SELinux relabelling to deny the VM future access to the now unused backing store.