On Mon, Mar 21, 2011 at 03:04:39PM -0600, Eric Blake wrote:
> 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?
Everything related to actual I/O is done virStreamRecv/virStreamSend.
These APIs merely open the volume and associate a stream with it. So
all these questions about number of bytes / EOF are not relevant in
this context.
> 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?
The concept of sparseness doesn't really fit into streams because
they're not seekable. With download, to avoid repeated round-trips,
the client doesn't actually issue individual read() calls to pull
data over the RPC. Instead the server pushes the data down to the
client continuously until EOF on the server.
> > +
> > +/**
> > + * 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
For upload, actually the 'length' parameter is not all that
much use and only really included for completeness. The client
just continuously pushes data upto the server. The server will
merely return an error if the client pushes more than 'length'
bytes of data, or if offset+length exceeds the size of the 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.
I don't think wiping really fits in practically with the way
streams operate, since this isn't a synchronous 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.
None of this is really possible, since streams just don't operate in this
way.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list