> > On 6 Mar 2018, at 17:15, Christophe Fergeau <cfergeau@xxxxxxxxxx> 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@xxxxxxxxxx> 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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel