[12/17] Convert message writing from C style to C++ style

Submitted by Christophe de Dinechin on Feb. 16, 2018, 4:15 p.m.

Details

Message ID 20180216161547.28110-13-christophe@dinechin.org
State New
Headers show
Series "WIP: Refactor the streaming agent towards a more standard C++ style" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Christophe de Dinechin Feb. 16, 2018, 4:15 p.m.
From: Christophe de Dinechin <dinechin@redhat.com>

- The Stream class now deals with locking and sending messages
- The Message<> template class deals with the general writing mechanisms
- Three classes, FormatMessage, FrameMessage and X11CursorMessage represent individual messages

The various classes should be moved to separate headers in a subsequent operation

The design uses templates to avoid any runtime overhead. All the calls are static.

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

Patch hide | download patch | download mbox

diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index a989ee7..c174ea4 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -48,24 +48,6 @@  namespace spice
 namespace streaming_agent
 {
 
-struct FormatMessage
-{
-    StreamDevHeader hdr;
-    StreamMsgFormat msg;
-};
-
-struct DataMessage
-{
-    StreamDevHeader hdr;
-    StreamMsgData msg;
-};
-
-struct CursorMessage
-{
-    StreamDevHeader hdr;
-    StreamMsgCursorSet msg;
-};
-
 class Stream
 {
 public:
@@ -79,24 +61,131 @@  public:
     {
         close(streamfd);
     }
-    operator int() { return streamfd; }
 
     int have_something_to_read(int timeout);
     int read_command_from_device(void);
     int read_command(bool blocking);
 
+
+    template <typename Message, typename ...PayloadArgs>
+    bool send(PayloadArgs... payload)
+    {
+        Message message(payload...);
+        std::lock_guard<std::mutex> stream_guard(mutex);
+        size_t expected = message.size(payload...);
+        size_t written = message.write(*this, payload...);
+        return written == expected;
+    }
+
     size_t write_all(const void *buf, const size_t len);
-    int send_format(unsigned w, unsigned h, uint8_t c);
-    int send_frame(const void *buf, const unsigned size);
-    void send_cursor(uint16_t width, uint16_t height,
-                     uint16_t hotspot_x, uint16_t hotspot_y,
-                     std::function<void(uint32_t *)> fill_cursor);
 
 private:
     int streamfd = -1;
     std::mutex mutex;
 };
 
+
+template <typename Payload, typename Info>
+struct Message
+{
+    template <typename ...PayloadArgs>
+    Message(PayloadArgs... payload)
+        : hdr(StreamDevHeader {
+              .protocol_version = STREAM_DEVICE_PROTOCOL,
+              .padding = 0,     // Workaround GCC bug "sorry: not implemented"
+              .type = Info::type,
+              .size = (uint32_t) (Info::size(payload...) - sizeof(hdr))
+          }),
+          msg(Info::make(payload...))
+    { }
+
+    StreamDevHeader hdr;
+    Payload         msg;
+};
+
+
+struct FormatMessage : Message<StreamMsgFormat, FormatMessage>
+{
+    FormatMessage(unsigned w, unsigned h, uint8_t c) : Message(w, h, c) {}
+    static const StreamMsgType type = STREAM_TYPE_FORMAT;
+    static size_t size(unsigned width, unsigned height, uint8_t codec)
+    {
+        return sizeof(FormatMessage);
+    }
+    static StreamMsgFormat make(unsigned w, unsigned h, uint8_t c)
+    {
+        return StreamMsgFormat{ .width = w, .height = h, .codec = c, .padding1 = {} };
+    }
+    size_t write(Stream &stream, unsigned w, unsigned h, uint8_t c)
+    {
+        return stream.write_all(this, sizeof(*this));
+    }
+};
+
+
+struct FrameMessage : Message<StreamMsgData, FrameMessage>
+{
+    FrameMessage(const void *frame, size_t length) : Message(frame, length) {}
+    static const StreamMsgType type = STREAM_TYPE_DATA;
+    static size_t size(const void *frame, size_t length)
+    {
+        return sizeof(FrameMessage) + length;
+    }
+    static StreamMsgData make(const void *frame, size_t length)
+    {
+        return StreamMsgData();
+    }
+    size_t write(Stream &stream, const void *frame, size_t length)
+    {
+        return stream.write_all(&hdr, sizeof(hdr)) + stream.write_all(frame, length);
+    }
+};
+
+struct X11CursorMessage : Message<StreamMsgCursorSet, X11CursorMessage>
+{
+    X11CursorMessage(XFixesCursorImage *cursor)
+        : Message(cursor),
+          pixels(new uint32_t[pixel_size(cursor)])
+    {
+        fill_pixels(cursor);
+    }
+    static const StreamMsgType type = STREAM_TYPE_CURSOR_SET;
+    static size_t pixel_size(XFixesCursorImage *cursor)
+    {
+        return cursor->width * cursor->height;
+    }
+    static size_t size(XFixesCursorImage *cursor)
+    {
+        return sizeof(X11CursorMessage) + sizeof(uint32_t) * pixel_size(cursor);
+    }
+    static StreamMsgCursorSet make(XFixesCursorImage *cursor)
+    {
+        return StreamMsgCursorSet
+        {
+            .width = cursor->width,
+            .height = cursor->height,
+            .hot_spot_x = cursor->xhot,
+            .hot_spot_y = cursor->yhot,
+            .type = SPICE_CURSOR_TYPE_ALPHA,
+            .padding1 = { },
+            .data = { }
+        };
+    }
+    size_t write(Stream &stream, XFixesCursorImage *cursor)
+    {
+        return stream.write_all(&hdr, sizeof(hdr)) + stream.write_all(pixels.get(), hdr.size);
+    }
+    void fill_pixels(XFixesCursorImage *cursor)
+    {
+        uint32_t *pixbuf = pixels.get();
+        unsigned count = cursor->width * cursor->height;
+        for (unsigned i = 0; i < count; ++i)
+            pixbuf[i] = cursor->pixels[i];
+    }
+private:
+    std::unique_ptr<uint32_t[]> pixels;
+};
+
 }} // namespace spice::streaming_agent
 
 
@@ -201,66 +290,6 @@  size_t Stream::write_all(const void *buf, const size_t len)
     return written;
 }
 
-int Stream::send_format(unsigned w, unsigned h, uint8_t c)
-{
-    const size_t msgsize = sizeof(FormatMessage);
-    const size_t hdrsize  = sizeof(StreamDevHeader);
-    FormatMessage 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(mutex);
-    if (write_all(&msg, msgsize) != msgsize) {
-        return EXIT_FAILURE;
-    }
-    return EXIT_SUCCESS;
-}
-
-int Stream::send_frame(const void *buf, const unsigned size)
-{
-    ssize_t n;
-    const size_t msgsize = sizeof(FormatMessage);
-    DataMessage 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 = {}
-    };
-
-    std::lock_guard<std::mutex> stream_guard(mutex);
-    n = write_all(&msg, msgsize);
-    syslog(LOG_DEBUG,
-           "wrote %ld bytes of header of data msg with frame of size %u bytes\n",
-           n, msg.hdr.size);
-    if (n != msgsize) {
-        syslog(LOG_WARNING, "write_all header: wrote %ld expected %lu\n",
-               n, msgsize);
-        return EXIT_FAILURE;
-    }
-    n = write_all(buf, size);
-    syslog(LOG_DEBUG, "wrote data msg body of size %ld\n", n);
-    if (n != size) {
-        syslog(LOG_WARNING, "write_all header: wrote %ld expected %u\n",
-               n, size);
-        return EXIT_FAILURE;
-    }
-    return EXIT_SUCCESS;
-}
-
 /* returns current time in micro-seconds */
 static uint64_t get_time(void)
 {
@@ -304,47 +333,6 @@  static void usage(const char *progname)
     exit(1);
 }
 
-void
-Stream::send_cursor(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;
-
-    const uint32_t cursor_msgsize =
-        sizeof(CursorMessage) + width * height * sizeof(uint32_t);
-    const uint32_t hdrsize  = sizeof(StreamDevHeader);
-
-    std::unique_ptr<uint8_t[]> storage(new uint8_t[cursor_msgsize]);
-
-    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 = cursor_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->msg.data);
-    fill_cursor(pixels);
-
-    std::lock_guard<std::mutex> stream_guard(mutex);
-    write_all(storage.get(), cursor_msgsize);
-}
-
 static void cursor_changes(Stream *stream, Display *display, int event_base)
 {
     unsigned long last_serial = 0;
@@ -363,12 +351,8 @@  static void cursor_changes(Stream *stream, Display *display, int event_base)
             continue;
 
         last_serial = cursor->cursor_serial;
-        auto fill_cursor = [cursor](uint32_t *pixels) {
-            for (unsigned i = 0; i < cursor->width * cursor->height; ++i)
-                pixels[i] = cursor->pixels[i];
-        };
-        stream->send_cursor(cursor->width, cursor->height,
-                            cursor->xhot, cursor->yhot, fill_cursor);
+        if (!stream->send<X11CursorMessage>(cursor))
+            syslog(LOG_WARNING, "FAILED to send cursor\n");
     }
 }
 
@@ -422,7 +406,7 @@  do_capture(Stream &stream, const char *streamport, FILE *f_log)
 
                 syslog(LOG_DEBUG, "wXh %uX%u  codec=%u\n", width, height, codec);
 
-                if (stream.send_format(width, height, codec) == EXIT_FAILURE)
+                if (!stream.send<FormatMessage>(width, height, codec))
                     throw std::runtime_error("FAILED to send format message");
             }
             if (f_log) {
@@ -434,7 +418,7 @@  do_capture(Stream &stream, const char *streamport, FILE *f_log)
                     hexdump(frame.buffer, frame.buffer_size, f_log);
                 }
             }
-            if (stream.send_frame(frame.buffer, frame.buffer_size) == EXIT_FAILURE) {
+            if (!stream.send<FrameMessage>(frame.buffer, frame.buffer_size)) {
                 syslog(LOG_ERR, "FAILED to send a frame\n");
                 break;
             }

Comments

On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin <dinechin@redhat.com>
> 
> - The Stream class now deals with locking and sending messages
> - The Message<> template class deals with the general writing mechanisms
> - Three classes, FormatMessage, FrameMessage and X11CursorMessage represent individual messages
> 
> The various classes should be moved to separate headers in a subsequent operation
> 
> The design uses templates to avoid any runtime overhead. All the calls are static.

All in all, a nice way to encapsulate the sending of messages. What I
worked on, actually, was the receiving of messages, as that is actually
more important for separating the code (as seen later in the problem
you had with client_codecs and streaming_requested static variables).

I am now wondering how to merge it with your changes and whether the
same Message class hierarchy could be used (without making a mess out
of it). We should discuss it later.

> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> ---
>  src/spice-streaming-agent.cpp | 250 ++++++++++++++++++++----------------------
>  1 file changed, 117 insertions(+), 133 deletions(-)
> 
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index a989ee7..c174ea4 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -48,24 +48,6 @@ namespace spice
>  namespace streaming_agent
>  {
>  
> -struct FormatMessage
> -{
> -    StreamDevHeader hdr;
> -    StreamMsgFormat msg;
> -};
> -
> -struct DataMessage
> -{
> -    StreamDevHeader hdr;
> -    StreamMsgData msg;
> -};
> -
> -struct CursorMessage
> -{
> -    StreamDevHeader hdr;
> -    StreamMsgCursorSet msg;
> -};
> -
>  class Stream
>  {
>  public:
> @@ -79,24 +61,131 @@ public:
>      {
>          close(streamfd);
>      }
> -    operator int() { return streamfd; }
>  
>      int have_something_to_read(int timeout);
>      int read_command_from_device(void);
>      int read_command(bool blocking);
>  
> +
> +    template <typename Message, typename ...PayloadArgs>
> +    bool send(PayloadArgs... payload)
> +    {
> +        Message message(payload...);
> +        std::lock_guard<std::mutex> stream_guard(mutex);
> +        size_t expected = message.size(payload...);
> +        size_t written = message.write(*this, payload...);
> +        return written == expected;
> +    }

Do you purposefully avoid throwing exceptions here, returning a bool?
The error and exception could actually be checked as low as at the end
of the write_all() method, avoiding all the latter size returning and
checking?

> +
>      size_t write_all(const void *buf, const size_t len);
> -    int send_format(unsigned w, unsigned h, uint8_t c);
> -    int send_frame(const void *buf, const unsigned size);
> -    void send_cursor(uint16_t width, uint16_t height,
> -                     uint16_t hotspot_x, uint16_t hotspot_y,
> -                     std::function<void(uint32_t *)> fill_cursor);
>  
>  private:
>      int streamfd = -1;
>      std::mutex mutex;
>  };
>  
> +
> +template <typename Payload, typename Info>

"Info" is quite an unimaginative name :) I understand it's the message
itself here and also find it curiously hard to come up with something
better :) "TheMessage" not very good, is it? Still better than "Info"?
Maybe a comment referencing the CRTP to help readers understand?

> +struct Message

Any reason to not stick with the "class" keyword instead of "struct"?

> +{
> +    template <typename ...PayloadArgs>
> +    Message(PayloadArgs... payload)

"PayloadArgs... payload_args", we have something called "Payload" here
too, getting a bit confusing.

> +        : hdr(StreamDevHeader {
> +              .protocol_version = STREAM_DEVICE_PROTOCOL,
> +              .padding = 0,     // Workaround GCC bug "sorry: not implemented"
> +              .type = Info::type,
> +              .size = (uint32_t) (Info::size(payload...) - sizeof(hdr))
> +          }),
> +          msg(Info::make(payload...))
> +    { }

I find it redundant that you pass the "payload..." (the args :)) to
Info::size() and Info::make() here and then also to Info::write() in
Stream::send(). In the three messages below, you also showcase three
distinct ways of serializing them, caused by this redundancy (I
understand it is partially given by the payload of the messages).

See comments under each class, which all relate to this (hope it's not
too confusing).

> +
> +    StreamDevHeader hdr;
> +    Payload         msg;
> +};
> +
> +
> +struct FormatMessage : Message<StreamMsgFormat, FormatMessage>
> +{
> +    FormatMessage(unsigned w, unsigned h, uint8_t c) : Message(w, h, c) {}
> +    static const StreamMsgType type = STREAM_TYPE_FORMAT;

Could the type actually be part of the template? As in:
struct FormatMessage : Message<STREAM_TYPE_FORMAT, StreamMsgFormat, FormatMessage>

> +    static size_t size(unsigned width, unsigned height, uint8_t codec)
> +    {
> +        return sizeof(FormatMessage);
> +    }
> +    static StreamMsgFormat make(unsigned w, unsigned h, uint8_t c)
> +    {
> +        return StreamMsgFormat{ .width = w, .height = h, .codec = c, .padding1 = {} };
> +    }
> +    size_t write(Stream &stream, unsigned w, unsigned h, uint8_t c)
> +    {
> +        return stream.write_all(this, sizeof(*this));
> +    }
> +};

This message has static size, so you construct it in make() and then
write this whole object.

> +
> +
> +struct FrameMessage : Message<StreamMsgData, FrameMessage>
> +{
> +    FrameMessage(const void *frame, size_t length) : Message(frame, length) {}
> +    static const StreamMsgType type = STREAM_TYPE_DATA;
> +    static size_t size(const void *frame, size_t length)
> +    {
> +        return sizeof(FrameMessage) + length;
> +    }
> +    static StreamMsgData make(const void *frame, size_t length)
> +    {
> +        return StreamMsgData();
> +    }
> +    size_t write(Stream &stream, const void *frame, size_t length)
> +    {
> +        return stream.write_all(&hdr, sizeof(hdr)) + stream.write_all(frame, length);
> +    }
> +};

Here the message is actually only the frame data and so you construct
an empty message in make(). In write() you just write the stream data
passed to it.

> +
> +struct X11CursorMessage : Message<StreamMsgCursorSet, X11CursorMessage>
> +{
> +    X11CursorMessage(XFixesCursorImage *cursor)
> +        : Message(cursor),
> +          pixels(new uint32_t[pixel_size(cursor)])
> +    {
> +        fill_pixels(cursor);
> +    }
> +    static const StreamMsgType type = STREAM_TYPE_CURSOR_SET;
> +    static size_t pixel_size(XFixesCursorImage *cursor)
> +    {
> +        return cursor->width * cursor->height;
> +    }
> +    static size_t size(XFixesCursorImage *cursor)
> +    {
> +        return sizeof(X11CursorMessage) + sizeof(uint32_t) * pixel_size(cursor);
> +    }
> +    static StreamMsgCursorSet make(XFixesCursorImage *cursor)
> +    {
> +        return StreamMsgCursorSet
> +        {
> +            .width = cursor->width,
> +            .height = cursor->height,
> +            .hot_spot_x = cursor->xhot,
> +            .hot_spot_y = cursor->yhot,
> +            .type = SPICE_CURSOR_TYPE_ALPHA,
> +            .padding1 = { },
> +            .data = { }
> +        };
> +    }
> +    size_t write(Stream &stream, XFixesCursorImage *cursor)
> +    {
> +        return stream.write_all(&hdr, sizeof(hdr)) + stream.write_all(pixels.get(), hdr.size);

You are not writing the msg data here, might be the reson for the
missing cursor.

> +    }
> +    void fill_pixels(XFixesCursorImage *cursor)
> +    {
> +        uint32_t *pixbuf = pixels.get();
> +        unsigned count = cursor->width * cursor->height;
> +        for (unsigned i = 0; i < count; ++i)
> +            pixbuf[i] = cursor->pixels[i];
> +    }
> +private:
> +    std::unique_ptr<uint32_t[]> pixels;
> +};

(note in this class some methods could be private and some arguments
const)

Here you add a private member pixels, which you fill in constructor. In
make(), you create the static part of the message. In write(), you
write them.

So, I ask, could you not actually construct all the data to write in
write(), and perhaps even remove the Message::msg member and the make()
method?

That would make the classes a bit simpler?

Cheers,
Lukas

> +
>  }} // namespace spice::streaming_agent
>  
>  
> @@ -201,66 +290,6 @@ size_t Stream::write_all(const void *buf, const size_t len)
>      return written;
>  }
>  
> -int Stream::send_format(unsigned w, unsigned h, uint8_t c)
> -{
> -    const size_t msgsize = sizeof(FormatMessage);
> -    const size_t hdrsize  = sizeof(StreamDevHeader);
> -    FormatMessage 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(mutex);
> -    if (write_all(&msg, msgsize) != msgsize) {
> -        return EXIT_FAILURE;
> -    }
> -    return EXIT_SUCCESS;
> -}
> -
> -int Stream::send_frame(const void *buf, const unsigned size)
> -{
> -    ssize_t n;
> -    const size_t msgsize = sizeof(FormatMessage);
> -    DataMessage 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 = {}
> -    };
> -
> -    std::lock_guard<std::mutex> stream_guard(mutex);
> -    n = write_all(&msg, msgsize);
> -    syslog(LOG_DEBUG,
> -           "wrote %ld bytes of header of data msg with frame of size %u bytes\n",
> -           n, msg.hdr.size);
> -    if (n != msgsize) {
> -        syslog(LOG_WARNING, "write_all header: wrote %ld expected %lu\n",
> -               n, msgsize);
> -        return EXIT_FAILURE;
> -    }
> -    n = write_all(buf, size);
> -    syslog(LOG_DEBUG, "wrote data msg body of size %ld\n", n);
> -    if (n != size) {
> -        syslog(LOG_WARNING, "write_all header: wrote %ld expected %u\n",
> -               n, size);
> -        return EXIT_FAILURE;
> -    }
> -    return EXIT_SUCCESS;
> -}
> -
>  /* returns current time in micro-seconds */
>  static uint64_t get_time(void)
>  {
> @@ -304,47 +333,6 @@ static void usage(const char *progname)
>      exit(1);
>  }
>  
> -void
> -Stream::send_cursor(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;
> -
> -    const uint32_t cursor_msgsize =
> -        sizeof(CursorMessage) + width * height * sizeof(uint32_t);
> -    const uint32_t hdrsize  = sizeof(StreamDevHeader);
> -
> -    std::unique_ptr<uint8_t[]> storage(new uint8_t[cursor_msgsize]);
> -
> -    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 = cursor_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->msg.data);
> -    fill_cursor(pixels);
> -
> -    std::lock_guard<std::mutex> stream_guard(mutex);
> -    write_all(storage.get(), cursor_msgsize);
> -}
> -
>  static void cursor_changes(Stream *stream, Display *display, int event_base)
>  {
>      unsigned long last_serial = 0;
> @@ -363,12 +351,8 @@ static void cursor_changes(Stream *stream, Display *display, int event_base)
>              continue;
>  
>          last_serial = cursor->cursor_serial;
> -        auto fill_cursor = [cursor](uint32_t *pixels) {
> -            for (unsigned i = 0; i < cursor->width * cursor->height; ++i)
> -                pixels[i] = cursor->pixels[i];
> -        };
> -        stream->send_cursor(cursor->width, cursor->height,
> -                            cursor->xhot, cursor->yhot, fill_cursor);
> +        if (!stream->send<X11CursorMessage>(cursor))
> +            syslog(LOG_WARNING, "FAILED to send cursor\n");
>      }
>  }
>  
> @@ -422,7 +406,7 @@ do_capture(Stream &stream, const char *streamport, FILE *f_log)
>  
>                  syslog(LOG_DEBUG, "wXh %uX%u  codec=%u\n", width, height, codec);
>  
> -                if (stream.send_format(width, height, codec) == EXIT_FAILURE)
> +                if (!stream.send<FormatMessage>(width, height, codec))
>                      throw std::runtime_error("FAILED to send format message");
>              }
>              if (f_log) {
> @@ -434,7 +418,7 @@ do_capture(Stream &stream, const char *streamport, FILE *f_log)
>                      hexdump(frame.buffer, frame.buffer_size, f_log);
>                  }
>              }
> -            if (stream.send_frame(frame.buffer, frame.buffer_size) == EXIT_FAILURE) {
> +            if (!stream.send<FrameMessage>(frame.buffer, frame.buffer_size)) {
>                  syslog(LOG_ERR, "FAILED to send a frame\n");
>                  break;
>              }
> On 20 Feb 2018, at 15:29, Lukáš Hrázký <lhrazky@redhat.com> wrote:
> 
> On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
>> From: Christophe de Dinechin <dinechin@redhat.com>
>> 
>> - The Stream class now deals with locking and sending messages
>> - The Message<> template class deals with the general writing mechanisms
>> - Three classes, FormatMessage, FrameMessage and X11CursorMessage represent individual messages
>> 
>> The various classes should be moved to separate headers in a subsequent operation
>> 
>> The design uses templates to avoid any runtime overhead. All the calls are static.
> 
> All in all, a nice way to encapsulate the sending of messages. What I
> worked on, actually, was the receiving of messages, as that is actually
> more important for separating the code (as seen later in the problem
> you had with client_codecs and streaming_requested static variables).
> 
> I am now wondering how to merge it with your changes and whether the
> same Message class hierarchy could be used (without making a mess out
> of it). We should discuss it later.

Do you have your WIP stuff in a private branch I could look at? Maybe I can help with the rebasing.

I would probably keep input and output messages in separate classes. I can’t see much commonality between the two. Using the same CRTP for input messages, and maybe rename Message as OutputMessage…

> 
>> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
>> ---
>> src/spice-streaming-agent.cpp | 250 ++++++++++++++++++++----------------------
>> 1 file changed, 117 insertions(+), 133 deletions(-)
>> 
>> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
>> index a989ee7..c174ea4 100644
>> --- a/src/spice-streaming-agent.cpp
>> +++ b/src/spice-streaming-agent.cpp
>> @@ -48,24 +48,6 @@ namespace spice
>> namespace streaming_agent
>> {
>> 
>> -struct FormatMessage
>> -{
>> -    StreamDevHeader hdr;
>> -    StreamMsgFormat msg;
>> -};
>> -
>> -struct DataMessage
>> -{
>> -    StreamDevHeader hdr;
>> -    StreamMsgData msg;
>> -};
>> -
>> -struct CursorMessage
>> -{
>> -    StreamDevHeader hdr;
>> -    StreamMsgCursorSet msg;
>> -};
>> -
>> class Stream
>> {
>> public:
>> @@ -79,24 +61,131 @@ public:
>>     {
>>         close(streamfd);
>>     }
>> -    operator int() { return streamfd; }
>> 
>>     int have_something_to_read(int timeout);
>>     int read_command_from_device(void);
>>     int read_command(bool blocking);
>> 
>> +
>> +    template <typename Message, typename ...PayloadArgs>
>> +    bool send(PayloadArgs... payload)
>> +    {
>> +        Message message(payload...);
>> +        std::lock_guard<std::mutex> stream_guard(mutex);
>> +        size_t expected = message.size(payload...);
>> +        size_t written = message.write(*this, payload...);
>> +        return written == expected;
>> +    }
> 
> Do you purposefully avoid throwing exceptions here, returning a bool?

You really love exceptions, don’t you ;-)

I usually reserve exceptions for cases which are truly “catastrophic”, i.e. going the long way and unwindig is the right way to do it. Unwinding the stack is a very messy business, takes a lot of time, and if you can just return, it’s about one and a half gazillion times faster, generates smaller code, etc etc.

In this specific case, though, I think that unwinding could be seen as appropriate, since ATM we have nothing better than error + abort when it happens. Plus this might avoid a scenario where you could write twice if the first write fails. So I’m sold, I think this is the right thing to do.

> The error and exception could actually be checked as low as at the end
> of the write_all() method, avoiding all the latter size returning and
> checking?
> 
>> +
>>     size_t write_all(const void *buf, const size_t len);
>> -    int send_format(unsigned w, unsigned h, uint8_t c);
>> -    int send_frame(const void *buf, const unsigned size);
>> -    void send_cursor(uint16_t width, uint16_t height,
>> -                     uint16_t hotspot_x, uint16_t hotspot_y,
>> -                     std::function<void(uint32_t *)> fill_cursor);
>> 
>> private:
>>     int streamfd = -1;
>>     std::mutex mutex;
>> };
>> 
>> +
>> +template <typename Payload, typename Info>
> 
> "Info" is quite an unimaginative name :) I understand it's the message
> itself here and also find it curiously hard to come up with something
> better :) "TheMessage" not very good, is it? Still better than "Info"?
> Maybe a comment referencing the CRTP to help readers understand?

I used ‘Info’ here because that’s the information about the message.

I can add the CRTP comment, could not remember the acronym when I wrote the class ;-)

> 
>> +struct Message
> 
> Any reason to not stick with the "class" keyword instead of "struct”?

Not really, more a matter of habit, using struct when it’s really about data and everything is public. But there, “Message” evolved far away from POD to warrant using ‘class’.

> 
>> +{
>> +    template <typename ...PayloadArgs>
>> +    Message(PayloadArgs... payload)
> 
> "PayloadArgs... payload_args", we have something called "Payload" here
> too, getting a bit confusing.

But that’s the same thing. Payload is initialized with PayloadArgs, so it makes sense.

Initially, I had “PayloadConstructorArgs”, but then I started using it outside of the ctor.

> 
>> +        : hdr(StreamDevHeader {
>> +              .protocol_version = STREAM_DEVICE_PROTOCOL,
>> +              .padding = 0,     // Workaround GCC bug "sorry: not implemented"
>> +              .type = Info::type,
>> +              .size = (uint32_t) (Info::size(payload...) - sizeof(hdr))
>> +          }),
>> +          msg(Info::make(payload...))
>> +    { }
> 
> I find it redundant that you pass the "payload..." (the args :)) to
> Info::size() and Info::make() here and then also to Info::write() in
> Stream::send().

I’m not sure “redundant” is the correct word. I could stash the information in Message, but then the compiler would have a much harder time inlining the actual expression, which is always very simple. Unfortunately, we have three different expressions that take different input, hence the CRTP and the passing of arguments.


> In the three messages below, you also showcase three
> distinct ways of serializing them, caused by this redundancy (I
> understand it is partially given by the payload of the messages).

It is *entirely* given by the payload, which I assume is a given, since it’s in the protocol. What else could come into play?

> 
> See comments under each class, which all relate to this (hope it's not
> too confusing).
> 
>> +
>> +    StreamDevHeader hdr;
>> +    Payload         msg;
>> +};
>> +
>> +
>> +struct FormatMessage : Message<StreamMsgFormat, FormatMessage>
>> +{
>> +    FormatMessage(unsigned w, unsigned h, uint8_t c) : Message(w, h, c) {}
>> +    static const StreamMsgType type = STREAM_TYPE_FORMAT;
> 
> Could the type actually be part of the template? As in:
> struct FormatMessage : Message<STREAM_TYPE_FORMAT, StreamMsgFormat, FormatMessage>

Sure, but I find it more consistent the way I wrote it. Also easier to read, because you have “type = TYPE” instead of just <TYPE>, but that’s really a personal preference.

> 
>> +    static size_t size(unsigned width, unsigned height, uint8_t codec)
>> +    {
>> +        return sizeof(FormatMessage);
>> +    }
>> +    static StreamMsgFormat make(unsigned w, unsigned h, uint8_t c)
>> +    {
>> +        return StreamMsgFormat{ .width = w, .height = h, .codec = c, .padding1 = {} };
>> +    }
>> +    size_t write(Stream &stream, unsigned w, unsigned h, uint8_t c)
>> +    {
>> +        return stream.write_all(this, sizeof(*this));
>> +    }
>> +};
> 
> This message has static size, so you construct it in make() and then
> write this whole object.

The message body has static size, yes. (Header is fixed size in all three cases).

> 
>> +
>> +
>> +struct FrameMessage : Message<StreamMsgData, FrameMessage>
>> +{
>> +    FrameMessage(const void *frame, size_t length) : Message(frame, length) {}
>> +    static const StreamMsgType type = STREAM_TYPE_DATA;
>> +    static size_t size(const void *frame, size_t length)
>> +    {
>> +        return sizeof(FrameMessage) + length;
>> +    }
>> +    static StreamMsgData make(const void *frame, size_t length)
>> +    {
>> +        return StreamMsgData();
>> +    }
>> +    size_t write(Stream &stream, const void *frame, size_t length)
>> +    {
>> +        return stream.write_all(&hdr, sizeof(hdr)) + stream.write_all(frame, length);
>> +    }
>> +};
> 
> Here the message is actually only the frame data and so you construct
> an empty message in make(). In write() you just write the stream data
> passed to it.

Yes.

> 
>> +
>> +struct X11CursorMessage : Message<StreamMsgCursorSet, X11CursorMessage>
>> +{
>> +    X11CursorMessage(XFixesCursorImage *cursor)
>> +        : Message(cursor),
>> +          pixels(new uint32_t[pixel_size(cursor)])
>> +    {
>> +        fill_pixels(cursor);
>> +    }
>> +    static const StreamMsgType type = STREAM_TYPE_CURSOR_SET;
>> +    static size_t pixel_size(XFixesCursorImage *cursor)
>> +    {
>> +        return cursor->width * cursor->height;
>> +    }
>> +    static size_t size(XFixesCursorImage *cursor)
>> +    {
>> +        return sizeof(X11CursorMessage) + sizeof(uint32_t) * pixel_size(cursor);
>> +    }
>> +    static StreamMsgCursorSet make(XFixesCursorImage *cursor)
>> +    {
>> +        return StreamMsgCursorSet
>> +        {
>> +            .width = cursor->width,
>> +            .height = cursor->height,
>> +            .hot_spot_x = cursor->xhot,
>> +            .hot_spot_y = cursor->yhot,
>> +            .type = SPICE_CURSOR_TYPE_ALPHA,
>> +            .padding1 = { },
>> +            .data = { }
>> +        };
>> +    }
>> +    size_t write(Stream &stream, XFixesCursorImage *cursor)
>> +    {
>> +        return stream.write_all(&hdr, sizeof(hdr)) + stream.write_all(pixels.get(), hdr.size);
> 
> You are not writing the msg data here, might be the reson for the
> missing cursor.

Ah, good catch. I thought of not writing *this because of the extra pixels field, but then wrote the wrong class.

> 
>> +    }
>> +    void fill_pixels(XFixesCursorImage *cursor)
>> +    {
>> +        uint32_t *pixbuf = pixels.get();
>> +        unsigned count = cursor->width * cursor->height;
>> +        for (unsigned i = 0; i < count; ++i)
>> +            pixbuf[i] = cursor->pixels[i];
>> +    }
>> +private:
>> +    std::unique_ptr<uint32_t[]> pixels;
>> +};
> 
> (note in this class some methods could be private and some arguments
> const)

Yes. Good idea.

> 
> Here you add a private member pixels, which you fill in constructor. In
> make(), you create the static part of the message. In write(), you
> write them.
> 
> So, I ask, could you not actually construct all the data to write in
> write(), and perhaps even remove the Message::msg member and the make()
> method?

Yes, but that would require to copy the frames data, which I considered an expensive-enough operation to try and avoid it. For cursor pixels, it’s less of an issue a) because we need to copy anyway, due to format conversion, and they happen much less frequently.

> 
> That would make the classes a bit simpler?

Simpler, yes but much less efficient in the important case, which is sending frames, where it would add one malloc / copy / free for each frame, which the present code avoids at the “mere” cost of passing the payload args around ;-)


Thanks a lot.

> 
> Cheers,
> Lukas
> 
>> +
>> }} // namespace spice::streaming_agent
>> 
>> 
>> @@ -201,66 +290,6 @@ size_t Stream::write_all(const void *buf, const size_t len)
>>     return written;
>> }
>> 
>> -int Stream::send_format(unsigned w, unsigned h, uint8_t c)
>> -{
>> -    const size_t msgsize = sizeof(FormatMessage);
>> -    const size_t hdrsize  = sizeof(StreamDevHeader);
>> -    FormatMessage 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(mutex);
>> -    if (write_all(&msg, msgsize) != msgsize) {
>> -        return EXIT_FAILURE;
>> -    }
>> -    return EXIT_SUCCESS;
>> -}
>> -
>> -int Stream::send_frame(const void *buf, const unsigned size)
>> -{
>> -    ssize_t n;
>> -    const size_t msgsize = sizeof(FormatMessage);
>> -    DataMessage 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 = {}
>> -    };
>> -
>> -    std::lock_guard<std::mutex> stream_guard(mutex);
>> -    n = write_all(&msg, msgsize);
>> -    syslog(LOG_DEBUG,
>> -           "wrote %ld bytes of header of data msg with frame of size %u bytes\n",
>> -           n, msg.hdr.size);
>> -    if (n != msgsize) {
>> -        syslog(LOG_WARNING, "write_all header: wrote %ld expected %lu\n",
>> -               n, msgsize);
>> -        return EXIT_FAILURE;
>> -    }
>> -    n = write_all(buf, size);
>> -    syslog(LOG_DEBUG, "wrote data msg body of size %ld\n", n);
>> -    if (n != size) {
>> -        syslog(LOG_WARNING, "write_all header: wrote %ld expected %u\n",
>> -               n, size);
>> -        return EXIT_FAILURE;
>> -    }
>> -    return EXIT_SUCCESS;
>> -}
>> -
>> /* returns current time in micro-seconds */
>> static uint64_t get_time(void)
>> {
>> @@ -304,47 +333,6 @@ static void usage(const char *progname)
>>     exit(1);
>> }
>> 
>> -void
>> -Stream::send_cursor(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;
>> -
>> -    const uint32_t cursor_msgsize =
>> -        sizeof(CursorMessage) + width * height * sizeof(uint32_t);
>> -    const uint32_t hdrsize  = sizeof(StreamDevHeader);
>> -
>> -    std::unique_ptr<uint8_t[]> storage(new uint8_t[cursor_msgsize]);
>> -
>> -    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 = cursor_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->msg.data);
>> -    fill_cursor(pixels);
>> -
>> -    std::lock_guard<std::mutex> stream_guard(mutex);
>> -    write_all(storage.get(), cursor_msgsize);
>> -}
>> -
>> static void cursor_changes(Stream *stream, Display *display, int event_base)
>> {
>>     unsigned long last_serial = 0;
>> @@ -363,12 +351,8 @@ static void cursor_changes(Stream *stream, Display *display, int event_base)
>>             continue;
>> 
>>         last_serial = cursor->cursor_serial;
>> -        auto fill_cursor = [cursor](uint32_t *pixels) {
>> -            for (unsigned i = 0; i < cursor->width * cursor->height; ++i)
>> -                pixels[i] = cursor->pixels[i];
>> -        };
>> -        stream->send_cursor(cursor->width, cursor->height,
>> -                            cursor->xhot, cursor->yhot, fill_cursor);
>> +        if (!stream->send<X11CursorMessage>(cursor))
>> +            syslog(LOG_WARNING, "FAILED to send cursor\n");
>>     }
>> }
>> 
>> @@ -422,7 +406,7 @@ do_capture(Stream &stream, const char *streamport, FILE *f_log)
>> 
>>                 syslog(LOG_DEBUG, "wXh %uX%u  codec=%u\n", width, height, codec);
>> 
>> -                if (stream.send_format(width, height, codec) == EXIT_FAILURE)
>> +                if (!stream.send<FormatMessage>(width, height, codec))
>>                     throw std::runtime_error("FAILED to send format message");
>>             }
>>             if (f_log) {
>> @@ -434,7 +418,7 @@ do_capture(Stream &stream, const char *streamport, FILE *f_log)
>>                     hexdump(frame.buffer, frame.buffer_size, f_log);
>>                 }
>>             }
>> -            if (stream.send_frame(frame.buffer, frame.buffer_size) == EXIT_FAILURE) {
>> +            if (!stream.send<FrameMessage>(frame.buffer, frame.buffer_size)) {
>>                 syslog(LOG_ERR, "FAILED to send a frame\n");
>>                 break;
>>             }
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
> From: Christophe de Dinechin <dinechin@redhat.com>
> 
> - The Stream class now deals with locking and sending messages
> - The Message<> template class deals with the general writing mechanisms
> - Three classes, FormatMessage, FrameMessage and X11CursorMessage represent
> individual messages
> 
> The various classes should be moved to separate headers in a subsequent
> operation
> 
> The design uses templates to avoid any runtime overhead. All the calls are
> static.
> 
> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> ---
>  src/spice-streaming-agent.cpp | 250
>  ++++++++++++++++++++----------------------
>  1 file changed, 117 insertions(+), 133 deletions(-)
> 
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index a989ee7..c174ea4 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -48,24 +48,6 @@ namespace spice
>  namespace streaming_agent
>  {
>  
> -struct FormatMessage
> -{
> -    StreamDevHeader hdr;
> -    StreamMsgFormat msg;
> -};
> -
> -struct DataMessage
> -{
> -    StreamDevHeader hdr;
> -    StreamMsgData msg;
> -};
> -
> -struct CursorMessage
> -{
> -    StreamDevHeader hdr;
> -    StreamMsgCursorSet msg;
> -};
> -
>  class Stream
>  {
>  public:
> @@ -79,24 +61,131 @@ public:
>      {
>          close(streamfd);
>      }
> -    operator int() { return streamfd; }
>  
>      int have_something_to_read(int timeout);
>      int read_command_from_device(void);
>      int read_command(bool blocking);
>  
> +
> +    template <typename Message, typename ...PayloadArgs>
> +    bool send(PayloadArgs... payload)
> +    {
> +        Message message(payload...);
> +        std::lock_guard<std::mutex> stream_guard(mutex);
> +        size_t expected = message.size(payload...);
> +        size_t written = message.write(*this, payload...);
> +        return written == expected;
> +    }
> +
>      size_t write_all(const void *buf, const size_t len);
> -    int send_format(unsigned w, unsigned h, uint8_t c);
> -    int send_frame(const void *buf, const unsigned size);
> -    void send_cursor(uint16_t width, uint16_t height,
> -                     uint16_t hotspot_x, uint16_t hotspot_y,
> -                     std::function<void(uint32_t *)> fill_cursor);
>  
>  private:
>      int streamfd = -1;
>      std::mutex mutex;
>  };
>  
> +
> +template <typename Payload, typename Info>
> +struct Message
> +{
> +    template <typename ...PayloadArgs>
> +    Message(PayloadArgs... payload)
> +        : hdr(StreamDevHeader {
> +              .protocol_version = STREAM_DEVICE_PROTOCOL,
> +              .padding = 0,     // Workaround GCC bug "sorry: not
> implemented"
> +              .type = Info::type,
> +              .size = (uint32_t) (Info::size(payload...) - sizeof(hdr))
> +          }),
> +          msg(Info::make(payload...))
> +    { }
> +
> +    StreamDevHeader hdr;
> +    Payload         msg;

style, no column indentation.

> +};
> +
> +
> +struct FormatMessage : Message<StreamMsgFormat, FormatMessage>
> +{
> +    FormatMessage(unsigned w, unsigned h, uint8_t c) : Message(w, h, c) {}
> +    static const StreamMsgType type = STREAM_TYPE_FORMAT;
> +    static size_t size(unsigned width, unsigned height, uint8_t codec)
> +    {
> +        return sizeof(FormatMessage);
> +    }
> +    static StreamMsgFormat make(unsigned w, unsigned h, uint8_t c)
> +    {
> +        return StreamMsgFormat{ .width = w, .height = h, .codec = c,
> .padding1 = {} };
> +    }
> +    size_t write(Stream &stream, unsigned w, unsigned h, uint8_t c)
> +    {
> +        return stream.write_all(this, sizeof(*this));
> +    }
> +};
> +
> +
> +struct FrameMessage : Message<StreamMsgData, FrameMessage>
> +{
> +    FrameMessage(const void *frame, size_t length) : Message(frame, length)
> {}
> +    static const StreamMsgType type = STREAM_TYPE_DATA;
> +    static size_t size(const void *frame, size_t length)
> +    {
> +        return sizeof(FrameMessage) + length;
> +    }
> +    static StreamMsgData make(const void *frame, size_t length)
> +    {
> +        return StreamMsgData();
> +    }
> +    size_t write(Stream &stream, const void *frame, size_t length)
> +    {
> +        return stream.write_all(&hdr, sizeof(hdr)) + stream.write_all(frame,
> length);
> +    }
> +};
> +
> +struct X11CursorMessage : Message<StreamMsgCursorSet, X11CursorMessage>
> +{
> +    X11CursorMessage(XFixesCursorImage *cursor)
> +        : Message(cursor),
> +          pixels(new uint32_t[pixel_size(cursor)])
> +    {
> +        fill_pixels(cursor);
> +    }
> +    static const StreamMsgType type = STREAM_TYPE_CURSOR_SET;
> +    static size_t pixel_size(XFixesCursorImage *cursor)
> +    {
> +        return cursor->width * cursor->height;
> +    }
> +    static size_t size(XFixesCursorImage *cursor)
> +    {
> +        return sizeof(X11CursorMessage) + sizeof(uint32_t) *
> pixel_size(cursor);

I think this is wrong as contains also the size of "pixels"
smart pointer.
Maybe you mean sizeof(Message<StreamMsgCursorSet, X11CursorMessage>) ?

> +    }
> +    static StreamMsgCursorSet make(XFixesCursorImage *cursor)
> +    {
> +        return StreamMsgCursorSet
> +        {
> +            .width = cursor->width,
> +            .height = cursor->height,
> +            .hot_spot_x = cursor->xhot,
> +            .hot_spot_y = cursor->yhot,
> +            .type = SPICE_CURSOR_TYPE_ALPHA,
> +            .padding1 = { },
> +            .data = { }
> +        };
> +    }
> +    size_t write(Stream &stream, XFixesCursorImage *cursor)
> +    {
> +        return stream.write_all(&hdr, sizeof(hdr)) +
> stream.write_all(pixels.get(), hdr.size);
> +    }
> +    void fill_pixels(XFixesCursorImage *cursor)
> +    {
> +        uint32_t *pixbuf = pixels.get();
> +        unsigned count = cursor->width * cursor->height;

you can reuse pixel_size here.

> +        for (unsigned i = 0; i < count; ++i)
> +            pixbuf[i] = cursor->pixels[i];

I know this is copied, would be best to fix and add brackets.

> +    }
> +private:
> +    std::unique_ptr<uint32_t[]> pixels;
> +};
> +
>  }} // namespace spice::streaming_agent
>  
>  
> @@ -201,66 +290,6 @@ size_t Stream::write_all(const void *buf, const size_t
> len)
>      return written;
>  }
>  
> -int Stream::send_format(unsigned w, unsigned h, uint8_t c)
> -{
> -    const size_t msgsize = sizeof(FormatMessage);
> -    const size_t hdrsize  = sizeof(StreamDevHeader);
> -    FormatMessage 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(mutex);
> -    if (write_all(&msg, msgsize) != msgsize) {
> -        return EXIT_FAILURE;
> -    }
> -    return EXIT_SUCCESS;
> -}
> -
> -int Stream::send_frame(const void *buf, const unsigned size)
> -{
> -    ssize_t n;
> -    const size_t msgsize = sizeof(FormatMessage);
> -    DataMessage 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 = {}
> -    };
> -
> -    std::lock_guard<std::mutex> stream_guard(mutex);
> -    n = write_all(&msg, msgsize);
> -    syslog(LOG_DEBUG,
> -           "wrote %ld bytes of header of data msg with frame of size %u
> bytes\n",
> -           n, msg.hdr.size);
> -    if (n != msgsize) {
> -        syslog(LOG_WARNING, "write_all header: wrote %ld expected %lu\n",
> -               n, msgsize);
> -        return EXIT_FAILURE;
> -    }
> -    n = write_all(buf, size);
> -    syslog(LOG_DEBUG, "wrote data msg body of size %ld\n", n);
> -    if (n != size) {
> -        syslog(LOG_WARNING, "write_all header: wrote %ld expected %u\n",
> -               n, size);
> -        return EXIT_FAILURE;
> -    }
> -    return EXIT_SUCCESS;
> -}
> -
>  /* returns current time in micro-seconds */
>  static uint64_t get_time(void)
>  {
> @@ -304,47 +333,6 @@ static void usage(const char *progname)
>      exit(1);
>  }
>  
> -void
> -Stream::send_cursor(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;
> -
> -    const uint32_t cursor_msgsize =
> -        sizeof(CursorMessage) + width * height * sizeof(uint32_t);
> -    const uint32_t hdrsize  = sizeof(StreamDevHeader);
> -
> -    std::unique_ptr<uint8_t[]> storage(new uint8_t[cursor_msgsize]);
> -
> -    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 = cursor_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->msg.data);
> -    fill_cursor(pixels);
> -
> -    std::lock_guard<std::mutex> stream_guard(mutex);
> -    write_all(storage.get(), cursor_msgsize);
> -}
> -
>  static void cursor_changes(Stream *stream, Display *display, int event_base)
>  {
>      unsigned long last_serial = 0;
> @@ -363,12 +351,8 @@ static void cursor_changes(Stream *stream, Display
> *display, int event_base)
>              continue;
>  
>          last_serial = cursor->cursor_serial;
> -        auto fill_cursor = [cursor](uint32_t *pixels) {
> -            for (unsigned i = 0; i < cursor->width * cursor->height; ++i)
> -                pixels[i] = cursor->pixels[i];
> -        };
> -        stream->send_cursor(cursor->width, cursor->height,
> -                            cursor->xhot, cursor->yhot, fill_cursor);
> +        if (!stream->send<X11CursorMessage>(cursor))
> +            syslog(LOG_WARNING, "FAILED to send cursor\n");

brackets.

>      }
>  }
>  
> @@ -422,7 +406,7 @@ do_capture(Stream &stream, const char *streamport, FILE
> *f_log)
>  
>                  syslog(LOG_DEBUG, "wXh %uX%u  codec=%u\n", width, height,
>                  codec);
>  
> -                if (stream.send_format(width, height, codec) ==
> EXIT_FAILURE)
> +                if (!stream.send<FormatMessage>(width, height, codec))
>                      throw std::runtime_error("FAILED to send format
>                      message");
>              }
>              if (f_log) {
> @@ -434,7 +418,7 @@ do_capture(Stream &stream, const char *streamport, FILE
> *f_log)
>                      hexdump(frame.buffer, frame.buffer_size, f_log);
>                  }
>              }
> -            if (stream.send_frame(frame.buffer, frame.buffer_size) ==
> EXIT_FAILURE) {
> +            if (!stream.send<FrameMessage>(frame.buffer, frame.buffer_size))
> {
>                  syslog(LOG_ERR, "FAILED to send a frame\n");
>                  break;
>              }

Frediano
On Tue, 2018-02-20 at 16:41 +0100, Christophe de Dinechin wrote:
> > On 20 Feb 2018, at 15:29, Lukáš Hrázký <lhrazky@redhat.com> wrote:
> > 
> > On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
> > > From: Christophe de Dinechin <dinechin@redhat.com>
> > > 
> > > - The Stream class now deals with locking and sending messages
> > > - The Message<> template class deals with the general writing mechanisms
> > > - Three classes, FormatMessage, FrameMessage and X11CursorMessage represent individual messages
> > > 
> > > The various classes should be moved to separate headers in a subsequent operation
> > > 
> > > The design uses templates to avoid any runtime overhead. All the calls are static.
> > 
> > All in all, a nice way to encapsulate the sending of messages. What I
> > worked on, actually, was the receiving of messages, as that is actually
> > more important for separating the code (as seen later in the problem
> > you had with client_codecs and streaming_requested static variables).
> > 
> > I am now wondering how to merge it with your changes and whether the
> > same Message class hierarchy could be used (without making a mess out
> > of it). We should discuss it later.
> 
> Do you have your WIP stuff in a private branch I could look at? Maybe I can help with the rebasing.

I'll push it somewhere so you can have a look, but I can manage the
rebase myself :) It would still be an effort to find out what you did
during the rebase.

> I would probably keep input and output messages in separate classes. I can’t see much commonality between the two. Using the same CRTP for input messages, and maybe rename Message as OutputMessage…

Agreed, probably...

> > 
> > > Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> > > ---
> > > src/spice-streaming-agent.cpp | 250 ++++++++++++++++++++----------------------
> > > 1 file changed, 117 insertions(+), 133 deletions(-)
> > > 
> > > diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> > > index a989ee7..c174ea4 100644
> > > --- a/src/spice-streaming-agent.cpp
> > > +++ b/src/spice-streaming-agent.cpp
> > > @@ -48,24 +48,6 @@ namespace spice
> > > namespace streaming_agent
> > > {
> > > 
> > > -struct FormatMessage
> > > -{
> > > -    StreamDevHeader hdr;
> > > -    StreamMsgFormat msg;
> > > -};
> > > -
> > > -struct DataMessage
> > > -{
> > > -    StreamDevHeader hdr;
> > > -    StreamMsgData msg;
> > > -};
> > > -
> > > -struct CursorMessage
> > > -{
> > > -    StreamDevHeader hdr;
> > > -    StreamMsgCursorSet msg;
> > > -};
> > > -
> > > class Stream
> > > {
> > > public:
> > > @@ -79,24 +61,131 @@ public:
> > >     {
> > >         close(streamfd);
> > >     }
> > > -    operator int() { return streamfd; }
> > > 
> > >     int have_something_to_read(int timeout);
> > >     int read_command_from_device(void);
> > >     int read_command(bool blocking);
> > > 
> > > +
> > > +    template <typename Message, typename ...PayloadArgs>
> > > +    bool send(PayloadArgs... payload)
> > > +    {
> > > +        Message message(payload...);
> > > +        std::lock_guard<std::mutex> stream_guard(mutex);
> > > +        size_t expected = message.size(payload...);
> > > +        size_t written = message.write(*this, payload...);
> > > +        return written == expected;
> > > +    }
> > 
> > Do you purposefully avoid throwing exceptions here, returning a bool?
> 
> You really love exceptions, don’t you ;-)

Well... I don't always get errors, but when I do, I use exceptions to
handle them. :D

Really, that's what it is. Error = exception. That's what they teach
you at the uni and what we've always done at my previous job. It's the
language's designated mechanism to deal with errors, standard library
uses them, ...

You brought new light into the topic for me, but I still don't know how
to deal with it... Though the fact that you are the author of the
exception handling implementation and you are reluctant to use the
exception really speaks volumes :)

> I usually reserve exceptions for cases which are truly “catastrophic”, i.e. going the long way and unwindig is the right way to do it. Unwinding the stack is a very messy business, takes a lot of time, and if you can just return, it’s about one and a half gazillion times faster, generates smaller code, etc etc.
> 
> In this specific case, though, I think that unwinding could be seen as appropriate, since ATM we have nothing better than error + abort when it happens. Plus this might avoid a scenario where you could write twice if the first write fails. So I’m sold, I think this is the right thing to do.
> 
> > The error and exception could actually be checked as low as at the end
> > of the write_all() method, avoiding all the latter size returning and
> > checking?
> > 
> > > +
> > >     size_t write_all(const void *buf, const size_t len);
> > > -    int send_format(unsigned w, unsigned h, uint8_t c);
> > > -    int send_frame(const void *buf, const unsigned size);
> > > -    void send_cursor(uint16_t width, uint16_t height,
> > > -                     uint16_t hotspot_x, uint16_t hotspot_y,
> > > -                     std::function<void(uint32_t *)> fill_cursor);
> > > 
> > > private:
> > >     int streamfd = -1;
> > >     std::mutex mutex;
> > > };
> > > 
> > > +
> > > +template <typename Payload, typename Info>
> > 
> > "Info" is quite an unimaginative name :) I understand it's the message
> > itself here and also find it curiously hard to come up with something
> > better :) "TheMessage" not very good, is it? Still better than "Info"?
> > Maybe a comment referencing the CRTP to help readers understand?
> 
> I used ‘Info’ here because that’s the information about the message.

I kind of understood that, but still... very vague for me...

> I can add the CRTP comment, could not remember the acronym when I wrote the class ;-)
> 
> > 
> > > +struct Message
> > 
> > Any reason to not stick with the "class" keyword instead of "struct”?
> 
> Not really, more a matter of habit, using struct when it’s really about data and everything is public. But there, “Message” evolved far away from POD to warrant using ‘class’.

I do like to take the shortcut using struct to spare myself the
'public:' line at the top, but I'd think it would be considered an
inconsistency by others... :)

> > 
> > > +{
> > > +    template <typename ...PayloadArgs>
> > > +    Message(PayloadArgs... payload)
> > 
> > "PayloadArgs... payload_args", we have something called "Payload" here
> > too, getting a bit confusing.
> 
> But that’s the same thing. Payload is initialized with PayloadArgs, so it makes sense.
> 
> Initially, I had “PayloadConstructorArgs”, but then I started using it outside of the ctor.

I don't think it's the same, here Payload is the message class and
PayloadArgs is the content/args. So naming the objects accordingly IMO
helps clarity.

> > 
> > > +        : hdr(StreamDevHeader {
> > > +              .protocol_version = STREAM_DEVICE_PROTOCOL,
> > > +              .padding = 0,     // Workaround GCC bug "sorry: not implemented"
> > > +              .type = Info::type,
> > > +              .size = (uint32_t) (Info::size(payload...) - sizeof(hdr))
> > > +          }),
> > > +          msg(Info::make(payload...))
> > > +    { }
> > 
> > I find it redundant that you pass the "payload..." (the args :)) to
> > Info::size() and Info::make() here and then also to Info::write() in
> > Stream::send().
> 
> I’m not sure “redundant” is the correct word. I could stash the information in Message, but then the compiler would have a much harder time inlining the actual expression, which is always very simple. Unfortunately, we have three different expressions that take different input, hence the CRTP and the passing of arguments.
> 
> 
> > In the three messages below, you also showcase three
> > distinct ways of serializing them, caused by this redundancy (I
> > understand it is partially given by the payload of the messages).
> 
> It is *entirely* given by the payload, which I assume is a given, since it’s in the protocol. What else could come into play?

What I mean is that the place you create the data for serialization is
only partially given by the payload. Because for example with the
FormatMessage, you place the data in the class as a member, but you
could have also created them on the stack at the beginning of the
write() method. And unless I'm missing something, you could do it this
way for all the cases, therefore unify the approach. For the
FrameMessage, you are constructing an empty msg member...

> > 
> > See comments under each class, which all relate to this (hope it's not
> > too confusing).
> > 
> > > +
> > > +    StreamDevHeader hdr;
> > > +    Payload         msg;
> > > +};
> > > +
> > > +
> > > +struct FormatMessage : Message<StreamMsgFormat, FormatMessage>
> > > +{
> > > +    FormatMessage(unsigned w, unsigned h, uint8_t c) : Message(w, h, c) {}
> > > +    static const StreamMsgType type = STREAM_TYPE_FORMAT;
> > 
> > Could the type actually be part of the template? As in:
> > struct FormatMessage : Message<STREAM_TYPE_FORMAT, StreamMsgFormat, FormatMessage>
> 
> Sure, but I find it more consistent the way I wrote it. Also easier to read, because you have “type = TYPE” instead of just <TYPE>, but that’s really a personal preference.
> 
> > 
> > > +    static size_t size(unsigned width, unsigned height, uint8_t codec)
> > > +    {
> > > +        return sizeof(FormatMessage);
> > > +    }
> > > +    static StreamMsgFormat make(unsigned w, unsigned h, uint8_t c)
> > > +    {
> > > +        return StreamMsgFormat{ .width = w, .height = h, .codec = c, .padding1 = {} };
> > > +    }
> > > +    size_t write(Stream &stream, unsigned w, unsigned h, uint8_t c)
> > > +    {
> > > +        return stream.write_all(this, sizeof(*this));
> > > +    }
> > > +};
> > 
> > This message has static size, so you construct it in make() and then
> > write this whole object.
> 
> The message body has static size, yes. (Header is fixed size in all three cases).
> 
> > 
> > > +
> > > +
> > > +struct FrameMessage : Message<StreamMsgData, FrameMessage>
> > > +{
> > > +    FrameMessage(const void *frame, size_t length) : Message(frame, length) {}
> > > +    static const StreamMsgType type = STREAM_TYPE_DATA;
> > > +    static size_t size(const void *frame, size_t length)
> > > +    {
> > > +        return sizeof(FrameMessage) + length;
> > > +    }
> > > +    static StreamMsgData make(const void *frame, size_t length)
> > > +    {
> > > +        return StreamMsgData();
> > > +    }
> > > +    size_t write(Stream &stream, const void *frame, size_t length)
> > > +    {
> > > +        return stream.write_all(&hdr, sizeof(hdr)) + stream.write_all(frame, length);
> > > +    }
> > > +};
> > 
> > Here the message is actually only the frame data and so you construct
> > an empty message in make(). In write() you just write the stream data
> > passed to it.
> 
> Yes.
> 
> > 
> > > +
> > > +struct X11CursorMessage : Message<StreamMsgCursorSet, X11CursorMessage>
> > > +{
> > > +    X11CursorMessage(XFixesCursorImage *cursor)
> > > +        : Message(cursor),
> > > +          pixels(new uint32_t[pixel_size(cursor)])
> > > +    {
> > > +        fill_pixels(cursor);
> > > +    }
> > > +    static const StreamMsgType type = STREAM_TYPE_CURSOR_SET;
> > > +    static size_t pixel_size(XFixesCursorImage *cursor)
> > > +    {
> > > +        return cursor->width * cursor->height;
> > > +    }
> > > +    static size_t size(XFixesCursorImage *cursor)
> > > +    {
> > > +        return sizeof(X11CursorMessage) + sizeof(uint32_t) * pixel_size(cursor);
> > > +    }
> > > +    static StreamMsgCursorSet make(XFixesCursorImage *cursor)
> > > +    {
> > > +        return StreamMsgCursorSet
> > > +        {
> > > +            .width = cursor->width,
> > > +            .height = cursor->height,
> > > +            .hot_spot_x = cursor->xhot,
> > > +            .hot_spot_y = cursor->yhot,
> > > +            .type = SPICE_CURSOR_TYPE_ALPHA,
> > > +            .padding1 = { },
> > > +            .data = { }
> > > +        };
> > > +    }
> > > +    size_t write(Stream &stream, XFixesCursorImage *cursor)
> > > +    {
> > > +        return stream.write_all(&hdr, sizeof(hdr)) + stream.write_all(pixels.get(), hdr.size);
> > 
> > You are not writing the msg data here, might be the reson for the
> > missing cursor.
> 
> Ah, good catch. I thought of not writing *this because of the extra pixels field, but then wrote the wrong class.
> 
> > 
> > > +    }
> > > +    void fill_pixels(XFixesCursorImage *cursor)
> > > +    {
> > > +        uint32_t *pixbuf = pixels.get();
> > > +        unsigned count = cursor->width * cursor->height;
> > > +        for (unsigned i = 0; i < count; ++i)
> > > +            pixbuf[i] = cursor->pixels[i];
> > > +    }
> > > +private:
> > > +    std::unique_ptr<uint32_t[]> pixels;
> > > +};
> > 
> > (note in this class some methods could be private and some arguments
> > const)
> 
> Yes. Good idea.
> 
> > 
> > Here you add a private member pixels, which you fill in constructor. In
> > make(), you create the static part of the message. In write(), you
> > write them.
> > 
> > So, I ask, could you not actually construct all the data to write in
> > write(), and perhaps even remove the Message::msg member and the make()
> > method?
> 
> Yes, but that would require to copy the frames data, which I considered an expensive-enough operation to try and avoid it. For cursor pixels, it’s less of an issue a) because we need to copy anyway, due to format conversion, and they happen much less frequently.

I don't think you would need to copy the data? What I propose actually
doesn't mean any change for this method in particular?

> > 
> > That would make the classes a bit simpler?
> 
> Simpler, yes but much less efficient in the important case, which is sending frames, where it would add one malloc / copy / free for each frame, which the present code avoids at the “mere” cost of passing the payload args around ;-)
> 
> 
> Thanks a lot.
> 
> > 
> > Cheers,
> > Lukas
> > 
> > > +
> > > }} // namespace spice::streaming_agent
> > > 
> > > 
> > > @@ -201,66 +290,6 @@ size_t Stream::write_all(const void *buf, const size_t len)
> > >     return written;
> > > }
> > > 
> > > -int Stream::send_format(unsigned w, unsigned h, uint8_t c)
> > > -{
> > > -    const size_t msgsize = sizeof(FormatMessage);
> > > -    const size_t hdrsize  = sizeof(StreamDevHeader);
> > > -    FormatMessage 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(mutex);
> > > -    if (write_all(&msg, msgsize) != msgsize) {
> > > -        return EXIT_FAILURE;
> > > -    }
> > > -    return EXIT_SUCCESS;
> > > -}
> > > -
> > > -int Stream::send_frame(const void *buf, const unsigned size)
> > > -{
> > > -    ssize_t n;
> > > -    const size_t msgsize = sizeof(FormatMessage);
> > > -    DataMessage 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 = {}
> > > -    };
> > > -
> > > -    std::lock_guard<std::mutex> stream_guard(mutex);
> > > -    n = write_all(&msg, msgsize);
> > > -    syslog(LOG_DEBUG,
> > > -           "wrote %ld bytes of header of data msg with frame of size %u bytes\n",
> > > -           n, msg.hdr.size);
> > > -    if (n != msgsize) {
> > > -        syslog(LOG_WARNING, "write_all header: wrote %ld expected %lu\n",
> > > -               n, msgsize);
> > > -        return EXIT_FAILURE;
> > > -    }
> > > -    n = write_all(buf, size);
> > > -    syslog(LOG_DEBUG, "wrote data msg body of size %ld\n", n);
> > > -    if (n != size) {
> > > -        syslog(LOG_WARNING, "write_all header: wrote %ld expected %u\n",
> > > -               n, size);
> > > -        return EXIT_FAILURE;
> > > -    }
> > > -    return EXIT_SUCCESS;
> > > -}
> > > -
> > > /* returns current time in micro-seconds */
> > > static uint64_t get_time(void)
> > > {
> > > @@ -304,47 +333,6 @@ static void usage(const char *progname)
> > >     exit(1);
> > > }
> > > 
> > > -void
> > > -Stream::send_cursor(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;
> > > -
> > > -    const uint32_t cursor_msgsize =
> > > -        sizeof(CursorMessage) + width * height * sizeof(uint32_t);
> > > -    const uint32_t hdrsize  = sizeof(StreamDevHeader);
> > > -
> > > -    std::unique_ptr<uint8_t[]> storage(new uint8_t[cursor_msgsize]);
> > > -
> > > -    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 = cursor_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->msg.data);
> > > -    fill_cursor(pixels);
> > > -
> > > -    std::lock_guard<std::mutex> stream_guard(mutex);
> > > -    write_all(storage.get(), cursor_msgsize);
> > > -}
> > > -
> > > static void cursor_changes(Stream *stream, Display *display, int event_base)
> > > {
> > >     unsigned long last_serial = 0;
> > > @@ -363,12 +351,8 @@ static void cursor_changes(Stream *stream, Display *display, int event_base)
> > >             continue;
> > > 
> > >         last_serial = cursor->cursor_serial;
> > > -        auto fill_cursor = [cursor](uint32_t *pixels) {
> > > -            for (unsigned i = 0; i < cursor->width * cursor->height; ++i)
> > > -                pixels[i] = cursor->pixels[i];
> > > -        };
> > > -        stream->send_cursor(cursor->width, cursor->height,
> > > -                            cursor->xhot, cursor->yhot, fill_cursor);
> > > +        if (!stream->send<X11CursorMessage>(cursor))
> > > +            syslog(LOG_WARNING, "FAILED to send cursor\n");
> > >     }
> > > }
> > > 
> > > @@ -422,7 +406,7 @@ do_capture(Stream &stream, const char *streamport, FILE *f_log)
> > > 
> > >                 syslog(LOG_DEBUG, "wXh %uX%u  codec=%u\n", width, height, codec);
> > > 
> > > -                if (stream.send_format(width, height, codec) == EXIT_FAILURE)
> > > +                if (!stream.send<FormatMessage>(width, height, codec))
> > >                     throw std::runtime_error("FAILED to send format message");
> > >             }
> > >             if (f_log) {
> > > @@ -434,7 +418,7 @@ do_capture(Stream &stream, const char *streamport, FILE *f_log)
> > >                     hexdump(frame.buffer, frame.buffer_size, f_log);
> > >                 }
> > >             }
> > > -            if (stream.send_frame(frame.buffer, frame.buffer_size) == EXIT_FAILURE) {
> > > +            if (!stream.send<FrameMessage>(frame.buffer, frame.buffer_size)) {
> > >                 syslog(LOG_ERR, "FAILED to send a frame\n");
> > >                 break;
> > >             }
> > 
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
>
> On 21 Feb 2018, at 10:45, Lukáš Hrázký <lhrazky@redhat.com> wrote:
> 
> On Tue, 2018-02-20 at 16:41 +0100, Christophe de Dinechin wrote:
>>> On 20 Feb 2018, at 15:29, Lukáš Hrázký <lhrazky@redhat.com> wrote:
>>> 
>>> On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
>>>> From: Christophe de Dinechin <dinechin@redhat.com>
>>>> 
>>>> - The Stream class now deals with locking and sending messages
>>>> - The Message<> template class deals with the general writing mechanisms
>>>> - Three classes, FormatMessage, FrameMessage and X11CursorMessage represent individual messages
>>>> 
>>>> The various classes should be moved to separate headers in a subsequent operation
>>>> 
>>>> The design uses templates to avoid any runtime overhead. All the calls are static.
>>> 
>>> All in all, a nice way to encapsulate the sending of messages. What I
>>> worked on, actually, was the receiving of messages, as that is actually
>>> more important for separating the code (as seen later in the problem
>>> you had with client_codecs and streaming_requested static variables).
>>> 
>>> I am now wondering how to merge it with your changes and whether the
>>> same Message class hierarchy could be used (without making a mess out
>>> of it). We should discuss it later.
>> 
>> Do you have your WIP stuff in a private branch I could look at? Maybe I can help with the rebasing.
> 
> I'll push it somewhere so you can have a look, but I can manage the
> rebase myself :) It would still be an effort to find out what you did
> during the rebase.
> 
>> I would probably keep input and output messages in separate classes. I can’t see much commonality between the two. Using the same CRTP for input messages, and maybe rename Message as OutputMessage…
> 
> Agreed, probably...
> 
>>> 
>>>> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
>>>> ---
>>>> src/spice-streaming-agent.cpp | 250 ++++++++++++++++++++----------------------
>>>> 1 file changed, 117 insertions(+), 133 deletions(-)
>>>> 
>>>> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
>>>> index a989ee7..c174ea4 100644
>>>> --- a/src/spice-streaming-agent.cpp
>>>> +++ b/src/spice-streaming-agent.cpp
>>>> @@ -48,24 +48,6 @@ namespace spice
>>>> namespace streaming_agent
>>>> {
>>>> 
>>>> -struct FormatMessage
>>>> -{
>>>> -    StreamDevHeader hdr;
>>>> -    StreamMsgFormat msg;
>>>> -};
>>>> -
>>>> -struct DataMessage
>>>> -{
>>>> -    StreamDevHeader hdr;
>>>> -    StreamMsgData msg;
>>>> -};
>>>> -
>>>> -struct CursorMessage
>>>> -{
>>>> -    StreamDevHeader hdr;
>>>> -    StreamMsgCursorSet msg;
>>>> -};
>>>> -
>>>> class Stream
>>>> {
>>>> public:
>>>> @@ -79,24 +61,131 @@ public:
>>>>    {
>>>>        close(streamfd);
>>>>    }
>>>> -    operator int() { return streamfd; }
>>>> 
>>>>    int have_something_to_read(int timeout);
>>>>    int read_command_from_device(void);
>>>>    int read_command(bool blocking);
>>>> 
>>>> +
>>>> +    template <typename Message, typename ...PayloadArgs>
>>>> +    bool send(PayloadArgs... payload)
>>>> +    {
>>>> +        Message message(payload...);
>>>> +        std::lock_guard<std::mutex> stream_guard(mutex);
>>>> +        size_t expected = message.size(payload...);
>>>> +        size_t written = message.write(*this, payload...);
>>>> +        return written == expected;
>>>> +    }
>>> 
>>> Do you purposefully avoid throwing exceptions here, returning a bool?
>> 
>> You really love exceptions, don’t you ;-)
> 
> Well... I don't always get errors, but when I do, I use exceptions to
> handle them. :D
> 
> Really, that's what it is. Error = exception.

No. C++CG E2 "Throw an exception to signal that a function can't perform its assigned task”.

Examples of errors that are not exceptions: FP NaN and infinities, compiler errors (and more generally parsing errors), etc. A parser error is not an exception because it’s the job of the parser to detect the error…

I could also give examples of exceptions that are not errors, though it’s more subtle and OT. Consider a C++ equivalent of an interrupted system call. Also, is bad_alloc an “error" or a limitation? (one could argue that the error would be e.g. to give you some memory belonging to someone else ;-)


> That's what they teach
> you at the uni and what we've always done at my previous job. It's the
> language's designated mechanism to deal with errors, standard library
> uses them, ...
> 
> You brought new light into the topic for me, but I still don't know how
> to deal with it... Though the fact that you are the author of the
> exception handling implementation and you are reluctant to use the
> exception really speaks volumes :)
> 
>> I usually reserve exceptions for cases which are truly “catastrophic”, i.e. going the long way and unwindig is the right way to do it. Unwinding the stack is a very messy business, takes a lot of time, and if you can just return, it’s about one and a half gazillion times faster, generates smaller code, etc etc.
>> 
>> In this specific case, though, I think that unwinding could be seen as appropriate, since ATM we have nothing better than error + abort when it happens. Plus this might avoid a scenario where you could write twice if the first write fails. So I’m sold, I think this is the right thing to do.
>> 
>>> The error and exception could actually be checked as low as at the end
>>> of the write_all() method, avoiding all the latter size returning and
>>> checking?
>>> 
>>>> +
>>>>    size_t write_all(const void *buf, const size_t len);
>>>> -    int send_format(unsigned w, unsigned h, uint8_t c);
>>>> -    int send_frame(const void *buf, const unsigned size);
>>>> -    void send_cursor(uint16_t width, uint16_t height,
>>>> -                     uint16_t hotspot_x, uint16_t hotspot_y,
>>>> -                     std::function<void(uint32_t *)> fill_cursor);
>>>> 
>>>> private:
>>>>    int streamfd = -1;
>>>>    std::mutex mutex;
>>>> };
>>>> 
>>>> +
>>>> +template <typename Payload, typename Info>
>>> 
>>> "Info" is quite an unimaginative name :) I understand it's the message
>>> itself here and also find it curiously hard to come up with something
>>> better :) "TheMessage" not very good, is it? Still better than "Info"?
>>> Maybe a comment referencing the CRTP to help readers understand?
>> 
>> I used ‘Info’ here because that’s the information about the message.
> 
> I kind of understood that, but still... very vague for me…

Will stay that way for the moment, unless you give me a really better name, sorry.

> 
>> I can add the CRTP comment, could not remember the acronym when I wrote the class ;-)
>> 
>>> 
>>>> +struct Message
>>> 
>>> Any reason to not stick with the "class" keyword instead of "struct”?
>> 
>> Not really, more a matter of habit, using struct when it’s really about data and everything is public. But there, “Message” evolved far away from POD to warrant using ‘class’.
> 
> I do like to take the shortcut using struct to spare myself the 'public:' line at the top, but I'd think it would be considered an inconsistency by others... :)

That’s not the reason. I tend to use it to mark a struct as “mostly data”, even if it has C++isms in it. Here, I initially saw the message is mostly data, i.e. I would not have minded the data members being public. But that’s no longer the case. Will change it.

> 
>>> 
>>>> +{
>>>> +    template <typename ...PayloadArgs>
>>>> +    Message(PayloadArgs... payload)
>>> 
>>> "PayloadArgs... payload_args", we have something called "Payload" here
>>> too, getting a bit confusing.
>> 
>> But that’s the same thing. Payload is initialized with PayloadArgs, so it makes sense.
>> 
>> Initially, I had “PayloadConstructorArgs”, but then I started using it outside of the ctor.
> 
> I don't think it's the same, here Payload is the message class and
> PayloadArgs is the content/args. So naming the objects accordingly IMO
> helps clarity.

The Payload is defined from the PayloadArgs, i.e. its constructor is Payload(PayloadArgs…). What is the problem with that?
> 
>>> 
>>>> +        : hdr(StreamDevHeader {
>>>> +              .protocol_version = STREAM_DEVICE_PROTOCOL,
>>>> +              .padding = 0,     // Workaround GCC bug "sorry: not implemented"
>>>> +              .type = Info::type,
>>>> +              .size = (uint32_t) (Info::size(payload...) - sizeof(hdr))
>>>> +          }),
>>>> +          msg(Info::make(payload...))
>>>> +    { }
>>> 
>>> I find it redundant that you pass the "payload..." (the args :)) to
>>> Info::size() and Info::make() here and then also to Info::write() in
>>> Stream::send().
>> 
>> I’m not sure “redundant” is the correct word. I could stash the information in Message, but then the compiler would have a much harder time inlining the actual expression, which is always very simple. Unfortunately, we have three different expressions that take different input, hence the CRTP and the passing of arguments.
>> 
>> 
>>> In the three messages below, you also showcase three
>>> distinct ways of serializing them, caused by this redundancy (I
>>> understand it is partially given by the payload of the messages).
>> 
>> It is *entirely* given by the payload, which I assume is a given, since it’s in the protocol. What else could come into play?
> 
> What I mean is that the place you create the data for serialization is
> only partially given by the payload. Because for example with the
> FormatMessage, you place the data in the class as a member, but you
> could have also created them on the stack at the beginning of the
> write() method. And unless I'm missing something, you could do it this
> way for all the cases, therefore unify the approach. For the
> FrameMessage, you are constructing an empty msg member…

I tell you that I don’t want to copy, say, 70K of frame data if I can avoid it, and you are suggesting I allocate 70K of stack instead? I think you did miss something, yes.

(if that clarifies, that frame is given to me by the capture, I did not ask the capture to use some buffer I pre-allocated)

> 
>>> 
>>> See comments under each class, which all relate to this (hope it's not
>>> too confusing).
>>> 
>>>> +
>>>> +    StreamDevHeader hdr;
>>>> +    Payload         msg;
>>>> +};
>>>> +
>>>> +
>>>> +struct FormatMessage : Message<StreamMsgFormat, FormatMessage>
>>>> +{
>>>> +    FormatMessage(unsigned w, unsigned h, uint8_t c) : Message(w, h, c) {}
>>>> +    static const StreamMsgType type = STREAM_TYPE_FORMAT;
>>> 
>>> Could the type actually be part of the template? As in:
>>> struct FormatMessage : Message<STREAM_TYPE_FORMAT, StreamMsgFormat, FormatMessage>
>> 
>> Sure, but I find it more consistent the way I wrote it. Also easier to read, because you have “type = TYPE” instead of just <TYPE>, but that’s really a personal preference.
>> 
>>> 
>>>> +    static size_t size(unsigned width, unsigned height, uint8_t codec)
>>>> +    {
>>>> +        return sizeof(FormatMessage);
>>>> +    }
>>>> +    static StreamMsgFormat make(unsigned w, unsigned h, uint8_t c)
>>>> +    {
>>>> +        return StreamMsgFormat{ .width = w, .height = h, .codec = c, .padding1 = {} };
>>>> +    }
>>>> +    size_t write(Stream &stream, unsigned w, unsigned h, uint8_t c)
>>>> +    {
>>>> +        return stream.write_all(this, sizeof(*this));
>>>> +    }
>>>> +};
>>> 
>>> This message has static size, so you construct it in make() and then
>>> write this whole object.
>> 
>> The message body has static size, yes. (Header is fixed size in all three cases).
>> 
>>> 
>>>> +
>>>> +
>>>> +struct FrameMessage : Message<StreamMsgData, FrameMessage>
>>>> +{
>>>> +    FrameMessage(const void *frame, size_t length) : Message(frame, length) {}
>>>> +    static const StreamMsgType type = STREAM_TYPE_DATA;
>>>> +    static size_t size(const void *frame, size_t length)
>>>> +    {
>>>> +        return sizeof(FrameMessage) + length;
>>>> +    }
>>>> +    static StreamMsgData make(const void *frame, size_t length)
>>>> +    {
>>>> +        return StreamMsgData();
>>>> +    }
>>>> +    size_t write(Stream &stream, const void *frame, size_t length)
>>>> +    {
>>>> +        return stream.write_all(&hdr, sizeof(hdr)) + stream.write_all(frame, length);
>>>> +    }
>>>> +};
>>> 
>>> Here the message is actually only the frame data and so you construct
>>> an empty message in make(). In write() you just write the stream data
>>> passed to it.
>> 
>> Yes.
>> 
>>> 
>>>> +
>>>> +struct X11CursorMessage : Message<StreamMsgCursorSet, X11CursorMessage>
>>>> +{
>>>> +    X11CursorMessage(XFixesCursorImage *cursor)
>>>> +        : Message(cursor),
>>>> +          pixels(new uint32_t[pixel_size(cursor)])
>>>> +    {
>>>> +        fill_pixels(cursor);
>>>> +    }
>>>> +    static const StreamMsgType type = STREAM_TYPE_CURSOR_SET;
>>>> +    static size_t pixel_size(XFixesCursorImage *cursor)
>>>> +    {
>>>> +        return cursor->width * cursor->height;
>>>> +    }
>>>> +    static size_t size(XFixesCursorImage *cursor)
>>>> +    {
>>>> +        return sizeof(X11CursorMessage) + sizeof(uint32_t) * pixel_size(cursor);
>>>> +    }
>>>> +    static StreamMsgCursorSet make(XFixesCursorImage *cursor)
>>>> +    {
>>>> +        return StreamMsgCursorSet
>>>> +        {
>>>> +            .width = cursor->width,
>>>> +            .height = cursor->height,
>>>> +            .hot_spot_x = cursor->xhot,
>>>> +            .hot_spot_y = cursor->yhot,
>>>> +            .type = SPICE_CURSOR_TYPE_ALPHA,
>>>> +            .padding1 = { },
>>>> +            .data = { }
>>>> +        };
>>>> +    }
>>>> +    size_t write(Stream &stream, XFixesCursorImage *cursor)
>>>> +    {
>>>> +        return stream.write_all(&hdr, sizeof(hdr)) + stream.write_all(pixels.get(), hdr.size);
>>> 
>>> You are not writing the msg data here, might be the reson for the
>>> missing cursor.
>> 
>> Ah, good catch. I thought of not writing *this because of the extra pixels field, but then wrote the wrong class.
>> 
>>> 
>>>> +    }
>>>> +    void fill_pixels(XFixesCursorImage *cursor)
>>>> +    {
>>>> +        uint32_t *pixbuf = pixels.get();
>>>> +        unsigned count = cursor->width * cursor->height;
>>>> +        for (unsigned i = 0; i < count; ++i)
>>>> +            pixbuf[i] = cursor->pixels[i];
>>>> +    }
>>>> +private:
>>>> +    std::unique_ptr<uint32_t[]> pixels;
>>>> +};
>>> 
>>> (note in this class some methods could be private and some arguments
>>> const)
>> 
>> Yes. Good idea.
>> 
>>> 
>>> Here you add a private member pixels, which you fill in constructor. In
>>> make(), you create the static part of the message. In write(), you
>>> write them.
>>> 
>>> So, I ask, could you not actually construct all the data to write in
>>> write(), and perhaps even remove the Message::msg member and the make()
>>> method?
>> 
>> Yes, but that would require to copy the frames data, which I considered an expensive-enough operation to try and avoid it. For cursor pixels, it’s less of an issue a) because we need to copy anyway, due to format conversion, and they happen much less frequently.
> 
> I don't think you would need to copy the data? What I propose actually
> doesn't mean any change for this method in particular?
> 
>>> 
>>> That would make the classes a bit simpler?
>> 
>> Simpler, yes but much less efficient in the important case, which is sending frames, where it would add one malloc / copy / free for each frame, which the present code avoids at the “mere” cost of passing the payload args around ;-)
>> 
>> 
>> Thanks a lot.
>> 
>>> 
>>> Cheers,
>>> Lukas
>>> 
>>>> +
>>>> }} // namespace spice::streaming_agent
>>>> 
>>>> 
>>>> @@ -201,66 +290,6 @@ size_t Stream::write_all(const void *buf, const size_t len)
>>>>    return written;
>>>> }
>>>> 
>>>> -int Stream::send_format(unsigned w, unsigned h, uint8_t c)
>>>> -{
>>>> -    const size_t msgsize = sizeof(FormatMessage);
>>>> -    const size_t hdrsize  = sizeof(StreamDevHeader);
>>>> -    FormatMessage 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(mutex);
>>>> -    if (write_all(&msg, msgsize) != msgsize) {
>>>> -        return EXIT_FAILURE;
>>>> -    }
>>>> -    return EXIT_SUCCESS;
>>>> -}
>>>> -
>>>> -int Stream::send_frame(const void *buf, const unsigned size)
>>>> -{
>>>> -    ssize_t n;
>>>> -    const size_t msgsize = sizeof(FormatMessage);
>>>> -    DataMessage 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 = {}
>>>> -    };
>>>> -
>>>> -    std::lock_guard<std::mutex> stream_guard(mutex);
>>>> -    n = write_all(&msg, msgsize);
>>>> -    syslog(LOG_DEBUG,
>>>> -           "wrote %ld bytes of header of data msg with frame of size %u bytes\n",
>>>> -           n, msg.hdr.size);
>>>> -    if (n != msgsize) {
>>>> -        syslog(LOG_WARNING, "write_all header: wrote %ld expected %lu\n",
>>>> -               n, msgsize);
>>>> -        return EXIT_FAILURE;
>>>> -    }
>>>> -    n = write_all(buf, size);
>>>> -    syslog(LOG_DEBUG, "wrote data msg body of size %ld\n", n);
>>>> -    if (n != size) {
>>>> -        syslog(LOG_WARNING, "write_all header: wrote %ld expected %u\n",
>>>> -               n, size);
>>>> -        return EXIT_FAILURE;
>>>> -    }
>>>> -    return EXIT_SUCCESS;
>>>> -}
>>>> -
>>>> /* returns current time in micro-seconds */
>>>> static uint64_t get_time(void)
>>>> {
>>>> @@ -304,47 +333,6 @@ static void usage(const char *progname)
>>>>    exit(1);
>>>> }
>>>> 
>>>> -void
>>>> -Stream::send_cursor(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;
>>>> -
>>>> -    const uint32_t cursor_msgsize =
>>>> -        sizeof(CursorMessage) + width * height * sizeof(uint32_t);
>>>> -    const uint32_t hdrsize  = sizeof(StreamDevHeader);
>>>> -
>>>> -    std::unique_ptr<uint8_t[]> storage(new uint8_t[cursor_msgsize]);
>>>> -
>>>> -    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 = cursor_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->msg.data);
>>>> -    fill_cursor(pixels);
>>>> -
>>>> -    std::lock_guard<std::mutex> stream_guard(mutex);
>>>> -    write_all(storage.get(), cursor_msgsize);
>>>> -}
>>>> -
>>>> static void cursor_changes(Stream *stream, Display *display, int event_base)
>>>> {
>>>>    unsigned long last_serial = 0;
>>>> @@ -363,12 +351,8 @@ static void cursor_changes(Stream *stream, Display *display, int event_base)
>>>>            continue;
>>>> 
>>>>        last_serial = cursor->cursor_serial;
>>>> -        auto fill_cursor = [cursor](uint32_t *pixels) {
>>>> -            for (unsigned i = 0; i < cursor->width * cursor->height; ++i)
>>>> -                pixels[i] = cursor->pixels[i];
>>>> -        };
>>>> -        stream->send_cursor(cursor->width, cursor->height,
>>>> -                            cursor->xhot, cursor->yhot, fill_cursor);
>>>> +        if (!stream->send<X11CursorMessage>(cursor))
>>>> +            syslog(LOG_WARNING, "FAILED to send cursor\n");
>>>>    }
>>>> }
>>>> 
>>>> @@ -422,7 +406,7 @@ do_capture(Stream &stream, const char *streamport, FILE *f_log)
>>>> 
>>>>                syslog(LOG_DEBUG, "wXh %uX%u  codec=%u\n", width, height, codec);
>>>> 
>>>> -                if (stream.send_format(width, height, codec) == EXIT_FAILURE)
>>>> +                if (!stream.send<FormatMessage>(width, height, codec))
>>>>                    throw std::runtime_error("FAILED to send format message");
>>>>            }
>>>>            if (f_log) {
>>>> @@ -434,7 +418,7 @@ do_capture(Stream &stream, const char *streamport, FILE *f_log)
>>>>                    hexdump(frame.buffer, frame.buffer_size, f_log);
>>>>                }
>>>>            }
>>>> -            if (stream.send_frame(frame.buffer, frame.buffer_size) == EXIT_FAILURE) {
>>>> +            if (!stream.send<FrameMessage>(frame.buffer, frame.buffer_size)) {
>>>>                syslog(LOG_ERR, "FAILED to send a frame\n");
>>>>                break;
>>>>            }
>>> 
>>> _______________________________________________
>>> Spice-devel mailing list
>>> Spice-devel@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>> 
>> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
On Thu, 2018-02-22 at 18:51 +0100, Christophe de Dinechin wrote:
> > On 21 Feb 2018, at 10:45, Lukáš Hrázký <lhrazky@redhat.com> wrote:
> > 
> > On Tue, 2018-02-20 at 16:41 +0100, Christophe de Dinechin wrote:
> > > > On 20 Feb 2018, at 15:29, Lukáš Hrázký <lhrazky@redhat.com> wrote:
> > > > 
> > > > On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
> > > > > From: Christophe de Dinechin <dinechin@redhat.com>
> > > > > 
> > > > > - The Stream class now deals with locking and sending messages
> > > > > - The Message<> template class deals with the general writing mechanisms
> > > > > - Three classes, FormatMessage, FrameMessage and X11CursorMessage represent individual messages
> > > > > 
> > > > > The various classes should be moved to separate headers in a subsequent operation
> > > > > 
> > > > > The design uses templates to avoid any runtime overhead. All the calls are static.
> > > > 
> > > > All in all, a nice way to encapsulate the sending of messages. What I
> > > > worked on, actually, was the receiving of messages, as that is actually
> > > > more important for separating the code (as seen later in the problem
> > > > you had with client_codecs and streaming_requested static variables).
> > > > 
> > > > I am now wondering how to merge it with your changes and whether the
> > > > same Message class hierarchy could be used (without making a mess out
> > > > of it). We should discuss it later.
> > > 
> > > Do you have your WIP stuff in a private branch I could look at? Maybe I can help with the rebasing.
> > 
> > I'll push it somewhere so you can have a look, but I can manage the
> > rebase myself :) It would still be an effort to find out what you did
> > during the rebase.
> > 
> > > I would probably keep input and output messages in separate classes. I can’t see much commonality between the two. Using the same CRTP for input messages, and maybe rename Message as OutputMessage…
> > 
> > Agreed, probably...
> > 
> > > > 
> > > > > Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> > > > > ---
> > > > > src/spice-streaming-agent.cpp | 250 ++++++++++++++++++++----------------------
> > > > > 1 file changed, 117 insertions(+), 133 deletions(-)
> > > > > 
> > > > > diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> > > > > index a989ee7..c174ea4 100644
> > > > > --- a/src/spice-streaming-agent.cpp
> > > > > +++ b/src/spice-streaming-agent.cpp
> > > > > @@ -48,24 +48,6 @@ namespace spice
> > > > > namespace streaming_agent
> > > > > {
> > > > > 
> > > > > -struct FormatMessage
> > > > > -{
> > > > > -    StreamDevHeader hdr;
> > > > > -    StreamMsgFormat msg;
> > > > > -};
> > > > > -
> > > > > -struct DataMessage
> > > > > -{
> > > > > -    StreamDevHeader hdr;
> > > > > -    StreamMsgData msg;
> > > > > -};
> > > > > -
> > > > > -struct CursorMessage
> > > > > -{
> > > > > -    StreamDevHeader hdr;
> > > > > -    StreamMsgCursorSet msg;
> > > > > -};
> > > > > -
> > > > > class Stream
> > > > > {
> > > > > public:
> > > > > @@ -79,24 +61,131 @@ public:
> > > > >    {
> > > > >        close(streamfd);
> > > > >    }
> > > > > -    operator int() { return streamfd; }
> > > > > 
> > > > >    int have_something_to_read(int timeout);
> > > > >    int read_command_from_device(void);
> > > > >    int read_command(bool blocking);
> > > > > 
> > > > > +
> > > > > +    template <typename Message, typename ...PayloadArgs>
> > > > > +    bool send(PayloadArgs... payload)
> > > > > +    {
> > > > > +        Message message(payload...);
> > > > > +        std::lock_guard<std::mutex> stream_guard(mutex);
> > > > > +        size_t expected = message.size(payload...);
> > > > > +        size_t written = message.write(*this, payload...);
> > > > > +        return written == expected;
> > > > > +    }
> > > > 
> > > > Do you purposefully avoid throwing exceptions here, returning a bool?
> > > 
> > > You really love exceptions, don’t you ;-)
> > 
> > Well... I don't always get errors, but when I do, I use exceptions to
> > handle them. :D
> > 
> > Really, that's what it is. Error = exception.
> 
> No. C++CG E2 "Throw an exception to signal that a function can't perform its assigned task”.
> 
> Examples of errors that are not exceptions: FP NaN and infinities, compiler errors (and more generally parsing errors), etc. A parser error is not an exception because it’s the job of the parser to detect the error…
> 
> I could also give examples of exceptions that are not errors, though it’s more subtle and OT. Consider a C++ equivalent of an interrupted system call. Also, is bad_alloc an “error" or a limitation? (one could argue that the error would be e.g. to give you some memory belonging to someone else ;-)

If you put it this way, sure.

> > That's what they teach
> > you at the uni and what we've always done at my previous job. It's the
> > language's designated mechanism to deal with errors, standard library
> > uses them, ...
> > 
> > You brought new light into the topic for me, but I still don't know how
> > to deal with it... Though the fact that you are the author of the
> > exception handling implementation and you are reluctant to use the
> > exception really speaks volumes :)
> > 
> > > I usually reserve exceptions for cases which are truly “catastrophic”, i.e. going the long way and unwindig is the right way to do it. Unwinding the stack is a very messy business, takes a lot of time, and if you can just return, it’s about one and a half gazillion times faster, generates smaller code, etc etc.
> > > 
> > > In this specific case, though, I think that unwinding could be seen as appropriate, since ATM we have nothing better than error + abort when it happens. Plus this might avoid a scenario where you could write twice if the first write fails. So I’m sold, I think this is the right thing to do.
> > > 
> > > > The error and exception could actually be checked as low as at the end
> > > > of the write_all() method, avoiding all the latter size returning and
> > > > checking?
> > > > 
> > > > > +
> > > > >    size_t write_all(const void *buf, const size_t len);
> > > > > -    int send_format(unsigned w, unsigned h, uint8_t c);
> > > > > -    int send_frame(const void *buf, const unsigned size);
> > > > > -    void send_cursor(uint16_t width, uint16_t height,
> > > > > -                     uint16_t hotspot_x, uint16_t hotspot_y,
> > > > > -                     std::function<void(uint32_t *)> fill_cursor);
> > > > > 
> > > > > private:
> > > > >    int streamfd = -1;
> > > > >    std::mutex mutex;
> > > > > };
> > > > > 
> > > > > +
> > > > > +template <typename Payload, typename Info>
> > > > 
> > > > "Info" is quite an unimaginative name :) I understand it's the message
> > > > itself here and also find it curiously hard to come up with something
> > > > better :) "TheMessage" not very good, is it? Still better than "Info"?
> > > > Maybe a comment referencing the CRTP to help readers understand?
> > > 
> > > I used ‘Info’ here because that’s the information about the message.
> > 
> > I kind of understood that, but still... very vague for me…
> 
> Will stay that way for the moment, unless you give me a really better name, sorry.
> 
> > 
> > > I can add the CRTP comment, could not remember the acronym when I wrote the class ;-)
> > > 
> > > > 
> > > > > +struct Message
> > > > 
> > > > Any reason to not stick with the "class" keyword instead of "struct”?
> > > 
> > > Not really, more a matter of habit, using struct when it’s really about data and everything is public. But there, “Message” evolved far away from POD to warrant using ‘class’.
> > 
> > I do like to take the shortcut using struct to spare myself the 'public:' line at the top, but I'd think it would be considered an inconsistency by others... :)
> 
> That’s not the reason. I tend to use it to mark a struct as “mostly data”, even if it has C++isms in it. Here, I initially saw the message is mostly data, i.e. I would not have minded the data members being public. But that’s no longer the case. Will change it.
> 
> > 
> > > > 
> > > > > +{
> > > > > +    template <typename ...PayloadArgs>
> > > > > +    Message(PayloadArgs... payload)
> > > > 
> > > > "PayloadArgs... payload_args", we have something called "Payload" here
> > > > too, getting a bit confusing.
> > > 
> > > But that’s the same thing. Payload is initialized with PayloadArgs, so it makes sense.
> > > 
> > > Initially, I had “PayloadConstructorArgs”, but then I started using it outside of the ctor.
> > 
> > I don't think it's the same, here Payload is the message class and
> > PayloadArgs is the content/args. So naming the objects accordingly IMO
> > helps clarity.
> 
> The Payload is defined from the PayloadArgs, i.e. its constructor is Payload(PayloadArgs…). What is the problem with that?

The problem is Payload(PayloadArgs... payload).

> > 
> > > > 
> > > > > +        : hdr(StreamDevHeader {
> > > > > +              .protocol_version = STREAM_DEVICE_PROTOCOL,
> > > > > +              .padding = 0,     // Workaround GCC bug "sorry: not implemented"
> > > > > +              .type = Info::type,
> > > > > +              .size = (uint32_t) (Info::size(payload...) - sizeof(hdr))
> > > > > +          }),
> > > > > +          msg(Info::make(payload...))
> > > > > +    { }
> > > > 
> > > > I find it redundant that you pass the "payload..." (the args :)) to
> > > > Info::size() and Info::make() here and then also to Info::write() in
> > > > Stream::send().
> > > 
> > > I’m not sure “redundant” is the correct word. I could stash the information in Message, but then the compiler would have a much harder time inlining the actual expression, which is always very simple. Unfortunately, we have three different expressions that take different input, hence the CRTP and the passing of arguments.
> > > 
> > > 
> > > > In the three messages below, you also showcase three
> > > > distinct ways of serializing them, caused by this redundancy (I
> > > > understand it is partially given by the payload of the messages).
> > > 
> > > It is *entirely* given by the payload, which I assume is a given, since it’s in the protocol. What else could come into play?
> > 
> > What I mean is that the place you create the data for serialization is
> > only partially given by the payload. Because for example with the
> > FormatMessage, you place the data in the class as a member, but you
> > could have also created them on the stack at the beginning of the
> > write() method. And unless I'm missing something, you could do it this
> > way for all the cases, therefore unify the approach. For the
> > FrameMessage, you are constructing an empty msg member…
> 
> I tell you that I don’t want to copy, say, 70K of frame data if I can avoid it, and you are suggesting I allocate 70K of stack instead? I think you did miss something, yes.

No, I am not telling you that.

> (if that clarifies, that frame is given to me by the capture, I did not ask the capture to use some buffer I pre-allocated)
> 
> > 
> > > > 
> > > > See comments under each class, which all relate to this (hope it's not
> > > > too confusing).
> > > > 
> > > > > +
> > > > > +    StreamDevHeader hdr;
> > > > > +    Payload         msg;
> > > > > +};
> > > > > +
> > > > > +
> > > > > +struct FormatMessage : Message<StreamMsgFormat, FormatMessage>
> > > > > +{
> > > > > +    FormatMessage(unsigned w, unsigned h, uint8_t c) : Message(w, h, c) {}
> > > > > +    static const StreamMsgType type = STREAM_TYPE_FORMAT;
> > > > 
> > > > Could the type actually be part of the template? As in:
> > > > struct FormatMessage : Message<STREAM_TYPE_FORMAT, StreamMsgFormat, FormatMessage>
> > > 
> > > Sure, but I find it more consistent the way I wrote it. Also easier to read, because you have “type = TYPE” instead of just <TYPE>, but that’s really a personal preference.
> > > 
> > > > 
> > > > > +    static size_t size(unsigned width, unsigned height, uint8_t codec)
> > > > > +    {
> > > > > +        return sizeof(FormatMessage);
> > > > > +    }
> > > > > +    static StreamMsgFormat make(unsigned w, unsigned h, uint8_t c)
> > > > > +    {
> > > > > +        return StreamMsgFormat{ .width = w, .height = h, .codec = c, .padding1 = {} };
> > > > > +    }
> > > > > +    size_t write(Stream &stream, unsigned w, unsigned h, uint8_t c)
> > > > > +    {
> > > > > +        return stream.write_all(this, sizeof(*this));
> > > > > +    }
> > > > > +};
> > > > 
> > > > This message has static size, so you construct it in make() and then
> > > > write this whole object.
> > > 
> > > The message body has static size, yes. (Header is fixed size in all three cases).
> > > 
> > > > 
> > > > > +
> > > > > +
> > > > > +struct FrameMessage : Message<StreamMsgData, FrameMessage>
> > > > > +{
> > > > > +    FrameMessage(const void *frame, size_t length) : Message(frame, length) {}
> > > > > +    static const StreamMsgType type = STREAM_TYPE_DATA;
> > > > > +    static size_t size(const void *frame, size_t length)
> > > > > +    {
> > > > > +        return sizeof(FrameMessage) + length;
> > > > > +    }
> > > > > +    static StreamMsgData make(const void *frame, size_t length)
> > > > > +    {
> > > > > +        return StreamMsgData();
> > > > > +    }
> > > > > +    size_t write(Stream &stream, const void *frame, size_t length)
> > > > > +    {
> > > > > +        return stream.write_all(&hdr, sizeof(hdr)) + stream.write_all(frame, length);
> > > > > +    }
> > > > > +};
> > > > 
> > > > Here the message is actually only the frame data and so you construct
> > > > an empty message in make(). In write() you just write the stream data
> > > > passed to it.
> > > 
> > > Yes.
> > > 
> > > > 
> > > > > +
> > > > > +struct X11CursorMessage : Message<StreamMsgCursorSet, X11CursorMessage>
> > > > > +{
> > > > > +    X11CursorMessage(XFixesCursorImage *cursor)
> > > > > +        : Message(cursor),
> > > > > +          pixels(new uint32_t[pixel_size(cursor)])
> > > > > +    {
> > > > > +        fill_pixels(cursor);
> > > > > +    }
> > > > > +    static const StreamMsgType type = STREAM_TYPE_CURSOR_SET;
> > > > > +    static size_t pixel_size(XFixesCursorImage *cursor)
> > > > > +    {
> > > > > +        return cursor->width * cursor->height;
> > > > > +    }
> > > > > +    static size_t size(XFixesCursorImage *cursor)
> > > > > +    {
> > > > > +        return sizeof(X11CursorMessage) + sizeof(uint32_t) * pixel_size(cursor);
> > > > > +    }
> > > > > +    static StreamMsgCursorSet make(XFixesCursorImage *cursor)
> > > > > +    {
> > > > > +        return StreamMsgCursorSet
> > > > > +        {
> > > > > +            .width = cursor->width,
> > > > > +            .height = cursor->height,
> > > > > +            .hot_spot_x = cursor->xhot,
> > > > > +            .hot_spot_y = cursor->yhot,
> > > > > +            .type = SPICE_CURSOR_TYPE_ALPHA,
> > > > > +            .padding1 = { },
> > > > > +            .data = { }
> > > > > +        };
> > > > > +    }
> > > > > +    size_t write(Stream &stream, XFixesCursorImage *cursor)
> > > > > +    {
> > > > > +        return stream.write_all(&hdr, sizeof(hdr)) + stream.write_all(pixels.get(), hdr.size);
> > > > 
> > > > You are not writing the msg data here, might be the reson for the
> > > > missing cursor.
> > > 
> > > Ah, good catch. I thought of not writing *this because of the extra pixels field, but then wrote the wrong class.
> > > 
> > > > 
> > > > > +    }
> > > > > +    void fill_pixels(XFixesCursorImage *cursor)
> > > > > +    {
> > > > > +        uint32_t *pixbuf = pixels.get();
> > > > > +        unsigned count = cursor->width * cursor->height;
> > > > > +        for (unsigned i = 0; i < count; ++i)
> > > > > +            pixbuf[i] = cursor->pixels[i];
> > > > > +    }
> > > > > +private:
> > > > > +    std::unique_ptr<uint32_t[]> pixels;
> > > > > +};
> > > > 
> > > > (note in this class some methods could be private and some arguments
> > > > const)
> > > 
> > > Yes. Good idea.
> > > 
> > > > 
> > > > Here you add a private member pixels, which you fill in constructor. In
> > > > make(), you create the static part of the message. In write(), you
> > > > write them.
> > > > 
> > > > So, I ask, could you not actually construct all the data to write in
> > > > write(), and perhaps even remove the Message::msg member and the make()
> > > > method?
> > > 
> > > Yes, but that would require to copy the frames data, which I considered an expensive-enough operation to try and avoid it. For cursor pixels, it’s less of an issue a) because we need to copy anyway, due to format conversion, and they happen much less frequently.
> > 
> > I don't think you would need to copy the data? What I propose actually
> > doesn't mean any change for this method in particular?
> > 
> > > > 
> > > > That would make the classes a bit simpler?
> > > 
> > > Simpler, yes but much less efficient in the important case, which is sending frames, where it would add one malloc / copy / free for each frame, which the present code avoids at the “mere” cost of passing the payload args around ;-)
> > > 
> > > 
> > > Thanks a lot.
> > > 
> > > > 
> > > > Cheers,
> > > > Lukas
> > > > 
> > > > > +
> > > > > }} // namespace spice::streaming_agent
> > > > > 
> > > > > 
> > > > > @@ -201,66 +290,6 @@ size_t Stream::write_all(const void *buf, const size_t len)
> > > > >    return written;
> > > > > }
> > > > > 
> > > > > -int Stream::send_format(unsigned w, unsigned h, uint8_t c)
> > > > > -{
> > > > > -    const size_t msgsize = sizeof(FormatMessage);
> > > > > -    const size_t hdrsize  = sizeof(StreamDevHeader);
> > > > > -    FormatMessage 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(mutex);
> > > > > -    if (write_all(&msg, msgsize) != msgsize) {
> > > > > -        return EXIT_FAILURE;
> > > > > -    }
> > > > > -    return EXIT_SUCCESS;
> > > > > -}
> > > > > -
> > > > > -int Stream::send_frame(const void *buf, const unsigned size)
> > > > > -{
> > > > > -    ssize_t n;
> > > > > -    const size_t msgsize = sizeof(FormatMessage);
> > > > > -    DataMessage 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 = {}
> > > > > -    };
> > > > > -
> > > > > -    std::lock_guard<std::mutex> stream_guard(mutex);
> > > > > -    n = write_all(&msg, msgsize);
> > > > > -    syslog(LOG_DEBUG,
> > > > > -           "wrote %ld bytes of header of data msg with frame of size %u bytes\n",
> > > > > -           n, msg.hdr.size);
> > > > > -    if (n != msgsize) {
> > > > > -        syslog(LOG_WARNING, "write_all header: wrote %ld expected %lu\n",
> > > > > -               n, msgsize);
> > > > > -        return EXIT_FAILURE;
> > > > > -    }
> > > > > -    n = write_all(buf, size);
> > > > > -    syslog(LOG_DEBUG, "wrote data msg body of size %ld\n", n);
> > > > > -    if (n != size) {
> > > > > -        syslog(LOG_WARNING, "write_all header: wrote %ld expected %u\n",
> > > > > -               n, size);
> > > > > -        return EXIT_FAILURE;
> > > > > -    }
> > > > > -    return EXIT_SUCCESS;
> > > > > -}
> > > > > -
> > > > > /* returns current time in micro-seconds */
> > > > > static uint64_t get_time(void)
> > > > > {
> > > > > @@ -304,47 +333,6 @@ static void usage(const char *progname)
> > > > >    exit(1);
> > > > > }
> > > > > 
> > > > > -void
> > > > > -Stream::send_cursor(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;
> > > > > -
> > > > > -    const uint32_t cursor_msgsize =
> > > > > -        sizeof(CursorMessage) + width * height * sizeof(uint32_t);
> > > > > -    const uint32_t hdrsize  = sizeof(StreamDevHeader);
> > > > > -
> > > > > -    std::unique_ptr<uint8_t[]> storage(new uint8_t[cursor_msgsize]);
> > > > > -
> > > > > -    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 = cursor_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->msg.data);
> > > > > -    fill_cursor(pixels);
> > > > > -
> > > > > -    std::lock_guard<std::mutex> stream_guard(mutex);
> > > > > -    write_all(storage.get(), cursor_msgsize);
> > > > > -}
> > > > > -
> > > > > static void cursor_changes(Stream *stream, Display *display, int event_base)
> > > > > {
> > > > >    unsigned long last_serial = 0;
> > > > > @@ -363,12 +351,8 @@ static void cursor_changes(Stream *stream, Display *display, int event_base)
> > > > >            continue;
> > > > > 
> > > > >        last_serial = cursor->cursor_serial;
> > > > > -        auto fill_cursor = [cursor](uint32_t *pixels) {
> > > > > -            for (unsigned i = 0; i < cursor->width * cursor->height; ++i)
> > > > > -                pixels[i] = cursor->pixels[i];
> > > > > -        };
> > > > > -        stream->send_cursor(cursor->width, cursor->height,
> > > > > -                            cursor->xhot, cursor->yhot, fill_cursor);
> > > > > +        if (!stream->send<X11CursorMessage>(cursor))
> > > > > +            syslog(LOG_WARNING, "FAILED to send cursor\n");
> > > > >    }
> > > > > }
> > > > > 
> > > > > @@ -422,7 +406,7 @@ do_capture(Stream &stream, const char *streamport, FILE *f_log)
> > > > > 
> > > > >                syslog(LOG_DEBUG, "wXh %uX%u  codec=%u\n", width, height, codec);
> > > > > 
> > > > > -                if (stream.send_format(width, height, codec) == EXIT_FAILURE)
> > > > > +                if (!stream.send<FormatMessage>(width, height, codec))
> > > > >                    throw std::runtime_error("FAILED to send format message");
> > > > >            }
> > > > >            if (f_log) {
> > > > > @@ -434,7 +418,7 @@ do_capture(Stream &stream, const char *streamport, FILE *f_log)
> > > > >                    hexdump(frame.buffer, frame.buffer_size, f_log);
> > > > >                }
> > > > >            }
> > > > > -            if (stream.send_frame(frame.buffer, frame.buffer_size) == EXIT_FAILURE) {
> > > > > +            if (!stream.send<FrameMessage>(frame.buffer, frame.buffer_size)) {
> > > > >                syslog(LOG_ERR, "FAILED to send a frame\n");
> > > > >                break;
> > > > >            }
> > > > 
> > > > _______________________________________________
> > > > Spice-devel mailing list
> > > > Spice-devel@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> > > 
> > > 
> > 
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
>
> On 22 Feb 2018, at 19:05, Lukáš Hrázký <lhrazky@redhat.com> wrote:
> 
> On Thu, 2018-02-22 at 18:51 +0100, Christophe de Dinechin wrote:
>>> On 21 Feb 2018, at 10:45, Lukáš Hrázký <lhrazky@redhat.com> wrote:
>>> 
>>> On Tue, 2018-02-20 at 16:41 +0100, Christophe de Dinechin wrote:
>>>>> On 20 Feb 2018, at 15:29, Lukáš Hrázký <lhrazky@redhat.com> wrote:
>>>>> 
>>>>> On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
>>>>>> From: Christophe de Dinechin <dinechin@redhat.com>
>>>>>> 
>>>>>> - The Stream class now deals with locking and sending messages
>>>>>> - The Message<> template class deals with the general writing mechanisms
>>>>>> - Three classes, FormatMessage, FrameMessage and X11CursorMessage represent individual messages
>>>>>> 
>>>>>> The various classes should be moved to separate headers in a subsequent operation
>>>>>> 
>>>>>> The design uses templates to avoid any runtime overhead. All the calls are static.
>>>>> 
>>>>> All in all, a nice way to encapsulate the sending of messages. What I
>>>>> worked on, actually, was the receiving of messages, as that is actually
>>>>> more important for separating the code (as seen later in the problem
>>>>> you had with client_codecs and streaming_requested static variables).
>>>>> 
>>>>> I am now wondering how to merge it with your changes and whether the
>>>>> same Message class hierarchy could be used (without making a mess out
>>>>> of it). We should discuss it later.
>>>> 
>>>> Do you have your WIP stuff in a private branch I could look at? Maybe I can help with the rebasing.
>>> 
>>> I'll push it somewhere so you can have a look, but I can manage the
>>> rebase myself :) It would still be an effort to find out what you did
>>> during the rebase.
>>> 
>>>> I would probably keep input and output messages in separate classes. I can’t see much commonality between the two. Using the same CRTP for input messages, and maybe rename Message as OutputMessage…
>>> 
>>> Agreed, probably...
>>> 
>>>>> 
>>>>>> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
>>>>>> ---
>>>>>> src/spice-streaming-agent.cpp | 250 ++++++++++++++++++++----------------------
>>>>>> 1 file changed, 117 insertions(+), 133 deletions(-)
>>>>>> 
>>>>>> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
>>>>>> index a989ee7..c174ea4 100644
>>>>>> --- a/src/spice-streaming-agent.cpp
>>>>>> +++ b/src/spice-streaming-agent.cpp
>>>>>> @@ -48,24 +48,6 @@ namespace spice
>>>>>> namespace streaming_agent
>>>>>> {
>>>>>> 
>>>>>> -struct FormatMessage
>>>>>> -{
>>>>>> -    StreamDevHeader hdr;
>>>>>> -    StreamMsgFormat msg;
>>>>>> -};
>>>>>> -
>>>>>> -struct DataMessage
>>>>>> -{
>>>>>> -    StreamDevHeader hdr;
>>>>>> -    StreamMsgData msg;
>>>>>> -};
>>>>>> -
>>>>>> -struct CursorMessage
>>>>>> -{
>>>>>> -    StreamDevHeader hdr;
>>>>>> -    StreamMsgCursorSet msg;
>>>>>> -};
>>>>>> -
>>>>>> class Stream
>>>>>> {
>>>>>> public:
>>>>>> @@ -79,24 +61,131 @@ public:
>>>>>>   {
>>>>>>       close(streamfd);
>>>>>>   }
>>>>>> -    operator int() { return streamfd; }
>>>>>> 
>>>>>>   int have_something_to_read(int timeout);
>>>>>>   int read_command_from_device(void);
>>>>>>   int read_command(bool blocking);
>>>>>> 
>>>>>> +
>>>>>> +    template <typename Message, typename ...PayloadArgs>
>>>>>> +    bool send(PayloadArgs... payload)
>>>>>> +    {
>>>>>> +        Message message(payload...);
>>>>>> +        std::lock_guard<std::mutex> stream_guard(mutex);
>>>>>> +        size_t expected = message.size(payload...);
>>>>>> +        size_t written = message.write(*this, payload...);
>>>>>> +        return written == expected;
>>>>>> +    }
>>>>> 
>>>>> Do you purposefully avoid throwing exceptions here, returning a bool?
>>>> 
>>>> You really love exceptions, don’t you ;-)
>>> 
>>> Well... I don't always get errors, but when I do, I use exceptions to
>>> handle them. :D
>>> 
>>> Really, that's what it is. Error = exception.
>> 
>> No. C++CG E2 "Throw an exception to signal that a function can't perform its assigned task”.
>> 
>> Examples of errors that are not exceptions: FP NaN and infinities, compiler errors (and more generally parsing errors), etc. A parser error is not an exception because it’s the job of the parser to detect the error…
>> 
>> I could also give examples of exceptions that are not errors, though it’s more subtle and OT. Consider a C++ equivalent of an interrupted system call. Also, is bad_alloc an “error" or a limitation? (one could argue that the error would be e.g. to give you some memory belonging to someone else ;-)
> 
> If you put it this way, sure.
> 
>>> That's what they teach
>>> you at the uni and what we've always done at my previous job. It's the
>>> language's designated mechanism to deal with errors, standard library
>>> uses them, ...
>>> 
>>> You brought new light into the topic for me, but I still don't know how
>>> to deal with it... Though the fact that you are the author of the
>>> exception handling implementation and you are reluctant to use the
>>> exception really speaks volumes :)
>>> 
>>>> I usually reserve exceptions for cases which are truly “catastrophic”, i.e. going the long way and unwindig is the right way to do it. Unwinding the stack is a very messy business, takes a lot of time, and if you can just return, it’s about one and a half gazillion times faster, generates smaller code, etc etc.
>>>> 
>>>> In this specific case, though, I think that unwinding could be seen as appropriate, since ATM we have nothing better than error + abort when it happens. Plus this might avoid a scenario where you could write twice if the first write fails. So I’m sold, I think this is the right thing to do.
>>>> 
>>>>> The error and exception could actually be checked as low as at the end
>>>>> of the write_all() method, avoiding all the latter size returning and
>>>>> checking?
>>>>> 
>>>>>> +
>>>>>>   size_t write_all(const void *buf, const size_t len);
>>>>>> -    int send_format(unsigned w, unsigned h, uint8_t c);
>>>>>> -    int send_frame(const void *buf, const unsigned size);
>>>>>> -    void send_cursor(uint16_t width, uint16_t height,
>>>>>> -                     uint16_t hotspot_x, uint16_t hotspot_y,
>>>>>> -                     std::function<void(uint32_t *)> fill_cursor);
>>>>>> 
>>>>>> private:
>>>>>>   int streamfd = -1;
>>>>>>   std::mutex mutex;
>>>>>> };
>>>>>> 
>>>>>> +
>>>>>> +template <typename Payload, typename Info>
>>>>> 
>>>>> "Info" is quite an unimaginative name :) I understand it's the message
>>>>> itself here and also find it curiously hard to come up with something
>>>>> better :) "TheMessage" not very good, is it? Still better than "Info"?
>>>>> Maybe a comment referencing the CRTP to help readers understand?
>>>> 
>>>> I used ‘Info’ here because that’s the information about the message.
>>> 
>>> I kind of understood that, but still... very vague for me…
>> 
>> Will stay that way for the moment, unless you give me a really better name, sorry.
>> 
>>> 
>>>> I can add the CRTP comment, could not remember the acronym when I wrote the class ;-)
>>>> 
>>>>> 
>>>>>> +struct Message
>>>>> 
>>>>> Any reason to not stick with the "class" keyword instead of "struct”?
>>>> 
>>>> Not really, more a matter of habit, using struct when it’s really about data and everything is public. But there, “Message” evolved far away from POD to warrant using ‘class’.
>>> 
>>> I do like to take the shortcut using struct to spare myself the 'public:' line at the top, but I'd think it would be considered an inconsistency by others... :)
>> 
>> That’s not the reason. I tend to use it to mark a struct as “mostly data”, even if it has C++isms in it. Here, I initially saw the message is mostly data, i.e. I would not have minded the data members being public. But that’s no longer the case. Will change it.
>> 
>>> 
>>>>> 
>>>>>> +{
>>>>>> +    template <typename ...PayloadArgs>
>>>>>> +    Message(PayloadArgs... payload)
>>>>> 
>>>>> "PayloadArgs... payload_args", we have something called "Payload" here
>>>>> too, getting a bit confusing.
>>>> 
>>>> But that’s the same thing. Payload is initialized with PayloadArgs, so it makes sense.
>>>> 
>>>> Initially, I had “PayloadConstructorArgs”, but then I started using it outside of the ctor.
>>> 
>>> I don't think it's the same, here Payload is the message class and
>>> PayloadArgs is the content/args. So naming the objects accordingly IMO
>>> helps clarity.
>> 
>> The Payload is defined from the PayloadArgs, i.e. its constructor is Payload(PayloadArgs…). What is the problem with that?
> 
> The problem is Payload(PayloadArgs... payload).

What is the problem with initializing something called a payload with something called payload args?

I’m sorry for being dense here, but I really don’t get your objection.

> 
>>> 
>>>>> 
>>>>>> +        : hdr(StreamDevHeader {
>>>>>> +              .protocol_version = STREAM_DEVICE_PROTOCOL,
>>>>>> +              .padding = 0,     // Workaround GCC bug "sorry: not implemented"
>>>>>> +              .type = Info::type,
>>>>>> +              .size = (uint32_t) (Info::size(payload...) - sizeof(hdr))
>>>>>> +          }),
>>>>>> +          msg(Info::make(payload...))
>>>>>> +    { }
>>>>> 
>>>>> I find it redundant that you pass the "payload..." (the args :)) to
>>>>> Info::size() and Info::make() here and then also to Info::write() in
>>>>> Stream::send().
>>>> 
>>>> I’m not sure “redundant” is the correct word. I could stash the information in Message, but then the compiler would have a much harder time inlining the actual expression, which is always very simple. Unfortunately, we have three different expressions that take different input, hence the CRTP and the passing of arguments.
>>>> 
>>>> 
>>>>> In the three messages below, you also showcase three
>>>>> distinct ways of serializing them, caused by this redundancy (I
>>>>> understand it is partially given by the payload of the messages).
>>>> 
>>>> It is *entirely* given by the payload, which I assume is a given, since it’s in the protocol. What else could come into play?
>>> 
>>> What I mean is that the place you create the data for serialization is
>>> only partially given by the payload. Because for example with the
>>> FormatMessage, you place the data in the class as a member, but you
>>> could have also created them on the stack at the beginning of the
>>> write() method. And unless I'm missing something, you could do it this
>>> way for all the cases, therefore unify the approach. For the
>>> FrameMessage, you are constructing an empty msg member…
>> 
>> I tell you that I don’t want to copy, say, 70K of frame data if I can avoid it, and you are suggesting I allocate 70K of stack instead? I think you did miss something, yes.
> 
> No, I am not telling you that.

Then, I have not understood what you mean. What do you mean with “created them on the stack at the beginning of the write function”? What is “them”? The format message?

Is your suggestion that FormatMessage could be split in two writes like the others, and that this would make the code more generic (if a tiny bit less efficient for that specific message)? If that’s what you are saying, then the problem is that the second part is distinct in all three cases anyway, so I’m not getting anything out of this, but I’m making the FormatMessage case less efficient…

Sorry, I read and re-read your explanations, I’m afraid I still don’t get it. Maybe with a code outline?


> 
>> (if that clarifies, that frame is given to me by the capture, I did not ask the capture to use some buffer I pre-allocated)
>> 
>>> 
>>>>> 
>>>>> See comments under each class, which all relate to this (hope it's not
>>>>> too confusing).
>>>>> 
>>>>>> +
>>>>>> +    StreamDevHeader hdr;
>>>>>> +    Payload         msg;
>>>>>> +};
>>>>>> +
>>>>>> +
>>>>>> +struct FormatMessage : Message<StreamMsgFormat, FormatMessage>
>>>>>> +{
>>>>>> +    FormatMessage(unsigned w, unsigned h, uint8_t c) : Message(w, h, c) {}
>>>>>> +    static const StreamMsgType type = STREAM_TYPE_FORMAT;
>>>>> 
>>>>> Could the type actually be part of the template? As in:
>>>>> struct FormatMessage : Message<STREAM_TYPE_FORMAT, StreamMsgFormat, FormatMessage>
>>>> 
>>>> Sure, but I find it more consistent the way I wrote it. Also easier to read, because you have “type = TYPE” instead of just <TYPE>, but that’s really a personal preference.
>>>> 
>>>>> 
>>>>>> +    static size_t size(unsigned width, unsigned height, uint8_t codec)
>>>>>> +    {
>>>>>> +        return sizeof(FormatMessage);
>>>>>> +    }
>>>>>> +    static StreamMsgFormat make(unsigned w, unsigned h, uint8_t c)
>>>>>> +    {
>>>>>> +        return StreamMsgFormat{ .width = w, .height = h, .codec = c, .padding1 = {} };
>>>>>> +    }
>>>>>> +    size_t write(Stream &stream, unsigned w, unsigned h, uint8_t c)
>>>>>> +    {
>>>>>> +        return stream.write_all(this, sizeof(*this));
>>>>>> +    }
>>>>>> +};
>>>>> 
>>>>> This message has static size, so you construct it in make() and then
>>>>> write this whole object.
>>>> 
>>>> The message body has static size, yes. (Header is fixed size in all three cases).
>>>> 
>>>>> 
>>>>>> +
>>>>>> +
>>>>>> +struct FrameMessage : Message<StreamMsgData, FrameMessage>
>>>>>> +{
>>>>>> +    FrameMessage(const void *frame, size_t length) : Message(frame, length) {}
>>>>>> +    static const StreamMsgType type = STREAM_TYPE_DATA;
>>>>>> +    static size_t size(const void *frame, size_t length)
>>>>>> +    {
>>>>>> +        return sizeof(FrameMessage) + length;
>>>>>> +    }
>>>>>> +    static StreamMsgData make(const void *frame, size_t length)
>>>>>> +    {
>>>>>> +        return StreamMsgData();
>>>>>> +    }
>>>>>> +    size_t write(Stream &stream, const void *frame, size_t length)
>>>>>> +    {
>>>>>> +        return stream.write_all(&hdr, sizeof(hdr)) + stream.write_all(frame, length);
>>>>>> +    }
>>>>>> +};
>>>>> 
>>>>> Here the message is actually only the frame data and so you construct
>>>>> an empty message in make(). In write() you just write the stream data
>>>>> passed to it.
>>>> 
>>>> Yes.
>>>> 
>>>>> 
>>>>>> +
>>>>>> +struct X11CursorMessage : Message<StreamMsgCursorSet, X11CursorMessage>
>>>>>> +{
>>>>>> +    X11CursorMessage(XFixesCursorImage *cursor)
>>>>>> +        : Message(cursor),
>>>>>> +          pixels(new uint32_t[pixel_size(cursor)])
>>>>>> +    {
>>>>>> +        fill_pixels(cursor);
>>>>>> +    }
>>>>>> +    static const StreamMsgType type = STREAM_TYPE_CURSOR_SET;
>>>>>> +    static size_t pixel_size(XFixesCursorImage *cursor)
>>>>>> +    {
>>>>>> +        return cursor->width * cursor->height;
>>>>>> +    }
>>>>>> +    static size_t size(XFixesCursorImage *cursor)
>>>>>> +    {
>>>>>> +        return sizeof(X11CursorMessage) + sizeof(uint32_t) * pixel_size(cursor);
>>>>>> +    }
>>>>>> +    static StreamMsgCursorSet make(XFixesCursorImage *cursor)
>>>>>> +    {
>>>>>> +        return StreamMsgCursorSet
>>>>>> +        {
>>>>>> +            .width = cursor->width,
>>>>>> +            .height = cursor->height,
>>>>>> +            .hot_spot_x = cursor->xhot,
>>>>>> +            .hot_spot_y = cursor->yhot,
>>>>>> +            .type = SPICE_CURSOR_TYPE_ALPHA,
>>>>>> +            .padding1 = { },
>>>>>> +            .data = { }
>>>>>> +        };
>>>>>> +    }
>>>>>> +    size_t write(Stream &stream, XFixesCursorImage *cursor)
>>>>>> +    {
>>>>>> +        return stream.write_all(&hdr, sizeof(hdr)) + stream.write_all(pixels.get(), hdr.size);
>>>>> 
>>>>> You are not writing the msg data here, might be the reson for the
>>>>> missing cursor.
>>>> 
>>>> Ah, good catch. I thought of not writing *this because of the extra pixels field, but then wrote the wrong class.
>>>> 
>>>>> 
>>>>>> +    }
>>>>>> +    void fill_pixels(XFixesCursorImage *cursor)
>>>>>> +    {
>>>>>> +        uint32_t *pixbuf = pixels.get();
>>>>>> +        unsigned count = cursor->width * cursor->height;
>>>>>> +        for (unsigned i = 0; i < count; ++i)
>>>>>> +            pixbuf[i] = cursor->pixels[i];
>>>>>> +    }
>>>>>> +private:
>>>>>> +    std::unique_ptr<uint32_t[]> pixels;
>>>>>> +};
>>>>> 
>>>>> (note in this class some methods could be private and some arguments
>>>>> const)
>>>> 
>>>> Yes. Good idea.
>>>> 
>>>>> 
>>>>> Here you add a private member pixels, which you fill in constructor. In
>>>>> make(), you create the static part of the message. In write(), you
>>>>> write them.
>>>>> 
>>>>> So, I ask, could you not actually construct all the data to write in
>>>>> write(), and perhaps even remove the Message::msg member and the make()
>>>>> method?
>>>> 
>>>> Yes, but that would require to copy the frames data, which I considered an expensive-enough operation to try and avoid it. For cursor pixels, it’s less of an issue a) because we need to copy anyway, due to format conversion, and they happen much less frequently.
>>> 
>>> I don't think you would need to copy the data? What I propose actually
>>> doesn't mean any change for this method in particular?
>>> 
>>>>> 
>>>>> That would make the classes a bit simpler?
>>>> 
>>>> Simpler, yes but much less efficient in the important case, which is sending frames, where it would add one malloc / copy / free for each frame, which the present code avoids at the “mere” cost of passing the payload args around ;-)
>>>> 
>>>> 
>>>> Thanks a lot.
>>>> 
>>>>> 
>>>>> Cheers,
>>>>> Lukas
>>>>> 
>>>>>> +
>>>>>> }} // namespace spice::streaming_agent
>>>>>> 
>>>>>> 
>>>>>> @@ -201,66 +290,6 @@ size_t Stream::write_all(const void *buf, const size_t len)
>>>>>>   return written;
>>>>>> }
>>>>>> 
>>>>>> -int Stream::send_format(unsigned w, unsigned h, uint8_t c)
>>>>>> -{
>>>>>> -    const size_t msgsize = sizeof(FormatMessage);
>>>>>> -    const size_t hdrsize  = sizeof(StreamDevHeader);
>>>>>> -    FormatMessage 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(mutex);
>>>>>> -    if (write_all(&msg, msgsize) != msgsize) {
>>>>>> -        return EXIT_FAILURE;
>>>>>> -    }
>>>>>> -    return EXIT_SUCCESS;
>>>>>> -}
>>>>>> -
>>>>>> -int Stream::send_frame(const void *buf, const unsigned size)
>>>>>> -{
>>>>>> -    ssize_t n;
>>>>>> -    const size_t msgsize = sizeof(FormatMessage);
>>>>>> -    DataMessage 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 = {}
>>>>>> -    };
>>>>>> -
>>>>>> -    std::lock_guard<std::mutex> stream_guard(mutex);
>>>>>> -    n = write_all(&msg, msgsize);
>>>>>> -    syslog(LOG_DEBUG,
>>>>>> -           "wrote %ld bytes of header of data msg with frame of size %u bytes\n",
>>>>>> -           n, msg.hdr.size);
>>>>>> -    if (n != msgsize) {
>>>>>> -        syslog(LOG_WARNING, "write_all header: wrote %ld expected %lu\n",
>>>>>> -               n, msgsize);
>>>>>> -        return EXIT_FAILURE;
>>>>>> -    }
>>>>>> -    n = write_all(buf, size);
>>>>>> -    syslog(LOG_DEBUG, "wrote data msg body of size %ld\n", n);
>>>>>> -    if (n != size) {
>>>>>> -        syslog(LOG_WARNING, "write_all header: wrote %ld expected %u\n",
>>>>>> -               n, size);
>>>>>> -        return EXIT_FAILURE;
>>>>>> -    }
>>>>>> -    return EXIT_SUCCESS;
>>>>>> -}
>>>>>> -
>>>>>> /* returns current time in micro-seconds */
>>>>>> static uint64_t get_time(void)
>>>>>> {
>>>>>> @@ -304,47 +333,6 @@ static void usage(const char *progname)
>>>>>>   exit(1);
>>>>>> }
>>>>>> 
>>>>>> -void
>>>>>> -Stream::send_cursor(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;
>>>>>> -
>>>>>> -    const uint32_t cursor_msgsize =
>>>>>> -        sizeof(CursorMessage) + width * height * sizeof(uint32_t);
>>>>>> -    const uint32_t hdrsize  = sizeof(StreamDevHeader);
>>>>>> -
>>>>>> -    std::unique_ptr<uint8_t[]> storage(new uint8_t[cursor_msgsize]);
>>>>>> -
>>>>>> -    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 = cursor_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->msg.data);
>>>>>> -    fill_cursor(pixels);
>>>>>> -
>>>>>> -    std::lock_guard<std::mutex> stream_guard(mutex);
>>>>>> -    write_all(storage.get(), cursor_msgsize);
>>>>>> -}
>>>>>> -
>>>>>> static void cursor_changes(Stream *stream, Display *display, int event_base)
>>>>>> {
>>>>>>   unsigned long last_serial = 0;
>>>>>> @@ -363,12 +351,8 @@ static void cursor_changes(Stream *stream, Display *display, int event_base)
>>>>>>           continue;
>>>>>> 
>>>>>>       last_serial = cursor->cursor_serial;
>>>>>> -        auto fill_cursor = [cursor](uint32_t *pixels) {
>>>>>> -            for (unsigned i = 0; i < cursor->width * cursor->height; ++i)
>>>>>> -                pixels[i] = cursor->pixels[i];
>>>>>> -        };
>>>>>> -        stream->send_cursor(cursor->width, cursor->height,
>>>>>> -                            cursor->xhot, cursor->yhot, fill_cursor);
>>>>>> +        if (!stream->send<X11CursorMessage>(cursor))
>>>>>> +            syslog(LOG_WARNING, "FAILED to send cursor\n");
>>>>>>   }
>>>>>> }
>>>>>> 
>>>>>> @@ -422,7 +406,7 @@ do_capture(Stream &stream, const char *streamport, FILE *f_log)
>>>>>> 
>>>>>>               syslog(LOG_DEBUG, "wXh %uX%u  codec=%u\n", width, height, codec);
>>>>>> 
>>>>>> -                if (stream.send_format(width, height, codec) == EXIT_FAILURE)
>>>>>> +                if (!stream.send<FormatMessage>(width, height, codec))
>>>>>>                   throw std::runtime_error("FAILED to send format message");
>>>>>>           }
>>>>>>           if (f_log) {
>>>>>> @@ -434,7 +418,7 @@ do_capture(Stream &stream, const char *streamport, FILE *f_log)
>>>>>>                   hexdump(frame.buffer, frame.buffer_size, f_log);
>>>>>>               }
>>>>>>           }
>>>>>> -            if (stream.send_frame(frame.buffer, frame.buffer_size) == EXIT_FAILURE) {
>>>>>> +            if (!stream.send<FrameMessage>(frame.buffer, frame.buffer_size)) {
>>>>>>               syslog(LOG_ERR, "FAILED to send a frame\n");
>>>>>>               break;
>>>>>>           }
>>>>> 
>>>>> _______________________________________________
>>>>> Spice-devel mailing list
>>>>> Spice-devel@lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>>>> 
>>>> 
>>> 
>>> _______________________________________________
>>> Spice-devel mailing list
>>> Spice-devel@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/spice-devel
On Fri, 2018-02-23 at 12:36 +0100, Christophe de Dinechin wrote:
> > On 22 Feb 2018, at 19:05, Lukáš Hrázký <lhrazky@redhat.com> wrote:
> > 
> > On Thu, 2018-02-22 at 18:51 +0100, Christophe de Dinechin wrote:
> > > > On 21 Feb 2018, at 10:45, Lukáš Hrázký <lhrazky@redhat.com>
> > > > wrote:
> > > > 
> > > > On Tue, 2018-02-20 at 16:41 +0100, Christophe de Dinechin
> > > > wrote:
> > > > > > On 20 Feb 2018, at 15:29, Lukáš Hrázký <lhrazky@redhat.com>
> > > > > > wrote:
> > > > > > 
> > > > > > On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin
> > > > > > wrote:
> > > > > > > From: Christophe de Dinechin <dinechin@redhat.com>
> > > > > > > 
> > > > > > > - The Stream class now deals with locking and sending
> > > > > > > messages
> > > > > > > - The Message<> template class deals with the general
> > > > > > > writing mechanisms
> > > > > > > - Three classes, FormatMessage, FrameMessage and
> > > > > > > X11CursorMessage represent individual messages
> > > > > > > 
> > > > > > > The various classes should be moved to separate headers
> > > > > > > in a subsequent operation
> > > > > > > 
> > > > > > > The design uses templates to avoid any runtime overhead.
> > > > > > > All the calls are static.
> > > > > > 
> > > > > > All in all, a nice way to encapsulate the sending of
> > > > > > messages. What I
> > > > > > worked on, actually, was the receiving of messages, as that
> > > > > > is actually
> > > > > > more important for separating the code (as seen later in
> > > > > > the problem
> > > > > > you had with client_codecs and streaming_requested static
> > > > > > variables).
> > > > > > 
> > > > > > I am now wondering how to merge it with your changes and
> > > > > > whether the
> > > > > > same Message class hierarchy could be used (without making
> > > > > > a mess out
> > > > > > of it). We should discuss it later.
> > > > > 
> > > > > Do you have your WIP stuff in a private branch I could look
> > > > > at? Maybe I can help with the rebasing.
> > > > 
> > > > I'll push it somewhere so you can have a look, but I can manage
> > > > the
> > > > rebase myself :) It would still be an effort to find out what
> > > > you did
> > > > during the rebase.
> > > > 
> > > > > I would probably keep input and output messages in separate
> > > > > classes. I can’t see much commonality between the two. Using
> > > > > the same CRTP for input messages, and maybe rename Message as
> > > > > OutputMessage…
> > > > 
> > > > Agreed, probably...
> > > > 
> > > > > > 
> > > > > > > Signed-off-by: Christophe de Dinechin <dinechin@redhat.co
> > > > > > > m>
> > > > > > > ---
> > > > > > > src/spice-streaming-agent.cpp | 250 ++++++++++++++++++++-
> > > > > > > ---------------------
> > > > > > > 1 file changed, 117 insertions(+), 133 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/src/spice-streaming-agent.cpp b/src/spice-
> > > > > > > streaming-agent.cpp
> > > > > > > index a989ee7..c174ea4 100644
> > > > > > > --- a/src/spice-streaming-agent.cpp
> > > > > > > +++ b/src/spice-streaming-agent.cpp
> > > > > > > @@ -48,24 +48,6 @@ namespace spice
> > > > > > > namespace streaming_agent
> > > > > > > {
> > > > > > > 
> > > > > > > -struct FormatMessage
> > > > > > > -{
> > > > > > > -    StreamDevHeader hdr;
> > > > > > > -    StreamMsgFormat msg;
> > > > > > > -};
> > > > > > > -
> > > > > > > -struct DataMessage
> > > > > > > -{
> > > > > > > -    StreamDevHeader hdr;
> > > > > > > -    StreamMsgData msg;
> > > > > > > -};
> > > > > > > -
> > > > > > > -struct CursorMessage
> > > > > > > -{
> > > > > > > -    StreamDevHeader hdr;
> > > > > > > -    StreamMsgCursorSet msg;
> > > > > > > -};
> > > > > > > -
> > > > > > > class Stream
> > > > > > > {
> > > > > > > public:
> > > > > > > @@ -79,24 +61,131 @@ public:
> > > > > > >   {
> > > > > > >       close(streamfd);
> > > > > > >   }
> > > > > > > -    operator int() { return streamfd; }
> > > > > > > 
> > > > > > >   int have_something_to_read(int timeout);
> > > > > > >   int read_command_from_device(void);
> > > > > > >   int read_command(bool blocking);
> > > > > > > 
> > > > > > > +
> > > > > > > +    template <typename Message, typename ...PayloadArgs>
> > > > > > > +    bool send(PayloadArgs... payload)
> > > > > > > +    {
> > > > > > > +        Message message(payload...);
> > > > > > > +        std::lock_guard<std::mutex> stream_guard(mutex);
> > > > > > > +        size_t expected = message.size(payload...);
> > > > > > > +        size_t written = message.write(*this,
> > > > > > > payload...);
> > > > > > > +        return written == expected;
> > > > > > > +    }
> > > > > > 
> > > > > > Do you purposefully avoid throwing exceptions here,
> > > > > > returning a bool?
> > > > > 
> > > > > You really love exceptions, don’t you ;-)
> > > > 
> > > > Well... I don't always get errors, but when I do, I use
> > > > exceptions to
> > > > handle them. :D
> > > > 
> > > > Really, that's what it is. Error = exception.
> > > 
> > > No. C++CG E2 "Throw an exception to signal that a function can't
> > > perform its assigned task”.
> > > 
> > > Examples of errors that are not exceptions: FP NaN and
> > > infinities, compiler errors (and more generally parsing errors),
> > > etc. A parser error is not an exception because it’s the job of
> > > the parser to detect the error…
> > > 
> > > I could also give examples of exceptions that are not errors,
> > > though it’s more subtle and OT. Consider a C++ equivalent of an
> > > interrupted system call. Also, is bad_alloc an “error" or a
> > > limitation? (one could argue that the error would be e.g. to give
> > > you some memory belonging to someone else ;-)
> > 
> > If you put it this way, sure.
> > 
> > > > That's what they teach
> > > > you at the uni and what we've always done at my previous job.
> > > > It's the
> > > > language's designated mechanism to deal with errors, standard
> > > > library
> > > > uses them, ...
> > > > 
> > > > You brought new light into the topic for me, but I still don't
> > > > know how
> > > > to deal with it... Though the fact that you are the author of
> > > > the
> > > > exception handling implementation and you are reluctant to use
> > > > the
> > > > exception really speaks volumes :)
> > > > 
> > > > > I usually reserve exceptions for cases which are truly
> > > > > “catastrophic”, i.e. going the long way and unwindig is the
> > > > > right way to do it. Unwinding the stack is a very messy
> > > > > business, takes a lot of time, and if you can just return,
> > > > > it’s about one and a half gazillion times faster, generates
> > > > > smaller code, etc etc.
> > > > > 
> > > > > In this specific case, though, I think that unwinding could
> > > > > be seen as appropriate, since ATM we have nothing better than
> > > > > error + abort when it happens. Plus this might avoid a
> > > > > scenario where you could write twice if the first write
> > > > > fails. So I’m sold, I think this is the right thing to do.
> > > > > 
> > > > > > The error and exception could actually be checked as low as
> > > > > > at the end
> > > > > > of the write_all() method, avoiding all the latter size
> > > > > > returning and
> > > > > > checking?
> > > > > > 
> > > > > > > +
> > > > > > >   size_t write_all(const void *buf, const size_t len);
> > > > > > > -    int send_format(unsigned w, unsigned h, uint8_t c);
> > > > > > > -    int send_frame(const void *buf, const unsigned
> > > > > > > size);
> > > > > > > -    void send_cursor(uint16_t width, uint16_t height,
> > > > > > > -                     uint16_t hotspot_x, uint16_t
> > > > > > > hotspot_y,
> > > > > > > -                     std::function<void(uint32_t *)>
> > > > > > > fill_cursor);
> > > > > > > 
> > > > > > > private:
> > > > > > >   int streamfd = -1;
> > > > > > >   std::mutex mutex;
> > > > > > > };
> > > > > > > 
> > > > > > > +
> > > > > > > +template <typename Payload, typename Info>
> > > > > > 
> > > > > > "Info" is quite an unimaginative name :) I understand it's
> > > > > > the message
> > > > > > itself here and also find it curiously hard to come up with
> > > > > > something
> > > > > > better :) "TheMessage" not very good, is it? Still better
> > > > > > than "Info"?
> > > > > > Maybe a comment referencing the CRTP to help readers
> > > > > > understand?
> > > > > 
> > > > > I used ‘Info’ here because that’s the information about the
> > > > > message.
> > > > 
> > > > I kind of understood that, but still... very vague for me…
> > > 
> > > Will stay that way for the moment, unless you give me a really
> > > better name, sorry.
> > > 
> > > > 
> > > > > I can add the CRTP comment, could not remember the acronym
> > > > > when I wrote the class ;-)
> > > > > 
> > > > > > 
> > > > > > > +struct Message
> > > > > > 
> > > > > > Any reason to not stick with the "class" keyword instead of
> > > > > > "struct”?
> > > > > 
> > > > > Not really, more a matter of habit, using struct when it’s
> > > > > really about data and everything is public. But there,
> > > > > “Message” evolved far away from POD to warrant using ‘class’.
> > > > 
> > > > I do like to take the shortcut using struct to spare myself the
> > > > 'public:' line at the top, but I'd think it would be considered
> > > > an inconsistency by others... :)
> > > 
> > > That’s not the reason. I tend to use it to mark a struct as
> > > “mostly data”, even if it has C++isms in it. Here, I initially
> > > saw the message is mostly data, i.e. I would not have minded the
> > > data members being public. But that’s no longer the case. Will
> > > change it.
> > > 
> > > > 
> > > > > > 
> > > > > > > +{
> > > > > > > +    template <typename ...PayloadArgs>
> > > > > > > +    Message(PayloadArgs... payload)
> > > > > > 
> > > > > > "PayloadArgs... payload_args", we have something called
> > > > > > "Payload" here
> > > > > > too, getting a bit confusing.
> > > > > 
> > > > > But that’s the same thing. Payload is initialized with
> > > > > PayloadArgs, so it makes sense.
> > > > > 
> > > > > Initially, I had “PayloadConstructorArgs”, but then I started
> > > > > using it outside of the ctor.
> > > > 
> > > > I don't think it's the same, here Payload is the message class
> > > > and
> > > > PayloadArgs is the content/args. So naming the objects
> > > > accordingly IMO
> > > > helps clarity.
> > > 
> > > The Payload is defined from the PayloadArgs, i.e. its constructor
> > > is Payload(PayloadArgs…). What is the problem with that?
> > 
> > The problem is Payload(PayloadArgs... payload).
> 
> What is the problem with initializing something called a payload with
> something called payload args?
> 
> I’m sorry for being dense here, but I really don’t get your
> objection.


I think the problem is that you have a typename Payload and a typename
PayloadArgs. But you call the concrete variable of type PayloadArgs
"payload", and you call the variable of type Payload "msg". Here's a
shortened version of your code:

template <typename Payload, typename Info>
struct Message
{
    template <typename ...PayloadArgs>
    Message(PayloadArgs... payload)
...

    Payload         msg;
};


If it was instead changed to something like this, it would be less
confusing:

template <typename Payload, typename Info>
struct Message
{
    template <typename ...PayloadArgs>
    Message(PayloadArgs... payload_args) // or just "args"?
...

    Payload         msg; // or "payload"?
};


Does that make sense? I found it a bit confusing when reading your code
as well, though I'm not sure that my confusion is the same as Lukáš's


> 
> > 
> > > > 
> > > > > > 
> > > > > > > +        : hdr(StreamDevHeader {
> > > > > > > +              .protocol_version =
> > > > > > > STREAM_DEVICE_PROTOCOL,
> > > > > > > +              .padding = 0,     // Workaround GCC bug
> > > > > > > "sorry: not implemented"
> > > > > > > +              .type = Info::type,
> > > > > > > +              .size = (uint32_t) (Info::size(payload...)
> > > > > > > - sizeof(hdr))
> > > > > > > +          }),
> > > > > > > +          msg(Info::make(payload...))
> > > > > > > +    { }
> > > > > > 
> > > > > > I find it redundant that you pass the "payload..." (the
> > > > > > args :)) to
> > > > > > Info::size() and Info::make() here and then also to
> > > > > > Info::write() in
> > > > > > Stream::send().
> > > > > 
> > > > > I’m not sure “redundant” is the correct word. I could stash
> > > > > the information in Message, but then the compiler would have
> > > > > a much harder time inlining the actual expression, which is
> > > > > always very simple. Unfortunately, we have three different
> > > > > expressions that take different input, hence the CRTP and the
> > > > > passing of arguments.
> > > > > 
> > > > > 
> > > > > > In the three messages below, you also showcase three
> > > > > > distinct ways of serializing them, caused by this
> > > > > > redundancy (I
> > > > > > understand it is partially given by the payload of the
> > > > > > messages).
> > > > > 
> > > > > It is *entirely* given by the payload, which I assume is a
> > > > > given, since it’s in the protocol. What else could come into
> > > > > play?
> > > > 
> > > > What I mean is that the place you create the data for
> > > > serialization is
> > > > only partially given by the payload. Because for example with
> > > > the
> > > > FormatMessage, you place the data in the class as a member, but
> > > > you
> > > > could have also created them on the stack at the beginning of
> > > > the
> > > > write() method. And unless I'm missing something, you could do
> > > > it this
> > > > way for all the cases, therefore unify the approach. For the
> > > > FrameMessage, you are constructing an empty msg member…
> > > 
> > > I tell you that I don’t want to copy, say, 70K of frame data if I
> > > can avoid it, and you are suggesting I allocate 70K of stack
> > > instead? I think you did miss something, yes.
> > 
> > No, I am not telling you that.
> 
> Then, I have not understood what you mean. What do you mean with
> “created them on the stack at the beginning of the write function”?
> What is “them”? The format message?
> 
> Is your suggestion that FormatMessage could be split in two writes
> like the others, and that this would make the code more generic (if a
> tiny bit less efficient for that specific message)? If that’s what
> you are saying, then the problem is that the second part is distinct
> in all three cases anyway, so I’m not getting anything out of this,
> but I’m making the FormatMessage case less efficient…
> 
> Sorry, I read and re-read your explanations, I’m afraid I still don’t
> get it. Maybe with a code outline?
> 
> 
> > 
> > > (if that clarifies, that frame is given to me by the capture, I
> > > did not ask the capture to use some buffer I pre-allocated)
> > > 
> > > > 
> > > > > > 
> > > > > > See comments under each class, which all relate to this
> > > > > > (hope it's not
> > > > > > too confusing).
> > > > > > 
> > > > > > > +
> > > > > > > +    StreamDevHeader hdr;
> > > > > > > +    Payload         msg;
> > > > > > > +};
> > > > > > > +
> > > > > > > +
> > > > > > > +struct FormatMessage : Message<StreamMsgFormat,
> > > > > > > FormatMessage>
> > > > > > > +{
> > > > > > > +    FormatMessage(unsigned w, unsigned h, uint8_t c) :
> > > > > > > Message(w, h, c) {}
> > > > > > > +    static const StreamMsgType type =
> > > > > > > STREAM_TYPE_FORMAT;
> > > > > > 
> > > > > > Could the type actually be part of the template? As in:
> > > > > > struct FormatMessage : Message<STREAM_TYPE_FORMAT,
> > > > > > StreamMsgFormat, FormatMessage>
> > > > > 
> > > > > Sure, but I find it more consistent the way I wrote it. Also
> > > > > easier to read, because you have “type = TYPE” instead of
> > > > > just <TYPE>, but that’s really a personal preference.
> > > > > 
> > > > > > 
> > > > > > > +    static size_t size(unsigned width, unsigned height,
> > > > > > > uint8_t codec)
> > > > > > > +    {
> > > > > > > +        return sizeof(FormatMessage);
> > > > > > > +    }
> > > > > > > +    static StreamMsgFormat make(unsigned w, unsigned h,
> > > > > > > uint8_t c)
> > > > > > > +    {
> > > > > > > +        return StreamMsgFormat{ .width = w, .height = h,
> > > > > > > .codec = c, .padding1 = {} };
> > > > > > > +    }
> > > > > > > +    size_t write(Stream &stream, unsigned w, unsigned h,
> > > > > > > uint8_t c)
> > > > > > > +    {
> > > > > > > +        return stream.write_all(this, sizeof(*this));
> > > > > > > +    }
> > > > > > > +};
> > > > > > 
> > > > > > This message has static size, so you construct it in make()
> > > > > > and then
> > > > > > write this whole object.
> > > > > 
> > > > > The message body has static size, yes. (Header is fixed size
> > > > > in all three cases).
> > > > > 
> > > > > > 
> > > > > > > +
> > > > > > > +
> > > > > > > +struct FrameMessage : Message<StreamMsgData,
> > > > > > > FrameMessage>
> > > > > > > +{
> > > > > > > +    FrameMessage(const void *frame, size_t length) :
> > > > > > > Message(frame, length) {}
> > > > > > > +    static const StreamMsgType type = STREAM_TYPE_DATA;
> > > > > > > +    static size_t size(const void *frame, size_t length)
> > > > > > > +    {
> > > > > > > +        return sizeof(FrameMessage) + length;
> > > > > > > +    }
> > > > > > > +    static StreamMsgData make(const void *frame, size_t
> > > > > > > length)
> > > > > > > +    {
> > > > > > > +        return StreamMsgData();
> > > > > > > +    }
> > > > > > > +    size_t write(Stream &stream, const void *frame,
> > > > > > > size_t length)
> > > > > > > +    {
> > > > > > > +        return stream.write_all(&hdr, sizeof(hdr)) +
> > > > > > > stream.write_all(frame, length);
> > > > > > > +    }
> > > > > > > +};
> > > > > > 
> > > > > > Here the message is actually only the frame data and so you
> > > > > > construct
> > > > > > an empty message in make(). In write() you just write the
> > > > > > stream data
> > > > > > passed to it.
> > > > > 
> > > > > Yes.
> > > > > 
> > > > > > 
> > > > > > > +
> > > > > > > +struct X11CursorMessage : Message<StreamMsgCursorSet,
> > > > > > > X11CursorMessage>
> > > > > > > +{
> > > > > > > +    X11CursorMessage(XFixesCursorImage *cursor)
> > > > > > > +        : Message(cursor),
> > > > > > > +          pixels(new uint32_t[pixel_size(cursor)])
> > > > > > > +    {
> > > > > > > +        fill_pixels(cursor);
> > > > > > > +    }
> > > > > > > +    static const StreamMsgType type =
> > > > > > > STREAM_TYPE_CURSOR_SET;
> > > > > > > +    static size_t pixel_size(XFixesCursorImage *cursor)
> > > > > > > +    {
> > > > > > > +        return cursor->width * cursor->height;
> > > > > > > +    }
> > > > > > > +    static size_t size(XFixesCursorImage *cursor)
> > > > > > > +    {
> > > > > > > +        return sizeof(X11CursorMessage) +
> > > > > > > sizeof(uint32_t) * pixel_size(cursor);
> > > > > > > +    }
> > > > > > > +    static StreamMsgCursorSet make(XFixesCursorImage
> > > > > > > *cursor)
> > > > > > > +    {
> > > > > > > +        return StreamMsgCursorSet
> > > > > > > +        {
> > > > > > > +            .width = cursor->width,
> > > > > > > +            .height = cursor->height,
> > > > > > > +            .hot_spot_x = cursor->xhot,
> > > > > > > +            .hot_spot_y = cursor->yhot,
> > > > > > > +            .type = SPICE_CURSOR_TYPE_ALPHA,
> > > > > > > +            .padding1 = { },
> > > > > > > +            .data = { }
> > > > > > > +        };
> > > > > > > +    }
> > > > > > > +    size_t write(Stream &stream, XFixesCursorImage
> > > > > > > *cursor)
> > > > > > > +    {
> > > > > > > +        return stream.write_all(&hdr, sizeof(hdr)) +
> > > > > > > stream.write_all(pixels.get(), hdr.size);
> > > > > > 
> > > > > > You are not writing the msg data here, might be the reson
> > > > > > for the
> > > > > > missing cursor.
> > > > > 
> > > > > Ah, good catch. I thought of not writing *this because of the
> > > > > extra pixels field, but then wrote the wrong class.
> > > > > 
> > > > > > 
> > > > > > > +    }
> > > > > > > +    void fill_pixels(XFixesCursorImage *cursor)
> > > > > > > +    {
> > > > > > > +        uint32_t *pixbuf = pixels.get();
> > > > > > > +        unsigned count = cursor->width * cursor->height;
> > > > > > > +        for (unsigned i = 0; i < count; ++i)
> > > > > > > +            pixbuf[i] = cursor->pixels[i];
> > > > > > > +    }
> > > > > > > +private:
> > > > > > > +    std::unique_ptr<uint32_t[]> pixels;
> > > > > > > +};
> > > > > > 
> > > > > > (note in this class some methods could be private and some
> > > > > > arguments
> > > > > > const)
> > > > > 
> > > > > Yes. Good idea.
> > > > > 
> > > > > > 
> > > > > > Here you add a private member pixels, which you fill in
> > > > > > constructor. In
> > > > > > make(), you create the static part of the message. In
> > > > > > write(), you
> > > > > > write them.
> > > > > > 
> > > > > > So, I ask, could you not actually construct all the data to
> > > > > > write in
> > > > > > write(), and perhaps even remove the Message::msg member
> > > > > > and the make()
> > > > > > method?
> > > > > 
> > > > > Yes, but that would require to copy the frames data, which I
> > > > > considered an expensive-enough operation to try and avoid it.
> > > > > For cursor pixels, it’s less of an issue a) because we need
> > > > > to copy anyway, due to format conversion, and they happen
> > > > > much less frequently.
> > > > 
> > > > I don't think you would need to copy the data? What I propose
> > > > actually
> > > > doesn't mean any change for this method in particular?
> > > > 
> > > > > > 
> > > > > > That would make the classes a bit simpler?
> > > > > 
> > > > > Simpler, yes but much less efficient in the important case,
> > > > > which is sending frames, where it would add one malloc / copy
> > > > > / free for each frame, which the present code avoids at the
> > > > > “mere” cost of passing the payload args around ;-)
> > > > > 
> > > > > 
> > > > > Thanks a lot.
> > > > > 
> > > > > > 
> > > > > > Cheers,
> > > > > > Lukas
> > > > > > 
> > > > > > > +
> > > > > > > }} // namespace spice::streaming_agent
> > > > > > > 
> > > > > > > 
> > > > > > > @@ -201,66 +290,6 @@ size_t Stream::write_all(const void
> > > > > > > *buf, const size_t len)
> > > > > > >   return written;
> > > > > > > }
> > > > > > > 
> > > > > > > -int Stream::send_format(unsigned w, unsigned h, uint8_t
> > > > > > > c)
> > > > > > > -{
> > > > > > > -    const size_t msgsize = sizeof(FormatMessage);
> > > > > > > -    const size_t hdrsize  = sizeof(StreamDevHeader);
> > > > > > > -    FormatMessage 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(mutex);
> > > > > > > -    if (write_all(&msg, msgsize) != msgsize) {
> > > > > > > -        return EXIT_FAILURE;
> > > > > > > -    }
> > > > > > > -    return EXIT_SUCCESS;
> > > > > > > -}
> > > > > > > -
> > > > > > > -int Stream::send_frame(const void *buf, const unsigned
> > > > > > > size)
> > > > > > > -{
> > > > > > > -    ssize_t n;
> > > > > > > -    const size_t msgsize = sizeof(FormatMessage);
> > > > > > > -    DataMessage 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 = {}
> > > > > > > -    };
> > > > > > > -
> > > > > > > -    std::lock_guard<std::mutex> stream_guard(mutex);
> > > > > > > -    n = write_all(&msg, msgsize);
> > > > > > > -    syslog(LOG_DEBUG,
> > > > > > > -           "wrote %ld bytes of header of data msg with
> > > > > > > frame of size %u bytes\n",
> > > > > > > -           n, msg.hdr.size);
> > > > > > > -    if (n != msgsize) {
> > > > > > > -        syslog(LOG_WARNING, "write_all header: wrote %ld
> > > > > > > expected %lu\n",
> > > > > > > -               n, msgsize);
> > > > > > > -        return EXIT_FAILURE;
> > > > > > > -    }
> > > > > > > -    n = write_all(buf, size);
> > > > > > > -    syslog(LOG_DEBUG, "wrote data msg body of size
> > > > > > > %ld\n", n);
> > > > > > > -    if (n != size) {
> > > > > > > -        syslog(LOG_WARNING, "write_all header: wrote %ld
> > > > > > > expected %u\n",
> > > > > > > -               n, size);
> > > > > > > -        return EXIT_FAILURE;
> > > > > > > -    }
> > > > > > > -    return EXIT_SUCCESS;
> > > > > > > -}
> > > > > > > -
> > > > > > > /* returns current time in micro-seconds */
> > > > > > > static uint64_t get_time(void)
> > > > > > > {
> > > > > > > @@ -304,47 +333,6 @@ static void usage(const char
> > > > > > > *progname)
> > > > > > >   exit(1);
> > > > > > > }
> > > > > > > 
> > > > > > > -void
> > > > > > > -Stream::send_cursor(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;
> > > > > > > -
> > > > > > > -    const uint32_t cursor_msgsize =
> > > > > > > -        sizeof(CursorMessage) + width * height *
> > > > > > > sizeof(uint32_t);
> > > > > > > -    const uint32_t hdrsize  = sizeof(StreamDevHeader);
> > > > > > > -
> > > > > > > -    std::unique_ptr<uint8_t[]> storage(new
> > > > > > > uint8_t[cursor_msgsize]);
> > > > > > > -
> > > > > > > -    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 = cursor_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->msg.data);
> > > > > > > -    fill_cursor(pixels);
> > > > > > > -
> > > > > > > -    std::lock_guard<std::mutex> stream_guard(mutex);
> > > > > > > -    write_all(storage.get(), cursor_msgsize);
> > > > > > > -}
> > > > > > > -
> > > > > > > static void cursor_changes(Stream *stream, Display
> > > > > > > *display, int event_base)
> > > > > > > {
> > > > > > >   unsigned long last_serial = 0;
> > > > > > > @@ -363,12 +351,8 @@ static void cursor_changes(Stream
> > > > > > > *stream, Display *display, int event_base)
> > > > > > >           continue;
> > > > > > > 
> > > > > > >       last_serial = cursor->cursor_serial;
> > > > > > > -        auto fill_cursor = [cursor](uint32_t *pixels) {
> > > > > > > -            for (unsigned i = 0; i < cursor->width *
> > > > > > > cursor->height; ++i)
> > > > > > > -                pixels[i] = cursor->pixels[i];
> > > > > > > -        };
> > > > > > > -        stream->send_cursor(cursor->width, cursor-
> > > > > > > >height,
> > > > > > > -                            cursor->xhot, cursor->yhot,
> > > > > > > fill_cursor);
> > > > > > > +        if (!stream->send<X11CursorMessage>(cursor))
> > > > > > > +            syslog(LOG_WARNING, "FAILED to send
> > > > > > > cursor\n");
> > > > > > >   }
> > > > > > > }
> > > > > > > 
> > > > > > > @@ -422,7 +406,7 @@ do_capture(Stream &stream, const char
> > > > > > > *streamport, FILE *f_log)
> > > > > > > 
> > > > > > >               syslog(LOG_DEBUG, "wXh %uX%u  codec=%u\n",
> > > > > > > width, height, codec);
> > > > > > > 
> > > > > > > -                if (stream.send_format(width, height,
> > > > > > > codec) == EXIT_FAILURE)
> > > > > > > +                if (!stream.send<FormatMessage>(width,
> > > > > > > height, codec))
> > > > > > >                   throw std::runtime_error("FAILED to
> > > > > > > send format message");
> > > > > > >           }
> > > > > > >           if (f_log) {
> > > > > > > @@ -434,7 +418,7 @@ do_capture(Stream &stream, const char
> > > > > > > *streamport, FILE *f_log)
> > > > > > >                   hexdump(frame.buffer,
> > > > > > > frame.buffer_size, f_log);
> > > > > > >               }
> > > > > > >           }
> > > > > > > -            if (stream.send_frame(frame.buffer,
> > > > > > > frame.buffer_size) == EXIT_FAILURE) {
> > > > > > > +            if (!stream.send<FrameMessage>(frame.buffer,
> > > > > > > frame.buffer_size)) {
> > > > > > >               syslog(LOG_ERR, "FAILED to send a
> > > > > > > frame\n");
> > > > > > >               break;
> > > > > > >           }
> > > > > > 
> > > > > > _______________________________________________
> > > > > > Spice-devel mailing list
> > > > > > Spice-devel@lists.freedesktop.org
> > > > > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> > > > > 
> > > > > 
> > > > 
> > > > _______________________________________________
> > > > Spice-devel mailing list
> > > > Spice-devel@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
> On 23 Feb 2018, at 17:37, Jonathon Jongsma <jjongsma@redhat.com> wrote:
> 
> On Fri, 2018-02-23 at 12:36 +0100, Christophe de Dinechin wrote:
>>> On 22 Feb 2018, at 19:05, Lukáš Hrázký <lhrazky@redhat.com> wrote:
>>> 
>>> On Thu, 2018-02-22 at 18:51 +0100, Christophe de Dinechin wrote:
>>>>> On 21 Feb 2018, at 10:45, Lukáš Hrázký <lhrazky@redhat.com>
>>>>> wrote:
>>>>> 
>>>>> On Tue, 2018-02-20 at 16:41 +0100, Christophe de Dinechin
>>>>> wrote:
>>>>>>> On 20 Feb 2018, at 15:29, Lukáš Hrázký <lhrazky@redhat.com>
>>>>>>> wrote:
>>>>>>> 
>>>>>>> On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin
>>>>>>> wrote:
>>>>>>>> From: Christophe de Dinechin <dinechin@redhat.com>
>>>>>>>> 
>>>>>>>> - The Stream class now deals with locking and sending
>>>>>>>> messages
>>>>>>>> - The Message<> template class deals with the general
>>>>>>>> writing mechanisms
>>>>>>>> - Three classes, FormatMessage, FrameMessage and
>>>>>>>> X11CursorMessage represent individual messages
>>>>>>>> 
>>>>>>>> The various classes should be moved to separate headers
>>>>>>>> in a subsequent operation
>>>>>>>> 
>>>>>>>> The design uses templates to avoid any runtime overhead.
>>>>>>>> All the calls are static.
>>>>>>> 
>>>>>>> All in all, a nice way to encapsulate the sending of
>>>>>>> messages. What I
>>>>>>> worked on, actually, was the receiving of messages, as that
>>>>>>> is actually
>>>>>>> more important for separating the code (as seen later in
>>>>>>> the problem
>>>>>>> you had with client_codecs and streaming_requested static
>>>>>>> variables).
>>>>>>> 
>>>>>>> I am now wondering how to merge it with your changes and
>>>>>>> whether the
>>>>>>> same Message class hierarchy could be used (without making
>>>>>>> a mess out
>>>>>>> of it). We should discuss it later.
>>>>>> 
>>>>>> Do you have your WIP stuff in a private branch I could look
>>>>>> at? Maybe I can help with the rebasing.
>>>>> 
>>>>> I'll push it somewhere so you can have a look, but I can manage
>>>>> the
>>>>> rebase myself :) It would still be an effort to find out what
>>>>> you did
>>>>> during the rebase.
>>>>> 
>>>>>> I would probably keep input and output messages in separate
>>>>>> classes. I can’t see much commonality between the two. Using
>>>>>> the same CRTP for input messages, and maybe rename Message as
>>>>>> OutputMessage…
>>>>> 
>>>>> Agreed, probably...
>>>>> 
>>>>>>> 
>>>>>>>> Signed-off-by: Christophe de Dinechin <dinechin@redhat.co
>>>>>>>> m>
>>>>>>>> ---
>>>>>>>> src/spice-streaming-agent.cpp | 250 ++++++++++++++++++++-
>>>>>>>> ---------------------
>>>>>>>> 1 file changed, 117 insertions(+), 133 deletions(-)
>>>>>>>> 
>>>>>>>> diff --git a/src/spice-streaming-agent.cpp b/src/spice-
>>>>>>>> streaming-agent.cpp
>>>>>>>> index a989ee7..c174ea4 100644
>>>>>>>> --- a/src/spice-streaming-agent.cpp
>>>>>>>> +++ b/src/spice-streaming-agent.cpp
>>>>>>>> @@ -48,24 +48,6 @@ namespace spice
>>>>>>>> namespace streaming_agent
>>>>>>>> {
>>>>>>>> 
>>>>>>>> -struct FormatMessage
>>>>>>>> -{
>>>>>>>> -    StreamDevHeader hdr;
>>>>>>>> -    StreamMsgFormat msg;
>>>>>>>> -};
>>>>>>>> -
>>>>>>>> -struct DataMessage
>>>>>>>> -{
>>>>>>>> -    StreamDevHeader hdr;
>>>>>>>> -    StreamMsgData msg;
>>>>>>>> -};
>>>>>>>> -
>>>>>>>> -struct CursorMessage
>>>>>>>> -{
>>>>>>>> -    StreamDevHeader hdr;
>>>>>>>> -    StreamMsgCursorSet msg;
>>>>>>>> -};
>>>>>>>> -
>>>>>>>> class Stream
>>>>>>>> {
>>>>>>>> public:
>>>>>>>> @@ -79,24 +61,131 @@ public:
>>>>>>>>  {
>>>>>>>>      close(streamfd);
>>>>>>>>  }
>>>>>>>> -    operator int() { return streamfd; }
>>>>>>>> 
>>>>>>>>  int have_something_to_read(int timeout);
>>>>>>>>  int read_command_from_device(void);
>>>>>>>>  int read_command(bool blocking);
>>>>>>>> 
>>>>>>>> +
>>>>>>>> +    template <typename Message, typename ...PayloadArgs>
>>>>>>>> +    bool send(PayloadArgs... payload)
>>>>>>>> +    {
>>>>>>>> +        Message message(payload...);
>>>>>>>> +        std::lock_guard<std::mutex> stream_guard(mutex);
>>>>>>>> +        size_t expected = message.size(payload...);
>>>>>>>> +        size_t written = message.write(*this,
>>>>>>>> payload...);
>>>>>>>> +        return written == expected;
>>>>>>>> +    }
>>>>>>> 
>>>>>>> Do you purposefully avoid throwing exceptions here,
>>>>>>> returning a bool?
>>>>>> 
>>>>>> You really love exceptions, don’t you ;-)
>>>>> 
>>>>> Well... I don't always get errors, but when I do, I use
>>>>> exceptions to
>>>>> handle them. :D
>>>>> 
>>>>> Really, that's what it is. Error = exception.
>>>> 
>>>> No. C++CG E2 "Throw an exception to signal that a function can't
>>>> perform its assigned task”.
>>>> 
>>>> Examples of errors that are not exceptions: FP NaN and
>>>> infinities, compiler errors (and more generally parsing errors),
>>>> etc. A parser error is not an exception because it’s the job of
>>>> the parser to detect the error…
>>>> 
>>>> I could also give examples of exceptions that are not errors,
>>>> though it’s more subtle and OT. Consider a C++ equivalent of an
>>>> interrupted system call. Also, is bad_alloc an “error" or a
>>>> limitation? (one could argue that the error would be e.g. to give
>>>> you some memory belonging to someone else ;-)
>>> 
>>> If you put it this way, sure.
>>> 
>>>>> That's what they teach
>>>>> you at the uni and what we've always done at my previous job.
>>>>> It's the
>>>>> language's designated mechanism to deal with errors, standard
>>>>> library
>>>>> uses them, ...
>>>>> 
>>>>> You brought new light into the topic for me, but I still don't
>>>>> know how
>>>>> to deal with it... Though the fact that you are the author of
>>>>> the
>>>>> exception handling implementation and you are reluctant to use
>>>>> the
>>>>> exception really speaks volumes :)
>>>>> 
>>>>>> I usually reserve exceptions for cases which are truly
>>>>>> “catastrophic”, i.e. going the long way and unwindig is the
>>>>>> right way to do it. Unwinding the stack is a very messy
>>>>>> business, takes a lot of time, and if you can just return,
>>>>>> it’s about one and a half gazillion times faster, generates
>>>>>> smaller code, etc etc.
>>>>>> 
>>>>>> In this specific case, though, I think that unwinding could
>>>>>> be seen as appropriate, since ATM we have nothing better than
>>>>>> error + abort when it happens. Plus this might avoid a
>>>>>> scenario where you could write twice if the first write
>>>>>> fails. So I’m sold, I think this is the right thing to do.
>>>>>> 
>>>>>>> The error and exception could actually be checked as low as
>>>>>>> at the end
>>>>>>> of the write_all() method, avoiding all the latter size
>>>>>>> returning and
>>>>>>> checking?
>>>>>>> 
>>>>>>>> +
>>>>>>>>  size_t write_all(const void *buf, const size_t len);
>>>>>>>> -    int send_format(unsigned w, unsigned h, uint8_t c);
>>>>>>>> -    int send_frame(const void *buf, const unsigned
>>>>>>>> size);
>>>>>>>> -    void send_cursor(uint16_t width, uint16_t height,
>>>>>>>> -                     uint16_t hotspot_x, uint16_t
>>>>>>>> hotspot_y,
>>>>>>>> -                     std::function<void(uint32_t *)>
>>>>>>>> fill_cursor);
>>>>>>>> 
>>>>>>>> private:
>>>>>>>>  int streamfd = -1;
>>>>>>>>  std::mutex mutex;
>>>>>>>> };
>>>>>>>> 
>>>>>>>> +
>>>>>>>> +template <typename Payload, typename Info>
>>>>>>> 
>>>>>>> "Info" is quite an unimaginative name :) I understand it's
>>>>>>> the message
>>>>>>> itself here and also find it curiously hard to come up with
>>>>>>> something
>>>>>>> better :) "TheMessage" not very good, is it? Still better
>>>>>>> than "Info"?
>>>>>>> Maybe a comment referencing the CRTP to help readers
>>>>>>> understand?
>>>>>> 
>>>>>> I used ‘Info’ here because that’s the information about the
>>>>>> message.
>>>>> 
>>>>> I kind of understood that, but still... very vague for me…
>>>> 
>>>> Will stay that way for the moment, unless you give me a really
>>>> better name, sorry.
>>>> 
>>>>> 
>>>>>> I can add the CRTP comment, could not remember the acronym
>>>>>> when I wrote the class ;-)
>>>>>> 
>>>>>>> 
>>>>>>>> +struct Message
>>>>>>> 
>>>>>>> Any reason to not stick with the "class" keyword instead of
>>>>>>> "struct”?
>>>>>> 
>>>>>> Not really, more a matter of habit, using struct when it’s
>>>>>> really about data and everything is public. But there,
>>>>>> “Message” evolved far away from POD to warrant using ‘class’.
>>>>> 
>>>>> I do like to take the shortcut using struct to spare myself the
>>>>> 'public:' line at the top, but I'd think it would be considered
>>>>> an inconsistency by others... :)
>>>> 
>>>> That’s not the reason. I tend to use it to mark a struct as
>>>> “mostly data”, even if it has C++isms in it. Here, I initially
>>>> saw the message is mostly data, i.e. I would not have minded the
>>>> data members being public. But that’s no longer the case. Will
>>>> change it.
>>>> 
>>>>> 
>>>>>>> 
>>>>>>>> +{
>>>>>>>> +    template <typename ...PayloadArgs>
>>>>>>>> +    Message(PayloadArgs... payload)
>>>>>>> 
>>>>>>> "PayloadArgs... payload_args", we have something called
>>>>>>> "Payload" here
>>>>>>> too, getting a bit confusing.
>>>>>> 
>>>>>> But that’s the same thing. Payload is initialized with
>>>>>> PayloadArgs, so it makes sense.
>>>>>> 
>>>>>> Initially, I had “PayloadConstructorArgs”, but then I started
>>>>>> using it outside of the ctor.
>>>>> 
>>>>> I don't think it's the same, here Payload is the message class
>>>>> and
>>>>> PayloadArgs is the content/args. So naming the objects
>>>>> accordingly IMO
>>>>> helps clarity.
>>>> 
>>>> The Payload is defined from the PayloadArgs, i.e. its constructor
>>>> is Payload(PayloadArgs…). What is the problem with that?
>>> 
>>> The problem is Payload(PayloadArgs... payload).
>> 
>> What is the problem with initializing something called a payload with
>> something called payload args?
>> 
>> I’m sorry for being dense here, but I really don’t get your
>> objection.
> 
> 
> I think the problem is that you have a typename Payload and a typename
> PayloadArgs. But you call the concrete variable of type PayloadArgs
> "payload", and you call the variable of type Payload "msg". Here's a
> shortened version of your code:
> 
> template <typename Payload, typename Info>
> struct Message
> {
>    template <typename ...PayloadArgs>
>    Message(PayloadArgs... payload)
> ...
> 
>    Payload         msg;
> };
> 
> 
> If it was instead changed to something like this, it would be less
> confusing:
> 
> template <typename Payload, typename Info>
> struct Message
> {
>    template <typename ...PayloadArgs>
>    Message(PayloadArgs... payload_args) // or just "args"?
> ...
> 
>    Payload         msg; // or "payload”?

The old code had ‘msg’, but renaming to payload sounds like a good idea based on your explanations.

> };
> 
> 
> Does that make sense? I found it a bit confusing when reading your code
> as well, though I'm not sure that my confusion is the same as Lukáš’s

Makes perfect sense. And easy to fix.

> 
> 
>> 
>>> 
>>>>> 
>>>>>>> 
>>>>>>>> +        : hdr(StreamDevHeader {
>>>>>>>> +              .protocol_version =
>>>>>>>> STREAM_DEVICE_PROTOCOL,
>>>>>>>> +              .padding = 0,     // Workaround GCC bug
>>>>>>>> "sorry: not implemented"
>>>>>>>> +              .type = Info::type,
>>>>>>>> +              .size = (uint32_t) (Info::size(payload...)
>>>>>>>> - sizeof(hdr))
>>>>>>>> +          }),
>>>>>>>> +          msg(Info::make(payload...))
>>>>>>>> +    { }
>>>>>>> 
>>>>>>> I find it redundant that you pass the "payload..." (the
>>>>>>> args :)) to
>>>>>>> Info::size() and Info::make() here and then also to
>>>>>>> Info::write() in
>>>>>>> Stream::send().
>>>>>> 
>>>>>> I’m not sure “redundant” is the correct word. I could stash
>>>>>> the information in Message, but then the compiler would have
>>>>>> a much harder time inlining the actual expression, which is
>>>>>> always very simple. Unfortunately, we have three different
>>>>>> expressions that take different input, hence the CRTP and the
>>>>>> passing of arguments.
>>>>>> 
>>>>>> 
>>>>>>> In the three messages below, you also showcase three
>>>>>>> distinct ways of serializing them, caused by this
>>>>>>> redundancy (I
>>>>>>> understand it is partially given by the payload of the
>>>>>>> messages).
>>>>>> 
>>>>>> It is *entirely* given by the payload, which I assume is a
>>>>>> given, since it’s in the protocol. What else could come into
>>>>>> play?
>>>>> 
>>>>> What I mean is that the place you create the data for
>>>>> serialization is
>>>>> only partially given by the payload. Because for example with
>>>>> the
>>>>> FormatMessage, you place the data in the class as a member, but
>>>>> you
>>>>> could have also created them on the stack at the beginning of
>>>>> the
>>>>> write() method. And unless I'm missing something, you could do
>>>>> it this
>>>>> way for all the cases, therefore unify the approach. For the
>>>>> FrameMessage, you are constructing an empty msg member…
>>>> 
>>>> I tell you that I don’t want to copy, say, 70K of frame data if I
>>>> can avoid it, and you are suggesting I allocate 70K of stack
>>>> instead? I think you did miss something, yes.
>>> 
>>> No, I am not telling you that.
>> 
>> Then, I have not understood what you mean. What do you mean with
>> “created them on the stack at the beginning of the write function”?
>> What is “them”? The format message?
>> 
>> Is your suggestion that FormatMessage could be split in two writes
>> like the others, and that this would make the code more generic (if a
>> tiny bit less efficient for that specific message)? If that’s what
>> you are saying, then the problem is that the second part is distinct
>> in all three cases anyway, so I’m not getting anything out of this,
>> but I’m making the FormatMessage case less efficient…
>> 
>> Sorry, I read and re-read your explanations, I’m afraid I still don’t
>> get it. Maybe with a code outline?
>> 
>> 
>>> 
>>>> (if that clarifies, that frame is given to me by the capture, I
>>>> did not ask the capture to use some buffer I pre-allocated)
>>>> 
>>>>> 
>>>>>>> 
>>>>>>> See comments under each class, which all relate to this
>>>>>>> (hope it's not
>>>>>>> too confusing).
>>>>>>> 
>>>>>>>> +
>>>>>>>> +    StreamDevHeader hdr;
>>>>>>>> +    Payload         msg;
>>>>>>>> +};
>>>>>>>> +
>>>>>>>> +
>>>>>>>> +struct FormatMessage : Message<StreamMsgFormat,
>>>>>>>> FormatMessage>
>>>>>>>> +{
>>>>>>>> +    FormatMessage(unsigned w, unsigned h, uint8_t c) :
>>>>>>>> Message(w, h, c) {}
>>>>>>>> +    static const StreamMsgType type =
>>>>>>>> STREAM_TYPE_FORMAT;
>>>>>>> 
>>>>>>> Could the type actually be part of the template? As in:
>>>>>>> struct FormatMessage : Message<STREAM_TYPE_FORMAT,
>>>>>>> StreamMsgFormat, FormatMessage>
>>>>>> 
>>>>>> Sure, but I find it more consistent the way I wrote it. Also
>>>>>> easier to read, because you have “type = TYPE” instead of
>>>>>> just <TYPE>, but that’s really a personal preference.
>>>>>> 
>>>>>>> 
>>>>>>>> +    static size_t size(unsigned width, unsigned height,
>>>>>>>> uint8_t codec)
>>>>>>>> +    {
>>>>>>>> +        return sizeof(FormatMessage);
>>>>>>>> +    }
>>>>>>>> +    static StreamMsgFormat make(unsigned w, unsigned h,
>>>>>>>> uint8_t c)
>>>>>>>> +    {
>>>>>>>> +        return StreamMsgFormat{ .width = w, .height = h,
>>>>>>>> .codec = c, .padding1 = {} };
>>>>>>>> +    }
>>>>>>>> +    size_t write(Stream &stream, unsigned w, unsigned h,
>>>>>>>> uint8_t c)
>>>>>>>> +    {
>>>>>>>> +        return stream.write_all(this, sizeof(*this));
>>>>>>>> +    }
>>>>>>>> +};
>>>>>>> 
>>>>>>> This message has static size, so you construct it in make()
>>>>>>> and then
>>>>>>> write this whole object.
>>>>>> 
>>>>>> The message body has static size, yes. (Header is fixed size
>>>>>> in all three cases).
>>>>>> 
>>>>>>> 
>>>>>>>> +
>>>>>>>> +
>>>>>>>> +struct FrameMessage : Message<StreamMsgData,
>>>>>>>> FrameMessage>
>>>>>>>> +{
>>>>>>>> +    FrameMessage(const void *frame, size_t length) :
>>>>>>>> Message(frame, length) {}
>>>>>>>> +    static const StreamMsgType type = STREAM_TYPE_DATA;
>>>>>>>> +    static size_t size(const void *frame, size_t length)
>>>>>>>> +    {
>>>>>>>> +        return sizeof(FrameMessage) + length;
>>>>>>>> +    }
>>>>>>>> +    static StreamMsgData make(const void *frame, size_t
>>>>>>>> length)
>>>>>>>> +    {
>>>>>>>> +        return StreamMsgData();
>>>>>>>> +    }
>>>>>>>> +    size_t write(Stream &stream, const void *frame,
>>>>>>>> size_t length)
>>>>>>>> +    {
>>>>>>>> +        return stream.write_all(&hdr, sizeof(hdr)) +
>>>>>>>> stream.write_all(frame, length);
>>>>>>>> +    }
>>>>>>>> +};
>>>>>>> 
>>>>>>> Here the message is actually only the frame data and so you
>>>>>>> construct
>>>>>>> an empty message in make(). In write() you just write the
>>>>>>> stream data
>>>>>>> passed to it.
>>>>>> 
>>>>>> Yes.
>>>>>> 
>>>>>>> 
>>>>>>>> +
>>>>>>>> +struct X11CursorMessage : Message<StreamMsgCursorSet,
>>>>>>>> X11CursorMessage>
>>>>>>>> +{
>>>>>>>> +    X11CursorMessage(XFixesCursorImage *cursor)
>>>>>>>> +        : Message(cursor),
>>>>>>>> +          pixels(new uint32_t[pixel_size(cursor)])
>>>>>>>> +    {
>>>>>>>> +        fill_pixels(cursor);
>>>>>>>> +    }
>>>>>>>> +    static const StreamMsgType type =
>>>>>>>> STREAM_TYPE_CURSOR_SET;
>>>>>>>> +    static size_t pixel_size(XFixesCursorImage *cursor)
>>>>>>>> +    {
>>>>>>>> +        return cursor->width * cursor->height;
>>>>>>>> +    }
>>>>>>>> +    static size_t size(XFixesCursorImage *cursor)
>>>>>>>> +    {
>>>>>>>> +        return sizeof(X11CursorMessage) +
>>>>>>>> sizeof(uint32_t) * pixel_size(cursor);
>>>>>>>> +    }
>>>>>>>> +    static StreamMsgCursorSet make(XFixesCursorImage
>>>>>>>> *cursor)
>>>>>>>> +    {
>>>>>>>> +        return StreamMsgCursorSet
>>>>>>>> +        {
>>>>>>>> +            .width = cursor->width,
>>>>>>>> +            .height = cursor->height,
>>>>>>>> +            .hot_spot_x = cursor->xhot,
>>>>>>>> +            .hot_spot_y = cursor->yhot,
>>>>>>>> +            .type = SPICE_CURSOR_TYPE_ALPHA,
>>>>>>>> +            .padding1 = { },
>>>>>>>> +            .data = { }
>>>>>>>> +        };
>>>>>>>> +    }
>>>>>>>> +    size_t write(Stream &stream, XFixesCursorImage
>>>>>>>> *cursor)
>>>>>>>> +    {
>>>>>>>> +        return stream.write_all(&hdr, sizeof(hdr)) +
>>>>>>>> stream.write_all(pixels.get(), hdr.size);
>>>>>>> 
>>>>>>> You are not writing the msg data here, might be the reson
>>>>>>> for the
>>>>>>> missing cursor.
>>>>>> 
>>>>>> Ah, good catch. I thought of not writing *this because of the
>>>>>> extra pixels field, but then wrote the wrong class.
>>>>>> 
>>>>>>> 
>>>>>>>> +    }
>>>>>>>> +    void fill_pixels(XFixesCursorImage *cursor)
>>>>>>>> +    {
>>>>>>>> +        uint32_t *pixbuf = pixels.get();
>>>>>>>> +        unsigned count = cursor->width * cursor->height;
>>>>>>>> +        for (unsigned i = 0; i < count; ++i)
>>>>>>>> +            pixbuf[i] = cursor->pixels[i];
>>>>>>>> +    }
>>>>>>>> +private:
>>>>>>>> +    std::unique_ptr<uint32_t[]> pixels;
>>>>>>>> +};
>>>>>>> 
>>>>>>> (note in this class some methods could be private and some
>>>>>>> arguments
>>>>>>> const)
>>>>>> 
>>>>>> Yes. Good idea.
>>>>>> 
>>>>>>> 
>>>>>>> Here you add a private member pixels, which you fill in
>>>>>>> constructor. In
>>>>>>> make(), you create the static part of the message. In
>>>>>>> write(), you
>>>>>>> write them.
>>>>>>> 
>>>>>>> So, I ask, could you not actually construct all the data to
>>>>>>> write in
>>>>>>> write(), and perhaps even remove the Message::msg member
>>>>>>> and the make()
>>>>>>> method?
>>>>>> 
>>>>>> Yes, but that would require to copy the frames data, which I
>>>>>> considered an expensive-enough operation to try and avoid it.
>>>>>> For cursor pixels, it’s less of an issue a) because we need
>>>>>> to copy anyway, due to format conversion, and they happen
>>>>>> much less frequently.
>>>>> 
>>>>> I don't think you would need to copy the data? What I propose
>>>>> actually
>>>>> doesn't mean any change for this method in particular?
>>>>> 
>>>>>>> 
>>>>>>> That would make the classes a bit simpler?
>>>>>> 
>>>>>> Simpler, yes but much less efficient in the important case,
>>>>>> which is sending frames, where it would add one malloc / copy
>>>>>> / free for each frame, which the present code avoids at the
>>>>>> “mere” cost of passing the payload args around ;-)
>>>>>> 
>>>>>> 
>>>>>> Thanks a lot.
>>>>>> 
>>>>>>> 
>>>>>>> Cheers,
>>>>>>> Lukas
>>>>>>> 
>>>>>>>> +
>>>>>>>> }} // namespace spice::streaming_agent
>>>>>>>> 
>>>>>>>> 
>>>>>>>> @@ -201,66 +290,6 @@ size_t Stream::write_all(const void
>>>>>>>> *buf, const size_t len)
>>>>>>>>  return written;
>>>>>>>> }
>>>>>>>> 
>>>>>>>> -int Stream::send_format(unsigned w, unsigned h, uint8_t
>>>>>>>> c)
>>>>>>>> -{
>>>>>>>> -    const size_t msgsize = sizeof(FormatMessage);
>>>>>>>> -    const size_t hdrsize  = sizeof(StreamDevHeader);
>>>>>>>> -    FormatMessage 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(mutex);
>>>>>>>> -    if (write_all(&msg, msgsize) != msgsize) {
>>>>>>>> -        return EXIT_FAILURE;
>>>>>>>> -    }
>>>>>>>> -    return EXIT_SUCCESS;
>>>>>>>> -}
>>>>>>>> -
>>>>>>>> -int Stream::send_frame(const void *buf, const unsigned
>>>>>>>> size)
>>>>>>>> -{
>>>>>>>> -    ssize_t n;
>>>>>>>> -    const size_t msgsize = sizeof(FormatMessage);
>>>>>>>> -    DataMessage 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 = {}
>>>>>>>> -    };
>>>>>>>> -
>>>>>>>> -    std::lock_guard<std::mutex> stream_guard(mutex);
>>>>>>>> -    n = write_all(&msg, msgsize);
>>>>>>>> -    syslog(LOG_DEBUG,
>>>>>>>> -           "wrote %ld bytes of header of data msg with
>>>>>>>> frame of size %u bytes\n",
>>>>>>>> -           n, msg.hdr.size);
>>>>>>>> -    if (n != msgsize) {
>>>>>>>> -        syslog(LOG_WARNING, "write_all header: wrote %ld
>>>>>>>> expected %lu\n",
>>>>>>>> -               n, msgsize);
>>>>>>>> -        return EXIT_FAILURE;
>>>>>>>> -    }
>>>>>>>> -    n = write_all(buf, size);
>>>>>>>> -    syslog(LOG_DEBUG, "wrote data msg body of size
>>>>>>>> %ld\n", n);
>>>>>>>> -    if (n != size) {
>>>>>>>> -        syslog(LOG_WARNING, "write_all header: wrote %ld
>>>>>>>> expected %u\n",
>>>>>>>> -               n, size);
>>>>>>>> -        return EXIT_FAILURE;
>>>>>>>> -    }
>>>>>>>> -    return EXIT_SUCCESS;
>>>>>>>> -}
>>>>>>>> -
>>>>>>>> /* returns current time in micro-seconds */
>>>>>>>> static uint64_t get_time(void)
>>>>>>>> {
>>>>>>>> @@ -304,47 +333,6 @@ static void usage(const char
>>>>>>>> *progname)
>>>>>>>>  exit(1);
>>>>>>>> }
>>>>>>>> 
>>>>>>>> -void
>>>>>>>> -Stream::send_cursor(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;
>>>>>>>> -
>>>>>>>> -    const uint32_t cursor_msgsize =
>>>>>>>> -        sizeof(CursorMessage) + width * height *
>>>>>>>> sizeof(uint32_t);
>>>>>>>> -    const uint32_t hdrsize  = sizeof(StreamDevHeader);
>>>>>>>> -
>>>>>>>> -    std::unique_ptr<uint8_t[]> storage(new
>>>>>>>> uint8_t[cursor_msgsize]);
>>>>>>>> -
>>>>>>>> -    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 = cursor_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->msg.data);
>>>>>>>> -    fill_cursor(pixels);
>>>>>>>> -
>>>>>>>> -    std::lock_guard<std::mutex> stream_guard(mutex);
>>>>>>>> -    write_all(storage.get(), cursor_msgsize);
>>>>>>>> -}
>>>>>>>> -
>>>>>>>> static void cursor_changes(Stream *stream, Display
>>>>>>>> *display, int event_base)
>>>>>>>> {
>>>>>>>>  unsigned long last_serial = 0;
>>>>>>>> @@ -363,12 +351,8 @@ static void cursor_changes(Stream
>>>>>>>> *stream, Display *display, int event_base)
>>>>>>>>          continue;
>>>>>>>> 
>>>>>>>>      last_serial = cursor->cursor_serial;
>>>>>>>> -        auto fill_cursor = [cursor](uint32_t *pixels) {
>>>>>>>> -            for (unsigned i = 0; i < cursor->width *
>>>>>>>> cursor->height; ++i)
>>>>>>>> -                pixels[i] = cursor->pixels[i];
>>>>>>>> -        };
>>>>>>>> -        stream->send_cursor(cursor->width, cursor-
>>>>>>>>> height,
>>>>>>>> -                            cursor->xhot, cursor->yhot,
>>>>>>>> fill_cursor);
>>>>>>>> +        if (!stream->send<X11CursorMessage>(cursor))
>>>>>>>> +            syslog(LOG_WARNING, "FAILED to send
>>>>>>>> cursor\n");
>>>>>>>>  }
>>>>>>>> }
>>>>>>>> 
>>>>>>>> @@ -422,7 +406,7 @@ do_capture(Stream &stream, const char
>>>>>>>> *streamport, FILE *f_log)
>>>>>>>> 
>>>>>>>>              syslog(LOG_DEBUG, "wXh %uX%u  codec=%u\n",
>>>>>>>> width, height, codec);
>>>>>>>> 
>>>>>>>> -                if (stream.send_format(width, height,
>>>>>>>> codec) == EXIT_FAILURE)
>>>>>>>> +                if (!stream.send<FormatMessage>(width,
>>>>>>>> height, codec))
>>>>>>>>                  throw std::runtime_error("FAILED to
>>>>>>>> send format message");
>>>>>>>>          }
>>>>>>>>          if (f_log) {
>>>>>>>> @@ -434,7 +418,7 @@ do_capture(Stream &stream, const char
>>>>>>>> *streamport, FILE *f_log)
>>>>>>>>                  hexdump(frame.buffer,
>>>>>>>> frame.buffer_size, f_log);
>>>>>>>>              }
>>>>>>>>          }
>>>>>>>> -            if (stream.send_frame(frame.buffer,
>>>>>>>> frame.buffer_size) == EXIT_FAILURE) {
>>>>>>>> +            if (!stream.send<FrameMessage>(frame.buffer,
>>>>>>>> frame.buffer_size)) {
>>>>>>>>              syslog(LOG_ERR, "FAILED to send a
>>>>>>>> frame\n");
>>>>>>>>              break;
>>>>>>>>          }
>>>>>>> 
>>>>>>> _______________________________________________
>>>>>>> Spice-devel mailing list
>>>>>>> Spice-devel@lists.freedesktop.org
>>>>>>> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>>>>>> 
>>>>>> 
>>>>> 
>>>>> _______________________________________________
>>>>> Spice-devel mailing list
>>>>> Spice-devel@lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>> 
>> _______________________________________________
>> Spice-devel mailing list
>> Spice-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/spice-devel
On Fri, 2018-02-23 at 19:07 +0100, Christophe de Dinechin wrote:
> > On 23 Feb 2018, at 17:37, Jonathon Jongsma <jjongsma@redhat.com> wrote:
> > 
> > On Fri, 2018-02-23 at 12:36 +0100, Christophe de Dinechin wrote:
> > > > On 22 Feb 2018, at 19:05, Lukáš Hrázký <lhrazky@redhat.com> wrote:
> > > > 
> > > > On Thu, 2018-02-22 at 18:51 +0100, Christophe de Dinechin wrote:
> > > > > > On 21 Feb 2018, at 10:45, Lukáš Hrázký <lhrazky@redhat.com>
> > > > > > wrote:
> > > > > > 
> > > > > > On Tue, 2018-02-20 at 16:41 +0100, Christophe de Dinechin
> > > > > > wrote:
> > > > > > > > On 20 Feb 2018, at 15:29, Lukáš Hrázký <lhrazky@redhat.com>
> > > > > > > > wrote:
> > > > > > > > 
> > > > > > > > On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin
> > > > > > > > wrote:
> > > > > > > > > From: Christophe de Dinechin <dinechin@redhat.com>
> > > > > > > > > 
> > > > > > > > > - The Stream class now deals with locking and sending
> > > > > > > > > messages
> > > > > > > > > - The Message<> template class deals with the general
> > > > > > > > > writing mechanisms
> > > > > > > > > - Three classes, FormatMessage, FrameMessage and
> > > > > > > > > X11CursorMessage represent individual messages
> > > > > > > > > 
> > > > > > > > > The various classes should be moved to separate headers
> > > > > > > > > in a subsequent operation
> > > > > > > > > 
> > > > > > > > > The design uses templates to avoid any runtime overhead.
> > > > > > > > > All the calls are static.
> > > > > > > > 
> > > > > > > > All in all, a nice way to encapsulate the sending of
> > > > > > > > messages. What I
> > > > > > > > worked on, actually, was the receiving of messages, as that
> > > > > > > > is actually
> > > > > > > > more important for separating the code (as seen later in
> > > > > > > > the problem
> > > > > > > > you had with client_codecs and streaming_requested static
> > > > > > > > variables).
> > > > > > > > 
> > > > > > > > I am now wondering how to merge it with your changes and
> > > > > > > > whether the
> > > > > > > > same Message class hierarchy could be used (without making
> > > > > > > > a mess out
> > > > > > > > of it). We should discuss it later.
> > > > > > > 
> > > > > > > Do you have your WIP stuff in a private branch I could look
> > > > > > > at? Maybe I can help with the rebasing.
> > > > > > 
> > > > > > I'll push it somewhere so you can have a look, but I can manage
> > > > > > the
> > > > > > rebase myself :) It would still be an effort to find out what
> > > > > > you did
> > > > > > during the rebase.
> > > > > > 
> > > > > > > I would probably keep input and output messages in separate
> > > > > > > classes. I can’t see much commonality between the two. Using
> > > > > > > the same CRTP for input messages, and maybe rename Message as
> > > > > > > OutputMessage…
> > > > > > 
> > > > > > Agreed, probably...
> > > > > > 
> > > > > > > > 
> > > > > > > > > Signed-off-by: Christophe de Dinechin <dinechin@redhat.co
> > > > > > > > > m>
> > > > > > > > > ---
> > > > > > > > > src/spice-streaming-agent.cpp | 250 ++++++++++++++++++++-
> > > > > > > > > ---------------------
> > > > > > > > > 1 file changed, 117 insertions(+), 133 deletions(-)
> > > > > > > > > 
> > > > > > > > > diff --git a/src/spice-streaming-agent.cpp b/src/spice-
> > > > > > > > > streaming-agent.cpp
> > > > > > > > > index a989ee7..c174ea4 100644
> > > > > > > > > --- a/src/spice-streaming-agent.cpp
> > > > > > > > > +++ b/src/spice-streaming-agent.cpp
> > > > > > > > > @@ -48,24 +48,6 @@ namespace spice
> > > > > > > > > namespace streaming_agent
> > > > > > > > > {
> > > > > > > > > 
> > > > > > > > > -struct FormatMessage
> > > > > > > > > -{
> > > > > > > > > -    StreamDevHeader hdr;
> > > > > > > > > -    StreamMsgFormat msg;
> > > > > > > > > -};
> > > > > > > > > -
> > > > > > > > > -struct DataMessage
> > > > > > > > > -{
> > > > > > > > > -    StreamDevHeader hdr;
> > > > > > > > > -    StreamMsgData msg;
> > > > > > > > > -};
> > > > > > > > > -
> > > > > > > > > -struct CursorMessage
> > > > > > > > > -{
> > > > > > > > > -    StreamDevHeader hdr;
> > > > > > > > > -    StreamMsgCursorSet msg;
> > > > > > > > > -};
> > > > > > > > > -
> > > > > > > > > class Stream
> > > > > > > > > {
> > > > > > > > > public:
> > > > > > > > > @@ -79,24 +61,131 @@ public:
> > > > > > > > >  {
> > > > > > > > >      close(streamfd);
> > > > > > > > >  }
> > > > > > > > > -    operator int() { return streamfd; }
> > > > > > > > > 
> > > > > > > > >  int have_something_to_read(int timeout);
> > > > > > > > >  int read_command_from_device(void);
> > > > > > > > >  int read_command(bool blocking);
> > > > > > > > > 
> > > > > > > > > +
> > > > > > > > > +    template <typename Message, typename ...PayloadArgs>
> > > > > > > > > +    bool send(PayloadArgs... payload)
> > > > > > > > > +    {
> > > > > > > > > +        Message message(payload...);
> > > > > > > > > +        std::lock_guard<std::mutex> stream_guard(mutex);
> > > > > > > > > +        size_t expected = message.size(payload...);
> > > > > > > > > +        size_t written = message.write(*this,
> > > > > > > > > payload...);
> > > > > > > > > +        return written == expected;
> > > > > > > > > +    }
> > > > > > > > 
> > > > > > > > Do you purposefully avoid throwing exceptions here,
> > > > > > > > returning a bool?
> > > > > > > 
> > > > > > > You really love exceptions, don’t you ;-)
> > > > > > 
> > > > > > Well... I don't always get errors, but when I do, I use
> > > > > > exceptions to
> > > > > > handle them. :D
> > > > > > 
> > > > > > Really, that's what it is. Error = exception.
> > > > > 
> > > > > No. C++CG E2 "Throw an exception to signal that a function can't
> > > > > perform its assigned task”.
> > > > > 
> > > > > Examples of errors that are not exceptions: FP NaN and
> > > > > infinities, compiler errors (and more generally parsing errors),
> > > > > etc. A parser error is not an exception because it’s the job of
> > > > > the parser to detect the error…
> > > > > 
> > > > > I could also give examples of exceptions that are not errors,
> > > > > though it’s more subtle and OT. Consider a C++ equivalent of an
> > > > > interrupted system call. Also, is bad_alloc an “error" or a
> > > > > limitation? (one could argue that the error would be e.g. to give
> > > > > you some memory belonging to someone else ;-)
> > > > 
> > > > If you put it this way, sure.
> > > > 
> > > > > > That's what they teach
> > > > > > you at the uni and what we've always done at my previous job.
> > > > > > It's the
> > > > > > language's designated mechanism to deal with errors, standard
> > > > > > library
> > > > > > uses them, ...
> > > > > > 
> > > > > > You brought new light into the topic for me, but I still don't
> > > > > > know how
> > > > > > to deal with it... Though the fact that you are the author of
> > > > > > the
> > > > > > exception handling implementation and you are reluctant to use
> > > > > > the
> > > > > > exception really speaks volumes :)
> > > > > > 
> > > > > > > I usually reserve exceptions for cases which are truly
> > > > > > > “catastrophic”, i.e. going the long way and unwindig is the
> > > > > > > right way to do it. Unwinding the stack is a very messy
> > > > > > > business, takes a lot of time, and if you can just return,
> > > > > > > it’s about one and a half gazillion times faster, generates
> > > > > > > smaller code, etc etc.
> > > > > > > 
> > > > > > > In this specific case, though, I think that unwinding could
> > > > > > > be seen as appropriate, since ATM we have nothing better than
> > > > > > > error + abort when it happens. Plus this might avoid a
> > > > > > > scenario where you could write twice if the first write
> > > > > > > fails. So I’m sold, I think this is the right thing to do.
> > > > > > > 
> > > > > > > > The error and exception could actually be checked as low as
> > > > > > > > at the end
> > > > > > > > of the write_all() method, avoiding all the latter size
> > > > > > > > returning and
> > > > > > > > checking?
> > > > > > > > 
> > > > > > > > > +
> > > > > > > > >  size_t write_all(const void *buf, const size_t len);
> > > > > > > > > -    int send_format(unsigned w, unsigned h, uint8_t c);
> > > > > > > > > -    int send_frame(const void *buf, const unsigned
> > > > > > > > > size);
> > > > > > > > > -    void send_cursor(uint16_t width, uint16_t height,
> > > > > > > > > -                     uint16_t hotspot_x, uint16_t
> > > > > > > > > hotspot_y,
> > > > > > > > > -                     std::function<void(uint32_t *)>
> > > > > > > > > fill_cursor);
> > > > > > > > > 
> > > > > > > > > private:
> > > > > > > > >  int streamfd = -1;
> > > > > > > > >  std::mutex mutex;
> > > > > > > > > };
> > > > > > > > > 
> > > > > > > > > +
> > > > > > > > > +template <typename Payload, typename Info>
> > > > > > > > 
> > > > > > > > "Info" is quite an unimaginative name :) I understand it's
> > > > > > > > the message
> > > > > > > > itself here and also find it curiously hard to come up with
> > > > > > > > something
> > > > > > > > better :) "TheMessage" not very good, is it? Still better
> > > > > > > > than "Info"?
> > > > > > > > Maybe a comment referencing the CRTP to help readers
> > > > > > > > understand?
> > > > > > > 
> > > > > > > I used ‘Info’ here because that’s the information about the
> > > > > > > message.
> > > > > > 
> > > > > > I kind of understood that, but still... very vague for me…
> > > > > 
> > > > > Will stay that way for the moment, unless you give me a really
> > > > > better name, sorry.
> > > > > 
> > > > > > 
> > > > > > > I can add the CRTP comment, could not remember the acronym
> > > > > > > when I wrote the class ;-)
> > > > > > > 
> > > > > > > > 
> > > > > > > > > +struct Message
> > > > > > > > 
> > > > > > > > Any reason to not stick with the "class" keyword instead of
> > > > > > > > "struct”?
> > > > > > > 
> > > > > > > Not really, more a matter of habit, using struct when it’s
> > > > > > > really about data and everything is public. But there,
> > > > > > > “Message” evolved far away from POD to warrant using ‘class’.
> > > > > > 
> > > > > > I do like to take the shortcut using struct to spare myself the
> > > > > > 'public:' line at the top, but I'd think it would be considered
> > > > > > an inconsistency by others... :)
> > > > > 
> > > > > That’s not the reason. I tend to use it to mark a struct as
> > > > > “mostly data”, even if it has C++isms in it. Here, I initially
> > > > > saw the message is mostly data, i.e. I would not have minded the
> > > > > data members being public. But that’s no longer the case. Will
> > > > > change it.
> > > > > 
> > > > > > 
> > > > > > > > 
> > > > > > > > > +{
> > > > > > > > > +    template <typename ...PayloadArgs>
> > > > > > > > > +    Message(PayloadArgs... payload)
> > > > > > > > 
> > > > > > > > "PayloadArgs... payload_args", we have something called
> > > > > > > > "Payload" here
> > > > > > > > too, getting a bit confusing.
> > > > > > > 
> > > > > > > But that’s the same thing. Payload is initialized with
> > > > > > > PayloadArgs, so it makes sense.
> > > > > > > 
> > > > > > > Initially, I had “PayloadConstructorArgs”, but then I started
> > > > > > > using it outside of the ctor.
> > > > > > 
> > > > > > I don't think it's the same, here Payload is the message class
> > > > > > and
> > > > > > PayloadArgs is the content/args. So naming the objects
> > > > > > accordingly IMO
> > > > > > helps clarity.
> > > > > 
> > > > > The Payload is defined from the PayloadArgs, i.e. its constructor
> > > > > is Payload(PayloadArgs…). What is the problem with that?
> > > > 
> > > > The problem is Payload(PayloadArgs... payload).
> > > 
> > > What is the problem with initializing something called a payload with
> > > something called payload args?
> > > 
> > > I’m sorry for being dense here, but I really don’t get your
> > > objection.
> > 
> > 
> > I think the problem is that you have a typename Payload and a typename
> > PayloadArgs. But you call the concrete variable of type PayloadArgs
> > "payload", and you call the variable of type Payload "msg". Here's a
> > shortened version of your code:
> > 
> > template <typename Payload, typename Info>
> > struct Message
> > {
> >    template <typename ...PayloadArgs>
> >    Message(PayloadArgs... payload)
> > ...
> > 
> >    Payload         msg;
> > };
> > 
> > 
> > If it was instead changed to something like this, it would be less
> > confusing:
> > 
> > template <typename Payload, typename Info>
> > struct Message
> > {
> >    template <typename ...PayloadArgs>
> >    Message(PayloadArgs... payload_args) // or just "args"?
> > ...
> > 
> >    Payload         msg; // or "payload”?
> 
> The old code had ‘msg’, but renaming to payload sounds like a good idea based on your explanations.
> 
> > };
> > 
> > 
> > Does that make sense? I found it a bit confusing when reading your code
> > as well, though I'm not sure that my confusion is the same as Lukáš’s
> 
> Makes perfect sense. And easy to fix.

That's what I meant as well. Apparently I need to work on my explaining
skills :)

> > 
> > 
> > > 
> > > > 
> > > > > > 
> > > > > > > > 
> > > > > > > > > +        : hdr(StreamDevHeader {
> > > > > > > > > +              .protocol_version =
> > > > > > > > > STREAM_DEVICE_PROTOCOL,
> > > > > > > > > +              .padding = 0,     // Workaround GCC bug
> > > > > > > > > "sorry: not implemented"
> > > > > > > > > +              .type = Info::type,
> > > > > > > > > +              .size = (uint32_t) (Info::size(payload...)
> > > > > > > > > - sizeof(hdr))
> > > > > > > > > +          }),
> > > > > > > > > +          msg(Info::make(payload...))
> > > > > > > > > +    { }
> > > > > > > > 
> > > > > > > > I find it redundant that you pass the "payload..." (the
> > > > > > > > args :)) to
> > > > > > > > Info::size() and Info::make() here and then also to
> > > > > > > > Info::write() in
> > > > > > > > Stream::send().
> > > > > > > 
> > > > > > > I’m not sure “redundant” is the correct word. I could stash
> > > > > > > the information in Message, but then the compiler would have
> > > > > > > a much harder time inlining the actual expression, which is
> > > > > > > always very simple. Unfortunately, we have three different
> > > > > > > expressions that take different input, hence the CRTP and the
> > > > > > > passing of arguments.
> > > > > > > 
> > > > > > > 
> > > > > > > > In the three messages below, you also showcase three
> > > > > > > > distinct ways of serializing them, caused by this
> > > > > > > > redundancy (I
> > > > > > > > understand it is partially given by the payload of the
> > > > > > > > messages).
> > > > > > > 
> > > > > > > It is *entirely* given by the payload, which I assume is a
> > > > > > > given, since it’s in the protocol. What else could come into
> > > > > > > play?
> > > > > > 
> > > > > > What I mean is that the place you create the data for
> > > > > > serialization is
> > > > > > only partially given by the payload. Because for example with
> > > > > > the
> > > > > > FormatMessage, you place the data in the class as a member, but
> > > > > > you
> > > > > > could have also created them on the stack at the beginning of
> > > > > > the
> > > > > > write() method. And unless I'm missing something, you could do
> > > > > > it this
> > > > > > way for all the cases, therefore unify the approach. For the
> > > > > > FrameMessage, you are constructing an empty msg member…
> > > > > 
> > > > > I tell you that I don’t want to copy, say, 70K of frame data if I
> > > > > can avoid it, and you are suggesting I allocate 70K of stack
> > > > > instead? I think you did miss something, yes.
> > > > 
> > > > No, I am not telling you that.
> > > 
> > > Then, I have not understood what you mean. What do you mean with
> > > “created them on the stack at the beginning of the write function”?
> > > What is “them”? The format message?
> > > 
> > > Is your suggestion that FormatMessage could be split in two writes
> > > like the others, and that this would make the code more generic (if a
> > > tiny bit less efficient for that specific message)? If that’s what
> > > you are saying, then the problem is that the second part is distinct
> > > in all three cases anyway, so I’m not getting anything out of this,
> > > but I’m making the FormatMessage case less efficient…
> > > 
> > > Sorry, I read and re-read your explanations, I’m afraid I still don’t
> > > get it. Maybe with a code outline?

Sending a patch in reply to this (coudln't think of a way to clearly
outline it in an email). It's hard to explain and I am not doing the
best job at that.

> > > 
> > > > 
> > > > > (if that clarifies, that frame is given to me by the capture, I
> > > > > did not ask the capture to use some buffer I pre-allocated)
> > > > > 
> > > > > > 
> > > > > > > > 
> > > > > > > > See comments under each class, which all relate to this
> > > > > > > > (hope it's not
> > > > > > > > too confusing).
> > > > > > > > 
> > > > > > > > > +
> > > > > > > > > +    StreamDevHeader hdr;
> > > > > > > > > +    Payload         msg;
> > > > > > > > > +};
> > > > > > > > > +
> > > > > > > > > +
> > > > > > > > > +struct FormatMessage : Message<StreamMsgFormat,
> > > > > > > > > FormatMessage>
> > > > > > > > > +{
> > > > > > > > > +    FormatMessage(unsigned w, unsigned h, uint8_t c) :
> > > > > > > > > Message(w, h, c) {}
> > > > > > > > > +    static const StreamMsgType type =
> > > > > > > > > STREAM_TYPE_FORMAT;
> > > > > > > > 
> > > > > > > > Could the type actually be part of the template? As in:
> > > > > > > > struct FormatMessage : Message<STREAM_TYPE_FORMAT,
> > > > > > > > StreamMsgFormat, FormatMessage>
> > > > > > > 
> > > > > > > Sure, but I find it more consistent the way I wrote it. Also
> > > > > > > easier to read, because you have “type = TYPE” instead of
> > > > > > > just <TYPE>, but that’s really a personal preference.
> > > > > > > 
> > > > > > > > 
> > > > > > > > > +    static size_t size(unsigned width, unsigned height,
> > > > > > > > > uint8_t codec)
> > > > > > > > > +    {
> > > > > > > > > +        return sizeof(FormatMessage);
> > > > > > > > > +    }
> > > > > > > > > +    static StreamMsgFormat make(unsigned w, unsigned h,
> > > > > > > > > uint8_t c)
> > > > > > > > > +    {
> > > > > > > > > +        return StreamMsgFormat{ .width = w, .height = h,
> > > > > > > > > .codec = c, .padding1 = {} };
> > > > > > > > > +    }
> > > > > > > > > +    size_t write(Stream &stream, unsigned w, unsigned h,
> > > > > > > > > uint8_t c)
> > > > > > > > > +    {
> > > > > > > > > +        return stream.write_all(this, sizeof(*this));
> > > > > > > > > +    }
> > > > > > > > > +};
> > > > > > > > 
> > > > > > > > This message has static size, so you construct it in make()
> > > > > > > > and then
> > > > > > > > write this whole object.
> > > > > > > 
> > > > > > > The message body has static size, yes. (Header is fixed size
> > > > > > > in all three cases).
> > > > > > > 
> > > > > > > > 
> > > > > > > > > +
> > > > > > > > > +
> > > > > > > > > +struct FrameMessage : Message<StreamMsgData,
> > > > > > > > > FrameMessage>
> > > > > > > > > +{
> > > > > > > > > +    FrameMessage(const void *frame, size_t length) :
> > > > > > > > > Message(frame, length) {}
> > > > > > > > > +    static const StreamMsgType type = STREAM_TYPE_DATA;
> > > > > > > > > +    static size_t size(const void *frame, size_t length)
> > > > > > > > > +    {
> > > > > > > > > +        return sizeof(FrameMessage) + length;
> > > > > > > > > +    }
> > > > > > > > > +    static StreamMsgData make(const void *frame, size_t
> > > > > > > > > length)
> > > > > > > > > +    {
> > > > > > > > > +        return StreamMsgData();
> > > > > > > > > +    }
> > > > > > > > > +    size_t write(Stream &stream, const void *frame,
> > > > > > > > > size_t length)
> > > > > > > > > +    {
> > > > > > > > > +        return stream.write_all(&hdr, sizeof(hdr)) +
> > > > > > > > > stream.write_all(frame, length);
> > > > > > > > > +    }
> > > > > > > > > +};
> > > > > > > > 
> > > > > > > > Here the message is actually only the frame data and so you
> > > > > > > > construct
> > > > > > > > an empty message in make(). In write() you just write the
> > > > > > > > stream data
> > > > > > > > passed to it.
> > > > > > > 
> > > > > > > Yes.
> > > > > > > 
> > > > > > > > 
> > > > > > > > > +
> > > > > > > > > +struct X11CursorMessage : Message<StreamMsgCursorSet,
> > > > > > > > > X11CursorMessage>
> > > > > > > > > +{
> > > > > > > > > +    X11CursorMessage(XFixesCursorImage *cursor)
> > > > > > > > > +        : Message(cursor),
> > > > > > > > > +          pixels(new uint32_t[pixel_size(cursor)])
> > > > > > > > > +    {
> > > > > > > > > +        fill_pixels(cursor);
> > > > > > > > > +    }
> > > > > > > > > +    static const StreamMsgType type =
> > > > > > > > > STREAM_TYPE_CURSOR_SET;
> > > > > > > > > +    static size_t pixel_size(XFixesCursorImage *cursor)
> > > > > > > > > +    {
> > > > > > > > > +        return cursor->width * cursor->height;
> > > > > > > > > +    }
> > > > > > > > > +    static size_t size(XFixesCursorImage *cursor)
> > > > > > > > > +    {
> > > > > > > > > +        return sizeof(X11CursorMessage) +
> > > > > > > > > sizeof(uint32_t) * pixel_size(cursor);
> > > > > > > > > +    }
> > > > > > > > > +    static StreamMsgCursorSet make(XFixesCursorImage
> > > > > > > > > *cursor)
> > > > > > > > > +    {
> > > > > > > > > +        return StreamMsgCursorSet
> > > > > > > > > +        {
> > > > > > > > > +            .width = cursor->width,
> > > > > > > > > +            .height = cursor->height,
> > > > > > > > > +            .hot_spot_x = cursor->xhot,
> > > > > > > > > +            .hot_spot_y = cursor->yhot,
> > > > > > > > > +            .type = SPICE_CURSOR_TYPE_ALPHA,
> > > > > > > > > +            .padding1 = { },
> > > > > > > > > +            .data = { }
> > > > > > > > > +        };
> > > > > > > > > +    }
> > > > > > > > > +    size_t write(Stream &stream, XFixesCursorImage
> > > > > > > > > *cursor)
> > > > > > > > > +    {
> > > > > > > > > +        return stream.write_all(&hdr, sizeof(hdr)) +
> > > > > > > > > stream.write_all(pixels.get(), hdr.size);
> > > > > > > > 
> > > > > > > > You are not writing the msg data here, might be the reson
> > > > > > > > for the
> > > > > > > > missing cursor.
> > > > > > > 
> > > > > > > Ah, good catch. I thought of not writing *this because of the
> > > > > > > extra pixels field, but then wrote the wrong class.
> > > > > > > 
> > > > > > > > 
> > > > > > > > > +    }
> > > > > > > > > +    void fill_pixels(XFixesCursorImage *cursor)
> > > > > > > > > +    {
> > > > > > > > > +        uint32_t *pixbuf = pixels.get();
> > > > > > > > > +        unsigned count = cursor->width * cursor->height;
> > > > > > > > > +        for (unsigned i = 0; i < count; ++i)
> > > > > > > > > +            pixbuf[i] = cursor->pixels[i];
> > > > > > > > > +    }
> > > > > > > > > +private:
> > > > > > > > > +    std::unique_ptr<uint32_t[]> pixels;
> > > > > > > > > +};
> > > > > > > > 
> > > > > > > > (note in this class some methods could be private and some
> > > > > > > > arguments
> > > > > > > > const)
> > > > > > > 
> > > > > > > Yes. Good idea.
> > > > > > > 
> > > > > > > > 
> > > > > > > > Here you add a private member pixels, which you fill in
> > > > > > > > constructor. In
> > > > > > > > make(), you create the static part of the message. In
> > > > > > > > write(), you
> > > > > > > > write them.
> > > > > > > > 
> > > > > > > > So, I ask, could you not actually construct all the data to
> > > > > > > > write in
> > > > > > > > write(), and perhaps even remove the Message::msg member
> > > > > > > > and the make()
> > > > > > > > method?
> > > > > > > 
> > > > > > > Yes, but that would require to copy the frames data, which I
> > > > > > > considered an expensive-enough operation to try and avoid it.
> > > > > > > For cursor pixels, it’s less of an issue a) because we need
> > > > > > > to copy anyway, due to format conversion, and they happen
> > > > > > > much less frequently.
> > > > > > 
> > > > > > I don't think you would need to copy the data? What I propose
> > > > > > actually
> > > > > > doesn't mean any change for this method in particular?
> > > > > > 
> > > > > > > > 
> > > > > > > > That would make the classes a bit simpler?
> > > > > > > 
> > > > > > > Simpler, yes but much less efficient in the important case,
> > > > > > > which is sending frames, where it would add one malloc / copy
> > > > > > > / free for each frame, which the present code avoids at the
> > > > > > > “mere” cost of passing the payload args around ;-)
> > > > > > > 
> > > > > > > 
> > > > > > > Thanks a lot.
> > > > > > > 
> > > > > > > > 
> > > > > > > > Cheers,
> > > > > > > > Lukas
> > > > > > > > 
> > > > > > > > > +
> > > > > > > > > }} // namespace spice::streaming_agent
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > @@ -201,66 +290,6 @@ size_t Stream::write_all(const void
> > > > > > > > > *buf, const size_t len)
> > > > > > > > >  return written;
> > > > > > > > > }
> > > > > > > > > 
> > > > > > > > > -int Stream::send_format(unsigned w, unsigned h, uint8_t
> > > > > > > > > c)
> > > > > > > > > -{
> > > > > > > > > -    const size_t msgsize = sizeof(FormatMessage);
> > > > > > > > > -    const size_t hdrsize  = sizeof(StreamDevHeader);
> > > > > > > > > -    FormatMessage 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(mutex);
> > > > > > > > > -    if (write_all(&msg, msgsize) != msgsize) {
> > > > > > > > > -        return EXIT_FAILURE;
> > > > > > > > > -    }
> > > > > > > > > -    return EXIT_SUCCESS;
> > > > > > > > > -}
> > > > > > > > > -
> > > > > > > > > -int Stream::send_frame(const void *buf, const unsigned
> > > > > > > > > size)
> > > > > > > > > -{
> > > > > > > > > -    ssize_t n;
> > > > > > > > > -    const size_t msgsize = sizeof(FormatMessage);
> > > > > > > > > -    DataMessage 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 = {}
> > > > > > > > > -    };
> > > > > > > > > -
> > > > > > > > > -    std::lock_guard<std::mutex> stream_guard(mutex);
> > > > > > > > > -    n = write_all(&msg, msgsize);
> > > > > > > > > -    syslog(LOG_DEBUG,
> > > > > > > > > -           "wrote %ld bytes of header of data msg with
> > > > > > > > > frame of size %u bytes\n",
> > > > > > > > > -           n, msg.hdr.size);
> > > > > > > > > -    if (n != msgsize) {
> > > > > > > > > -        syslog(LOG_WARNING, "write_all header: wrote %ld
> > > > > > > > > expected %lu\n",
> > > > > > > > > -               n, msgsize);
> > > > > > > > > -        return EXIT_FAILURE;
> > > > > > > > > -    }
> > > > > > > > > -    n = write_all(buf, size);
> > > > > > > > > -    syslog(LOG_DEBUG, "wrote data msg body of size
> > > > > > > > > %ld\n", n);
> > > > > > > > > -    if (n != size) {
> > > > > > > > > -        syslog(LOG_WARNING, "write_all header: wrote %ld
> > > > > > > > > expected %u\n",
> > > > > > > > > -               n, size);
> > > > > > > > > -        return EXIT_FAILURE;
> > > > > > > > > -    }
> > > > > > > > > -    return EXIT_SUCCESS;
> > > > > > > > > -}
> > > > > > > > > -
> > > > > > > > > /* returns current time in micro-seconds */
> > > > > > > > > static uint64_t get_time(void)
> > > > > > > > > {
> > > > > > > > > @@ -304,47 +333,6 @@ static void usage(const char
> > > > > > > > > *progname)
> > > > > > > > >  exit(1);
> > > > > > > > > }
> > > > > > > > > 
> > > > > > > > > -void
> > > > > > > > > -Stream::send_cursor(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;
> > > > > > > > > -
> > > > > > > > > -    const uint32_t cursor_msgsize =
> > > > > > > > > -        sizeof(CursorMessage) + width * height *
> > > > > > > > > sizeof(uint32_t);
> > > > > > > > > -    const uint32_t hdrsize  = sizeof(StreamDevHeader);
> > > > > > > > > -
> > > > > > > > > -    std::unique_ptr<uint8_t[]> storage(new
> > > > > > > > > uint8_t[cursor_msgsize]);
> > > > > > > > > -
> > > > > > > > > -    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 = cursor_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->msg.data);
> > > > > > > > > -    fill_cursor(pixels);
> > > > > > > > > -
> > > > > > > > > -    std::lock_guard<std::mutex> stream_guard(mutex);
> > > > > > > > > -    write_all(storage.get(), cursor_msgsize);
> > > > > > > > > -}
> > > > > > > > > -
> > > > > > > > > static void cursor_changes(Stream *stream, Display
> > > > > > > > > *display, int event_base)
> > > > > > > > > {
> > > > > > > > >  unsigned long last_serial = 0;
> > > > > > > > > @@ -363,12 +351,8 @@ static void cursor_changes(Stream
> > > > > > > > > *stream, Display *display, int event_base)
> > > > > > > > >          continue;
> > > > > > > > > 
> > > > > > > > >      last_serial = cursor->cursor_serial;
> > > > > > > > > -        auto fill_cursor = [cursor](uint32_t *pixels) {
> > > > > > > > > -            for (unsigned i = 0; i < cursor->width *
> > > > > > > > > cursor->height; ++i)
> > > > > > > > > -                pixels[i] = cursor->pixels[i];
> > > > > > > > > -        };
> > > > > > > > > -        stream->send_cursor(cursor->width, cursor-
> > > > > > > > > > height,
> > > > > > > > > 
> > > > > > > > > -                            cursor->xhot, cursor->yhot,
> > > > > > > > > fill_cursor);
> > > > > > > > > +        if (!stream->send<X11CursorMessage>(cursor))
> > > > > > > > > +            syslog(LOG_WARNING, "FAILED to send
> > > > > > > > > cursor\n");
> > > > > > > > >  }
> > > > > > > > > }
> > > > > > > > > 
> > > > > > > > > @@ -422,7 +406,7 @@ do_capture(Stream &stream, const char
> > > > > > > > > *streamport, FILE *f_log)
> > > > > > > > > 
> > > > > > > > >              syslog(LOG_DEBUG, "wXh %uX%u  codec=%u\n",
> > > > > > > > > width, height, codec);
> > > > > > > > > 
> > > > > > > > > -                if (stream.send_format(width, height,
> > > > > > > > > codec) == EXIT_FAILURE)
> > > > > > > > > +                if (!stream.send<FormatMessage>(width,
> > > > > > > > > height, codec))
> > > > > > > > >                  throw std::runtime_error("FAILED to
> > > > > > > > > send format message");
> > > > > > > > >          }
> > > > > > > > >          if (f_log) {
> > > > > > > > > @@ -434,7 +418,7 @@ do_capture(Stream &stream, const char
> > > > > > > > > *streamport, FILE *f_log)
> > > > > > > > >                  hexdump(frame.buffer,
> > > > > > > > > frame.buffer_size, f_log);
> > > > > > > > >              }
> > > > > > > > >          }
> > > > > > > > > -            if (stream.send_frame(frame.buffer,
> > > > > > > > > frame.buffer_size) == EXIT_FAILURE) {
> > > > > > > > > +            if (!stream.send<FrameMessage>(frame.buffer,
> > > > > > > > > frame.buffer_size)) {
> > > > > > > > >              syslog(LOG_ERR, "FAILED to send a
> > > > > > > > > frame\n");
> > > > > > > > >              break;
> > > > > > > > >          }
> > > > > > > > 
> > > > > > > > _______________________________________________
> > > > > > > > Spice-devel mailing list
> > > > > > > > Spice-devel@lists.freedesktop.org
> > > > > > > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> > > > > > > 
> > > > > > > 
> > > > > > 
> > > > > > _______________________________________________
> > > > > > Spice-devel mailing list
> > > > > > Spice-devel@lists.freedesktop.org
> > > > > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> > > 
> > > _______________________________________________
> > > Spice-devel mailing list
> > > Spice-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
>
Good idea. Will do it that way.

> On 26 Feb 2018, at 11:05, Lukáš Hrázký <lhrazky@redhat.com> wrote:
> 
> Get rid of the 'make()' method and the 'msg' member, create the payload
> on the stack in 'write()' and write the header and message in two
> writes.
> ---
> It does split the write for FormatMessage into two, but makes the code a
> bit simpler I think. It reduces the number of combinations the messages
> are constructed/written. Untested, but should work...
> 
> src/spice-streaming-agent.cpp | 25 ++++++-------------------
> 1 file changed, 6 insertions(+), 19 deletions(-)
> 
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index c174ea4..109b683 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -95,12 +95,10 @@ struct Message
>               .padding = 0,     // Workaround GCC bug "sorry: not implemented"
>               .type = Info::type,
>               .size = (uint32_t) (Info::size(payload...) - sizeof(hdr))
> -          }),
> -          msg(Info::make(payload...))
> +          })
>     { }
> 
>     StreamDevHeader hdr;
> -    Payload         msg;
> };
> 
> 
> @@ -112,13 +110,10 @@ struct FormatMessage : Message<StreamMsgFormat, FormatMessage>
>     {
>         return sizeof(FormatMessage);
>     }
> -    static StreamMsgFormat make(unsigned w, unsigned h, uint8_t c)
> -    {
> -        return StreamMsgFormat{ .width = w, .height = h, .codec = c, .padding1 = {} };
> -    }
>     size_t write(Stream &stream, unsigned w, unsigned h, uint8_t c)
>     {
> -        return stream.write_all(this, sizeof(*this));
> +        StreamMsgFormat msg{ .width = w, .height = h, .codec = c, .padding1 = {} };
> +        return stream.write_all(&hdr, sizeof(hdr)) + stream.write_all(&msg, sizeof(msg));
>     }
> };
> 
> @@ -131,10 +126,6 @@ struct FrameMessage : Message<StreamMsgData, FrameMessage>
>     {
>         return sizeof(FrameMessage) + length;
>     }
> -    static StreamMsgData make(const void *frame, size_t length)
> -    {
> -        return StreamMsgData();
> -    }
>     size_t write(Stream &stream, const void *frame, size_t length)
>     {
>         return stream.write_all(&hdr, sizeof(hdr)) + stream.write_all(frame, length);
> @@ -158,10 +149,9 @@ struct X11CursorMessage : Message<StreamMsgCursorSet, X11CursorMessage>
>     {
>         return sizeof(X11CursorMessage) + sizeof(uint32_t) * pixel_size(cursor);
>     }
> -    static StreamMsgCursorSet make(XFixesCursorImage *cursor)
> +    size_t write(Stream &stream, XFixesCursorImage *cursor)
>     {
> -        return StreamMsgCursorSet
> -        {
> +        StreamMsgCursorSet msg{
>             .width = cursor->width,
>             .height = cursor->height,
>             .hot_spot_x = cursor->xhot,
> @@ -170,10 +160,7 @@ struct X11CursorMessage : Message<StreamMsgCursorSet, X11CursorMessage>
>             .padding1 = { },
>             .data = { }
>         };
> -    }
> -    size_t write(Stream &stream, XFixesCursorImage *cursor)
> -    {
> -        return stream.write_all(&hdr, sizeof(hdr)) + stream.write_all(pixels.get(), hdr.size);
> +        return stream.write_all(&hdr, sizeof(hdr)) + stream.write_all(&msg, sizeof(msg)) + stream.write_all(pixels.get(), hdr.size);
>     }
>     void fill_pixels(XFixesCursorImage *cursor)
>     {
> -- 
> 2.16.1
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel