[libvirt] [PATCH v2 25/38] virnetclientstream: Introduce virNetClientStreamHandleSkip

[John Ferlan]

On 04/20/2017 06:01 AM, Michal Privoznik wrote:
> This is a function that handles an incoming STREAM_SKIP packet.
> Even though it is not wired up yet, it will be soon. At the
> beginning do couple of checks whether server plays nicely and
> sent us a STREAM_SKIP packed only after we've enabled sparse

s/packed/packet

> streams. Then decodes the message payload to see how big the hole

s/decodes/decode

> is and stores it in passed @length argument.

s/stores/store

> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/rpc/virnetclientstream.c | 63 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 63 insertions(+)
> 
> diff --git a/src/rpc/virnetclientstream.c b/src/rpc/virnetclientstream.c
> index 1e30080..027ffde 100644
> --- a/src/rpc/virnetclientstream.c
> +++ b/src/rpc/virnetclientstream.c
> @@ -28,6 +28,7 @@
>  #include "virerror.h"
>  #include "virlog.h"
>  #include "virthread.h"
> +#include "libvirt_internal.h"

This doesn't seem necessary, yet or ever during the series...

>  
>  #define VIR_FROM_THIS VIR_FROM_RPC
>  
> @@ -55,6 +56,7 @@ struct _virNetClientStream {
>      bool incomingEOF;
>  
>      bool skippable; /* User requested skippable stream */
> +    unsigned long long skipLength;  /* Size of incoming hole in stream. */

But as I read the code - it appears to be the cumulative size...

>  
>      virNetClientStreamEventCallback cb;
>      void *cbOpaque;
> @@ -356,6 +358,67 @@ int virNetClientStreamSendPacket(virNetClientStreamPtr st,
>      return -1;
>  }
>  

Some intro comments would be nice - especially the part about expecting
to be processing a locked stream...

> +
> +static int ATTRIBUTE_UNUSED

It's been pointed out to me in the past... why not just wait until this
ATTRIBUTE_UNUSED is no longer necessary... e.g. patch 30.

IDC really, but they could be combined then...  I would certainly make
it more obvious that the lock was already held (for me).

John


> +virNetClientStreamHandleSkip(virNetClientPtr client,
> +                             virNetClientStreamPtr st)
> +{
> +    virNetMessagePtr msg;
> +    virNetStreamSkip data;
> +    int ret = -1;
> +
> +    VIR_DEBUG("client=%p st=%p", client, st);
> +
> +    msg = st->rx;
> +    memset(&data, 0, sizeof(data));
> +
> +    /* We should not be called unless there's VIR_NET_STREAM_SKIP
> +     * message at the head of the list. But doesn't hurt to check */
> +    if (!msg) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("No message in the queue"));
> +        goto cleanup;
> +    }
> +
> +    if (msg->header.type != VIR_NET_STREAM_SKIP) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Invalid message prog=%d type=%d serial=%u proc=%d"),
> +                       msg->header.prog,
> +                       msg->header.type,
> +                       msg->header.serial,
> +                       msg->header.proc);
> +        goto cleanup;
> +    }
> +
> +    /* Server should not send us VIR_NET_STREAM_SKIP unless we
> +     * have requested so. But does not hurt to check ... */
> +    if (!st->skippable) {

skippable would be allowSkip

> +        virReportError(VIR_ERR_RPC, "%s",
> +                       _("Unexpected stream skip"));
> +        goto cleanup;
> +    }
> +
> +    if (virNetMessageDecodePayload(msg,
> +                                   (xdrproc_t) xdr_virNetStreamSkip,
> +                                   &data) < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("Malformed stream skip packet"));
> +        goto cleanup;
> +    }
> +
> +    virNetMessageQueueServe(&st->rx);
> +    virNetMessageFree(msg);
> +    st->skipLength += data.length;

Why is this += ?  Isn't the skip length/size set for each and not
cumulative?  Or is this because this is part of some while loop once
patch30 hits.  Of course reading patch30 in conjunction with this patch
has me quite confused.

> +
> +    ret = 0;
> + cleanup:
> +    if (ret < 0) {
> +        /* Abort stream? */
> +    }

I dunno, maybe make that something the caller does...

John

> +    return ret;
> +}
> +
> +
>  int virNetClientStreamRecvPacket(virNetClientStreamPtr st,
>                                   virNetClientPtr client,
>                                   char *data,
>