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

[Christophe de Dinechin]

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

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


> +    msg[len] = '\0'; // make sure to terminate the string - TODO is there a better way?

Put this after the read_message, safer…

> +
> +    read_message(msg, len);
> +    StreamMsgNotifyError *error_msg = (StreamMsgNotifyError*) msg;

Yucky in C++ code, and I believe unnecessary here.

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

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