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

[Frediano Ziglio]

> 
> On 03/20/2018 01:28 PM, Frediano Ziglio wrote:
> >>
> >> 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)
> 
> Currently no capability is defined, so:
>    STREAM_CAP_END = MAX_GUEST_CAPABILITIES_BYTES = 0
> 

Yes

> >>> +
> >>> 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;
> 
> ...
> 
> >>> @@ -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.
> 
> 
> Since, currently, there are no capabilities
> dev->hdr.size - dev->msg_pos = 0 - 0 = 0
> 

No, you are assuming the client is not sending capabilities which is
wrong, client should be free to send whatever it wants, but yes,
currently will be 0.

> The call sif->read(sin,buffer,0) returns 0.
> 

Yes, as standard Unix read does and is correctly supported.

> Uri

Frediano