On 03/18/2011 10:36 AM, Daniel P. Berrange wrote:
> New APIs are added allowing streaming of content to/from
> storage volumes. A new API for creating volumes is also
> added allowing the content to be provided immediately at
> time of creation
Delete this last sentence; it's stale information from an earlier
version of the commit.
>
> * include/libvirt/libvirt.h.in: Add virStorageVolUpload and
> virStorageVolDownload APIs
> * src/driver.h, src/libvirt.c, src/libvirt_public.syms: Stub
> code for new APIs
> * src/storage/storage_driver.c, src/esx/esx_storage_driver.c:
> Add dummy entries in driver table for new APIs
> ---
> include/libvirt/libvirt.h.in | 10 ++++
> include/libvirt/virterror.h | 1 +
> src/driver.h | 14 +++++
> src/esx/esx_storage_driver.c | 2 +
> src/libvirt.c | 123 ++++++++++++++++++++++++++++++++++++++++++
> src/libvirt_public.syms | 2 +
> src/storage/storage_driver.c | 2 +
> src/util/virterror.c | 6 ++
> 8 files changed, 160 insertions(+), 0 deletions(-)
>
> +++ b/src/libvirt.c
> @@ -9065,6 +9065,129 @@ error:
>
>
> /**
> + * virStorageVolDownload:
> + * @pool: pointer to volume to download
> + * @stream: stream to use as output
> + * @offset: position to start reading from
Since there are two effective files (the stream and the volume), it
would help to make it clear which file the offset applies to (it can
somewhat be implied, because streams are not seekable and thus do not
have an offset, but more wording never hurts).
> + * @length: limit on amount of data to download
> + * @flags: flags for creation (unused, pass 0)
What is being created? The stream and volume already exist.
> + *
> + * Download the content of the volume as a stream. If @length
> + * is zero, then the entire file contents will be downloaded.
You have to use offset=0 to get the entire file.
Also, mention that it is an error if length+offset > size of volume.
What happens if the stream hits EOF before all data read from the volume
has been written to the stream? Should the API support a way to tell
how many bytes were successfully downloaded in the case of a short
stream? That is, instead of returning 0 on success, should Upload and
Download return a ull with how many bytes were transferred in/out of the
stream?
These days, efficient sparse handling is a cool kernel feature waiting
to be exploited (see coreutils 8.10 use of FIEMAP in cp). Should we
make it easier to detect holes in a volume, by exposing some of this
information back through this API?
> +
> +/**
> + * virStorageVolUpload:
> + * @pool: pointer to volume to download
> + * @stream: stream to use as output
Two lines with too much copy and paste.
> + * @offset: position to start writing to
> + * @length: limit on amount of data to upload
> + * @flags: flags for creation (unused, pass 0)
> + *
> + * Upload new content to the volume from a stream. If @length
> + * is non-zero, and an error will be raised if an attempt is
> + * made to upload greater than @length bytes of data.
I'm still a bit confused on the semantics. Is this the desired
interpretation:
Suppose the volume has size S > 1.
offset=0 length=0 => read S bytes from stream to fill S bytes of volume
offset=1 length=0 => read S-1 bytes from stream
offset=1 length=S => error (length+offset > S)
offset=0 length=1 => read 1 byte from stream, write 1 byte to volume
In all cases, if stream supplies fewer bytes than the resulting size to
be written to the volume, then does this fail after writing all supplied
bytes? Should the API tell how many bytes were successfully transferred
to the volume in the case of a short stream? Is this a case where it
might be worth letting flags=1 imply that all remaining bytes of volume
should be set to NUL (that is, when uploading from a smaller source to a
larger destination volume, do we want to guarantee that the tail end of
volume has been wiped)? I'm thinking that if we return the length of
bytes read from the stream, then anyone that cares can do a subsequent
upload starting from that offset to upload NULs to any further bytes.
For that matter, that sounds like a great use case for wiping a portion
of a volume (aka punching holes into an image). virStorageVolWipe can
only wipe the entire volume, but virStorageVolUpload could be used to
intentionally wipe any given offset and length via a flag.
So, does this look any better as a proposed API?
/**
* virStorageVolDownload:
* @pool: pointer to volume to download from
* @stream: stream to use as output
* @offset: position in @pool to start reading from
* @length: limit on amount of data to download
* @flags: future flags (unused, pass 0)
*
* Download the content of the volume as a stream. If @length
* is zero, then the remaining contents after @offset will be
* downloaded. It is an error if @length + @offset exceeds
* the size of the volume.
*
* Returns number of bytes written to stream, or -1 upon error.
*/
unsigned long long
virStorageVolDownload(virStorageVolPtr vol,
virStreamPtr stream,
unsigned long long offset,
unsigned long long length,
unsigned int flags);
enum {
VIR_STORAGE_VOLUME_UPLOAD_WIPE = 1,
};
/**
* virStorageVolUpload:
* @pool: pointer to volume to upload to
* @stream: stream to use as input
* @offset: position in @pool to start writing to
* @length: limit on amount of data to upload
* @flags: VIR_STORAGE_VOLUME_UPLOAD flags
*
* Upload new content to the volume from a stream. It is an
* error if @length + @offset exceeds the size of the
* volume. If @length is zero, then use the volume size minus
* @length bytes of data.
*
* If @flags is zero, data is read from stream; if it is
* VIR_STORAGE_VOLUME_UPLOAD_WIPE, NUL bytes are written
* instead and @stream may be NULL.
*
* Returns the bytes 0, or -1 upon error.
*/
int
virStorageVolUpload(virStorageVolPtr vol,
virStreamPtr stream,
unsigned long long offset,
unsigned long long length,
unsigned int flags);
virStorageVolWipe(vol, flags) is shorthand for virStorageVolUpload(vol,
NULL, 0, 0, VIR_STORAGE_VOLUME_UPLOAD_WIPE | flags)
The rest of this patch looks great, but I think we have to nail the
right API in libvirt.c (with possible fallout to the types of the driver
callback functions) before committing anything, so this will need a v3.
--
Eric Blake eblake@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list