>
> 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.
>
Would be great to have a mnemonic in stream-device.h.
> >
> > > + } 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