9:22 a.m.
On 03/22/2011 05:45 AM, Daniel P. Berrange wrote:
> 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.
Yeah, I figured that out in patch 3/6.
Still, there were some improvements to make to the wording.
> 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.
Then we can save sparseness for another day and another patch. I
imagine it might be possible to extend streams to support some OOB
sparseness data (if both server and client know how to interpret OOB,
then both sides can skip over holes in the transfer; if either side
lacks support for OOB, then holes are transferred as normal data).
> 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.
Agreed. So with that, here's the incremental changes I recommend that
you squash in, and then you have my:
ACK
diff --git i/src/libvirt.c w/src/libvirt.c
index f09421f..df7df7a 100644
--- i/src/libvirt.c
+++ w/src/libvirt.c
@@ -9067,14 +9067,19 @@ error:
/**
* virStorageVolDownload:
- * @pool: pointer to volume to download
+ * @pool: pointer to volume to download from
* @stream: stream to use as output
- * @offset: position to start reading from
+ * @offset: position in @pool to start reading from
* @length: limit on amount of data to download
- * @flags: flags for creation (unused, pass 0)
+ * @flags: future flags (unused, pass 0)
*
* Download the content of the volume as a stream. If @length
- * is zero, then the entire file contents will be downloaded.
+ * is zero, then the contents of volume after @offset will be downloaded.
+ *
+ * This call sets up an asynchronous stream; subsequent use of
+ * stream APIs is necessary to determine how much data is successfully
+ * transferred. Use caution when multiple streams are visiting the
+ * same volume simultaneously.
*
* Returns 0, or -1 upon error.
*/
@@ -9085,7 +9090,8 @@ virStorageVolDownload(virStorageVolPtr vol,
unsigned long long length,
unsigned int flags)
{
- VIR_DEBUG("vol=%p stream=%p offset=%llu length=%llu flags=%u", vol,
stream, offset, length, flags);
+ VIR_DEBUG("vol=%p stream=%p offset=%llu length=%llu flags=%u",
+ vol, stream, offset, length, flags);
virResetLastError();
@@ -9128,15 +9134,21 @@ error:
/**
* virStorageVolUpload:
- * @pool: pointer to volume to download
- * @stream: stream to use as output
- * @offset: position to start writing to
+ * @pool: pointer to volume to upload into
+ * @stream: stream to use as input
+ * @offset: position in @pool to start writing to
* @length: limit on amount of data to upload
- * @flags: flags for creation (unused, pass 0)
+ * @flags: future flags (unused, pass 0)
+ *
+ * Upload new content to the volume from a stream. This call will
+ * fail if @offset + @length exceeds the volume size; otherwise,
+ * if @length is non-zero, the stream will raise an error if an
+ * attempt is made to upload greater than @length bytes of data.
*
- * 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.
+ * This call sets up an asynchronous stream; subsequent use of
+ * stream APIs is necessary to determine how much data is successfully
+ * transferred. Use caution when multiple streams are visiting the
+ * same volume simultaneously.
*
* Returns 0, or -1 upon error.
*/
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org