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.
>
>> + 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
s/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.
> Anyway, it looks fine to me, so with the locking issue above fixed,
>
> Reviewed-by: Martin Kletzander <mkletzan@xxxxxxxxxx>
Michal
