[Spice-devel] [PATCH spice-server v6 13/19] stream-device: Start supporting resetting device when close/open on guest

[Jonathon Jongsma]

On Thu, 2017-10-12 at 15:52 +0100, Frediano Ziglio wrote:
> When guest close the device the host device has to be reset too.
> This make easier to restart the guest device which can happen in case
> of reboot, agent issues or if we want to update the agent.
> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  server/stream-channel.c | 34 ++++++++++++++++++++++++++++++++++
>  server/stream-channel.h |  7 ++++++-
>  server/stream-device.c  | 49
> +++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 89 insertions(+), 1 deletion(-)
> 
> diff --git a/server/stream-channel.c b/server/stream-channel.c
> index e89563b82..51b8badf9 100644
> --- a/server/stream-channel.c
> +++ b/server/stream-channel.c
> @@ -483,3 +483,37 @@ stream_channel_register_start_cb(StreamChannel
> *channel,
>      channel->start_cb = cb;
>      channel->start_opaque = opaque;
>  }
> +
> +void
> +stream_channel_reset(StreamChannel *channel)
> +{
> +    struct {
> +        StreamMsgStartStop base;
> +        uint8_t codecs_buffer[MAX_SUPPORTED_CODECS];
> +    } start_msg;
> +    StreamMsgStartStop *const start = &start_msg.base;

Personally, this strikes me as a somewhat confusing way of allocating
the message struct. It took me a while to realize that
start_msg.codecs_buffer is the same buffer as start->codecs. The
advantage of doing it this way is that no dynamic allocation is
necessary, but I think it kind of makes the code a bit more confusing.
I guess I don't care too much, but a comment might be useful.

Acked-by: Jonathon Jongsma <jjongsma at redhat.com>



> +    RedChannel *red_channel = RED_CHANNEL(channel);
> +
> +    // send destroy old stream
> +    red_channel_pipes_add_type(red_channel,
> RED_PIPE_ITEM_TYPE_STREAM_DESTROY);
> +
> +    // destroy display surface
> +    if (channel->width != 0 && channel->height != 0) {
> +        red_channel_pipes_add_type(red_channel,
> RED_PIPE_ITEM_TYPE_SURFACE_DESTROY);
> +    }
> +
> +    channel->stream_id = -1;
> +    channel->width = 0;
> +    channel->height = 0;
> +
> +    if (!red_channel_is_connected(red_channel)) {
> +        return;
> +    }
> +
> +    // try to request a new stream, this should start a new stream
> +    // if the guest is connected to the device and a client is
> already connected
> +    start->num_codecs = stream_channel_get_supported_codecs(channel,
> start->codecs);
> +    // send in any case, even if list is not changed
> +    // notify device about changes
> +    request_new_stream(channel, start);
> +}
> diff --git a/server/stream-channel.h b/server/stream-channel.h
> index ba098df49..bd075a951 100644
> --- a/server/stream-channel.h
> +++ b/server/stream-channel.h
> @@ -48,7 +48,12 @@ GType stream_channel_get_type(void) G_GNUC_CONST;
>   */
>  StreamChannel* stream_channel_new(RedsState *server, uint32_t id);
>  
> -struct StreamMsgFormat;
> +/**
> + * Reset channel at initial state
> + */
> +void stream_channel_reset(StreamChannel *channel);
> +
> +struct StreamMsgStreamFormat;
>  struct StreamMsgStartStop;
>  
>  void stream_channel_change_format(StreamChannel *channel,
> diff --git a/server/stream-device.c b/server/stream-device.c
> index 6e78b1a99..9e401f8ed 100644
> --- a/server/stream-device.c
> +++ b/server/stream-device.c
> @@ -43,6 +43,7 @@ struct StreamDevice {
>      StreamDevHeader hdr;
>      uint8_t hdr_pos;
>      bool has_error;
> +    bool opened;
>      StreamChannel *stream_channel;
>  };
>  
> @@ -203,6 +204,35 @@ stream_device_remove_client(RedCharDevice *self,
> RedClient *client)
>  {
>  }
>  
> +static void
> +stream_device_stream_start(void *opaque, StreamMsgStartStop *start,
> +                           StreamChannel *stream_channel
> G_GNUC_UNUSED)
> +{
> +    StreamDevice *dev = (StreamDevice *) opaque;
> +
> +    if (!dev->opened) {
> +        return;
> +    }
> +
> +    int msg_size = sizeof(*start) + sizeof(start->codecs[0]) *
> start->num_codecs;
> +    int total_size = sizeof(StreamDevHeader) + msg_size;
> +
> +    RedCharDevice *char_dev = RED_CHAR_DEVICE(dev);
> +    RedCharDeviceWriteBuffer *buf =
> +        red_char_device_write_buffer_get_server_no_token(char_dev,
> total_size);
> +    buf->buf_used = total_size;
> +
> +    StreamDevHeader *hdr = (StreamDevHeader *)buf->buf;
> +    hdr->protocol_version = STREAM_DEVICE_PROTOCOL;
> +    hdr->padding = 0;
> +    hdr->type = GUINT16_TO_LE(STREAM_TYPE_START_STOP);
> +    hdr->size = GUINT32_TO_LE(msg_size);
> +
> +    memcpy(&hdr[1], start, msg_size);
> +
> +    red_char_device_write_buffer_add(char_dev, buf);
> +}
> +
>  RedCharDevice *
>  stream_device_connect(RedsState *reds, SpiceCharDeviceInstance *sin)
>  {
> @@ -212,6 +242,7 @@ stream_device_connect(RedsState *reds,
> SpiceCharDeviceInstance *sin)
>  
>      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) {
> @@ -234,6 +265,23 @@ stream_device_dispose(GObject *object)
>  }
>  
>  static void
> +stream_device_port_event(RedCharDevice *char_dev, uint8_t event)
> +{
> +    if (event != SPICE_PORT_EVENT_OPENED && event !=
> SPICE_PORT_EVENT_CLOSED) {
> +        return;
> +    }
> +
> +    StreamDevice *dev = STREAM_DEVICE(char_dev);
> +
> +    // reset device and channel on close/open
> +    dev->opened = (event == SPICE_PORT_EVENT_OPENED);
> +    dev->hdr_pos = 0;
> +    dev->has_error = false;
> +    red_char_device_reset(char_dev);
> +    stream_channel_reset(dev->stream_channel);
> +}
> +
> +static void
>  stream_device_class_init(StreamDeviceClass *klass)
>  {
>      GObjectClass *object_class = G_OBJECT_CLASS(klass);
> @@ -245,6 +293,7 @@ stream_device_class_init(StreamDeviceClass
> *klass)
>      char_dev_class->send_msg_to_client =
> stream_device_send_msg_to_client;
>      char_dev_class->send_tokens_to_client =
> stream_device_send_tokens_to_client;
>      char_dev_class->remove_client = stream_device_remove_client;
> +    char_dev_class->port_event = stream_device_port_event;
>  }
>  
>  static void