[08/22] Use C++ style for cursor message initialization instead of C style

Submitted by Christophe de Dinechin on Feb. 28, 2018, 3:43 p.m.

Details

Message ID 20180228154325.25791-9-christophe@dinechin.org
State New
Headers show
Series "streaming-agent: C++ refactoring" ( rev: 2 1 ) in Spice

Not browsing as part of any series.

Commit Message

Christophe de Dinechin Feb. 28, 2018, 3:43 p.m.
From: Christophe de Dinechin <dinechin@redhat.com>

Use C++ "placement new" to directly initialize a C++ object at a
specified memory location. This makes it possible to get rid of
type-unsafe memset, low-level unsafe pointer + sizeof displacement
computations, and type-unsafe reinterpret_cast<>.

Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
---
 src/spice-streaming-agent.cpp | 44 +++++++++++++++++++++++++------------------
 1 file changed, 26 insertions(+), 18 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index 5c36906..21f9c31 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -334,36 +334,44 @@  static void usage(const char *progname)
 }
 
 static void
-send_cursor(int streamfd, unsigned width, unsigned height, int hotspot_x, int hotspot_y,
+send_cursor(int streamfd,
+            uint16_t width, uint16_t height,
+            uint16_t hotspot_x, uint16_t hotspot_y,
             std::function<void(uint32_t *)> fill_cursor)
 {
     if (width >= STREAM_MSG_CURSOR_SET_MAX_WIDTH || height >= STREAM_MSG_CURSOR_SET_MAX_HEIGHT) {
         return;
     }
 
-    size_t cursor_size = sizeof(CursorMessage) + width * height * sizeof(uint32_t);
-    std::unique_ptr<uint8_t[]> msg(new uint8_t[cursor_size]);
+    const uint32_t msgsize = sizeof(CursorMessage) + width * height * sizeof(uint32_t);
+    const uint32_t hdrsize  = sizeof(StreamDevHeader);
 
-    StreamDevHeader &dev_hdr(*reinterpret_cast<StreamDevHeader*>(msg.get()));
-    memset(&dev_hdr, 0, sizeof(dev_hdr));
-    dev_hdr.protocol_version = STREAM_DEVICE_PROTOCOL;
-    dev_hdr.type = STREAM_TYPE_CURSOR_SET;
-    dev_hdr.size = cursor_size - sizeof(StreamDevHeader);
+    std::unique_ptr<uint8_t[]> storage(new uint8_t[msgsize]);
 
-    StreamMsgCursorSet &cursor_msg(*reinterpret_cast<StreamMsgCursorSet *>(msg.get() + sizeof(StreamDevHeader)));
-    memset(&cursor_msg, 0, sizeof(cursor_msg));
-
-    cursor_msg.type = SPICE_CURSOR_TYPE_ALPHA;
-    cursor_msg.width = width;
-    cursor_msg.height = height;
-    cursor_msg.hot_spot_x = hotspot_x;
-    cursor_msg.hot_spot_y = hotspot_y;
+    CursorMessage *cursor_msg =
+        new(storage.get()) CursorMessage {
+        .hdr = {
+            .protocol_version = STREAM_DEVICE_PROTOCOL,
+            .padding = 0,       // Workaround GCC internal / not implemented compiler error
+            .type = STREAM_TYPE_CURSOR_SET,
+            .size = msgsize - hdrsize
+        },
+        .msg = {
+            .width = width,
+            .height = height,
+            .hot_spot_x = hotspot_x,
+            .hot_spot_y = hotspot_y,
+            .type = SPICE_CURSOR_TYPE_ALPHA,
+            .padding1 = { },
+            .data = { }
+        }
+    };
 
-    uint32_t *pixels = reinterpret_cast<uint32_t *>(cursor_msg.data);
+    uint32_t *pixels = reinterpret_cast<uint32_t *>(cursor_msg->msg.data);
     fill_cursor(pixels);
 
     std::lock_guard<std::mutex> stream_guard(stream_mtx);
-    write_all(streamfd, msg.get(), cursor_size);
+    write_all(streamfd, storage.get(), msgsize);
 }
 
 static void cursor_changes(int streamfd, Display *display, int event_base)

Comments

On Wed, 2018-02-28 at 16:43 +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin <dinechin@redhat.com>
> 
> Use C++ "placement new" to directly initialize a C++ object at a
> specified memory location. This makes it possible to get rid of
> type-unsafe memset, low-level unsafe pointer + sizeof displacement
> computations, and type-unsafe reinterpret_cast<>.
> 
> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> ---
>  src/spice-streaming-agent.cpp | 44 +++++++++++++++++++++++++------------------
>  1 file changed, 26 insertions(+), 18 deletions(-)
> 
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index 5c36906..21f9c31 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -334,36 +334,44 @@ static void usage(const char *progname)
>  }
>  
>  static void
> -send_cursor(int streamfd, unsigned width, unsigned height, int hotspot_x, int hotspot_y,
> +send_cursor(int streamfd,
> +            uint16_t width, uint16_t height,
> +            uint16_t hotspot_x, uint16_t hotspot_y,
>              std::function<void(uint32_t *)> fill_cursor)
>  {
>      if (width >= STREAM_MSG_CURSOR_SET_MAX_WIDTH || height >= STREAM_MSG_CURSOR_SET_MAX_HEIGHT) {
>          return;
>      }
>  
> -    size_t cursor_size = sizeof(CursorMessage) + width * height * sizeof(uint32_t);
> -    std::unique_ptr<uint8_t[]> msg(new uint8_t[cursor_size]);
> +    const uint32_t msgsize = sizeof(CursorMessage) + width * height * sizeof(uint32_t);
> +    const uint32_t hdrsize  = sizeof(StreamDevHeader);

Double space. Why use uint32_t instead of size_t?

As I noted before, hdrsize is only used once, did you consider using
sizeof(StreamDevHeader) in-place?

Lukas

> -    StreamDevHeader &dev_hdr(*reinterpret_cast<StreamDevHeader*>(msg.get()));
> -    memset(&dev_hdr, 0, sizeof(dev_hdr));
> -    dev_hdr.protocol_version = STREAM_DEVICE_PROTOCOL;
> -    dev_hdr.type = STREAM_TYPE_CURSOR_SET;
> -    dev_hdr.size = cursor_size - sizeof(StreamDevHeader);
> +    std::unique_ptr<uint8_t[]> storage(new uint8_t[msgsize]);
>  
> -    StreamMsgCursorSet &cursor_msg(*reinterpret_cast<StreamMsgCursorSet *>(msg.get() + sizeof(StreamDevHeader)));
> -    memset(&cursor_msg, 0, sizeof(cursor_msg));
> -
> -    cursor_msg.type = SPICE_CURSOR_TYPE_ALPHA;
> -    cursor_msg.width = width;
> -    cursor_msg.height = height;
> -    cursor_msg.hot_spot_x = hotspot_x;
> -    cursor_msg.hot_spot_y = hotspot_y;
> +    CursorMessage *cursor_msg =
> +        new(storage.get()) CursorMessage {
> +        .hdr = {
> +            .protocol_version = STREAM_DEVICE_PROTOCOL,
> +            .padding = 0,       // Workaround GCC internal / not implemented compiler error
> +            .type = STREAM_TYPE_CURSOR_SET,
> +            .size = msgsize - hdrsize
> +        },
> +        .msg = {
> +            .width = width,
> +            .height = height,
> +            .hot_spot_x = hotspot_x,
> +            .hot_spot_y = hotspot_y,
> +            .type = SPICE_CURSOR_TYPE_ALPHA,
> +            .padding1 = { },
> +            .data = { }
> +        }
> +    };
>  
> -    uint32_t *pixels = reinterpret_cast<uint32_t *>(cursor_msg.data);
> +    uint32_t *pixels = reinterpret_cast<uint32_t *>(cursor_msg->msg.data);
>      fill_cursor(pixels);
>  
>      std::lock_guard<std::mutex> stream_guard(stream_mtx);
> -    write_all(streamfd, msg.get(), cursor_size);
> +    write_all(streamfd, storage.get(), msgsize);
>  }
>  
>  static void cursor_changes(int streamfd, Display *display, int event_base)
> On 1 Mar 2018, at 11:02, Lukáš Hrázký <lhrazky@redhat.com> wrote:
> 
> On Wed, 2018-02-28 at 16:43 +0100, Christophe de Dinechin wrote:
>> From: Christophe de Dinechin <dinechin@redhat.com>
>> 
>> Use C++ "placement new" to directly initialize a C++ object at a
>> specified memory location. This makes it possible to get rid of
>> type-unsafe memset, low-level unsafe pointer + sizeof displacement
>> computations, and type-unsafe reinterpret_cast<>.
>> 
>> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
>> ---
>> src/spice-streaming-agent.cpp | 44 +++++++++++++++++++++++++------------------
>> 1 file changed, 26 insertions(+), 18 deletions(-)
>> 
>> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
>> index 5c36906..21f9c31 100644
>> --- a/src/spice-streaming-agent.cpp
>> +++ b/src/spice-streaming-agent.cpp
>> @@ -334,36 +334,44 @@ static void usage(const char *progname)
>> }
>> 
>> static void
>> -send_cursor(int streamfd, unsigned width, unsigned height, int hotspot_x, int hotspot_y,
>> +send_cursor(int streamfd,
>> +            uint16_t width, uint16_t height,
>> +            uint16_t hotspot_x, uint16_t hotspot_y,
>>             std::function<void(uint32_t *)> fill_cursor)
>> {
>>     if (width >= STREAM_MSG_CURSOR_SET_MAX_WIDTH || height >= STREAM_MSG_CURSOR_SET_MAX_HEIGHT) {
>>         return;
>>     }
>> 
>> -    size_t cursor_size = sizeof(CursorMessage) + width * height * sizeof(uint32_t);
>> -    std::unique_ptr<uint8_t[]> msg(new uint8_t[cursor_size]);
>> +    const uint32_t msgsize = sizeof(CursorMessage) + width * height * sizeof(uint32_t);
>> +    const uint32_t hdrsize  = sizeof(StreamDevHeader);
> 
> Double space. Why use uint32_t instead of size_t?

defined by the protocol.

> 
> As I noted before, hdrsize is only used once, did you consider using
> sizeof(StreamDevHeader) in-place?

yes. the goal is to converge pieces of code that were doing the same thing differently, so I rewrote them to highlight similarities.

> 
> Lukas
> 
>> -    StreamDevHeader &dev_hdr(*reinterpret_cast<StreamDevHeader*>(msg.get()));
>> -    memset(&dev_hdr, 0, sizeof(dev_hdr));
>> -    dev_hdr.protocol_version = STREAM_DEVICE_PROTOCOL;
>> -    dev_hdr.type = STREAM_TYPE_CURSOR_SET;
>> -    dev_hdr.size = cursor_size - sizeof(StreamDevHeader);
>> +    std::unique_ptr<uint8_t[]> storage(new uint8_t[msgsize]);
>> 
>> -    StreamMsgCursorSet &cursor_msg(*reinterpret_cast<StreamMsgCursorSet *>(msg.get() + sizeof(StreamDevHeader)));
>> -    memset(&cursor_msg, 0, sizeof(cursor_msg));
>> -
>> -    cursor_msg.type = SPICE_CURSOR_TYPE_ALPHA;
>> -    cursor_msg.width = width;
>> -    cursor_msg.height = height;
>> -    cursor_msg.hot_spot_x = hotspot_x;
>> -    cursor_msg.hot_spot_y = hotspot_y;
>> +    CursorMessage *cursor_msg =
>> +        new(storage.get()) CursorMessage {
>> +        .hdr = {
>> +            .protocol_version = STREAM_DEVICE_PROTOCOL,
>> +            .padding = 0,       // Workaround GCC internal / not implemented compiler error
>> +            .type = STREAM_TYPE_CURSOR_SET,
>> +            .size = msgsize - hdrsize
>> +        },
>> +        .msg = {
>> +            .width = width,
>> +            .height = height,
>> +            .hot_spot_x = hotspot_x,
>> +            .hot_spot_y = hotspot_y,
>> +            .type = SPICE_CURSOR_TYPE_ALPHA,
>> +            .padding1 = { },
>> +            .data = { }
>> +        }
>> +    };
>> 
>> -    uint32_t *pixels = reinterpret_cast<uint32_t *>(cursor_msg.data);
>> +    uint32_t *pixels = reinterpret_cast<uint32_t *>(cursor_msg->msg.data);
>>     fill_cursor(pixels);
>> 
>>     std::lock_guard<std::mutex> stream_guard(stream_mtx);
>> -    write_all(streamfd, msg.get(), cursor_size);
>> +    write_all(streamfd, storage.get(), msgsize);
>> }
>> 
>> static void cursor_changes(int streamfd, Display *display, int event_base)