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

[Frediano Ziglio]

> > On 6 Mar 2018, at 17:15, Christophe Fergeau <cfergeau at redhat.com> wrote:
> > 
> > On Thu, Mar 01, 2018 at 09:01:37PM +0100, Christophe de Dinechin wrote:
> >>> On 28 Feb 2018, at 17:36, Christophe Fergeau <cfergeau at redhat.com> 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.
> >> 
> >> It’s a bit difficult to drop that from the series, as it is a core element
> >> of the next steps if you look carefully.
> > 
> > I only looked at the code with the full series applied, but it really seems
> > like both way would
> > be possible?
> > 
> > diff --git a/src/concrete-agent.cpp b/src/concrete-agent.cpp
> > index fe8564f..2e1472a 100644
> > --- a/src/concrete-agent.cpp
> > +++ b/src/concrete-agent.cpp
> > @@ -140,7 +140,10 @@ public:
> >     }
> >     void write_message_body(Stream &stream, unsigned w, unsigned h, uint8_t
> >     c)
> >     {
> > -        StreamMsgFormat msg = { .width = w, .height = h, .codec = c,
> > .padding1 = {} };
> > +        StreamMsgFormat msg;
> > +        msg.width = w;
> > +        msg.height = h;
> > +        msg.codec = c;
> >         stream.write_all("format", &msg, sizeof(msg));
> >     }
> > };
> > diff --git a/src/message.hpp b/src/message.hpp
> > index fd69033..674e122 100644
> > --- a/src/message.hpp
> > +++ b/src/message.hpp
> > @@ -21,13 +21,12 @@ class Message
> > public:
> >     template <typename ...PayloadArgs>
> >     Message(PayloadArgs... payload_args)
> > -        : hdr(StreamDevHeader {
> > -              .protocol_version = STREAM_DEVICE_PROTOCOL,
> > -              .padding = 0,     // Workaround GCC bug "sorry: not
> > implemented"
> > -              .type = Type,
> > -              .size = (uint32_t) Info::size(payload_args...)
> > -          })
> > -    { }
> > +    {
> > +        hdr.protocol_version = STREAM_DEVICE_PROTOCOL;
> > +        hdr.padding = 0;
> > +        hdr.type = Type;
> > +        hdr.size = (uint32_t) Info::size(payload_args...);
> > +    }
> >     void write_header(Stream &stream)
> >     {
> >         stream.write_all("header", &hdr, sizeof(hdr));
> > 
> > Not strongly advocating for that change to be made just now, I was just
> > a bit surprised by how you dismissed this ;)
> 
> I did not dismiss it, and I regret you interpreted it that way ;-)
> 
> The meaning I gave to “core element of the next steps” was that practically
> every patch touches that part in one way or another. So changing it and
> keeping it consistent essentially means redoing a good fraction of the whole
> series by hand. That’s a lot of churn.
> 
> I could do it if I saw value in doing the change. But I feel like I replied
> to Frediano’s objections on this topic, as well as to yours. I also believe
> that the code is significantly simpler, more type-safe and more readable the
> way I wrote it, for example because we get a warning or an error if we
> forget a field.
> 

I confirm, the problem of the paddings and security was discussed
and agreed is not a problem.

> Does that address your objection?
> 
> > 
> > Christophe
> 

There are however still some issues:
- the syntax is using C++20 while we state we use C++11 syntax, this
  is basically using C compatibility extensions. I just tried and for
  instance this code is not accepted on Visual C++ 2015 (not an issue
  at the moment);
- the "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." is wrong, this is not a
  GCC errors, just that GCC is respecting C++. Fields are required
  to be initialized in the correct order in C++ (this differs from
  C99);
- the "Writing this patch also highlighted an inconsistency between the
  type of a 'codec' local variable and the type used in the actual data
  type." is misleading, the protocol is defined that way not for
  inconsistency but being a C file is subject to C limits and
  you cannot portably specify a binary size for enumerations.

As I said previously if the problem is the time that take to
do the change to the patch I can do it.

Frediano