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

On 05/12/2017 03:29 AM, Michal Privoznik wrote: > 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] Would a 5.4G file exist on a 32b arch since the OS couldn't address it nor could it get anything beyond 4GB-1 as a size and then only if using unsigned as the sizing... It's been far too long since I worked on 32 bit arches (or less). The 90's are over aren't they ;-)