[05/17] Use RAII to cleanup stream in case of exception or return

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

Details

Message ID 20180216161547.28110-6-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>

Get rid of C-style 'goto done' in do_capture.
Get rid of global streamfd, pass it around (cleaned up in later patch)
Fixes a race condition, make sure we only use stream after opening

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

Patch hide | download patch | download mbox

diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index 646d15b..9b8fd45 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -53,14 +53,38 @@  struct SpiceStreamDataMessage
     StreamMsgData msg;
 };
 
+struct SpiceStreamCursorMessage
+{
+    StreamDevHeader hdr;
+    StreamMsgCursorSet msg;
+};
+
+class Stream
+{
+public:
+    Stream(const char *name)
+    {
+        fd = open(name, O_RDWR);
+        if (fd < 0)
+            throw std::runtime_error("failed to open streaming device");
+    }
+    ~Stream()
+    {
+        close(fd);
+    }
+    operator int() { return fd; }
+
+private:
+    int fd = -1;
+};
+
 static bool streaming_requested = false;
 static bool quit_requested = false;
 static bool log_binary = false;
 static std::set<SpiceVideoCodecType> client_codecs;
-static int streamfd = -1;
 static std::mutex stream_mtx;
 
-static int have_something_to_read(int timeout)
+static int have_something_to_read(int streamfd, int timeout)
 {
     struct pollfd pollfd = {streamfd, POLLIN, 0};
 
@@ -76,7 +100,7 @@  static int have_something_to_read(int timeout)
     return 0;
 }
 
-static int read_command_from_device(void)
+static int read_command_from_device(int streamfd)
 {
     StreamDevHeader hdr;
     uint8_t msg[64];
@@ -121,18 +145,18 @@  static int read_command_from_device(void)
     return 1;
 }
 
-static int read_command(bool blocking)
+static int read_command(int streamfd, bool blocking)
 {
     int timeout = blocking?-1:0;
     while (!quit_requested) {
-        if (!have_something_to_read(timeout)) {
+        if (!have_something_to_read(streamfd, timeout)) {
             if (!blocking) {
                 return 0;
             }
             sleep(1);
             continue;
         }
-        return read_command_from_device();
+        return read_command_from_device(streamfd);
     }
 
     return 1;
@@ -157,7 +181,7 @@  write_all(int fd, const void *buf, const size_t len)
     return written;
 }
 
-static int spice_stream_send_format(unsigned w, unsigned h, unsigned c)
+static int spice_stream_send_format(int streamfd, unsigned w, unsigned h, unsigned c)
 {
 
     SpiceStreamFormatMessage msg;
@@ -178,7 +202,7 @@  static int spice_stream_send_format(unsigned w, unsigned h, unsigned c)
     return EXIT_SUCCESS;
 }
 
-static int spice_stream_send_frame(const void *buf, const unsigned size)
+static int spice_stream_send_frame(int streamfd, const void *buf, const unsigned size)
 {
     SpiceStreamDataMessage msg;
     const size_t msgsize = sizeof(msg);
@@ -254,7 +278,7 @@  static void usage(const char *progname)
 }
 
 static void
-send_cursor(unsigned width, unsigned height, int hotspot_x, int hotspot_y,
+send_cursor(int streamfd, unsigned width, unsigned height, int hotspot_x, int hotspot_y,
             std::function<void(uint32_t *)> fill_cursor)
 {
     if (width >= STREAM_MSG_CURSOR_SET_MAX_WIDTH ||
@@ -288,7 +312,7 @@  send_cursor(unsigned width, unsigned height, int hotspot_x, int hotspot_y,
     write_all(streamfd, msg.get(), cursor_size);
 }
 
-static void cursor_changes(Display *display, int event_base)
+static void cursor_changes(int streamfd, Display *display, int event_base)
 {
     unsigned long last_serial = 0;
 
@@ -310,25 +334,20 @@  static void cursor_changes(Display *display, int event_base)
             for (unsigned i = 0; i < cursor->width * cursor->height; ++i)
                 pixels[i] = cursor->pixels[i];
         };
-        send_cursor(cursor->width, cursor->height, cursor->xhot, cursor->yhot, fill_cursor);
+        send_cursor(streamfd,
+                    cursor->width, cursor->height, cursor->xhot, cursor->yhot, fill_cursor);
     }
 }
 
 static void
-do_capture(const char *streamport, FILE *f_log)
+do_capture(int streamfd, const char *streamport, FILE *f_log)
 {
-    streamfd = open(streamport, O_RDWR);
-    if (streamfd < 0)
-        throw std::runtime_error("failed to open the streaming device (" +
-                                 std::string(streamport) + "): "
-                                 + strerror(errno));
-
     unsigned int frame_count = 0;
     while (!quit_requested) {
         while (!quit_requested && !streaming_requested) {
-            if (read_command(true) < 0) {
+            if (read_command(streamfd, true) < 0) {
                 syslog(LOG_ERR, "FAILED to read command\n");
-                goto done;
+                return;
             }
         }
 
@@ -370,7 +389,7 @@  do_capture(const char *streamport, FILE *f_log)
 
                 syslog(LOG_DEBUG, "wXh %uX%u  codec=%u\n", width, height, codec);
 
-                if (spice_stream_send_format(width, height, codec) == EXIT_FAILURE)
+                if (spice_stream_send_format(streamfd, width, height, codec) == EXIT_FAILURE)
                     throw std::runtime_error("FAILED to send format message");
             }
             if (f_log) {
@@ -382,23 +401,18 @@  do_capture(const char *streamport, FILE *f_log)
                     hexdump(frame.buffer, frame.buffer_size, f_log);
                 }
             }
-            if (spice_stream_send_frame(frame.buffer, frame.buffer_size) == EXIT_FAILURE) {
+            if (spice_stream_send_frame(streamfd,
+                                        frame.buffer, frame.buffer_size) == EXIT_FAILURE) {
                 syslog(LOG_ERR, "FAILED to send a frame\n");
                 break;
             }
             //usleep(1);
-            if (read_command(false) < 0) {
+            if (read_command(streamfd, false) < 0) {
                 syslog(LOG_ERR, "FAILED to read command\n");
-                goto done;
+                return;
             }
         }
     }
-
-done:
-    if (streamfd >= 0) {
-        close(streamfd);
-        streamfd = -1;
-    }
 }
 
 #define arg_error(...) syslog(LOG_ERR, ## __VA_ARGS__);
@@ -481,12 +495,13 @@  int main(int argc, char* argv[])
     Window rootwindow = DefaultRootWindow(display);
     XFixesSelectCursorInput(display, rootwindow, XFixesDisplayCursorNotifyMask);
 
-    std::thread cursor_th(cursor_changes, display, event_base);
+    Stream streamfd(streamport);
+    std::thread cursor_th(cursor_changes, (int) streamfd, display, event_base);
     cursor_th.detach();
 
     int ret = EXIT_SUCCESS;
     try {
-        do_capture(streamport, f_log);
+        do_capture(streamfd, streamport, f_log);
     }
     catch (std::runtime_error &err) {
         syslog(LOG_ERR, "%s\n", err.what());

Comments

> 
> From: Christophe de Dinechin <dinechin@redhat.com>
> 
> Get rid of C-style 'goto done' in do_capture.
> Get rid of global streamfd, pass it around (cleaned up in later patch)
> Fixes a race condition, make sure we only use stream after opening
> 
> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> ---
>  src/spice-streaming-agent.cpp | 79
>  +++++++++++++++++++++++++------------------
>  1 file changed, 47 insertions(+), 32 deletions(-)
> 
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index 646d15b..9b8fd45 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -53,14 +53,38 @@ struct SpiceStreamDataMessage
>      StreamMsgData msg;
>  };
>  
> +struct SpiceStreamCursorMessage
> +{
> +    StreamDevHeader hdr;
> +    StreamMsgCursorSet msg;
> +};
> +

This is weird in this patch

> +class Stream
> +{
> +public:
> +    Stream(const char *name)
> +    {
> +        fd = open(name, O_RDWR);
> +        if (fd < 0)
> +            throw std::runtime_error("failed to open streaming device");
> +    }
> +    ~Stream()
> +    {
> +        close(fd);

You probably what to check for -1 to avoid calling close(-1)

> +    }
> +    operator int() { return fd; }

Sure you want this? I think is safer to avoid implicit cast
also considering stuff like

  Stream s;
  if (s) ...

> +
> +private:
> +    int fd = -1;

So with a default constructor you want a class with
an invalid state?
Or just disable default constructor.
I would disable also copy.

> +};
> +
>  static bool streaming_requested = false;
>  static bool quit_requested = false;
>  static bool log_binary = false;
>  static std::set<SpiceVideoCodecType> client_codecs;
> -static int streamfd = -1;
>  static std::mutex stream_mtx;
>  
> -static int have_something_to_read(int timeout)
> +static int have_something_to_read(int streamfd, int timeout)

maybe would be better to use the "fd" name here, is no more
bound to stream.

>  {
>      struct pollfd pollfd = {streamfd, POLLIN, 0};
>  
> @@ -76,7 +100,7 @@ static int have_something_to_read(int timeout)
>      return 0;
>  }
>  
> -static int read_command_from_device(void)
> +static int read_command_from_device(int streamfd)

Have you considered Stream::read_command?
Ok, mostly in a following patch

>  {
>      StreamDevHeader hdr;
>      uint8_t msg[64];
> @@ -121,18 +145,18 @@ static int read_command_from_device(void)
>      return 1;
>  }
>  
> -static int read_command(bool blocking)
> +static int read_command(int streamfd, bool blocking)
>  {
>      int timeout = blocking?-1:0;
>      while (!quit_requested) {
> -        if (!have_something_to_read(timeout)) {
> +        if (!have_something_to_read(streamfd, timeout)) {
>              if (!blocking) {
>                  return 0;
>              }
>              sleep(1);
>              continue;
>          }
> -        return read_command_from_device();
> +        return read_command_from_device(streamfd);
>      }
>  
>      return 1;
> @@ -157,7 +181,7 @@ write_all(int fd, const void *buf, const size_t len)
>      return written;
>  }
>  
> -static int spice_stream_send_format(unsigned w, unsigned h, unsigned c)
> +static int spice_stream_send_format(int streamfd, unsigned w, unsigned h,
> unsigned c)
>  {
>  
>      SpiceStreamFormatMessage msg;
> @@ -178,7 +202,7 @@ static int spice_stream_send_format(unsigned w, unsigned
> h, unsigned c)
>      return EXIT_SUCCESS;
>  }
>  
> -static int spice_stream_send_frame(const void *buf, const unsigned size)
> +static int spice_stream_send_frame(int streamfd, const void *buf, const
> unsigned size)
>  {
>      SpiceStreamDataMessage msg;
>      const size_t msgsize = sizeof(msg);
> @@ -254,7 +278,7 @@ static void usage(const char *progname)
>  }
>  
>  static void
> -send_cursor(unsigned width, unsigned height, int hotspot_x, int hotspot_y,
> +send_cursor(int streamfd, unsigned width, unsigned height, int hotspot_x,
> int hotspot_y,
>              std::function<void(uint32_t *)> fill_cursor)
>  {
>      if (width >= STREAM_MSG_CURSOR_SET_MAX_WIDTH ||
> @@ -288,7 +312,7 @@ send_cursor(unsigned width, unsigned height, int
> hotspot_x, int hotspot_y,
>      write_all(streamfd, msg.get(), cursor_size);
>  }
>  
> -static void cursor_changes(Display *display, int event_base)
> +static void cursor_changes(int streamfd, Display *display, int event_base)
>  {
>      unsigned long last_serial = 0;
>  
> @@ -310,25 +334,20 @@ static void cursor_changes(Display *display, int
> event_base)
>              for (unsigned i = 0; i < cursor->width * cursor->height; ++i)
>                  pixels[i] = cursor->pixels[i];
>          };
> -        send_cursor(cursor->width, cursor->height, cursor->xhot,
> cursor->yhot, fill_cursor);
> +        send_cursor(streamfd,
> +                    cursor->width, cursor->height, cursor->xhot,
> cursor->yhot, fill_cursor);
>      }
>  }
>  
>  static void
> -do_capture(const char *streamport, FILE *f_log)
> +do_capture(int streamfd, const char *streamport, FILE *f_log)
>  {
> -    streamfd = open(streamport, O_RDWR);
> -    if (streamfd < 0)
> -        throw std::runtime_error("failed to open the streaming device (" +
> -                                 std::string(streamport) + "): "
> -                                 + strerror(errno));
> -
>      unsigned int frame_count = 0;
>      while (!quit_requested) {
>          while (!quit_requested && !streaming_requested) {
> -            if (read_command(true) < 0) {
> +            if (read_command(streamfd, true) < 0) {
>                  syslog(LOG_ERR, "FAILED to read command\n");
> -                goto done;
> +                return;
>              }
>          }
>  
> @@ -370,7 +389,7 @@ do_capture(const char *streamport, FILE *f_log)
>  
>                  syslog(LOG_DEBUG, "wXh %uX%u  codec=%u\n", width, height,
>                  codec);
>  
> -                if (spice_stream_send_format(width, height, codec) ==
> EXIT_FAILURE)
> +                if (spice_stream_send_format(streamfd, width, height, codec)
> == EXIT_FAILURE)
>                      throw std::runtime_error("FAILED to send format
>                      message");
>              }
>              if (f_log) {
> @@ -382,23 +401,18 @@ do_capture(const char *streamport, FILE *f_log)
>                      hexdump(frame.buffer, frame.buffer_size, f_log);
>                  }
>              }
> -            if (spice_stream_send_frame(frame.buffer, frame.buffer_size) ==
> EXIT_FAILURE) {
> +            if (spice_stream_send_frame(streamfd,
> +                                        frame.buffer, frame.buffer_size) ==
> EXIT_FAILURE) {
>                  syslog(LOG_ERR, "FAILED to send a frame\n");
>                  break;
>              }
>              //usleep(1);
> -            if (read_command(false) < 0) {
> +            if (read_command(streamfd, false) < 0) {
>                  syslog(LOG_ERR, "FAILED to read command\n");
> -                goto done;
> +                return;
>              }
>          }
>      }
> -
> -done:
> -    if (streamfd >= 0) {
> -        close(streamfd);
> -        streamfd = -1;
> -    }
>  }
>  
>  #define arg_error(...) syslog(LOG_ERR, ## __VA_ARGS__);
> @@ -481,12 +495,13 @@ int main(int argc, char* argv[])
>      Window rootwindow = DefaultRootWindow(display);
>      XFixesSelectCursorInput(display, rootwindow,
>      XFixesDisplayCursorNotifyMask);
>  
> -    std::thread cursor_th(cursor_changes, display, event_base);
> +    Stream streamfd(streamport);
> +    std::thread cursor_th(cursor_changes, (int) streamfd, display,
> event_base);
>      cursor_th.detach();
>  
>      int ret = EXIT_SUCCESS;
>      try {
> -        do_capture(streamport, f_log);
> +        do_capture(streamfd, streamport, f_log);

Not a fun of this implicit conversion.

>      }
>      catch (std::runtime_error &err) {
>          syslog(LOG_ERR, "%s\n", err.what());

Frediano
> On 16 Feb 2018, at 17:40, Frediano Ziglio <fziglio@redhat.com> wrote:
> 
>> 
>> From: Christophe de Dinechin <dinechin@redhat.com>
>> 
>> Get rid of C-style 'goto done' in do_capture.
>> Get rid of global streamfd, pass it around (cleaned up in later patch)
>> Fixes a race condition, make sure we only use stream after opening
>> 
>> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
>> ---
>> src/spice-streaming-agent.cpp | 79
>> +++++++++++++++++++++++++------------------
>> 1 file changed, 47 insertions(+), 32 deletions(-)
>> 
>> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
>> index 646d15b..9b8fd45 100644
>> --- a/src/spice-streaming-agent.cpp
>> +++ b/src/spice-streaming-agent.cpp
>> @@ -53,14 +53,38 @@ struct SpiceStreamDataMessage
>>     StreamMsgData msg;
>> };
>> 
>> +struct SpiceStreamCursorMessage
>> +{
>> +    StreamDevHeader hdr;
>> +    StreamMsgCursorSet msg;
>> +};
>> +
> 
> This is weird in this patch

Yes. I may have merged two patches. Will check.

> 
>> +class Stream
>> +{
>> +public:
>> +    Stream(const char *name)
>> +    {
>> +        fd = open(name, O_RDWR);
>> +        if (fd < 0)
>> +            throw std::runtime_error("failed to open streaming device");
>> +    }
>> +    ~Stream()
>> +    {
>> +        close(fd);
> 
> You probably what to check for -1 to avoid calling close(-1)

I think this cannot happen, since in that case you’d have thrown from the ctor.

> 
>> +    }
>> +    operator int() { return fd; }
> 
> Sure you want this? I think is safer to avoid implicit cast
> also considering stuff like
> 
>  Stream s;
>  if (s) …

It is removed in a later patch once it’s no longer needed. The objective here is to minimize noisy changes at each step.

> 
>> +
>> +private:
>> +    int fd = -1;
> 
> So with a default constructor you want a class with
> an invalid state?
> Or just disable default constructor.

It’s disabled, there’s a ctor with args.

> I would disable also copy.

Also implicitly disabled.

> 
>> +};
>> +
>> static bool streaming_requested = false;
>> static bool quit_requested = false;
>> static bool log_binary = false;
>> static std::set<SpiceVideoCodecType> client_codecs;
>> -static int streamfd = -1;
>> static std::mutex stream_mtx;
>> 
>> -static int have_something_to_read(int timeout)
>> +static int have_something_to_read(int streamfd, int timeout)
> 
> maybe would be better to use the "fd" name here, is no more
> bound to stream.

Kept to minimize changes

>> {
>>     struct pollfd pollfd = {streamfd, POLLIN, 0};
>> 
>> @@ -76,7 +100,7 @@ static int have_something_to_read(int timeout)
>>     return 0;
>> }
>> 
>> -static int read_command_from_device(void)
>> +static int read_command_from_device(int streamfd)
> 
> Have you considered Stream::read_command?

For that specific case, I liked the “from_device”. But…

> Ok, mostly in a following patch


> 
>> {
>>     StreamDevHeader hdr;
>>     uint8_t msg[64];
>> @@ -121,18 +145,18 @@ static int read_command_from_device(void)
>>     return 1;
>> }
>> 
>> -static int read_command(bool blocking)
>> +static int read_command(int streamfd, bool blocking)
>> {
>>     int timeout = blocking?-1:0;
>>     while (!quit_requested) {
>> -        if (!have_something_to_read(timeout)) {
>> +        if (!have_something_to_read(streamfd, timeout)) {
>>             if (!blocking) {
>>                 return 0;
>>             }
>>             sleep(1);
>>             continue;
>>         }
>> -        return read_command_from_device();
>> +        return read_command_from_device(streamfd);
>>     }
>> 
>>     return 1;
>> @@ -157,7 +181,7 @@ write_all(int fd, const void *buf, const size_t len)
>>     return written;
>> }
>> 
>> -static int spice_stream_send_format(unsigned w, unsigned h, unsigned c)
>> +static int spice_stream_send_format(int streamfd, unsigned w, unsigned h,
>> unsigned c)
>> {
>> 
>>     SpiceStreamFormatMessage msg;
>> @@ -178,7 +202,7 @@ static int spice_stream_send_format(unsigned w, unsigned
>> h, unsigned c)
>>     return EXIT_SUCCESS;
>> }
>> 
>> -static int spice_stream_send_frame(const void *buf, const unsigned size)
>> +static int spice_stream_send_frame(int streamfd, const void *buf, const
>> unsigned size)
>> {
>>     SpiceStreamDataMessage msg;
>>     const size_t msgsize = sizeof(msg);
>> @@ -254,7 +278,7 @@ static void usage(const char *progname)
>> }
>> 
>> static void
>> -send_cursor(unsigned width, unsigned height, int hotspot_x, int hotspot_y,
>> +send_cursor(int streamfd, unsigned width, unsigned height, int hotspot_x,
>> int hotspot_y,
>>             std::function<void(uint32_t *)> fill_cursor)
>> {
>>     if (width >= STREAM_MSG_CURSOR_SET_MAX_WIDTH ||
>> @@ -288,7 +312,7 @@ send_cursor(unsigned width, unsigned height, int
>> hotspot_x, int hotspot_y,
>>     write_all(streamfd, msg.get(), cursor_size);
>> }
>> 
>> -static void cursor_changes(Display *display, int event_base)
>> +static void cursor_changes(int streamfd, Display *display, int event_base)
>> {
>>     unsigned long last_serial = 0;
>> 
>> @@ -310,25 +334,20 @@ static void cursor_changes(Display *display, int
>> event_base)
>>             for (unsigned i = 0; i < cursor->width * cursor->height; ++i)
>>                 pixels[i] = cursor->pixels[i];
>>         };
>> -        send_cursor(cursor->width, cursor->height, cursor->xhot,
>> cursor->yhot, fill_cursor);
>> +        send_cursor(streamfd,
>> +                    cursor->width, cursor->height, cursor->xhot,
>> cursor->yhot, fill_cursor);
>>     }
>> }
>> 
>> static void
>> -do_capture(const char *streamport, FILE *f_log)
>> +do_capture(int streamfd, const char *streamport, FILE *f_log)
>> {
>> -    streamfd = open(streamport, O_RDWR);
>> -    if (streamfd < 0)
>> -        throw std::runtime_error("failed to open the streaming device (" +
>> -                                 std::string(streamport) + "): "
>> -                                 + strerror(errno));
>> -
>>     unsigned int frame_count = 0;
>>     while (!quit_requested) {
>>         while (!quit_requested && !streaming_requested) {
>> -            if (read_command(true) < 0) {
>> +            if (read_command(streamfd, true) < 0) {
>>                 syslog(LOG_ERR, "FAILED to read command\n");
>> -                goto done;
>> +                return;
>>             }
>>         }
>> 
>> @@ -370,7 +389,7 @@ do_capture(const char *streamport, FILE *f_log)
>> 
>>                 syslog(LOG_DEBUG, "wXh %uX%u  codec=%u\n", width, height,
>>                 codec);
>> 
>> -                if (spice_stream_send_format(width, height, codec) ==
>> EXIT_FAILURE)
>> +                if (spice_stream_send_format(streamfd, width, height, codec)
>> == EXIT_FAILURE)
>>                     throw std::runtime_error("FAILED to send format
>>                     message");
>>             }
>>             if (f_log) {
>> @@ -382,23 +401,18 @@ do_capture(const char *streamport, FILE *f_log)
>>                     hexdump(frame.buffer, frame.buffer_size, f_log);
>>                 }
>>             }
>> -            if (spice_stream_send_frame(frame.buffer, frame.buffer_size) ==
>> EXIT_FAILURE) {
>> +            if (spice_stream_send_frame(streamfd,
>> +                                        frame.buffer, frame.buffer_size) ==
>> EXIT_FAILURE) {
>>                 syslog(LOG_ERR, "FAILED to send a frame\n");
>>                 break;
>>             }
>>             //usleep(1);
>> -            if (read_command(false) < 0) {
>> +            if (read_command(streamfd, false) < 0) {
>>                 syslog(LOG_ERR, "FAILED to read command\n");
>> -                goto done;
>> +                return;
>>             }
>>         }
>>     }
>> -
>> -done:
>> -    if (streamfd >= 0) {
>> -        close(streamfd);
>> -        streamfd = -1;
>> -    }
>> }
>> 
>> #define arg_error(...) syslog(LOG_ERR, ## __VA_ARGS__);
>> @@ -481,12 +495,13 @@ int main(int argc, char* argv[])
>>     Window rootwindow = DefaultRootWindow(display);
>>     XFixesSelectCursorInput(display, rootwindow,
>>     XFixesDisplayCursorNotifyMask);
>> 
>> -    std::thread cursor_th(cursor_changes, display, event_base);
>> +    Stream streamfd(streamport);
>> +    std::thread cursor_th(cursor_changes, (int) streamfd, display,
>> event_base);
>>     cursor_th.detach();
>> 
>>     int ret = EXIT_SUCCESS;
>>     try {
>> -        do_capture(streamport, f_log);
>> +        do_capture(streamfd, streamport, f_log);
> 
> Not a fun of this implicit conversion

Well, we can make it explicitl. But this is temporary, and is suppressed by the end of the series.

> .
> 
>>     }
>>     catch (std::runtime_error &err) {
>>         syslog(LOG_ERR, "%s\n", err.what());
> 
> Frediano
On Fri, 2018-02-16 at 17:59 +0100, Christophe de Dinechin wrote:
> > On 16 Feb 2018, at 17:40, Frediano Ziglio <fziglio@redhat.com> wrote:
> > 
> > > 
> > > From: Christophe de Dinechin <dinechin@redhat.com>
> > > 
> > > Get rid of C-style 'goto done' in do_capture.
> > > Get rid of global streamfd, pass it around (cleaned up in later patch)
> > > Fixes a race condition, make sure we only use stream after opening
> > > 
> > > Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> > > ---
> > > src/spice-streaming-agent.cpp | 79
> > > +++++++++++++++++++++++++------------------
> > > 1 file changed, 47 insertions(+), 32 deletions(-)
> > > 
> > > diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> > > index 646d15b..9b8fd45 100644
> > > --- a/src/spice-streaming-agent.cpp
> > > +++ b/src/spice-streaming-agent.cpp
> > > @@ -53,14 +53,38 @@ struct SpiceStreamDataMessage
> > >     StreamMsgData msg;
> > > };
> > > 
> > > +struct SpiceStreamCursorMessage
> > > +{
> > > +    StreamDevHeader hdr;
> > > +    StreamMsgCursorSet msg;
> > > +};
> > > +
> > 
> > This is weird in this patch
> 
> Yes. I may have merged two patches. Will check.
> 
> > 
> > > +class Stream
> > > +{
> > > +public:
> > > +    Stream(const char *name)
> > > +    {
> > > +        fd = open(name, O_RDWR);
> > > +        if (fd < 0)
> > > +            throw std::runtime_error("failed to open streaming device");

Braces around if body (according to the coding style). Repeated a few
times below.

> > > +    }
> > > +    ~Stream()
> > > +    {
> > > +        close(fd);
> > 
> > You probably what to check for -1 to avoid calling close(-1)
> 
> I think this cannot happen, since in that case you’d have thrown from the ctor.
> 
> > 
> > > +    }
> > > +    operator int() { return fd; }
> > 
> > Sure you want this? I think is safer to avoid implicit cast
> > also considering stuff like
> > 
> >  Stream s;
> >  if (s) …
> 
> It is removed in a later patch once it’s no longer needed. The objective here is to minimize noisy changes at each step.

I'd also rather not see it, but if it's removed later, no big deal.

> > 
> > > +
> > > +private:
> > > +    int fd = -1;
> > 
> > So with a default constructor you want a class with
> > an invalid state?
> > Or just disable default constructor.
> 
> It’s disabled, there’s a ctor with args.
> 
> > I would disable also copy.
> 
> Also implicitly disabled.

It is not, actually, disabled here. It gets disabled later when you add
the mutex member, as that is a non-copyable type.

It might be a good idea to explicitly delete the copy ctr/ao anyway...
In case someone ever removes the mutex and doesn't realize this.

> > 
> > > +};
> > > +
> > > static bool streaming_requested = false;
> > > static bool quit_requested = false;
> > > static bool log_binary = false;
> > > static std::set<SpiceVideoCodecType> client_codecs;
> > > -static int streamfd = -1;
> > > static std::mutex stream_mtx;
> > > 
> > > -static int have_something_to_read(int timeout)
> > > +static int have_something_to_read(int streamfd, int timeout)
> > 
> > maybe would be better to use the "fd" name here, is no more
> > bound to stream.
> 
> Kept to minimize changes
> 
> > > {
> > >     struct pollfd pollfd = {streamfd, POLLIN, 0};
> > > 
> > > @@ -76,7 +100,7 @@ static int have_something_to_read(int timeout)
> > >     return 0;
> > > }
> > > 
> > > -static int read_command_from_device(void)
> > > +static int read_command_from_device(int streamfd)
> > 
> > Have you considered Stream::read_command?
> 
> For that specific case, I liked the “from_device”. But…
> 
> > Ok, mostly in a following patch
> 
> 
> > 
> > > {
> > >     StreamDevHeader hdr;
> > >     uint8_t msg[64];
> > > @@ -121,18 +145,18 @@ static int read_command_from_device(void)
> > >     return 1;
> > > }
> > > 
> > > -static int read_command(bool blocking)
> > > +static int read_command(int streamfd, bool blocking)
> > > {
> > >     int timeout = blocking?-1:0;
> > >     while (!quit_requested) {
> > > -        if (!have_something_to_read(timeout)) {
> > > +        if (!have_something_to_read(streamfd, timeout)) {
> > >             if (!blocking) {
> > >                 return 0;
> > >             }
> > >             sleep(1);
> > >             continue;
> > >         }
> > > -        return read_command_from_device();
> > > +        return read_command_from_device(streamfd);
> > >     }
> > > 
> > >     return 1;
> > > @@ -157,7 +181,7 @@ write_all(int fd, const void *buf, const size_t len)
> > >     return written;
> > > }
> > > 
> > > -static int spice_stream_send_format(unsigned w, unsigned h, unsigned c)
> > > +static int spice_stream_send_format(int streamfd, unsigned w, unsigned h,
> > > unsigned c)
> > > {
> > > 
> > >     SpiceStreamFormatMessage msg;
> > > @@ -178,7 +202,7 @@ static int spice_stream_send_format(unsigned w, unsigned
> > > h, unsigned c)
> > >     return EXIT_SUCCESS;
> > > }
> > > 
> > > -static int spice_stream_send_frame(const void *buf, const unsigned size)
> > > +static int spice_stream_send_frame(int streamfd, const void *buf, const
> > > unsigned size)
> > > {
> > >     SpiceStreamDataMessage msg;
> > >     const size_t msgsize = sizeof(msg);
> > > @@ -254,7 +278,7 @@ static void usage(const char *progname)
> > > }
> > > 
> > > static void
> > > -send_cursor(unsigned width, unsigned height, int hotspot_x, int hotspot_y,
> > > +send_cursor(int streamfd, unsigned width, unsigned height, int hotspot_x,
> > > int hotspot_y,
> > >             std::function<void(uint32_t *)> fill_cursor)
> > > {
> > >     if (width >= STREAM_MSG_CURSOR_SET_MAX_WIDTH ||
> > > @@ -288,7 +312,7 @@ send_cursor(unsigned width, unsigned height, int
> > > hotspot_x, int hotspot_y,
> > >     write_all(streamfd, msg.get(), cursor_size);
> > > }
> > > 
> > > -static void cursor_changes(Display *display, int event_base)
> > > +static void cursor_changes(int streamfd, Display *display, int event_base)
> > > {
> > >     unsigned long last_serial = 0;
> > > 
> > > @@ -310,25 +334,20 @@ static void cursor_changes(Display *display, int
> > > event_base)
> > >             for (unsigned i = 0; i < cursor->width * cursor->height; ++i)
> > >                 pixels[i] = cursor->pixels[i];
> > >         };
> > > -        send_cursor(cursor->width, cursor->height, cursor->xhot,
> > > cursor->yhot, fill_cursor);
> > > +        send_cursor(streamfd,
> > > +                    cursor->width, cursor->height, cursor->xhot,
> > > cursor->yhot, fill_cursor);
> > >     }
> > > }
> > > 
> > > static void
> > > -do_capture(const char *streamport, FILE *f_log)
> > > +do_capture(int streamfd, const char *streamport, FILE *f_log)
> > > {
> > > -    streamfd = open(streamport, O_RDWR);
> > > -    if (streamfd < 0)
> > > -        throw std::runtime_error("failed to open the streaming device (" +
> > > -                                 std::string(streamport) + "): "
> > > -                                 + strerror(errno));
> > > -
> > >     unsigned int frame_count = 0;
> > >     while (!quit_requested) {
> > >         while (!quit_requested && !streaming_requested) {
> > > -            if (read_command(true) < 0) {
> > > +            if (read_command(streamfd, true) < 0) {
> > >                 syslog(LOG_ERR, "FAILED to read command\n");
> > > -                goto done;
> > > +                return;
> > >             }
> > >         }
> > > 
> > > @@ -370,7 +389,7 @@ do_capture(const char *streamport, FILE *f_log)
> > > 
> > >                 syslog(LOG_DEBUG, "wXh %uX%u  codec=%u\n", width, height,
> > >                 codec);
> > > 
> > > -                if (spice_stream_send_format(width, height, codec) ==
> > > EXIT_FAILURE)
> > > +                if (spice_stream_send_format(streamfd, width, height, codec)
> > > == EXIT_FAILURE)
> > >                     throw std::runtime_error("FAILED to send format
> > >                     message");
> > >             }
> > >             if (f_log) {
> > > @@ -382,23 +401,18 @@ do_capture(const char *streamport, FILE *f_log)
> > >                     hexdump(frame.buffer, frame.buffer_size, f_log);
> > >                 }
> > >             }
> > > -            if (spice_stream_send_frame(frame.buffer, frame.buffer_size) ==
> > > EXIT_FAILURE) {
> > > +            if (spice_stream_send_frame(streamfd,
> > > +                                        frame.buffer, frame.buffer_size) ==
> > > EXIT_FAILURE) {
> > >                 syslog(LOG_ERR, "FAILED to send a frame\n");
> > >                 break;
> > >             }
> > >             //usleep(1);
> > > -            if (read_command(false) < 0) {
> > > +            if (read_command(streamfd, false) < 0) {
> > >                 syslog(LOG_ERR, "FAILED to read command\n");
> > > -                goto done;
> > > +                return;
> > >             }
> > >         }
> > >     }
> > > -
> > > -done:
> > > -    if (streamfd >= 0) {
> > > -        close(streamfd);
> > > -        streamfd = -1;
> > > -    }
> > > }
> > > 
> > > #define arg_error(...) syslog(LOG_ERR, ## __VA_ARGS__);
> > > @@ -481,12 +495,13 @@ int main(int argc, char* argv[])
> > >     Window rootwindow = DefaultRootWindow(display);
> > >     XFixesSelectCursorInput(display, rootwindow,
> > >     XFixesDisplayCursorNotifyMask);
> > > 
> > > -    std::thread cursor_th(cursor_changes, display, event_base);
> > > +    Stream streamfd(streamport);

Better call it 'stream', it's not just a fd anymore. Along with the
implicit conversion, this is quite confusing.

Lukas

> > > +    std::thread cursor_th(cursor_changes, (int) streamfd, display,
> > > event_base);
> > >     cursor_th.detach();
> > > 
> > >     int ret = EXIT_SUCCESS;
> > >     try {
> > > -        do_capture(streamport, f_log);
> > > +        do_capture(streamfd, streamport, f_log);
> > 
> > Not a fun of this implicit conversion
> 
> Well, we can make it explicitl. But this is temporary, and is suppressed by the end of the series.
> 
> > .
> > 
> > >     }
> > >     catch (std::runtime_error &err) {
> > >         syslog(LOG_ERR, "%s\n", err.what());
> > 
> > Frediano
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Forgot to send, sorry for delay]

> On 19 Feb 2018, at 19:03, Lukáš Hrázký <lhrazky@redhat.com> wrote:
> 
> On Fri, 2018-02-16 at 17:59 +0100, Christophe de Dinechin wrote:
>>> On 16 Feb 2018, at 17:40, Frediano Ziglio <fziglio@redhat.com> wrote:
>>> 
>>>> 
>>>> From: Christophe de Dinechin <dinechin@redhat.com>
>>>> 
>>>> Get rid of C-style 'goto done' in do_capture.
>>>> Get rid of global streamfd, pass it around (cleaned up in later patch)
>>>> Fixes a race condition, make sure we only use stream after opening
>>>> 
>>>> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
>>>> ---
>>>> src/spice-streaming-agent.cpp | 79
>>>> +++++++++++++++++++++++++------------------
>>>> 1 file changed, 47 insertions(+), 32 deletions(-)
>>>> 
>>>> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
>>>> index 646d15b..9b8fd45 100644
>>>> --- a/src/spice-streaming-agent.cpp
>>>> +++ b/src/spice-streaming-agent.cpp
>>>> @@ -53,14 +53,38 @@ struct SpiceStreamDataMessage
>>>>    StreamMsgData msg;
>>>> };
>>>> 
>>>> +struct SpiceStreamCursorMessage
>>>> +{
>>>> +    StreamDevHeader hdr;
>>>> +    StreamMsgCursorSet msg;
>>>> +};
>>>> +
>>> 
>>> This is weird in this patch
>> 
>> Yes. I may have merged two patches. Will check.
>> 
>>> 
>>>> +class Stream
>>>> +{
>>>> +public:
>>>> +    Stream(const char *name)
>>>> +    {
>>>> +        fd = open(name, O_RDWR);
>>>> +        if (fd < 0)
>>>> +            throw std::runtime_error("failed to open streaming device");
> 
> Braces around if body (according to the coding style). Repeated a few
> times below.
> 
>>>> +    }
>>>> +    ~Stream()
>>>> +    {
>>>> +        close(fd);
>>> 
>>> You probably what to check for -1 to avoid calling close(-1)
>> 
>> I think this cannot happen, since in that case you’d have thrown from the ctor.
>> 
>>> 
>>>> +    }
>>>> +    operator int() { return fd; }
>>> 
>>> Sure you want this? I think is safer to avoid implicit cast
>>> also considering stuff like
>>> 
>>> Stream s;
>>> if (s) …
>> 
>> It is removed in a later patch once it’s no longer needed. The objective here is to minimize noisy changes at each step.
> 
> I'd also rather not see it, but if it's removed later, no big deal.

So I feel the general agreement is that it’s better to add an explicit getter and use that rather than implicitly convert.

> 
>>> 
>>>> +
>>>> +private:
>>>> +    int fd = -1;
>>> 
>>> So with a default constructor you want a class with
>>> an invalid state?
>>> Or just disable default constructor.
>> 
>> It’s disabled, there’s a ctor with args.
>> 
>>> I would disable also copy.
>> 
>> Also implicitly disabled.
> 
> It is not, actually, disabled here. It gets disabled later when you add
> the mutex member, as that is a non-copyable type.

You are right, of course. But like for the above, the idea is to minimize “noise” in the series.

Of course, I could explicitly delete constructors and assignment. I feel, but I may be wrong, that having a mutex field “says that”, both in terms of language safety and in terms of “what it means” (i.e. you are really creating a class for the purpose of RAII and thread safety).

If you think it’s better to add a few lines, I’m OK with it, but I don’t like it much as I read it more as noise than anything else. There are a few choices here, roughly in my personal order of preference:
1. Introduce = delete in this patch, remove it when mutex is added
2. Introduce = delete in this patch and leave it there even when it becomes “useless"
3 Leave a temporary “unsafe” state for a couple of commits
4. Merge the two and make sure there’s the mutex at the beginning

As a side note, this reminds me of a talk I highly recommend, https://www.youtube.com/watch?v=4AfRAVcThyA in case you have not seen it. Among other things, Herb Sutter proposes a $class mechanism that makes it possible to define a class of classes, e.g. “interface”, so that you explain with $class interface { … } what an interface is, and then you can write “interface X { .. }” and have an interface class (e.g. only pure virtual methods, etc).

Here, I see the mutex as indicative of a “locked resource” class, which I understand as having only explicit ctors for initialization and nothing else to consider, no copy, no assignment, etc. But it could also be made explicit with this mechanism. Post C++ 20, so not right away…

> 
> It might be a good idea to explicitly delete the copy ctr/ao anyway...
> In case someone ever removes the mutex and doesn't realize this.

That’s my choice #2, it could easily become #1.

> 
>>> 
>>>> +};
>>>> +
>>>> static bool streaming_requested = false;
>>>> static bool quit_requested = false;
>>>> static bool log_binary = false;
>>>> static std::set<SpiceVideoCodecType> client_codecs;
>>>> -static int streamfd = -1;
>>>> static std::mutex stream_mtx;
>>>> 
>>>> -static int have_something_to_read(int timeout)
>>>> +static int have_something_to_read(int streamfd, int timeout)
>>> 
>>> maybe would be better to use the "fd" name here, is no more
>>> bound to stream.
>> 
>> Kept to minimize changes
>> 
>>>> {
>>>>    struct pollfd pollfd = {streamfd, POLLIN, 0};
>>>> 
>>>> @@ -76,7 +100,7 @@ static int have_something_to_read(int timeout)
>>>>    return 0;
>>>> }
>>>> 
>>>> -static int read_command_from_device(void)
>>>> +static int read_command_from_device(int streamfd)
>>> 
>>> Have you considered Stream::read_command?
>> 
>> For that specific case, I liked the “from_device”. But…
>> 
>>> Ok, mostly in a following patch
>> 
>> 
>>> 
>>>> {
>>>>    StreamDevHeader hdr;
>>>>    uint8_t msg[64];
>>>> @@ -121,18 +145,18 @@ static int read_command_from_device(void)
>>>>    return 1;
>>>> }
>>>> 
>>>> -static int read_command(bool blocking)
>>>> +static int read_command(int streamfd, bool blocking)
>>>> {
>>>>    int timeout = blocking?-1:0;
>>>>    while (!quit_requested) {
>>>> -        if (!have_something_to_read(timeout)) {
>>>> +        if (!have_something_to_read(streamfd, timeout)) {
>>>>            if (!blocking) {
>>>>                return 0;
>>>>            }
>>>>            sleep(1);
>>>>            continue;
>>>>        }
>>>> -        return read_command_from_device();
>>>> +        return read_command_from_device(streamfd);
>>>>    }
>>>> 
>>>>    return 1;
>>>> @@ -157,7 +181,7 @@ write_all(int fd, const void *buf, const size_t len)
>>>>    return written;
>>>> }
>>>> 
>>>> -static int spice_stream_send_format(unsigned w, unsigned h, unsigned c)
>>>> +static int spice_stream_send_format(int streamfd, unsigned w, unsigned h,
>>>> unsigned c)
>>>> {
>>>> 
>>>>    SpiceStreamFormatMessage msg;
>>>> @@ -178,7 +202,7 @@ static int spice_stream_send_format(unsigned w, unsigned
>>>> h, unsigned c)
>>>>    return EXIT_SUCCESS;
>>>> }
>>>> 
>>>> -static int spice_stream_send_frame(const void *buf, const unsigned size)
>>>> +static int spice_stream_send_frame(int streamfd, const void *buf, const
>>>> unsigned size)
>>>> {
>>>>    SpiceStreamDataMessage msg;
>>>>    const size_t msgsize = sizeof(msg);
>>>> @@ -254,7 +278,7 @@ static void usage(const char *progname)
>>>> }
>>>> 
>>>> static void
>>>> -send_cursor(unsigned width, unsigned height, int hotspot_x, int hotspot_y,
>>>> +send_cursor(int streamfd, unsigned width, unsigned height, int hotspot_x,
>>>> int hotspot_y,
>>>>            std::function<void(uint32_t *)> fill_cursor)
>>>> {
>>>>    if (width >= STREAM_MSG_CURSOR_SET_MAX_WIDTH ||
>>>> @@ -288,7 +312,7 @@ send_cursor(unsigned width, unsigned height, int
>>>> hotspot_x, int hotspot_y,
>>>>    write_all(streamfd, msg.get(), cursor_size);
>>>> }
>>>> 
>>>> -static void cursor_changes(Display *display, int event_base)
>>>> +static void cursor_changes(int streamfd, Display *display, int event_base)
>>>> {
>>>>    unsigned long last_serial = 0;
>>>> 
>>>> @@ -310,25 +334,20 @@ static void cursor_changes(Display *display, int
>>>> event_base)
>>>>            for (unsigned i = 0; i < cursor->width * cursor->height; ++i)
>>>>                pixels[i] = cursor->pixels[i];
>>>>        };
>>>> -        send_cursor(cursor->width, cursor->height, cursor->xhot,
>>>> cursor->yhot, fill_cursor);
>>>> +        send_cursor(streamfd,
>>>> +                    cursor->width, cursor->height, cursor->xhot,
>>>> cursor->yhot, fill_cursor);
>>>>    }
>>>> }
>>>> 
>>>> static void
>>>> -do_capture(const char *streamport, FILE *f_log)
>>>> +do_capture(int streamfd, const char *streamport, FILE *f_log)
>>>> {
>>>> -    streamfd = open(streamport, O_RDWR);
>>>> -    if (streamfd < 0)
>>>> -        throw std::runtime_error("failed to open the streaming device (" +
>>>> -                                 std::string(streamport) + "): "
>>>> -                                 + strerror(errno));
>>>> -
>>>>    unsigned int frame_count = 0;
>>>>    while (!quit_requested) {
>>>>        while (!quit_requested && !streaming_requested) {
>>>> -            if (read_command(true) < 0) {
>>>> +            if (read_command(streamfd, true) < 0) {
>>>>                syslog(LOG_ERR, "FAILED to read command\n");
>>>> -                goto done;
>>>> +                return;
>>>>            }
>>>>        }
>>>> 
>>>> @@ -370,7 +389,7 @@ do_capture(const char *streamport, FILE *f_log)
>>>> 
>>>>                syslog(LOG_DEBUG, "wXh %uX%u  codec=%u\n", width, height,
>>>>                codec);
>>>> 
>>>> -                if (spice_stream_send_format(width, height, codec) ==
>>>> EXIT_FAILURE)
>>>> +                if (spice_stream_send_format(streamfd, width, height, codec)
>>>> == EXIT_FAILURE)
>>>>                    throw std::runtime_error("FAILED to send format
>>>>                    message");
>>>>            }
>>>>            if (f_log) {
>>>> @@ -382,23 +401,18 @@ do_capture(const char *streamport, FILE *f_log)
>>>>                    hexdump(frame.buffer, frame.buffer_size, f_log);
>>>>                }
>>>>            }
>>>> -            if (spice_stream_send_frame(frame.buffer, frame.buffer_size) ==
>>>> EXIT_FAILURE) {
>>>> +            if (spice_stream_send_frame(streamfd,
>>>> +                                        frame.buffer, frame.buffer_size) ==
>>>> EXIT_FAILURE) {
>>>>                syslog(LOG_ERR, "FAILED to send a frame\n");
>>>>                break;
>>>>            }
>>>>            //usleep(1);
>>>> -            if (read_command(false) < 0) {
>>>> +            if (read_command(streamfd, false) < 0) {
>>>>                syslog(LOG_ERR, "FAILED to read command\n");
>>>> -                goto done;
>>>> +                return;
>>>>            }
>>>>        }
>>>>    }
>>>> -
>>>> -done:
>>>> -    if (streamfd >= 0) {
>>>> -        close(streamfd);
>>>> -        streamfd = -1;
>>>> -    }
>>>> }
>>>> 
>>>> #define arg_error(...) syslog(LOG_ERR, ## __VA_ARGS__);
>>>> @@ -481,12 +495,13 @@ int main(int argc, char* argv[])
>>>>    Window rootwindow = DefaultRootWindow(display);
>>>>    XFixesSelectCursorInput(display, rootwindow,
>>>>    XFixesDisplayCursorNotifyMask);
>>>> 
>>>> -    std::thread cursor_th(cursor_changes, display, event_base);
>>>> +    Stream streamfd(streamport);
> 
> Better call it 'stream', it's not just a fd anymore. Along with the
> implicit conversion, this is quite confusing.

OK. Will be done in next round.

> 
> Lukas
> 
>>>> +    std::thread cursor_th(cursor_changes, (int) streamfd, display,
>>>> event_base);
>>>>    cursor_th.detach();
>>>> 
>>>>    int ret = EXIT_SUCCESS;
>>>>    try {
>>>> -        do_capture(streamport, f_log);
>>>> +        do_capture(streamfd, streamport, f_log);
>>> 
>>> Not a fun of this implicit conversion
>> 
>> Well, we can make it explicitl. But this is temporary, and is suppressed by the end of the series.
>> 
>>> .
>>> 
>>>>    }
>>>>    catch (std::runtime_error &err) {
>>>>        syslog(LOG_ERR, "%s\n", err.what());
>>> 
>>> Frediano
>> 
>> _______________________________________________
>> 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-16 at 17:15 +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin <dinechin@redhat.com>
> 
> Get rid of C-style 'goto done' in do_capture.
> Get rid of global streamfd, pass it around (cleaned up in later
> patch)
> Fixes a race condition, make sure we only use stream after opening
> 
> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> ---
>  src/spice-streaming-agent.cpp | 79 +++++++++++++++++++++++++------
> ------------
>  1 file changed, 47 insertions(+), 32 deletions(-)
> 
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-
> agent.cpp
> index 646d15b..9b8fd45 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -53,14 +53,38 @@ struct SpiceStreamDataMessage
>      StreamMsgData msg;
>  };
>  
> +struct SpiceStreamCursorMessage
> +{
> +    StreamDevHeader hdr;
> +    StreamMsgCursorSet msg;
> +};
> +
> +class Stream
> +{
> +public:
> +    Stream(const char *name)
> +    {
> +        fd = open(name, O_RDWR);
> +        if (fd < 0)
> +            throw std::runtime_error("failed to open streaming
> device");
> +    }
> +    ~Stream()
> +    {
> +        close(fd);
> +    }
> +    operator int() { return fd; }
> +
> +private:
> +    int fd = -1;
> +};


What about making the 'Stream' constructor take an fd so that the same
class can be used to encapsulate e.g. socket fds as well?


> +
>  static bool streaming_requested = false;
>  static bool quit_requested = false;
>  static bool log_binary = false;
>  static std::set<SpiceVideoCodecType> client_codecs;
> -static int streamfd = -1;
>  static std::mutex stream_mtx;
>  
> -static int have_something_to_read(int timeout)
> +static int have_something_to_read(int streamfd, int timeout)
>  {
>      struct pollfd pollfd = {streamfd, POLLIN, 0};
>  
> @@ -76,7 +100,7 @@ static int have_something_to_read(int timeout)
>      return 0;
>  }
>  
> -static int read_command_from_device(void)
> +static int read_command_from_device(int streamfd)
>  {
>      StreamDevHeader hdr;
>      uint8_t msg[64];
> @@ -121,18 +145,18 @@ static int read_command_from_device(void)
>      return 1;
>  }
>  
> -static int read_command(bool blocking)
> +static int read_command(int streamfd, bool blocking)
>  {
>      int timeout = blocking?-1:0;
>      while (!quit_requested) {
> -        if (!have_something_to_read(timeout)) {
> +        if (!have_something_to_read(streamfd, timeout)) {
>              if (!blocking) {
>                  return 0;
>              }
>              sleep(1);
>              continue;
>          }
> -        return read_command_from_device();
> +        return read_command_from_device(streamfd);
>      }
>  
>      return 1;
> @@ -157,7 +181,7 @@ write_all(int fd, const void *buf, const size_t
> len)
>      return written;
>  }
>  
> -static int spice_stream_send_format(unsigned w, unsigned h, unsigned
> c)
> +static int spice_stream_send_format(int streamfd, unsigned w,
> unsigned h, unsigned c)
>  {
>  
>      SpiceStreamFormatMessage msg;
> @@ -178,7 +202,7 @@ static int spice_stream_send_format(unsigned w,
> unsigned h, unsigned c)
>      return EXIT_SUCCESS;
>  }
>  
> -static int spice_stream_send_frame(const void *buf, const unsigned
> size)
> +static int spice_stream_send_frame(int streamfd, const void *buf,
> const unsigned size)
>  {
>      SpiceStreamDataMessage msg;
>      const size_t msgsize = sizeof(msg);
> @@ -254,7 +278,7 @@ static void usage(const char *progname)
>  }
>  
>  static void
> -send_cursor(unsigned width, unsigned height, int hotspot_x, int
> hotspot_y,
> +send_cursor(int streamfd, unsigned width, unsigned height, int
> hotspot_x, int hotspot_y,
>              std::function<void(uint32_t *)> fill_cursor)
>  {
>      if (width >= STREAM_MSG_CURSOR_SET_MAX_WIDTH ||
> @@ -288,7 +312,7 @@ send_cursor(unsigned width, unsigned height, int
> hotspot_x, int hotspot_y,
>      write_all(streamfd, msg.get(), cursor_size);
>  }
>  
> -static void cursor_changes(Display *display, int event_base)
> +static void cursor_changes(int streamfd, Display *display, int
> event_base)
>  {
>      unsigned long last_serial = 0;
>  
> @@ -310,25 +334,20 @@ static void cursor_changes(Display *display,
> int event_base)
>              for (unsigned i = 0; i < cursor->width * cursor->height; 
> ++i)
>                  pixels[i] = cursor->pixels[i];
>          };
> -        send_cursor(cursor->width, cursor->height, cursor->xhot,
> cursor->yhot, fill_cursor);
> +        send_cursor(streamfd,
> +                    cursor->width, cursor->height, cursor->xhot,
> cursor->yhot, fill_cursor);
>      }
>  }
>  
>  static void
> -do_capture(const char *streamport, FILE *f_log)
> +do_capture(int streamfd, const char *streamport, FILE *f_log)
>  {
> -    streamfd = open(streamport, O_RDWR);
> -    if (streamfd < 0)
> -        throw std::runtime_error("failed to open the streaming
> device (" +
> -                                 std::string(streamport) + "): "
> -                                 + strerror(errno));
> -
>      unsigned int frame_count = 0;
>      while (!quit_requested) {
>          while (!quit_requested && !streaming_requested) {
> -            if (read_command(true) < 0) {
> +            if (read_command(streamfd, true) < 0) {
>                  syslog(LOG_ERR, "FAILED to read command\n");
> -                goto done;
> +                return;
>              }
>          }
>  
> @@ -370,7 +389,7 @@ do_capture(const char *streamport, FILE *f_log)
>  
>                  syslog(LOG_DEBUG, "wXh %uX%u  codec=%u\n", width,
> height, codec);
>  
> -                if (spice_stream_send_format(width, height, codec)
> == EXIT_FAILURE)
> +                if (spice_stream_send_format(streamfd, width,
> height, codec) == EXIT_FAILURE)
>                      throw std::runtime_error("FAILED to send format
> message");
>              }
>              if (f_log) {
> @@ -382,23 +401,18 @@ do_capture(const char *streamport, FILE *f_log)
>                      hexdump(frame.buffer, frame.buffer_size, f_log);
>                  }
>              }
> -            if (spice_stream_send_frame(frame.buffer,
> frame.buffer_size) == EXIT_FAILURE) {
> +            if (spice_stream_send_frame(streamfd,
> +                                        frame.buffer,
> frame.buffer_size) == EXIT_FAILURE) {
>                  syslog(LOG_ERR, "FAILED to send a frame\n");
>                  break;
>              }
>              //usleep(1);
> -            if (read_command(false) < 0) {
> +            if (read_command(streamfd, false) < 0) {
>                  syslog(LOG_ERR, "FAILED to read command\n");
> -                goto done;
> +                return;
>              }
>          }
>      }
> -
> -done:
> -    if (streamfd >= 0) {
> -        close(streamfd);
> -        streamfd = -1;
> -    }
>  }
>  
>  #define arg_error(...) syslog(LOG_ERR, ## __VA_ARGS__);
> @@ -481,12 +495,13 @@ int main(int argc, char* argv[])
>      Window rootwindow = DefaultRootWindow(display);
>      XFixesSelectCursorInput(display, rootwindow,
> XFixesDisplayCursorNotifyMask);
>  
> -    std::thread cursor_th(cursor_changes, display, event_base);
> +    Stream streamfd(streamport);
> +    std::thread cursor_th(cursor_changes, (int) streamfd, display,
> event_base);
>      cursor_th.detach();
>  
>      int ret = EXIT_SUCCESS;
>      try {
> -        do_capture(streamport, f_log);
> +        do_capture(streamfd, streamport, f_log);
>      }
>      catch (std::runtime_error &err) {
>          syslog(LOG_ERR, "%s\n", err.what());
> On 20 Feb 2018, at 22:38, Jonathon Jongsma <jjongsma@redhat.com> wrote:
> 
> On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
>> From: Christophe de Dinechin <dinechin@redhat.com>
>> 
>> Get rid of C-style 'goto done' in do_capture.
>> Get rid of global streamfd, pass it around (cleaned up in later
>> patch)
>> Fixes a race condition, make sure we only use stream after opening
>> 
>> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
>> ---
>> src/spice-streaming-agent.cpp | 79 +++++++++++++++++++++++++------
>> ------------
>> 1 file changed, 47 insertions(+), 32 deletions(-)
>> 
>> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-
>> agent.cpp
>> index 646d15b..9b8fd45 100644
>> --- a/src/spice-streaming-agent.cpp
>> +++ b/src/spice-streaming-agent.cpp
>> @@ -53,14 +53,38 @@ struct SpiceStreamDataMessage
>>     StreamMsgData msg;
>> };
>> 
>> +struct SpiceStreamCursorMessage
>> +{
>> +    StreamDevHeader hdr;
>> +    StreamMsgCursorSet msg;
>> +};
>> +
>> +class Stream
>> +{
>> +public:
>> +    Stream(const char *name)
>> +    {
>> +        fd = open(name, O_RDWR);
>> +        if (fd < 0)
>> +            throw std::runtime_error("failed to open streaming
>> device");
>> +    }
>> +    ~Stream()
>> +    {
>> +        close(fd);
>> +    }
>> +    operator int() { return fd; }
>> +
>> +private:
>> +    int fd = -1;
>> +};
> 
> 
> What about making the 'Stream' constructor take an fd so that the same
> class can be used to encapsulate e.g. socket fds as well?

 I guess you are thinking about the vsock case?

Well, the whole idea of RAII is that one class has both open and close, so I don’t think passing an fd as input would work.

If we need to deal with a socket fd, we could have an additional ctor doing a ‘socket’ call (the dtor closing the socket), and another one looking like ‘accept’, something like:

	Stream(int domain, int type, int protocol); // socket()-like
	Stream(int sockfd, struct sockaddr *addr, socklen_t *len); // accept-like

If we do that, then each ‘Stream’ would own one fd and only one, and be responsible for closing it. But then, we’d probably want to split the Stream class between the fd-management aspect (deals with the fd only, offers read and write), and the message packaging aspect (holds a mutex, responsible for writing or receiving messages, the function I called ‘send’, …)

That being said, my objective was not to create some sort of general purpose class, KISS, just what we need. If what we need is for vsocks, that can be done in a later step as I outlined above without too much disruption.


Christophe
> 
> 
>> +
>> static bool streaming_requested = false;
>> static bool quit_requested = false;
>> static bool log_binary = false;
>> static std::set<SpiceVideoCodecType> client_codecs;
>> -static int streamfd = -1;
>> static std::mutex stream_mtx;
>> 
>> -static int have_something_to_read(int timeout)
>> +static int have_something_to_read(int streamfd, int timeout)
>> {
>>     struct pollfd pollfd = {streamfd, POLLIN, 0};
>> 
>> @@ -76,7 +100,7 @@ static int have_something_to_read(int timeout)
>>     return 0;
>> }
>> 
>> -static int read_command_from_device(void)
>> +static int read_command_from_device(int streamfd)
>> {
>>     StreamDevHeader hdr;
>>     uint8_t msg[64];
>> @@ -121,18 +145,18 @@ static int read_command_from_device(void)
>>     return 1;
>> }
>> 
>> -static int read_command(bool blocking)
>> +static int read_command(int streamfd, bool blocking)
>> {
>>     int timeout = blocking?-1:0;
>>     while (!quit_requested) {
>> -        if (!have_something_to_read(timeout)) {
>> +        if (!have_something_to_read(streamfd, timeout)) {
>>             if (!blocking) {
>>                 return 0;
>>             }
>>             sleep(1);
>>             continue;
>>         }
>> -        return read_command_from_device();
>> +        return read_command_from_device(streamfd);
>>     }
>> 
>>     return 1;
>> @@ -157,7 +181,7 @@ write_all(int fd, const void *buf, const size_t
>> len)
>>     return written;
>> }
>> 
>> -static int spice_stream_send_format(unsigned w, unsigned h, unsigned
>> c)
>> +static int spice_stream_send_format(int streamfd, unsigned w,
>> unsigned h, unsigned c)
>> {
>> 
>>     SpiceStreamFormatMessage msg;
>> @@ -178,7 +202,7 @@ static int spice_stream_send_format(unsigned w,
>> unsigned h, unsigned c)
>>     return EXIT_SUCCESS;
>> }
>> 
>> -static int spice_stream_send_frame(const void *buf, const unsigned
>> size)
>> +static int spice_stream_send_frame(int streamfd, const void *buf,
>> const unsigned size)
>> {
>>     SpiceStreamDataMessage msg;
>>     const size_t msgsize = sizeof(msg);
>> @@ -254,7 +278,7 @@ static void usage(const char *progname)
>> }
>> 
>> static void
>> -send_cursor(unsigned width, unsigned height, int hotspot_x, int
>> hotspot_y,
>> +send_cursor(int streamfd, unsigned width, unsigned height, int
>> hotspot_x, int hotspot_y,
>>             std::function<void(uint32_t *)> fill_cursor)
>> {
>>     if (width >= STREAM_MSG_CURSOR_SET_MAX_WIDTH ||
>> @@ -288,7 +312,7 @@ send_cursor(unsigned width, unsigned height, int
>> hotspot_x, int hotspot_y,
>>     write_all(streamfd, msg.get(), cursor_size);
>> }
>> 
>> -static void cursor_changes(Display *display, int event_base)
>> +static void cursor_changes(int streamfd, Display *display, int
>> event_base)
>> {
>>     unsigned long last_serial = 0;
>> 
>> @@ -310,25 +334,20 @@ static void cursor_changes(Display *display,
>> int event_base)
>>             for (unsigned i = 0; i < cursor->width * cursor->height; 
>> ++i)
>>                 pixels[i] = cursor->pixels[i];
>>         };
>> -        send_cursor(cursor->width, cursor->height, cursor->xhot,
>> cursor->yhot, fill_cursor);
>> +        send_cursor(streamfd,
>> +                    cursor->width, cursor->height, cursor->xhot,
>> cursor->yhot, fill_cursor);
>>     }
>> }
>> 
>> static void
>> -do_capture(const char *streamport, FILE *f_log)
>> +do_capture(int streamfd, const char *streamport, FILE *f_log)
>> {
>> -    streamfd = open(streamport, O_RDWR);
>> -    if (streamfd < 0)
>> -        throw std::runtime_error("failed to open the streaming
>> device (" +
>> -                                 std::string(streamport) + "): "
>> -                                 + strerror(errno));
>> -
>>     unsigned int frame_count = 0;
>>     while (!quit_requested) {
>>         while (!quit_requested && !streaming_requested) {
>> -            if (read_command(true) < 0) {
>> +            if (read_command(streamfd, true) < 0) {
>>                 syslog(LOG_ERR, "FAILED to read command\n");
>> -                goto done;
>> +                return;
>>             }
>>         }
>> 
>> @@ -370,7 +389,7 @@ do_capture(const char *streamport, FILE *f_log)
>> 
>>                 syslog(LOG_DEBUG, "wXh %uX%u  codec=%u\n", width,
>> height, codec);
>> 
>> -                if (spice_stream_send_format(width, height, codec)
>> == EXIT_FAILURE)
>> +                if (spice_stream_send_format(streamfd, width,
>> height, codec) == EXIT_FAILURE)
>>                     throw std::runtime_error("FAILED to send format
>> message");
>>             }
>>             if (f_log) {
>> @@ -382,23 +401,18 @@ do_capture(const char *streamport, FILE *f_log)
>>                     hexdump(frame.buffer, frame.buffer_size, f_log);
>>                 }
>>             }
>> -            if (spice_stream_send_frame(frame.buffer,
>> frame.buffer_size) == EXIT_FAILURE) {
>> +            if (spice_stream_send_frame(streamfd,
>> +                                        frame.buffer,
>> frame.buffer_size) == EXIT_FAILURE) {
>>                 syslog(LOG_ERR, "FAILED to send a frame\n");
>>                 break;
>>             }
>>             //usleep(1);
>> -            if (read_command(false) < 0) {
>> +            if (read_command(streamfd, false) < 0) {
>>                 syslog(LOG_ERR, "FAILED to read command\n");
>> -                goto done;
>> +                return;
>>             }
>>         }
>>     }
>> -
>> -done:
>> -    if (streamfd >= 0) {
>> -        close(streamfd);
>> -        streamfd = -1;
>> -    }
>> }
>> 
>> #define arg_error(...) syslog(LOG_ERR, ## __VA_ARGS__);
>> @@ -481,12 +495,13 @@ int main(int argc, char* argv[])
>>     Window rootwindow = DefaultRootWindow(display);
>>     XFixesSelectCursorInput(display, rootwindow,
>> XFixesDisplayCursorNotifyMask);
>> 
>> -    std::thread cursor_th(cursor_changes, display, event_base);
>> +    Stream streamfd(streamport);
>> +    std::thread cursor_th(cursor_changes, (int) streamfd, display,
>> event_base);
>>     cursor_th.detach();
>> 
>>     int ret = EXIT_SUCCESS;
>>     try {
>> -        do_capture(streamport, f_log);
>> +        do_capture(streamfd, streamport, f_log);
>>     }
>>     catch (std::runtime_error &err) {
>>         syslog(LOG_ERR, "%s\n", err.what());
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
On Tue, 2018-02-20 at 18:37 +0100, Christophe de Dinechin wrote:
> [Forgot to send, sorry for delay]
> 
> > On 19 Feb 2018, at 19:03, Lukáš Hrázký <lhrazky@redhat.com> wrote:
> > 
> > On Fri, 2018-02-16 at 17:59 +0100, Christophe de Dinechin wrote:
> > > > On 16 Feb 2018, at 17:40, Frediano Ziglio <fziglio@redhat.com> wrote:
> > > > 
> > > > > 
> > > > > From: Christophe de Dinechin <dinechin@redhat.com>
> > > > > 
> > > > > Get rid of C-style 'goto done' in do_capture.
> > > > > Get rid of global streamfd, pass it around (cleaned up in later patch)
> > > > > Fixes a race condition, make sure we only use stream after opening
> > > > > 
> > > > > Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> > > > > ---
> > > > > src/spice-streaming-agent.cpp | 79
> > > > > +++++++++++++++++++++++++------------------
> > > > > 1 file changed, 47 insertions(+), 32 deletions(-)
> > > > > 
> > > > > diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> > > > > index 646d15b..9b8fd45 100644
> > > > > --- a/src/spice-streaming-agent.cpp
> > > > > +++ b/src/spice-streaming-agent.cpp
> > > > > @@ -53,14 +53,38 @@ struct SpiceStreamDataMessage
> > > > >    StreamMsgData msg;
> > > > > };
> > > > > 
> > > > > +struct SpiceStreamCursorMessage
> > > > > +{
> > > > > +    StreamDevHeader hdr;
> > > > > +    StreamMsgCursorSet msg;
> > > > > +};
> > > > > +
> > > > 
> > > > This is weird in this patch
> > > 
> > > Yes. I may have merged two patches. Will check.
> > > 
> > > > 
> > > > > +class Stream
> > > > > +{
> > > > > +public:
> > > > > +    Stream(const char *name)
> > > > > +    {
> > > > > +        fd = open(name, O_RDWR);
> > > > > +        if (fd < 0)
> > > > > +            throw std::runtime_error("failed to open streaming device");
> > 
> > Braces around if body (according to the coding style). Repeated a few
> > times below.
> > 
> > > > > +    }
> > > > > +    ~Stream()
> > > > > +    {
> > > > > +        close(fd);
> > > > 
> > > > You probably what to check for -1 to avoid calling close(-1)
> > > 
> > > I think this cannot happen, since in that case you’d have thrown from the ctor.
> > > 
> > > > 
> > > > > +    }
> > > > > +    operator int() { return fd; }
> > > > 
> > > > Sure you want this? I think is safer to avoid implicit cast
> > > > also considering stuff like
> > > > 
> > > > Stream s;
> > > > if (s) …
> > > 
> > > It is removed in a later patch once it’s no longer needed. The objective here is to minimize noisy changes at each step.
> > 
> > I'd also rather not see it, but if it's removed later, no big deal.
> 
> So I feel the general agreement is that it’s better to add an explicit getter and use that rather than implicitly convert.
> 
> > 
> > > > 
> > > > > +
> > > > > +private:
> > > > > +    int fd = -1;
> > > > 
> > > > So with a default constructor you want a class with
> > > > an invalid state?
> > > > Or just disable default constructor.
> > > 
> > > It’s disabled, there’s a ctor with args.
> > > 
> > > > I would disable also copy.
> > > 
> > > Also implicitly disabled.
> > 
> > It is not, actually, disabled here. It gets disabled later when you add
> > the mutex member, as that is a non-copyable type.
> 
> You are right, of course. But like for the above, the idea is to minimize “noise” in the series.
> 
> Of course, I could explicitly delete constructors and assignment. I feel, but I may be wrong, that having a mutex field “says that”, both in terms of language safety and in terms of “what it means” (i.e. you are really creating a class for the purpose of RAII and thread safety).
> 
> If you think it’s better to add a few lines, I’m OK with it, but I don’t like it much as I read it more as noise than anything else. There are a few choices here, roughly in my personal order of preference:
> 1. Introduce = delete in this patch, remove it when mutex is added
> 2. Introduce = delete in this patch and leave it there even when it becomes “useless"
> 3 Leave a temporary “unsafe” state for a couple of commits
> 4. Merge the two and make sure there’s the mutex at the beginning
> 
> As a side note, this reminds me of a talk I highly recommend, https://www.youtube.com/watch?v=4AfRAVcThyA in case you have not seen it. Among other things, Herb Sutter proposes a $class mechanism that makes it possible to define a class of classes, e.g. “interface”, so that you explain with $class interface { … } what an interface is, and then you can write “interface X { .. }” and have an interface class (e.g. only pure virtual methods, etc).
> 
> Here, I see the mutex as indicative of a “locked resource” class, which I understand as having only explicit ctors for initialization and nothing else to consider, no copy, no assignment, etc. But it could also be made explicit with this mechanism. Post C++ 20, so not right away…
> 
> > 
> > It might be a good idea to explicitly delete the copy ctr/ao anyway...
> > In case someone ever removes the mutex and doesn't realize this.
> 
> That’s my choice #2, it could easily become #1.

No strong opinion on this :) I know some would argue my point, so I
mentioned it for completeness. I can agree with your order of
preference, and I don't care as much for "unsafe" state between commits
in your series, so 3 is fine by me too.

I just mentioned the above so that nobody could get confused and
thought that the class in this patch somehow has those copy ctor and
otor disabled...

> > 
> > > > 
> > > > > +};
> > > > > +
> > > > > static bool streaming_requested = false;
> > > > > static bool quit_requested = false;
> > > > > static bool log_binary = false;
> > > > > static std::set<SpiceVideoCodecType> client_codecs;
> > > > > -static int streamfd = -1;
> > > > > static std::mutex stream_mtx;
> > > > > 
> > > > > -static int have_something_to_read(int timeout)
> > > > > +static int have_something_to_read(int streamfd, int timeout)
> > > > 
> > > > maybe would be better to use the "fd" name here, is no more
> > > > bound to stream.
> > > 
> > > Kept to minimize changes
> > > 
> > > > > {
> > > > >    struct pollfd pollfd = {streamfd, POLLIN, 0};
> > > > > 
> > > > > @@ -76,7 +100,7 @@ static int have_something_to_read(int timeout)
> > > > >    return 0;
> > > > > }
> > > > > 
> > > > > -static int read_command_from_device(void)
> > > > > +static int read_command_from_device(int streamfd)
> > > > 
> > > > Have you considered Stream::read_command?
> > > 
> > > For that specific case, I liked the “from_device”. But…
> > > 
> > > > Ok, mostly in a following patch
> > > 
> > > 
> > > > 
> > > > > {
> > > > >    StreamDevHeader hdr;
> > > > >    uint8_t msg[64];
> > > > > @@ -121,18 +145,18 @@ static int read_command_from_device(void)
> > > > >    return 1;
> > > > > }
> > > > > 
> > > > > -static int read_command(bool blocking)
> > > > > +static int read_command(int streamfd, bool blocking)
> > > > > {
> > > > >    int timeout = blocking?-1:0;
> > > > >    while (!quit_requested) {
> > > > > -        if (!have_something_to_read(timeout)) {
> > > > > +        if (!have_something_to_read(streamfd, timeout)) {
> > > > >            if (!blocking) {
> > > > >                return 0;
> > > > >            }
> > > > >            sleep(1);
> > > > >            continue;
> > > > >        }
> > > > > -        return read_command_from_device();
> > > > > +        return read_command_from_device(streamfd);
> > > > >    }
> > > > > 
> > > > >    return 1;
> > > > > @@ -157,7 +181,7 @@ write_all(int fd, const void *buf, const size_t len)
> > > > >    return written;
> > > > > }
> > > > > 
> > > > > -static int spice_stream_send_format(unsigned w, unsigned h, unsigned c)
> > > > > +static int spice_stream_send_format(int streamfd, unsigned w, unsigned h,
> > > > > unsigned c)
> > > > > {
> > > > > 
> > > > >    SpiceStreamFormatMessage msg;
> > > > > @@ -178,7 +202,7 @@ static int spice_stream_send_format(unsigned w, unsigned
> > > > > h, unsigned c)
> > > > >    return EXIT_SUCCESS;
> > > > > }
> > > > > 
> > > > > -static int spice_stream_send_frame(const void *buf, const unsigned size)
> > > > > +static int spice_stream_send_frame(int streamfd, const void *buf, const
> > > > > unsigned size)
> > > > > {
> > > > >    SpiceStreamDataMessage msg;
> > > > >    const size_t msgsize = sizeof(msg);
> > > > > @@ -254,7 +278,7 @@ static void usage(const char *progname)
> > > > > }
> > > > > 
> > > > > static void
> > > > > -send_cursor(unsigned width, unsigned height, int hotspot_x, int hotspot_y,
> > > > > +send_cursor(int streamfd, unsigned width, unsigned height, int hotspot_x,
> > > > > int hotspot_y,
> > > > >            std::function<void(uint32_t *)> fill_cursor)
> > > > > {
> > > > >    if (width >= STREAM_MSG_CURSOR_SET_MAX_WIDTH ||
> > > > > @@ -288,7 +312,7 @@ send_cursor(unsigned width, unsigned height, int
> > > > > hotspot_x, int hotspot_y,
> > > > >    write_all(streamfd, msg.get(), cursor_size);
> > > > > }
> > > > > 
> > > > > -static void cursor_changes(Display *display, int event_base)
> > > > > +static void cursor_changes(int streamfd, Display *display, int event_base)
> > > > > {
> > > > >    unsigned long last_serial = 0;
> > > > > 
> > > > > @@ -310,25 +334,20 @@ static void cursor_changes(Display *display, int
> > > > > event_base)
> > > > >            for (unsigned i = 0; i < cursor->width * cursor->height; ++i)
> > > > >                pixels[i] = cursor->pixels[i];
> > > > >        };
> > > > > -        send_cursor(cursor->width, cursor->height, cursor->xhot,
> > > > > cursor->yhot, fill_cursor);
> > > > > +        send_cursor(streamfd,
> > > > > +                    cursor->width, cursor->height, cursor->xhot,
> > > > > cursor->yhot, fill_cursor);
> > > > >    }
> > > > > }
> > > > > 
> > > > > static void
> > > > > -do_capture(const char *streamport, FILE *f_log)
> > > > > +do_capture(int streamfd, const char *streamport, FILE *f_log)
> > > > > {
> > > > > -    streamfd = open(streamport, O_RDWR);
> > > > > -    if (streamfd < 0)
> > > > > -        throw std::runtime_error("failed to open the streaming device (" +
> > > > > -                                 std::string(streamport) + "): "
> > > > > -                                 + strerror(errno));
> > > > > -
> > > > >    unsigned int frame_count = 0;
> > > > >    while (!quit_requested) {
> > > > >        while (!quit_requested && !streaming_requested) {
> > > > > -            if (read_command(true) < 0) {
> > > > > +            if (read_command(streamfd, true) < 0) {
> > > > >                syslog(LOG_ERR, "FAILED to read command\n");
> > > > > -                goto done;
> > > > > +                return;
> > > > >            }
> > > > >        }
> > > > > 
> > > > > @@ -370,7 +389,7 @@ do_capture(const char *streamport, FILE *f_log)
> > > > > 
> > > > >                syslog(LOG_DEBUG, "wXh %uX%u  codec=%u\n", width, height,
> > > > >                codec);
> > > > > 
> > > > > -                if (spice_stream_send_format(width, height, codec) ==
> > > > > EXIT_FAILURE)
> > > > > +                if (spice_stream_send_format(streamfd, width, height, codec)
> > > > > == EXIT_FAILURE)
> > > > >                    throw std::runtime_error("FAILED to send format
> > > > >                    message");
> > > > >            }
> > > > >            if (f_log) {
> > > > > @@ -382,23 +401,18 @@ do_capture(const char *streamport, FILE *f_log)
> > > > >                    hexdump(frame.buffer, frame.buffer_size, f_log);
> > > > >                }
> > > > >            }
> > > > > -            if (spice_stream_send_frame(frame.buffer, frame.buffer_size) ==
> > > > > EXIT_FAILURE) {
> > > > > +            if (spice_stream_send_frame(streamfd,
> > > > > +                                        frame.buffer, frame.buffer_size) ==
> > > > > EXIT_FAILURE) {
> > > > >                syslog(LOG_ERR, "FAILED to send a frame\n");
> > > > >                break;
> > > > >            }
> > > > >            //usleep(1);
> > > > > -            if (read_command(false) < 0) {
> > > > > +            if (read_command(streamfd, false) < 0) {
> > > > >                syslog(LOG_ERR, "FAILED to read command\n");
> > > > > -                goto done;
> > > > > +                return;
> > > > >            }
> > > > >        }
> > > > >    }
> > > > > -
> > > > > -done:
> > > > > -    if (streamfd >= 0) {
> > > > > -        close(streamfd);
> > > > > -        streamfd = -1;
> > > > > -    }
> > > > > }
> > > > > 
> > > > > #define arg_error(...) syslog(LOG_ERR, ## __VA_ARGS__);
> > > > > @@ -481,12 +495,13 @@ int main(int argc, char* argv[])
> > > > >    Window rootwindow = DefaultRootWindow(display);
> > > > >    XFixesSelectCursorInput(display, rootwindow,
> > > > >    XFixesDisplayCursorNotifyMask);
> > > > > 
> > > > > -    std::thread cursor_th(cursor_changes, display, event_base);
> > > > > +    Stream streamfd(streamport);
> > 
> > Better call it 'stream', it's not just a fd anymore. Along with the
> > implicit conversion, this is quite confusing.
> 
> OK. Will be done in next round.
> 
> > 
> > Lukas
> > 
> > > > > +    std::thread cursor_th(cursor_changes, (int) streamfd, display,
> > > > > event_base);
> > > > >    cursor_th.detach();
> > > > > 
> > > > >    int ret = EXIT_SUCCESS;
> > > > >    try {
> > > > > -        do_capture(streamport, f_log);
> > > > > +        do_capture(streamfd, streamport, f_log);
> > > > 
> > > > Not a fun of this implicit conversion
> > > 
> > > Well, we can make it explicitl. But this is temporary, and is suppressed by the end of the series.
> > > 
> > > > .
> > > > 
> > > > >    }
> > > > >    catch (std::runtime_error &err) {
> > > > >        syslog(LOG_ERR, "%s\n", err.what());
> > > > 
> > > > Frediano
> > > 
> > > _______________________________________________
> > > 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
> 
>