> 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.
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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel