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

[Christophe de Dinechin]

Following the explanations,

Acked-by: Christophe de Dinechin  

(“preparation” patch, uppercase, etc can all be followups)


> On 21 Mar 2018, at 18:07, Frediano Ziglio  wrote:
> 
>>> 
>>> On 20 Mar 2018, at 12:28, Frediano Ziglio  wrote:
>>> 
 
 Looks good, with minor nits.
 
> On 19 Mar 2018, at 17:46, 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 
> ---
> 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) {
>> 
>> As an aside, the fact that you used a lower-case for an enum in violation of
>> the existing practice in the code makes this look like a regular test, not a
>> configuration test.
>> 
> 
> You are right, maybe upper case would be better.
> 
 
 Premature optimization.
 
>>> 
>>> Extra is not expensive
>> 
>> If it’s not expensive, then what is it? (See my comments in other patch, the
>> alternatives to “expensive” in english are “superfluous” or “superior”).
>> 
>>> and code is coherent with other part.
>> 
>> Yes, I’m well aware you consistently ignored my input on this topic earlier
>> :-)
>> 
>> To be clear, I”m nacking the “if”, not the assert, in
>> 
>>  if (spice_extra_check)
>>  {
>>  spice_assert(dev->hdr_pos >= sizeof(StreamDevHeader));
>>  spice_assert(dev->hdr.type == STREAM_TYPE_CAPABILITIES);
>>  }
>> 
>> The “if” means “these tests have something special that makes them worth
>> skipping by default”. Therefore, the “if” test is basically lying to me :-)
>> which is why I want it gone.
>> 
> 
> In this case the reason is "paranoia". The size test is a duplicate,
> the type is like a malloc function that is checking if the caller
> really wants to allocate memory.
> 
>> A wise man once wrote on this list that he would rather have a core than a
>> remote execution. I believe that you should listen to him.
>> 
> 
> These tests do not prevent a remote execution. These kind of test are
> more used during testing if you really screw up things.
> 
>> 

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

[Christophe de Dinechin]

> On 21 Mar 2018, at 18:07, Frediano Ziglio  wrote:
> 
>>> 
>>> On 20 Mar 2018, at 12:28, Frediano Ziglio  wrote:
>>> 
 
 Looks good, with minor nits.
 
> On 19 Mar 2018, at 17:46, 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 
> ---
> 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) {
>> 
>> As an aside, the fact that you used a lower-case for an enum in violation of
>> the existing practice in the code makes this look like a regular test, not a
>> configuration test.
>> 
> 
> You are right, maybe upper case would be better.
> 
 
 Premature optimization.
 
>>> 
>>> Extra is not expensive
>> 
>> If it’s not expensive, then what is it? (See my comments in other patch, the
>> alternatives to “expensive” in english are “superfluous” or “superior”).
>> 
>>> and code is coherent with other part.
>> 
>> Yes, I’m well aware you consistently ignored my input on this topic earlier
>> :-)
>> 
>> To be clear, I”m nacking the “if”, not the assert, in
>> 
>>  if (spice_extra_check)
>>  {
>>  spice_assert(dev->hdr_pos >= sizeof(StreamDevHeader));
>>  spice_assert(dev->hdr.type == STREAM_TYPE_CAPABILITIES);
>>  }
>> 
>> The “if” means “these tests have something special that makes them worth
>> skipping by default”. Therefore, the “if” test is basically lying to me :-)
>> which is why I want it gone.
>> 
> 
> In this case the reason is "paranoia". The size test is a duplicate,
> the type is like a malloc function that is checking if the caller
> really wants to allocate memory.

I really believe that it is a bad habit to disable asserts locally on a 
case-by-case basis. It makes code really hard to read and unnecessarily 
puzzling. If I have to scratch my head looking at this and wait until you 
explain that the extra if is because you think the caller already validated 
that, the if is unhelpful.

That being said, I am very much in favor of finer-grained assert classes, that 
actually document your 

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

[Frediano Ziglio]

> 
> > On 20 Mar 2018, at 12:28, Frediano Ziglio  wrote:
> > 
> >> 
> >> Looks good, with minor nits.
> >> 
> >>> On 19 Mar 2018, at 17:46, 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 
> >>> ---
> >>> 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) {
> 
> As an aside, the fact that you used a lower-case for an enum in violation of
> the existing practice in the code makes this look like a regular test, not a
> configuration test.
> 

You are right, maybe upper case would be better.

> >> 
> >> Premature optimization.
> >> 
> > 
> > Extra is not expensive
> 
> If it’s not expensive, then what is it? (See my comments in other patch, the
> alternatives to “expensive” in english are “superfluous” or “superior”).
> 
> > and code is coherent with other part.
> 
> Yes, I’m well aware you consistently ignored my input on this topic earlier
> :-)
> 
> To be clear, I”m nacking the “if”, not the assert, in
> 
>   if (spice_extra_check)
>   {
>   spice_assert(dev->hdr_pos >= sizeof(StreamDevHeader));
>   spice_assert(dev->hdr.type == STREAM_TYPE_CAPABILITIES);
>   }
> 
> The “if” means “these tests have something special that makes them worth
> skipping by default”. Therefore, the “if” test is basically lying to me :-)
> which is why I want it gone.
> 

In this case the reason is "paranoia". The size test is a duplicate,
the type is like a malloc function that is checking if the caller
really wants to allocate memory.

> A wise man once wrote on this list that he would rather have a core than a
> remote execution. I believe that you should listen to him.
> 

These tests do not prevent a remote execution. These kind of test are
more used during testing if you really screw up things.

> 
> 
> >>> +}
> > 
> >>> +spice_assert(dev->hdr_pos >= sizeof(StreamDevHeader));
> >>> +spice_assert(dev->hdr.type == STREAM_TYPE_CAPABILITIES);
> >>> +}
> >>> +
> >>> +if (dev->hdr.size > STREAM_MSG_CAPABILITIES_MAX_BYTES) {
> 

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

[Christophe de Dinechin]

> On 20 Mar 2018, at 12:28, Frediano Ziglio  wrote:
> 
>> 
>> Looks good, with minor nits.
>> 
>>> On 19 Mar 2018, at 17:46, 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 
>>> ---
>>> 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) {

As an aside, the fact that you used a lower-case for an enum in violation of 
the existing practice in the code makes this look like a regular test, not a 
configuration test.

>> 
>> Premature optimization.
>> 
> 
> Extra is not expensive

If it’s not expensive, then what is it? (See my comments in other patch, the 
alternatives to “expensive” in english are “superfluous” or “superior”).

> and code is coherent with other part.

Yes, I’m well aware you consistently ignored my input on this topic earlier :-)

To be clear, I”m nacking the “if”, not the assert, in

if (spice_extra_check)
{
spice_assert(dev->hdr_pos >= sizeof(StreamDevHeader));
spice_assert(dev->hdr.type == STREAM_TYPE_CAPABILITIES);
}

The “if” means “these tests have something special that makes them worth 
skipping by default”. Therefore, the “if” test is basically lying to me :-) 
which is why I want it gone.

A wise man once wrote on this list that he would rather have a core than a 
remote execution. I believe that you should listen to him.



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

That’s 

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

[Frediano Ziglio]

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

No, you are confusing STREAM_MSG_CAPABILITIES_MAX_BYTES which is
the protocol limit (currently 1024) for the capabilities with
MAX_GUEST_CAPABILITIES_BYTES which is based on current defined capabilities.

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

Yes, you are right, changed dev->msg->buf with 
dev->msg->capabilities.capabilities.
I also changes MAX_GUEST_CAPABILITIES_BYTES here with 
sizeof(dev->guest_capabilities)
to avoid confusion.

Frediano

> 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 

Re: [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  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 
> >>> ---
> >>> 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
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [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 
---
  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);
 

Re: [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  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 
---
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
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [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  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 
> > ---
> > 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 @@ 

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

[Christophe de Dinechin]

Looks good, with minor nits.

> On 19 Mar 2018, at 17:46, 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 
> ---
> 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