On Wed, 2017-08-30 at 10:43 -0400, Frediano Ziglio wrote:
> >
> > Right now, each handle_msg_*() function is required to reset the
> > header
> > parsing state after it is done handling the message. Although the
> > code
> > duplication is pretty minimal (just resetting hdr_pos to 0), it
> > seems a
> > bit cleaner to have the calling function
> > (stream_device_read_msg_from_dev()) handle this. Change the message
> > handler functions to return a boolean value that signals whether
> > they've
> > fully handled the message or not. If the return value is true, the
> > calling function will reset the message parsing state so that we're
> > prepared to parse the next mesage.
> > ---
> >
> > Proposed addition to Frediano's patch series. (Also somewhat
> > related to
> > Christophe D's question about resetting hdr_pos to 0 in patch 22)
> >
>
> Was thinking about doing something about it.
>
> Looks good.
>
> I was also thinking instead of passing a SpiceCharDeviceInstance
> to pass a callback to read data and stop when message data is
> finished
> but maybe is just overkilling.
Hmm, it could be interesting, but I'm not sure if it's necessary.
>
> Should I squash this change along the other patches (beside last
> hunk) ?
Yeah, feel free to squash with other patches if you want.
>
> Frediano
>
> > server/stream-device.c | 69
> > +++++++++++++++++++++++++++-----------------------
> > 1 file changed, 37 insertions(+), 32 deletions(-)
> >
> > diff --git a/server/stream-device.c b/server/stream-device.c
> > index 3d33ca13d..a0383ae3e 100644
> > --- a/server/stream-device.c
> > +++ b/server/stream-device.c
> > @@ -61,7 +61,7 @@ static StreamDevice
> > *stream_device_new(SpiceCharDeviceInstance *sin, RedsState *
> >
> > G_DEFINE_TYPE(StreamDevice, stream_device, RED_TYPE_CHAR_DEVICE)
> >
> > -typedef void StreamMsgHandler(StreamDevice *dev,
> > SpiceCharDeviceInstance
> > *sin);
> > +typedef bool StreamMsgHandler(StreamDevice *dev,
> > SpiceCharDeviceInstance
> > *sin);
> >
> > static StreamMsgHandler handle_msg_format, handle_msg_data,
> > handle_msg_invalid,
> > handle_msg_cursor_set, handle_msg_cursor_move;
> > @@ -72,6 +72,7 @@ stream_device_read_msg_from_dev(RedCharDevice
> > *self,
> > SpiceCharDeviceInstance *si
> > StreamDevice *dev = STREAM_DEVICE(self);
> > SpiceCharDeviceInterface *sif;
> > int n;
> > + bool handled = false;
> >
> > if (dev->has_error || dev->flow_stopped || !dev-
> > >stream_channel) {
> > return NULL;
> > @@ -100,35 +101,41 @@ stream_device_read_msg_from_dev(RedCharDevice
> > *self,
> > SpiceCharDeviceInstance *si
> > switch ((StreamMsgType) dev->hdr.type) {
> > case STREAM_TYPE_FORMAT:
> > if (dev->hdr.size != sizeof(StreamMsgFormat)) {
> > - handle_msg_invalid(dev, sin);
> > + handled = handle_msg_invalid(dev, sin);
> > } else {
> > - handle_msg_format(dev, sin);
> > + handled = handle_msg_format(dev, sin);
> > }
> > break;
> > case STREAM_TYPE_DATA:
> > - handle_msg_data(dev, sin);
> > + handled = handle_msg_data(dev, sin);
> > break;
> > case STREAM_TYPE_CURSOR_SET:
> > - handle_msg_cursor_set(dev, sin);
> > + handled = handle_msg_cursor_set(dev, sin);
> > break;
> > case STREAM_TYPE_CURSOR_MOVE:
> > if (dev->hdr.size != sizeof(StreamMsgCursorMove)) {
> > - handle_msg_invalid(dev, sin);
> > + handled = handle_msg_invalid(dev, sin);
> > } else {
> > - handle_msg_cursor_move(dev, sin);
> > + handled = handle_msg_cursor_move(dev, sin);
> > }
> > break;
> > case STREAM_TYPE_CAPABILITIES:
> > /* FIXME */
> > default:
> > - handle_msg_invalid(dev, sin);
> > + handled = handle_msg_invalid(dev, sin);
> > break;
> > }
> >
> > + /* current message has been handled, so reset state and get
> > ready to
> > parse
> > + * the next message */
> > + if (handled) {
> > + dev->hdr_pos = 0;
> > + }
> > +
> > return NULL;
> > }
> >
> > -static void
> > +static bool
> > handle_msg_invalid(StreamDevice *dev, SpiceCharDeviceInstance
> > *sin)
> > {
> > static const char error_msg[] = "Wrong protocol";
> > @@ -154,28 +161,29 @@ handle_msg_invalid(StreamDevice *dev,
> > SpiceCharDeviceInstance *sin)
> > red_char_device_write_buffer_add(char_dev, buf);
> >
> > dev->has_error = true;
> > + return true;
> > }
> >
> > -static void
> > +static bool
> > handle_msg_format(StreamDevice *dev, SpiceCharDeviceInstance *sin)
> > {
> > StreamMsgFormat fmt;
> > SpiceCharDeviceInterface *sif =
> > spice_char_device_get_interface(sin);
> > int n = sif->read(sin, (uint8_t *) &fmt, sizeof(fmt));
> > if (n == 0) {
> > - return;
> > + return false;
> > }
> > if (n != sizeof(fmt)) {
> > - handle_msg_invalid(dev, sin);
> > - return;
> > + return handle_msg_invalid(dev, sin);
> > }
> > fmt.width = GUINT32_FROM_LE(fmt.width);
> > fmt.height = GUINT32_FROM_LE(fmt.height);
> > stream_channel_change_format(dev->stream_channel, &fmt);
> > - dev->hdr_pos = 0;
> > +
> > + return true;
> > }
> >
> > -static void
> > +static bool
> > handle_msg_data(StreamDevice *dev, SpiceCharDeviceInstance *sin)
> > {
> > SpiceCharDeviceInterface *sif =
> > spice_char_device_get_interface(sin);
> > @@ -193,8 +201,10 @@ handle_msg_data(StreamDevice *dev,
> > SpiceCharDeviceInstance *sin)
> > dev->hdr.size -= n;
> > }
> > if (dev->hdr.size == 0) {
> > - dev->hdr_pos = 0;
> > + return true;
> > }
> > +
> > + return false;
> > }
> >
> > /*
> > @@ -262,7 +272,7 @@ stream_msg_cursor_set_to_cursor_cmd(const
> > StreamMsgCursorSet *msg, size_t msg_si
> > return cmd;
> > }
> >
> > -static void
> > +static bool
> > handle_msg_cursor_set(StreamDevice *dev, SpiceCharDeviceInstance
> > *sin)
> > {
> > const unsigned int max_cursor_set_size =
> > @@ -273,45 +283,41 @@ handle_msg_cursor_set(StreamDevice *dev,
> > SpiceCharDeviceInstance *sin)
> > // read part of the message till we get all
> > if (!dev->msg_buf) {
> > if (dev->hdr.size >= max_cursor_set_size || dev->hdr.size
> > <
> > sizeof(StreamMsgCursorSet)) {
> > - handle_msg_invalid(dev, sin);
> > - return;
> > + return handle_msg_invalid(dev, sin);
> > }
> > dev->msg_buf = spice_malloc(dev->hdr.size);
> > dev->msg_len = 0;
> > }
> > int n = sif->read(sin, (uint8_t *) dev->msg_buf + dev-
> > >msg_len,
> > dev->hdr.size - dev->msg_len);
> > if (n <= 0) {
> > - return;
> > + return false;
> > }
> > dev->msg_len += n;
> > if (dev->msg_len != dev->hdr.size) {
> > - return;
> > + return false;
> > }
> >
> > - // got the full message, prepare for future message
> > - dev->hdr_pos = 0;
> > -
> > // trasform the message to a cursor command and process it
> > RedCursorCmd *cmd = stream_msg_cursor_set_to_cursor_cmd(dev-
> > >msg_buf,
> > dev->msg_len);
> > if (!cmd) {
> > - handle_msg_invalid(dev, sin);
> > - return;
> > + return handle_msg_invalid(dev, sin);
> > }
> > cursor_channel_process_cmd(dev->cursor_channel, cmd);
> > +
> > + return true;
> > }
> >
> > -static void
> > +static bool
> > handle_msg_cursor_move(StreamDevice *dev, SpiceCharDeviceInstance
> > *sin)
> > {
> > StreamMsgCursorMove move;
> > SpiceCharDeviceInterface *sif =
> > spice_char_device_get_interface(sin);
> > int n = sif->read(sin, (uint8_t *) &move, sizeof(move));
> > if (n == 0) {
> > - return;
> > + return false;
> > }
> > if (n != sizeof(move)) {
> > - handle_msg_invalid(dev, sin);
> > - return;
> > + return handle_msg_invalid(dev, sin);
> > }
> > move.x = GINT32_FROM_LE(move.x);
> > move.y = GINT32_FROM_LE(move.y);
> > @@ -323,7 +329,7 @@ handle_msg_cursor_move(StreamDevice *dev,
> > SpiceCharDeviceInstance *sin)
> >
> > cursor_channel_process_cmd(dev->cursor_channel, cmd);
> >
> > - dev->hdr_pos = 0;
> > + return true;
> > }
> >
> > static void
> > @@ -482,7 +488,6 @@ stream_device_port_event(RedCharDevice
> > *char_dev, uint8_t
> > event)
> > if (device->opened) {
> > allocate_channels(device);
> > }
> > - device->hdr_pos = 0;
> > device->has_error = false;
> > device->flow_stopped = false;
> > red_char_device_reset(char_dev);
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel