[PATCH 2/3] rpc: Introduce virNetClientStreamInData()

[Martin Kletzander]

On Tue, Dec 07, 2021 at 04:34:41PM +0100, Michal Privoznik wrote:
>The aim of this function is to look at a virNetClientStream and
>tell whether the incoming packet (if there's one) contains data
>(type VIR_NET_STREAM) or a hole (type VIR_NET_STREAM_HOLE) and
>how big the section is. This function will be called from the
>remote driver in one of future commits.
>
>Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>---
> src/libvirt_remote.syms      |  1 +
> src/rpc/virnetclientstream.c | 61 ++++++++++++++++++++++++++++++++++++
> src/rpc/virnetclientstream.h |  4 +++
> 3 files changed, 66 insertions(+)
>
>diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms
>index 942e1013a6..07d22e368b 100644
>--- a/src/libvirt_remote.syms
>+++ b/src/libvirt_remote.syms
>@@ -66,6 +66,7 @@ virNetClientStreamEOF;
> virNetClientStreamEventAddCallback;
> virNetClientStreamEventRemoveCallback;
> virNetClientStreamEventUpdateCallback;
>+virNetClientStreamInData;
> virNetClientStreamMatches;
> virNetClientStreamNew;
> virNetClientStreamQueuePacket;
>diff --git a/src/rpc/virnetclientstream.c b/src/rpc/virnetclientstream.c
>index 1ba6167a1d..ffc702cdc3 100644
>--- a/src/rpc/virnetclientstream.c
>+++ b/src/rpc/virnetclientstream.c
>@@ -801,3 +801,64 @@ bool virNetClientStreamEOF(virNetClientStream *st)
> {
>     return st->incomingEOF;
> }
>+
>+
>+int virNetClientStreamInData(virNetClientStream *st,
>+                             int *inData,
>+                             long long *length)
>+{
>+    int ret = 0;
>+    virNetMessage *msg = NULL;
>+
>+    if (!st->allowSkip) {

The object should be already locked here I presume.

>+        virReportError(VIR_ERR_OPERATION_INVALID, "%s",
>+                       _("Holes are not supported with this stream"));
>+        return -1;
>+    }
>+
>+    virObjectLock(st);
>+
>+    if (virNetClientStreamCheckState(st) < 0)
>+        goto cleanup;
>+
>+    msg = st->rx;
>+
>+    if (!msg) {
>+        /* No incoming message. This means that the stream is at its end. In
>+         * this case, virStreamInData() should set both inData and length to
>+         * zero and return success. */
>+        *inData = 0;
>+        *length = 0;
>+    } else if (msg->header.type == VIR_NET_STREAM) {
>+        *inData = 1;
>+        *length = msg->bufferLength - msg->bufferOffset;
>+    } else if (msg->header.type == VIR_NET_STREAM_HOLE) {
>+        *inData = 0;
>+
>+        if (st->holeLength == 0 &&
>+            virNetClientStreamHandleHole(NULL, st) < 0)

I was never a fried with our way of decoding some data and I guess that
this is one of the occasions and st->holeLength just shows whether the
message is already decoded.

One of the problems I have here is that most of the functions double
check their input states, although this one does not, making it called
conditionally, which seems weird ...

>+            goto cleanup;
>+
>+        *length = st->holeLength;
>+        st->holeLength = 0;
>+
>+        /* virNetClientStreamHandleHole() called above did pop the message from
>+         * the queue (and freed it). Instead of trying to push it back let's
>+         * just signal to the caller what we did. */

... especially when this comment makes it seem like it relies on that
function being called.

Anyway, it looks fine to me, so with the locking issue above fixed,

Reviewed-by: Martin Kletzander <mkletzan at redhat.com>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20211208/e1dfcf45/attachment-0001.sig>