[Spice-devel] [PATCH 06/22] Get rid of C-style memset initializations, use C++ style aggregates

[Lukáš Hrázký]

On Wed, 2018-02-28 at 17:36 +0100, Christophe Fergeau wrote:
> My understanding is that the previous iteration was quite controversial,
> I would just drop it from the series unless you get acks from everyone
> involved this time.

I've only commented on compiler support, it seems it's fine, so no
problem for me. I'll leave the ack to Frediano, he understands better
the padding issues.

> Christophe
> 
> On Wed, Feb 28, 2018 at 04:43:09PM +0100, Christophe de Dinechin wrote:
> > From: Christophe de Dinechin <dinechin at redhat.com>
> > 
> > C++-style aggregate initialization with explicit field names is more
> > readable and requires less repeated typing.
> > 
> > Like zero initialization, aggregate initialization preserves C++ type
> > safety.  However, unlike zero initialization, it does not guarantee
> > that padding is properly initialized. This is not a real problem in
> > our code where all padding is explicit, but it it worth keeping in

"it is".

> > mind.
> > 
> > Also note that GCC is a bit picky and will generate strange
> > "unsupported" or "not implemented" errors if the fields are not all
> > initialized in exact declaration order. It is useful, however, to get
> > an error if a field is added and not initialized.
> > 
> > Writing this patch also highlighted an inconsistency between the type
> > of a 'codec' local variable and the type used in the actual data
> > type. Changed the type of the variable to uint8_t to match that in the
> > structure declaration.
> > 
> > Signed-off-by: Christophe de Dinechin <dinechin at redhat.com>
> > ---
> >  src/spice-streaming-agent.cpp | 47 ++++++++++++++++++++++++++-----------------
> >  1 file changed, 28 insertions(+), 19 deletions(-)
> > 
> > diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> > index acae939..7b56458 100644
> > --- a/src/spice-streaming-agent.cpp
> > +++ b/src/spice-streaming-agent.cpp
> > @@ -221,19 +221,24 @@ write_all(int fd, const void *buf, const size_t len)
> >      return written;
> >  }
> >  
> > -static int spice_stream_send_format(int streamfd, unsigned w, unsigned h, unsigned c)
> > +static int spice_stream_send_format(int streamfd, unsigned w, unsigned h, uint8_t c)
> >  {
> > -
> > -    SpiceStreamFormatMessage msg;
> > -    const size_t msgsize = sizeof(msg);
> > -    const size_t hdrsize  = sizeof(msg.hdr);
> > -    memset(&msg, 0, msgsize);
> > -    msg.hdr.protocol_version = STREAM_DEVICE_PROTOCOL;
> > -    msg.hdr.type = STREAM_TYPE_FORMAT;
> > -    msg.hdr.size = msgsize - hdrsize; /* includes only the body? */
> > -    msg.msg.width = w;
> > -    msg.msg.height = h;
> > -    msg.msg.codec = c;
> > +    const size_t msgsize = sizeof(SpiceStreamFormatMessage);
> > +    const size_t hdrsize  = sizeof(StreamDevHeader);
> > +    SpiceStreamFormatMessage msg = {
> > +        .hdr = {
> > +            .protocol_version = STREAM_DEVICE_PROTOCOL,
> > +            .padding = 0,       // Workaround GCC "not implemented" bug
> > +            .type = STREAM_TYPE_FORMAT,
> > +            .size = msgsize - hdrsize
> > +        },
> > +        .msg = {
> > +            .width = w,
> > +            .height = h,
> > +            .codec = c,
> > +            .padding1 = { }
> > +        }
> > +    };
> >      syslog(LOG_DEBUG, "writing format\n");
> >      std::lock_guard<std::mutex> stream_guard(stream_mtx);
> >      if (write_all(streamfd, &msg, msgsize) != msgsize) {
> > @@ -244,14 +249,18 @@ static int spice_stream_send_format(int streamfd, unsigned w, unsigned h, unsign
> >  
> >  static int spice_stream_send_frame(int streamfd, const void *buf, const unsigned size)
> >  {
> > -    SpiceStreamDataMessage msg;
> > -    const size_t msgsize = sizeof(msg);
> >      ssize_t n;
> > +    const size_t msgsize = sizeof(SpiceStreamFormatMessage);
> > +    SpiceStreamDataMessage msg = {
> > +        .hdr = {
> > +            .protocol_version = STREAM_DEVICE_PROTOCOL,
> > +            .padding = 0,       // Workaround GCC "not implemented" bug
> > +            .type = STREAM_TYPE_DATA,
> > +            .size = size  /* includes only the body? */

The question in the comment? :)

> > +        },
> > +        .msg = {}
> > +    };
> >  
> > -    memset(&msg, 0, msgsize);
> > -    msg.hdr.protocol_version = STREAM_DEVICE_PROTOCOL;
> > -    msg.hdr.type = STREAM_TYPE_DATA;
> > -    msg.hdr.size = size; /* includes only the body? */
> >      std::lock_guard<std::mutex> stream_guard(stream_mtx);
> >      n = write_all(streamfd, &msg, msgsize);
> >      syslog(LOG_DEBUG,
> > @@ -424,7 +433,7 @@ do_capture(int streamfd, const char *streamport, FILE *f_log)
> >  
> >              if (frame.stream_start) {
> >                  unsigned width, height;
> > -                unsigned char codec;
> > +                uint8_t codec;
> >  
> >                  width = frame.size.width;
> >                  height = frame.size.height;
> > -- 
> > 2.13.5 (Apple Git-94)
> > 
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel