[Spice-devel] [PATCH spice-streaming-agent 2/2] Do not redefine "msg" field

[Lukáš Hrázký]

On Fri, 2018-03-09 at 02:41 -0500, Frediano Ziglio wrote:
> > 
> > msg.msg was redefining msg.StreamMsgNotifyError::msg.
> > This cause some confusion.
> > 
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> >  src/spice-streaming-agent.cpp | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> > index 37addf4..777e330 100644
> > --- a/src/spice-streaming-agent.cpp
> > +++ b/src/spice-streaming-agent.cpp
> > @@ -146,13 +146,13 @@ static void handle_stream_error(size_t len)
> >      }
> >  
> >      struct NotifyError : StreamMsgNotifyError {
> > -        uint8_t msg[1024];
> > +        uint8_t msg_buffer[1024];
> >      } msg;
> >  
> >      size_t len_to_read = std::min(len, sizeof(msg) - 1);
> >  
> >      read_all(&msg, len_to_read);
> > -    msg.msg[len_to_read - sizeof(StreamMsgNotifyError)] = '\0';
> > +    ((uint8_t *) &msg)[len_to_read] = '\0';
> >  
> 
> Not much proud of this. Maybe a reinterpret_cast would help?

reinterpret_cast would be better in C++, but it's not the cast itself
that's ugly :)

> Defining a union and a more complicated structure looks like
> overwhelming here. On the other hand more subtly we do the same
> conversion in the line above (read_all call).

While we do the same conversion in read_all, it is the fact that we do
it for the purpose of putting an '\0' at the end, which is a semantic
aspect of char* strings, but we cast a struct pointer to do it.

The cast in read_all is I would say a common, generic way to read data
into a structure directly.

I would say both ways (the old and the new introduced in the patch) are
ugly, but the old way is ugly at first glance, the new one you have to
stop and think to find out it's nasty :)

No idea what to do with it, maybe my original solution with a uint8_t
buffer and later cast it to a struct pointer was better after all?

Cheers,
Lukas

> >      syslog(LOG_ERR, "Received NotifyError message from the server: %d -
> >      %s\n",
> >          msg.error_code, msg.msg);
> 
> Frediano
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel