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>
> ---
> 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?
Michal