5:55 a.m.
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(a)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.
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 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.
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(a)redhat.com>
Michal