On Wed, 2017-11-01 at 05:02 -0400, Frediano Ziglio wrote:
> >
> > This is currently unlikely to happen since we communicate over a
> > pipe
> > and the pipe buffer is sufficiently large to avoid splitting the
> > message. But for completeness, we should handle this scenario.
> >
> > Signed-off-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
> > ---
> > server/stream-device.c | 33 +++++++++++++++++++++++----------
> > 1 file changed, 23 insertions(+), 10 deletions(-)
> >
> > diff --git a/server/stream-device.c b/server/stream-device.c
> > index fc5b50659..259527246 100644
> > --- a/server/stream-device.c
> > +++ b/server/stream-device.c
> > @@ -42,6 +42,12 @@ struct StreamDevice {
> >
> > StreamDevHeader hdr;
> > uint8_t hdr_pos;
> > + union {
> > + StreamMsgFormat format;
> > + StreamMsgCapabilities capabilities;
> > + uint8_t buf[1024];
>
> why capabilities ?
Just because it's another potential message (that we're not handling
yet). I was just preparing for the future when we implement this
message. I considered adding StreamMsgData to this union as well, but
thought that perhaps 16*1024 buffer was larger than we wanted within
the object?
> why a buffer of 1024 ?
The 1024 was chosen simply because the StreamMsgCapabilities message
specified to have a limit of 1024.
>
> > + } msg;
> > + uint8_t msg_pos;
>
> uint8_t is not big enough for the size of msg.
>
> > bool has_error;
> > bool opened;
> > bool flow_stopped;
> > @@ -155,20 +161,27 @@ handle_msg_invalid(StreamDevice *dev,
> > SpiceCharDeviceInstance *sin, const char *
> > 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 false;
> > - }
> > - if (n != sizeof(fmt)) {
> > +
> > + spice_assert(dev->hdr_pos >= sizeof(StreamDevHeader));
> > + spice_assert(dev->hdr.type == STREAM_TYPE_FORMAT);
> > +
> > + int n = sif->read(sin, dev->msg.buf + dev->msg_pos,
> > sizeof(StreamMsgFormat) - dev->msg_pos);
> > + if (n < 0) {
> > return handle_msg_invalid(dev, sin, NULL);
> > }
> > - fmt.width = GUINT32_FROM_LE(fmt.width);
> > - fmt.height = GUINT32_FROM_LE(fmt.height);
> > - stream_channel_change_format(dev->stream_channel, &fmt);
> >
> > - return true;
> > + dev->msg_pos += n;
> > +
> > + if (dev->msg_pos >= sizeof(StreamMsgFormat)) {
>
> I would do a "if (dev->msg_pos < sizeof(StreamMsgFormat)) return
> false" instead.
>
> > + dev->msg.format.width = GUINT32_FROM_LE(dev-
> > >msg.format.width);
> > + dev->msg.format.height = GUINT32_FROM_LE(dev-
> > >msg.format.height);
> > + stream_channel_change_format(dev->stream_channel, &dev-
> > >msg.format);
> > + dev->msg_pos = 0;
>
> should this reset be in the main code ? I think so.
>
> > + return true;
> > + }
> > +
> > + return false;
> > }
> >
> > static bool
>
> Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel