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

On 03/18/2011 10:36 AM, Daniel P. Berrange wrote: Delete this last sentence; it's stale information from an earlier version of the commit. 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). What is being created? The stream and volume already exist. 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? Two lines with too much copy and paste. 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(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org