On Thu, 2018-02-22 at 19:25 +0100, Christophe de Dinechin wrote:
> > On 22 Feb 2018, at 15:11, Lukáš Hrázký <lhrazky@xxxxxxxxxx> wrote:
> >
> > Signed-off-by: Lukáš Hrázký <lhrazky@xxxxxxxxxx>
> > ---
> > src/spice-streaming-agent.cpp | 18 ++++++++++++++++--
> > 1 file changed, 16 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> > index b88225f..73baa48 100644
> > --- a/src/spice-streaming-agent.cpp
> > +++ b/src/spice-streaming-agent.cpp
> > @@ -130,8 +130,22 @@ static void handle_stream_capabilities(uint32_t len)
> >
> > static void handle_stream_error(uint32_t len)
> > {
> > - // TODO read message and use it
> > - throw std::runtime_error("got an error message from server");
> > + const size_t MSG_SIZE = 256;
> > +
> > + // TODO the message should have an upper size limit defined somewhere?
> > + if (len >= MSG_SIZE) {
> > + throw std::runtime_error("NotifyError message size " + std::to_string(len) +
> > + " is too long (longer than " + std::to_string(MSG_SIZE) + ")”);
>
> I would try to read the first MSG_SIZE bytes, this may give us a clue as to what the server wanted to tell us.
Good idea. However, I can't think of an elegant way to then read out
the rest of the message from the fd. Because we do not abort on
receiving the error message, so I need to either abort or read the
whole message out.
And since this shouldn't really happen anyway, I'm thinking of just
leave it as-is, unless you can suggest a simple way of reading out the
message?
> > + }
> > +
> > + uint8_t msg[MSG_SIZE];
>
> Why do a low-level byte allocation like this to then cast the pointer? Why not have a StreamMsgNotifyErrorXYZ on the stack (which is the StreamMsgNotify with non-zero space for the message)?
Not following, is StreamMsgNotifyErrorXYZ a real type?
>
> > + msg[len] = '\0'; // make sure to terminate the string - TODO is there a better way?
>
> Put this after the read_message, safer…
Will do.
> > +
> > + read_message(msg, len);
> > + StreamMsgNotifyError *error_msg = (StreamMsgNotifyError*) msg;
>
> Yucky in C++ code, and I believe unnecessary here.
True, reinterpret_cast. Or something better related to the
StreamMsgNotifyErrorXYZ above?
> > +
> > + syslog(LOG_ERR, "Received NotifyError message from the client: %d - %s\n",
> > + error_msg->error_code, error_msg->msg);
>
> Does syslog accept %.*s? If so, that might be safer and cheaper than relying on zero termination.
But with this the length specified is 'static' in the string? Not sure
how I could use it?
Thanks,
Lukas
> > }
> >
> > static void read_command_from_device(void)
> > --
> > 2.16.1
> >
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel