On Thu, Dec 09, 2021 at 09:29:07AM +0100, Michal Prívozník wrote:
On 12/8/21 21:57, Martin Kletzander wrote: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@xxxxxxxxxx> --- 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.I could move the lock, sure. But allowSkip is set and can never change throughout stream lifetime (one simply doesn't change classic stream to be sparse or vice versa). Also, this is of "better double check" type - virStreamInData() shouldn't be ever called if stream isn't sparse, i.e. allowSkip == true.
Since we're double-checking something we expect (allowSkip == true) just in case someone calls it without allowSkip, we should also make sure we have the lock just in case. It will never add any time nor complexity, it will be more readable and future-proof. Since we do not have a way to say which parts of a struct is covered by a lock and changeable (apart from a comment in said struct) we should not rely on that in places where it is not necessary (I know we have some places where this is needed). So it makes sense for me to move that even if we never implement changing the value after creation.
+ 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 thats/fried/friend/ ;-)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 ...I'm not sure I follow. In order to call the hole handler there has to be an incoming message of VIR_NET_STREAM_HOLE type and no hole pending. The decode function then checks the same conditions.+ 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.Ah, yes, ret = 1 should be returned only if the function was called. Let me respin this patch.
So that shows even more that I'm not a friend of this part of the codebase and my review for this patch should not be taken into account =D
Anyway, it looks fine to me, so with the locking issue above fixed, Reviewed-by: Martin Kletzander <mkletzan@xxxxxxxxxx>Michal
Attachment:
signature.asc
Description: PGP signature
