> On 7 Mar 2018, at 11:50, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
>
>>> 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.
Thanks.
>
>> 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);
No, but it is annoying. Will make that obvious in the commit log.
> - 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);
It’s picky relative to clang. But I agree that rephrasing the comment would improve it.
(I don’t want to refer to C++20, since this is WIP and that specific aspect may still change)
> - 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.
I understand the rationale, and I can add it. But the comment is not misleading, it was a response to a question you had about the reason for the type change (the reason being that I had a warning without it).
Changed in working branch to:
Also note that designated initializers is a C99 or C++20 feature.
It is presently accepted by both gcc and clang, but not VS2015.
Also note that gcc is picky relative to clang, but closer to the
current working draft for C++20, in that it requires fields to be
initialized in declaration order and to all be initialized, and that
you don't get a clear error message but an "unsupported" or
"unimplementd" message if you do not follow that rule.
Writing this patch also highlighted an inconsistency between the type
of a 'codec' local variable and the type used in the actual data
type (which must obey C rules, since it's in the protocol).
In order to avoid a warning, this patch changes the type of the
variable to uint8_t to match that in the structure declaration.
>
> As I said previously if the problem is the time that take to
> do the change to the patch I can do it.
It will not really help, since that series already received a fair number of changes, and so you patch would add on top of the pile.
>
> 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