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

[John Ferlan]

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.

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.

> 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.

> >
>  /**
>   * virStreamSourceFunc:
> diff --git a/src/driver-stream.h b/src/driver-stream.h
> index d4b0480..20ea13f 100644
> --- a/src/driver-stream.h
> +++ b/src/driver-stream.h
> @@ -42,6 +42,10 @@ typedef int
>                           unsigned int flags);
>  
>  typedef int
> +(*virDrvStreamSkip)(virStreamPtr st,
> +                    unsigned long long length);
> +
> +typedef int
>  (*virDrvStreamEventAddCallback)(virStreamPtr stream,
>                                  int events,
>                                  virStreamEventCallback cb,
> @@ -68,6 +72,7 @@ struct _virStreamDriver {
>      virDrvStreamSend streamSend;
>      virDrvStreamRecv streamRecv;
>      virDrvStreamRecvFlags streamRecvFlags;
> +    virDrvStreamSkip streamSkip;
>      virDrvStreamEventAddCallback streamEventAddCallback;
>      virDrvStreamEventUpdateCallback streamEventUpdateCallback;
>      virDrvStreamEventRemoveCallback streamEventRemoveCallback;
> diff --git a/src/libvirt-stream.c b/src/libvirt-stream.c
> index 80b2d47..55f3ef5 100644
> --- a/src/libvirt-stream.c
> +++ b/src/libvirt-stream.c
> @@ -346,6 +346,63 @@ virStreamRecvFlags(virStreamPtr stream,
>  
>  
>  /**
> + * virStreamSkip:
> + * @stream: pointer to the stream object
> + * @length: number of bytes to skip
> + *
> + * Skip @length bytes in the stream. This is useful when there's
> + * no actual data in the stream, just a hole. If that's the case,
> + * this API can be used to skip the hole properly instead of
> + * transmitting zeroes to the other side.

Consider something like:

Rather than transmitting empty file space, this API directs the @stream
target to create @length bytes of empty space. This API would be used
when uploading or downloading sparsely populated files to avoid the
needless copy of empty file space.

Although I suppose there could be more applications than file targets.

I'd say ACK, but I think the function name discussion needs to be
complete first...

John

> + *
> + * An example using this with a hypothetical file upload API
> + * looks like:
> + *
> + *   virStream st;
> + *
> + *   while (1) {
> + *     char buf[4096];
> + *     size_t len;
> + *     if (..in hole...) {
> + *       ..get hole size...
> + *       virStreamSkip(st, len);
> + *     } else {
> + *       ...read len bytes...
> + *       virStreamSend(st, buf, len);
> + *     }
> + *   }
> + *
> + * Returns 0 on success,
> + *        -1 error
> + */
> +int
> +virStreamSkip(virStreamPtr stream,
> +              unsigned long long length)
> +{
> +    VIR_DEBUG("stream=%p, length=%llu", stream, length);
> +
> +    virResetLastError();
> +
> +    virCheckStreamReturn(stream, -1);
> +
> +    if (stream->driver &&
> +        stream->driver->streamSkip) {
> +        int ret;
> +        ret = (stream->driver->streamSkip)(stream, length);
> +        if (ret < 0)
> +            goto error;
> +        return ret;
> +    }
> +
> +    virReportUnsupportedError();
> +
> + error:
> +    virDispatchError(stream->conn);
> +    return -1;
> +}
> +
> +
> +/**
>   * virStreamSendAll:
>   * @stream: pointer to the stream object
>   * @handler: source callback for reading data from application
> diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms
> index af863bb..acadda8 100644
> --- a/src/libvirt_public.syms
> +++ b/src/libvirt_public.syms
> @@ -762,6 +762,7 @@ LIBVIRT_3.1.0 {
>  LIBVIRT_3.3.0 {
>      global:
>          virStreamRecvFlags;
> +        virStreamSkip;
>  } LIBVIRT_3.1.0;
>  
>  # .... define new API here using predicted next version number ....
>