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

[Frediano Ziglio]

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

Not necessarily, but allow to allocate space for it.
Not far from the implication (base*) g_malloc(sizeof(base) + sizeof(other))
we have in lot of different places (and is also not always true).

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

The same declaration happens in another function/patch.
Maybe the structure should be declared somewhere?

> I guess I don't care too much, but a comment might be useful.

Suggestions?

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