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

[Christophe de Dinechin]

> On 23 Feb 2018, at 10:53, Christophe Fergeau <cfergeau at redhat.com> wrote:
> 
> Given the lengthy debate over what is mostly a small cosmetic patch, I
> suggest that we postpone this one for now and drop it from the series.

memset in C++ code is not just a style issue, it’s dangerous. It completely wipes out C++ type guarantees. For example, if someone inits a field with

	int x = 1;

Then all constructors will guarantee that x == -1, but a memset after object creation wipes out that guarantee. Same thing if we make of of the objects being memset-initialized contain some C++ object with a vtable. And so on. All these problems do not exist with C++ zero-initialization. Which is also significantly shorter to write.

Frankly, there should be no “lengthy debate” about this.


Regards,
Christophe

> 
> Christophe
> 
> On Fri, Feb 16, 2018 at 05:15:37PM +0100, Christophe de Dinechin wrote:
>> From: Christophe de Dinechin <dinechin at redhat.com>
>> 
>> 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 69c27a3..1e19e43 100644
>> --- a/src/spice-streaming-agent.cpp
>> +++ b/src/spice-streaming-agent.cpp
>> @@ -181,19 +181,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) {
>> @@ -204,14 +209,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? */
>> +        },
>> +        .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,
>> @@ -379,7 +388,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