[Spice-devel] [vdagent-win PATCH v3 01/10] Reduce indentation returning earlier

[Jonathon Jongsma]

On Fri, 2018-06-29 at 08:11 +0100, Frediano Ziglio wrote:
> Also add some comments.
> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  vdagent/vdagent.cpp | 45 ++++++++++++++++++++++++++-----------------
> --
>  1 file changed, 26 insertions(+), 19 deletions(-)
> 
> diff --git a/vdagent/vdagent.cpp b/vdagent/vdagent.cpp
> index e22687c..7318725 100644
> --- a/vdagent/vdagent.cpp
> +++ b/vdagent/vdagent.cpp
> @@ -1339,6 +1339,7 @@ void VDAgent::handle_chunk(VDIChunk* chunk)
>  {
>      //FIXME: currently assumes that multi-part msg arrives only from
> client port
>      if (_in_msg_pos == 0 || chunk->hdr.port == VDP_SERVER_PORT) {
> +        // wait for the full header

Is this comment accurate? It seems to imply that we're going to wait
for the rest of the header to arrive and then try again. But as far as
I can tell, we're simply discarding this chunk because it's not the
expected size.

>          if (chunk->hdr.size < sizeof(VDAgentMessage)) {
>              return;
>          }
> @@ -1349,29 +1350,35 @@ void VDAgent::handle_chunk(VDIChunk* chunk)
>              return;
>          }
>          uint32_t msg_size = sizeof(VDAgentMessage) + msg->size;
> +        // we got an entire message, handle it
>          if (chunk->hdr.size == msg_size) {

Maybe move this comment inside the if statement?

>              dispatch_message(msg, chunk->hdr.port);
> -        } else {
> -            ASSERT(chunk->hdr.size < msg_size);
> -            _in_msg = (VDAgentMessage*)new uint8_t[msg_size];
> -            memcpy(_in_msg, chunk->data, chunk->hdr.size);
> -            _in_msg_pos = chunk->hdr.size;
> -        }
> -    } else {
> -        memcpy((uint8_t*)_in_msg + _in_msg_pos, chunk->data, chunk-
> >hdr.size);
> -        _in_msg_pos += chunk->hdr.size;
> -        // update clipboard tick on each clipboard chunk for timeout
> setting
> -        if (_in_msg->type == VD_AGENT_CLIPBOARD && _clipboard_tick)
> {
> -            _clipboard_tick = GetTickCount();
> +            return;
>          }
> -        if (_in_msg_pos == sizeof(VDAgentMessage) + _in_msg->size) {
> -            if (_in_msg->type == VD_AGENT_CLIPBOARD &&
> !_clipboard_tick) {
> -                vd_printf("Clipboard received but dropped due to
> timeout");
> -            } else {
> -                dispatch_message(_in_msg, 0);
> -            }
> -            cleanup_in_msg();
> +
> +        // got just the start, starts to collapse all chunks into a
> +        // single buffer

starts -> start

> +        ASSERT(chunk->hdr.size < msg_size);
> +        _in_msg = (VDAgentMessage*)new uint8_t[msg_size];
> +        memcpy(_in_msg, chunk->data, chunk->hdr.size);
> +        _in_msg_pos = chunk->hdr.size;
> +        return;
> +    }
> +
> +    // append chunk to partial message

Perhaps expand slightly? Something like

"The previous chunk was a partial message, so append this chunk to the
previous chunk"

> +    memcpy((uint8_t*)_in_msg + _in_msg_pos, chunk->data, chunk-
> >hdr.size);
> +    _in_msg_pos += chunk->hdr.size;
> +    // update clipboard tick on each clipboard chunk for timeout
> setting
> +    if (_in_msg->type == VD_AGENT_CLIPBOARD && _clipboard_tick) {
> +        _clipboard_tick = GetTickCount();
> +    }
> +    if (_in_msg_pos == sizeof(VDAgentMessage) + _in_msg->size) {

Perhaps add comment here: 

"This chunk completed the current message, so process it"

> +        if (_in_msg->type == VD_AGENT_CLIPBOARD && !_clipboard_tick)
> {
> +            vd_printf("Clipboard received but dropped due to
> timeout");
> +        } else {
> +            dispatch_message(_in_msg, 0);
>          }
> +        cleanup_in_msg();
>      }
>  }
>  

Aside from those minor comments, it looks good
Jonathon