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

[Uri Lublin]

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

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

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

Uri