[Spice-devel] [PATCH spice-server 2/2] stream-device: Handle capabilities
Christophe de Dinechin
cdupontd at redhat.com
Tue Mar 20 10:03:18 UTC 2018
Looks good, with minor nits.
> On 19 Mar 2018, at 17:46, Frediano Ziglio <fziglio at redhat.com> 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) {
Premature optimization.
> + 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) {
Reading the various sif->read, the convention on return values is a bit unclear. Most other places seem to check for <= 0. Only handle_msg_format uses < 0. Someone could teach me why?
Is it possible for sif->read to return 0 on error?
Is it possible for sif->read to return less than the requested size (e.g. EINTR)?
> + 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
I think it’s “capabilities we know about”, since ifsd we get extra, it’s probably stuff added after this version.
> + memset(dev->guest_capabilities, 0, sizeof(dev->guest_capabilities));
> + memcpy(dev->guest_capabilities, dev->msg->buf, MIN(MAX_GUEST_CAPABILITIES_BYTES, dev->hdr.size));
> +
> + 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);
We don’t have a macro/function taking care of filling a StreamDevHeader?
> +
> + 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
The name already says that (except it uses “consume” and not “remove”.
The comment would be more helpful to me if it explained why this is necessary.
Based on the fact this is to make a test pass, I think that “skip_server_capabilities” or “ignore_server_capbilities” would be a better name.
> +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);
> --
> 2.14.3
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
More information about the Spice-devel
mailing list