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

On 05/05/2017 04:48 PM, Daniel P. Berrange wrote: > 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. So I've given this some though and ran some experiments. On a 32bit arch I've found this: long long 8 signed size_t 4 unsigned ssize_t 4 signed off_t 4 signed So size_t is 4 bytes long and long long is 8 bytes. This got me thinking, size_t type makes sense for those APIs where we need to address individual bytes. But what would happen if I have the following file on a 32 bit arch: [2MB data] -> [5GB hole] -> [2M data] The hole does not fit into size_t, but it would fit into long long. On the other hand, we are very likely to hit lseek() error as off_t is 4 bytes also (WTF?!). On a 64 bit arch everything is as expected: