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

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?