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

[Daniel P. Berrange]

On Fri, May 12, 2017 at 09:29:27AM +0200, 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 at 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
> >>
> >>>
> >>>> 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:

Were you testing libvirt, or testing a standalone demo program ?

Libvirt should be defining the macro that enables large file support,
which turns off_t into a 64-bit type.  So in fact, we would actually
be wrong to use size_t - off_t or long long is actually what we need
here.

I was wrong about consistency with Send/Recv, since the arg there is
about the length of the data array which is size_t and this is a
different boundary than max file size which is off_t.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|