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

[Frediano Ziglio]

> 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.

I think a 

  size_t to_read = std::min(len_to_read, sizeof(msg));
  // read message
  ...

  if (to_read > len_to_read) {
    throw std::runtime_error("Unexpected long error message from server");
  }

is easy and enough.

> 
> 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?
> 

Some kind of

    struct : StreamMsgNotifyError {
        uint8_t msg_buffer[1024];
    } msg;

> > 
> > > +    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?
> 

with the definition above casts are not required.

> > > +
> > > +    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
> 

I don't see this format as safer than explicit termination.

> > > }
> > > 
> > > static void read_command_from_device(void)

Frediano