Re: [libvirt] [PATCH v2 11/38] Introduce virStreamSkip

On Fri, May 05, 2017 at 01:25:34PM +0200, 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(a)redhat.com&gt; >>> --- >>> 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 > >> >>> 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? If anything, we would use size_t, for consistency with the Send/Recv methods.