On 05/05/2017 07:25 AM, Michal Privoznik wrote:
On 05/04/2017 11:29 PM, John Ferlan wrote:
On 04/20/2017 06:01 AM, Michal Privoznik wrote:
This API can be used to tell the other side of the stream to skip
s/can be/is (unless it can be used for something else ;-))
some bytes in the stream. This can be used to create a sparse file on the receiving side of a stream.
It takes just one argument @length, which says how big the hole is. Since our streams are not rewindable like regular files, we don't need @whence argument like seek(2) has.
lseek is an implementation detail... However, it could be stated that the skipping would be from the current point in the file forward by some number of bytes. It's expected to be used in conjunction with code that is copying over the real (or non-zero) data and should be considered an optimization over sending zere data segments.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- include/libvirt/libvirt-stream.h | 3 +++ src/driver-stream.h | 5 ++++ src/libvirt-stream.c | 57 ++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 1 + 4 files changed, 66 insertions(+)
While it would be unused for now, should @flags be added. Who knows what use it could have, but avoids a new Flags API, but does cause a few other wording changes here.
Ah sure. We should have @flags there. Good point.
Perhaps it's just me - but "Skip" and "HoleSize" just seem awkward. Would "virStreamSetSkip" and "virStreamGetSkip" be more apropos? (or s/Skip/HoleSize/ - ewww). Names would then follow our more recent function naming guidelines. I think I dislike the HoleSize much more than the Skip.
SetSkip and GetSkip sound wrong to me instead :D
I understand completely (in more ways than one)! However, I still have the hacking/coding style guide to fallback upon that requires certain syntax ;-)... While Skip "qualifies" as a verb and perhaps squeaks by on that technicality - the "HoleSize" has no verb telling me what HoleSize is doing. The dichotomy that "Skip" doesn't have a Size object, but Hole does isn't lost either. I actually think Size is a "given", but as someone who has trouble creating names that pass muster when my code is reviewed - who am I to give too much advice here! Also now that I'm further down the road - I keep having to attempt to remind myself whether this is a we're setting/performing the set skip operation or this is a we're getting the set hole size operation. Function, RPC, structures, etc. names would help unmuddle things.
diff --git a/include/libvirt/libvirt-stream.h b/include/libvirt/libvirt-stream.h index bee2516..4e0a599 100644 --- a/include/libvirt/libvirt-stream.h +++ b/include/libvirt/libvirt-stream.h @@ -50,6 +50,9 @@ int virStreamRecvFlags(virStreamPtr st, size_t nbytes, unsigned int flags);
+int virStreamSkip(virStreamPtr st, + unsigned long long length);
Was there consideration for using 'off_t' instead of ULL? I know it's an implementation detail of virFDStreamData and lseek() usage, but it does hide things... IDC either way.
The problem with off_t is that it is signed type, while ULL is unsigned. There's not much point in sending a negative offset, is there? Moreover, we use ULL for arguments like offset (not sure really why). Frankly, I don't really know why. Perhaps some types don't exist everywhere?
Michal
The thing is the implementation uses a function that expects an off_t. When I see ULL I'm usually considering memory sizes which can be large. Not that a file couldn't be, but it usually isn't. The concern is - could we run into a situation where a coding error somewhere supplies a negative value that will now appear to be some inordinately large positive value; whereas, we could avoid that by stipulating 'virStreamSkip' (or whatever it gets called) expects a positive and/or greater than 0 value (e.g. virCheckPositiveArgGoto). Also as I see it, virFileInData calculates *length from two off_t's and in none of those calculations is it possible to generate a negative number. If we did, something is wrong. Still since virStreamSkip can be called and not necessarily use that calculation. I'll also point out the coding example provided for virStreamSkip: + * while (1) { + * char buf[4096]; + * size_t len; <===== + * if (..in hole...) { + * ..get hole size... + * virStreamSkip(st, len); ^^^^ <=== Not a ULL... Although the corollary did use the ULL for virStreamHoleSize in its example. Similar argument for virStreamHoleSize could be made - we shouldn't receive a negative value, but we have no way of differentiating a coding mistake and an inordinately large value. At least by enforcing usage of 'off_t' we're saying - go forward... John