[Spice-devel] [RFC PATCH spice-server v4 16/22] stream-device: Create channel when needed

[Jonathon Jongsma]

I guess this depends on patch 2 ("notify client... dynamically"), so my
 concerns there will potentially affect this patch. on additional
thought below.

On Fri, 2017-08-25 at 10:54 +0100, Frediano Ziglio wrote:
> This allows a better id allocation as devices are created after
> fixed ones.
> Also will allow to support more easily multiple monitor.
> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  server/stream-device.c | 32 ++++++++++++++++++++++++++------
>  1 file changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/server/stream-device.c b/server/stream-device.c
> index 8909c9ba..f72140ab 100644
> --- a/server/stream-device.c
> +++ b/server/stream-device.c
> @@ -67,7 +67,7 @@ stream_device_read_msg_from_dev(RedCharDevice
> *self, SpiceCharDeviceInstance *si
>      SpiceCharDeviceInterface *sif;
>      int n;
>  
> -    if (dev->has_error) {
> +    if (dev->has_error || !dev->stream_channel) {
>          return NULL;
>      }
>  
> @@ -223,11 +223,7 @@ stream_device_connect(RedsState *reds,
> SpiceCharDeviceInstance *sin)
>  {
>      SpiceCharDeviceInterface *sif;
>  
> -    StreamChannel *stream_channel = stream_channel_new(reds, 1); //
> TODO id
> -
>      StreamDevice *dev = stream_device_new(sin, reds);
> -    dev->stream_channel = stream_channel;
> -    stream_channel_register_start_cb(stream_channel,
> stream_device_stream_start, dev);
>  
>      sif = spice_char_device_get_interface(sin);
>      if (sif->state) {
> @@ -250,6 +246,25 @@ stream_device_dispose(GObject *object)
>  }
>  
>  static void
> +allocate_channels(StreamDevice *dev)

I suppose you called it allocate_channels() (plural) since you want to
potentially support multiple stream channels in the future. Maybe it's
worth a comment here saying "at the moment we only support a single
channel?

> +{
> +    if (dev->stream_channel) {
> +        return;
> +    }
> +
> +    SpiceServer* reds =
> red_char_device_get_server(RED_CHAR_DEVICE(dev));
> +
> +    int id = reds_get_free_channel_id(reds, SPICE_CHANNEL_DISPLAY);
> +    g_return_if_fail(id >= 0);
> +
> +    StreamChannel *stream_channel = stream_channel_new(reds, id);
> +
> +    dev->stream_channel = stream_channel;
> +
> +    stream_channel_register_start_cb(stream_channel,
> stream_device_stream_start, dev);
> +}
> +
> +static void
>  stream_device_port_event(RedCharDevice *char_dev, uint8_t event)
>  {
>      if (event != SPICE_PORT_EVENT_OPENED && event !=
> SPICE_PORT_EVENT_CLOSED) {
> @@ -260,10 +275,15 @@ stream_device_port_event(RedCharDevice
> *char_dev, uint8_t event)
>  
>      // reset device and channel on close/open
>      device->opened = (event == SPICE_PORT_EVENT_OPENED);
> +    if (device->opened) {
> +        allocate_channels(device);
> +    }
>      device->hdr_pos = 0;
>      device->has_error = false;
>      red_char_device_reset(char_dev);
> -    stream_channel_reset(device->stream_channel);
> +    if (device->stream_channel) {
> +        stream_channel_reset(device->stream_channel);
> +    }


There's seems to be a bit of a mismatch here. First you call
allocate_channels(), whose name suggests that it might create multiple
channels (and indeed, we could update the implementation of that
function in the future to add support for multiple stream channels).
But then you check for the existence of a single channel to decide
whether to reset it. What about introducing another function here to
encapsulate the multiple channel support? e.g.

static void reset_channels(StreamDevice *dev)
{
  // for now we only support a single channel
  if (device->stream_channel) {
    stream_channel_reset(device->stream_channel);
  }
}


>  }
>  
>  static void