[Spice-devel] [PATCH spice-server 2/2] stream-device: Handle capabilities

[Uri Lublin]

On 03/19/2018 06:46 PM, Frediano Ziglio wrote:
> Handle capabilities from guest device.
> Send capability to the guest when device is opened.
> Currently there's no capabilities set on the message sent.
> On the tests we need to discard the capability message before
> reading the error.
> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>   server/red-stream-device.c        | 66 +++++++++++++++++++++++++++++++++++++--
>   server/tests/test-stream-device.c | 22 +++++++++++++
>   2 files changed, 85 insertions(+), 3 deletions(-)
> 
> Changes since v1:
> - rebased on master (with minor fix due to rename).
> 
> diff --git a/server/red-stream-device.c b/server/red-stream-device.c
> index e91df88d..1732b888 100644
> --- a/server/red-stream-device.c
> +++ b/server/red-stream-device.c
> @@ -1,6 +1,6 @@
>   /* spice-server character device to handle a video stream
>   
> -   Copyright (C) 2017 Red Hat, Inc.
> +   Copyright (C) 2017-2018 Red Hat, Inc.
>   
>      This library is free software; you can redistribute it and/or
>      modify it under the terms of the GNU Lesser General Public
> @@ -25,6 +25,8 @@
>   #include "cursor-channel.h"
>   #include "reds.h"
>   
> +#define MAX_GUEST_CAPABILITIES_BYTES ((STREAM_CAP_END+7)/8)
> +
>   struct StreamDevice {
>       RedCharDevice parent;
>   
> @@ -42,6 +44,7 @@ struct StreamDevice {
>       bool has_error;
>       bool opened;
>       bool flow_stopped;
> +    uint8_t guest_capabilities[MAX_GUEST_CAPABILITIES_BYTES];
>       StreamChannel *stream_channel;
>       CursorChannel *cursor_channel;
>       SpiceTimer *close_timer;
> @@ -61,7 +64,7 @@ typedef bool StreamMsgHandler(StreamDevice *dev, SpiceCharDeviceInstance *sin)
>       SPICE_GNUC_WARN_UNUSED_RESULT;
>   
>   static StreamMsgHandler handle_msg_format, handle_msg_data, handle_msg_cursor_set,
> -    handle_msg_cursor_move;
> +    handle_msg_cursor_move, handle_msg_capabilities;
>   
>   static bool handle_msg_invalid(StreamDevice *dev, SpiceCharDeviceInstance *sin,
>                                  const char *error_msg) SPICE_GNUC_WARN_UNUSED_RESULT;
> @@ -147,7 +150,8 @@ stream_device_partial_read(StreamDevice *dev, SpiceCharDeviceInstance *sin)
>           }
>           break;
>       case STREAM_TYPE_CAPABILITIES:
> -        /* FIXME */
> +        handled = handle_msg_capabilities(dev, sin);
> +        break;
>       default:
>           handled = handle_msg_invalid(dev, sin, "Invalid message type");
>           break;
> @@ -254,6 +258,38 @@ handle_msg_format(StreamDevice *dev, SpiceCharDeviceInstance *sin)
>       return true;
>   }
>   
> +static bool
> +handle_msg_capabilities(StreamDevice *dev, SpiceCharDeviceInstance *sin)
> +{
> +    SpiceCharDeviceInterface *sif = spice_char_device_get_interface(sin);
> +
> +    if (spice_extra_checks) {
> +        spice_assert(dev->hdr_pos >= sizeof(StreamDevHeader));
> +        spice_assert(dev->hdr.type == STREAM_TYPE_CAPABILITIES);
> +    }
> +
> +    if (dev->hdr.size > STREAM_MSG_CAPABILITIES_MAX_BYTES) {
(*)
> +        return handle_msg_invalid(dev, sin, "Wrong size for StreamMsgCapabilities") > +    }
> +
> +    int n = sif->read(sin, dev->msg->buf + dev->msg_pos, dev->hdr.size - dev->msg_pos);
> +    if (n < 0) {
> +        return handle_msg_invalid(dev, sin, NULL);
> +    }
> +
> +    dev->msg_pos += n;
> +
> +    if (dev->msg_pos < dev->hdr.size) {
> +        return false;
> +    }
> +
> +    // copy only capabilities we care about
> +    memset(dev->guest_capabilities, 0, sizeof(dev->guest_capabilities));
> +    memcpy(dev->guest_capabilities, dev->msg->buf, MIN(MAX_GUEST_CAPABILITIES_BYTES, dev->hdr.size));

nits:
1. If the this line is reached (see *), there is no need for MIN as
    dev->hdr.size <= MAX_GUEST_CAPABILITIES_BYTES

2. All other messages got a structure in spice-protocol.
    This message can have one too (similar to StreamMsgData)

Uri.

> +
> +    return true;
> +}
> +
>   static bool
>   handle_msg_data(StreamDevice *dev, SpiceCharDeviceInstance *sin)
>   {
> @@ -586,6 +622,28 @@ char_device_set_state(RedCharDevice *char_dev, int state)
>       }
>   }
>   
> +static void
> +send_capabilities(RedCharDevice *char_dev)
> +{
> +    int msg_size = MAX_GUEST_CAPABILITIES_BYTES;
> +    int total_size = sizeof(StreamDevHeader) + msg_size;
> +
> +    RedCharDeviceWriteBuffer *buf =
> +        red_char_device_write_buffer_get_server_no_token(char_dev, total_size);
> +    buf->buf_used = total_size;
> +
> +    StreamDevHeader *const hdr = (StreamDevHeader *)buf->buf;
> +    hdr->protocol_version = STREAM_DEVICE_PROTOCOL;
> +    hdr->padding = 0;
> +    hdr->type = GUINT16_TO_LE(STREAM_TYPE_CAPABILITIES);
> +    hdr->size = GUINT32_TO_LE(msg_size);
> +
> +    StreamMsgCapabilities *const caps = (StreamMsgCapabilities *)(hdr+1);
> +    memset(caps, 0, msg_size);
> +
> +    red_char_device_write_buffer_add(char_dev, buf);
> +}
> +
>   static void
>   stream_device_port_event(RedCharDevice *char_dev, uint8_t event)
>   {
> @@ -599,6 +657,8 @@ stream_device_port_event(RedCharDevice *char_dev, uint8_t event)
>       dev->opened = (event == SPICE_PORT_EVENT_OPENED);
>       if (dev->opened) {
>           stream_device_create_channel(dev);
> +
> +        send_capabilities(char_dev);
>       }
>       dev->hdr_pos = 0;
>       dev->msg_pos = 0;
> diff --git a/server/tests/test-stream-device.c b/server/tests/test-stream-device.c
> index 3c9209a4..43011f9d 100644
> --- a/server/tests/test-stream-device.c
> +++ b/server/tests/test-stream-device.c
> @@ -135,12 +135,33 @@ static uint8_t *add_format(uint8_t *p, uint32_t w, uint32_t h, SpiceVideoCodecTy
>       return p + sizeof(fmt);
>   }
>   
> +// remove capabilities from server reply
> +static void
> +consume_server_capabilities(void)
> +{
> +    StreamDevHeader hdr;
> +
> +    if (vmc_write_pos == 0) {
> +        return;
> +    }
> +    g_assert(vmc_write_pos >= sizeof(hdr));
> +
> +    memcpy(&hdr, vmc_write_buf, sizeof(hdr));
> +    if (GUINT16_FROM_LE(hdr.type) == STREAM_TYPE_CAPABILITIES) {
> +        g_assert_cmpint(GUINT32_FROM_LE(hdr.size), <=, vmc_write_pos - sizeof(hdr));
> +        vmc_write_pos -= GUINT32_FROM_LE(hdr.size) + sizeof(hdr);
> +        memmove(vmc_write_buf, vmc_write_buf + GUINT32_FROM_LE(hdr.size) + sizeof(hdr), vmc_write_pos);
> +    }
> +}
> +
>   // check we have an error message on the write buffer
>   static void
>   check_vmc_error_message(void)
>   {
>       StreamDevHeader hdr;
>   
> +    consume_server_capabilities();
> +
>       g_assert(vmc_write_pos >= sizeof(hdr));
>   
>       memcpy(&hdr, vmc_write_buf, sizeof(hdr));
> @@ -245,6 +266,7 @@ static void test_stream_device_unfinished(void)
>       g_assert(message_sizes_curr - message_sizes == 1);
>   
>       // we should have no data from the device
> +    consume_server_capabilities();
>       g_assert_cmpint(vmc_write_pos, ==, 0);
>   
>       test_destroy(test);
>