> On 1 Mar 2018, at 10:51, Lukáš Hrázký <lhrazky@xxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxx>
>>>
>>> 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”.
Fixed
>
>>> 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@xxxxxxxxxx>
>>> ---
>>> 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? :)
It was in the original. Not sure why.
>
>>> + },
>>> + .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@xxxxxxxxxxxxxxxxxxxxx
>>> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>>
>> _______________________________________________
>> 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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel