[Spice-devel] [RFC PATCH spice-streaming-agent 2/2] Implement handling of error messages from the server

[Lukáš Hrázký]

On Thu, 2018-02-22 at 19:25 +0100, Christophe de Dinechin wrote:
> > On 22 Feb 2018, at 15:11, Lukáš Hrázký <lhrazky at redhat.com> wrote:
> > 
> > Signed-off-by: Lukáš Hrázký <lhrazky at redhat.com>
> > ---
> > 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 at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
>