Re: [PATCH spice-streaming-agent 2/2] Do not redefine "msg" field

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]