On Fri, Jan 29, 2016 at 02:26:53PM +0100, Michal Privoznik wrote:
> When dealing with sparse files we need to be able to jump over
> holes as there's no added value in reading/writing them. For
> that, we need new set of send and receive APIs that will have
> @offset argument. Sending data to a stream would be easy - just
> say from which offset we are sending data. Reading is a bit
> tricky - we need read function which can detect holes and thus
> when requested to read from one it will set @offset to new value
> that contains data.
>
> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> ---
> include/libvirt/libvirt-stream.h | 8 +++
> src/driver-stream.h | 13 +++++
> src/libvirt-stream.c | 113 +++++++++++++++++++++++++++++++++++++++
> src/libvirt_public.syms | 6 +++
> 4 files changed, 140 insertions(+)
>
> diff --git a/include/libvirt/libvirt-stream.h b/include/libvirt/libvirt-stream.h
> index 831640d..5a2bde3 100644
> --- a/include/libvirt/libvirt-stream.h
> +++ b/include/libvirt/libvirt-stream.h
> @@ -40,11 +40,19 @@ int virStreamRef(virStreamPtr st);
> int virStreamSend(virStreamPtr st,
> const char *data,
> size_t nbytes);
> +int virStreamSendOffset(virStreamPtr stream,
> + unsigned long long offset,
> + const char *data,
> + size_t nbytes);
>
> int virStreamRecv(virStreamPtr st,
> char *data,
> size_t nbytes);
>
> +int virStreamRecvOffset(virStreamPtr stream,
> + unsigned long long *offset,
> + char *data,
> + size_t nbytes);
>
> /**
> * virStreamSourceFunc:
> diff --git a/src/driver-stream.h b/src/driver-stream.h
> index 85b4e3b..5419b85 100644
> --- a/src/driver-stream.h
> +++ b/src/driver-stream.h
> @@ -31,9 +31,20 @@ typedef int
> size_t nbytes);
>
> typedef int
> +(*virDrvStreamSendOffset)(virStreamPtr st,
> + unsigned long long offset,
> + const char *data,
> + size_t nbytes);
> +
> +typedef int
> (*virDrvStreamRecv)(virStreamPtr st,
> char *data,
> size_t nbytes);
> +typedef int
> +(*virDrvStreamRecvOffset)(virStreamPtr st,
> + unsigned long long *offset,
> + char *data,
> + size_t nbytes);
>
> typedef int
> (*virDrvStreamEventAddCallback)(virStreamPtr stream,
> @@ -60,7 +71,9 @@ typedef virStreamDriver *virStreamDriverPtr;
>
> struct _virStreamDriver {
> virDrvStreamSend streamSend;
> + virDrvStreamSendOffset streamSendOffset;
> virDrvStreamRecv streamRecv;
> + virDrvStreamRecvOffset streamRecvOffset;
> virDrvStreamEventAddCallback streamEventAddCallback;
> virDrvStreamEventUpdateCallback streamEventUpdateCallback;
> virDrvStreamEventRemoveCallback streamEventRemoveCallback;
> diff --git a/src/libvirt-stream.c b/src/libvirt-stream.c
> index c16f586..1df188c 100644
> --- a/src/libvirt-stream.c
> +++ b/src/libvirt-stream.c
> @@ -192,6 +192,58 @@ virStreamSend(virStreamPtr stream,
>
>
> /**
> + * virStreamSendOffset:
> + * @stream: pointer to the stream object
> + * @offset: <something>
> + * @data: buffer to write to stream
> + * @nbytes: size of @data buffer
> + *
> + * Sends some data down the pipe.
> + *
> + * Returns the number of bytes written, which may be less
> + * than requested.
> + *
> + * Returns -1 upon error, at which time the stream will
> + * be marked as aborted, and the caller should now release
> + * the stream with virStreamFree.
> + *
> + * Returns -2 if the outgoing transmit buffers are full &
> + * the stream is marked as non-blocking.
> + */
> +int
> +virStreamSendOffset(virStreamPtr stream,
> + unsigned long long offset,
> + const char *data,
> + size_t nbytes)
> +{
> + VIR_DEBUG("stream=%p, offset=%llu, data=%p, nbytes=%zu",
> + stream, offset, data, nbytes);
> +
> + virResetLastError();
> +
> + virCheckStreamReturn(stream, -1);
> + virCheckNonNullArgGoto(data, error);
> +
> + if (stream->driver &&
> + stream->driver->streamSendOffset) {
> + int ret;
> + ret = (stream->driver->streamSendOffset)(stream, offset, data, nbytes);
> + if (ret == -2)
> + return -2;
> + if (ret < 0)
> + goto error;
> + return ret;
> + }
> +
> + virReportUnsupportedError();
> +
> + error:
> + virDispatchError(stream->conn);
> + return -1;
> +}
> +
> +
> +/**
> * virStreamRecv:
> * @stream: pointer to the stream object
> * @data: buffer to read into from stream
> @@ -285,6 +337,67 @@ virStreamRecv(virStreamPtr stream,
>
>
> /**
> + * virStreamRecvOffset:
> + * @stream: pointer to the stream object
> + * @offset: <something>
> + * @data: buffer to write to stream
> + * @nbytes: size of @data buffer
> + *
> + * Recieve some data from stream. On return set offset to next location.
> + *
> + * Returns the number of bytes read, which may be less
> + * than requested.
> + *
> + * Returns 0 when either the end of the stream is reached, or
> + * there are no data to be sent at current @offset. In case of
> + * the former, the stream should be finished by calling
> + * virStreamFinish(). However, in case of the latter, @offset
> + * should be set to new position where interesting data is.
> + * Failing to do so will result in assumption that there is no
> + * data left.
> + *
> + * Returns -1 upon error, at which time the stream will
> + * be marked as aborted, and the caller should now release
> + * the stream with virStreamFree.
> + *
> + * Returns -2 if there is no data pending to be read & the
> + * stream is marked as non-blocking.
> + */
It is not entirely clear from these docs, but from the impl
against 'fdstream', you seem to treating 'offset' as an
input and output parameter.
I don't think that is going to fly wrt to the RPC protocol.
The reads are explicitly asynchronous wrt the remote server
transmission of data. ie once the stream is open, the server
will push out the data in a continuous stream. There is no
"read" operation requested against the server. So it is not
possible to send an 'offset' across to the server. So the
'offset' really needs to be an output only parameter.
I think it is important that we consider the design additions
to the wire protocol at the same time as the API design we're
considering.
>From an RPC layer POV we currently have VIR_NET_STREAM packets
to transmit the data payload. There is no offset information
provided in these packets, as it is designed as a continuous
stream of data. So from this POV adding offset to the send/recv
methods in the API is not a good fit for the wire proocol.
We use the status field to distinguish between a packet with
payload and a packet without data. ie VIR_NET_CONTINUE status
indicates that there is a a byte[] payload. We could introduce
a new status field VIR_NET_SKIP to represent a hole in the
stream, and the payload would simply be the size of the hole.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list