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

[Frediano Ziglio]

> 
> 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.
> 

Extra is not expensive and code is coherent with other part.

> > +        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?

No, 0 is 0 byte in the current buffer which does not mean end of file,
there's no EAGAIN behaviour.
Basically with <=0 you handle either 0 bytes or error while with <0 only
errors.

> Is it possible for sif->read to return less than the requested size (e.g.
> EINTR)?
> 

There's no EINTR but what can happen is that guest did a partial write
or the buffer was full so the write was truncated. The interface is not
blocking so partial read are normal.

> 
> > +        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.
> 

updated

> > +    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?
> 

No, there are not much messages for the guest, can be introduced

> > +
> > +    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.
> 

"consume" is used in the demarshalling code, I think is more appropriate
as indicate that data is removed from the buffer.

Yes, comment does not add much, I'll remove

> > +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);

Frediano