Re: [libvirt] [PATCH 2/6] Add public APIs for storage volume upload/download

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. */