[04/22] Use RAII to cleanup stream in case of exception or return

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

Details

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

Not browsing as part of any series.

Commit Message

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

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

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

Patch hide | download patch | download mbox

diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index 18f2b1a..7304576 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -56,14 +56,39 @@  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);
+    }
+    int file_descriptor() { 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};
 
@@ -79,7 +104,7 @@  static int have_something_to_read(int timeout)
     return 0;
 }
 
-static void handle_stream_start_stop(uint32_t len)
+static void handle_stream_start_stop(int streamfd, uint32_t len)
 {
     uint8_t msg[256];
 
@@ -101,7 +126,7 @@  static void handle_stream_start_stop(uint32_t len)
     }
 }
 
-static void handle_stream_capabilities(uint32_t len)
+static void handle_stream_capabilities(int streamfd, uint32_t len)
 {
     uint8_t caps[STREAM_MSG_CAPABILITIES_MAX_BYTES];
 
@@ -126,13 +151,13 @@  static void handle_stream_capabilities(uint32_t len)
     }
 }
 
-static void handle_stream_error(uint32_t len)
+static void handle_stream_error(int streamfd, uint32_t len)
 {
     // TODO read message and use it
     throw std::runtime_error("got an error message from server");
 }
 
-static void read_command_from_device(void)
+static void read_command_from_device(int streamfd)
 {
     StreamDevHeader hdr;
     int n;
@@ -150,27 +175,27 @@  static void read_command_from_device(void)
 
     switch (hdr.type) {
     case STREAM_TYPE_CAPABILITIES:
-        return handle_stream_capabilities(hdr.size);
+        return handle_stream_capabilities(streamfd, hdr.size);
     case STREAM_TYPE_NOTIFY_ERROR:
-        return handle_stream_error(hdr.size);
+        return handle_stream_error(streamfd, hdr.size);
     case STREAM_TYPE_START_STOP:
-        return handle_stream_start_stop(hdr.size);
+        return handle_stream_start_stop(streamfd, hdr.size);
     }
     throw std::runtime_error("UNKNOWN msg of type " + std::to_string(hdr.type));
 }
 
-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;
         }
-        read_command_from_device();
+        read_command_from_device(streamfd);
         break;
     }
 
@@ -196,7 +221,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;
@@ -217,7 +242,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);
@@ -294,7 +319,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 || height >= STREAM_MSG_CURSOR_SET_MAX_HEIGHT) {
@@ -328,7 +353,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;
 
@@ -353,26 +378,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;
             }
         }
 
@@ -415,7 +434,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");
                 }
             }
@@ -428,23 +447,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__);
@@ -540,12 +554,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 stream(streamport);
+    std::thread cursor_th(cursor_changes, stream.file_descriptor(), display, event_base);
     cursor_th.detach();
 
     int ret = EXIT_SUCCESS;
     try {
-        do_capture(streamport, f_log);
+        do_capture(stream.file_descriptor(), streamport, f_log);
     }
     catch (std::runtime_error &err) {
         syslog(LOG_ERR, "%s\n", err.what());

Comments

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

Spurious lines.

> +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);
> +    }
> +    int file_descriptor() { 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};
>  
> @@ -79,7 +104,7 @@ static int have_something_to_read(int timeout)
>      return 0;
>  }
>  
> -static void handle_stream_start_stop(uint32_t len)
> +static void handle_stream_start_stop(int streamfd, uint32_t len)
>  {
>      uint8_t msg[256];
>  
> @@ -101,7 +126,7 @@ static void handle_stream_start_stop(uint32_t len)
>      }
>  }
>  
> -static void handle_stream_capabilities(uint32_t len)
> +static void handle_stream_capabilities(int streamfd, uint32_t len)
>  {
>      uint8_t caps[STREAM_MSG_CAPABILITIES_MAX_BYTES];
>  
> @@ -126,13 +151,13 @@ static void handle_stream_capabilities(uint32_t len)
>      }
>  }
>  
> -static void handle_stream_error(uint32_t len)
> +static void handle_stream_error(int streamfd, uint32_t len)
>  {
>      // TODO read message and use it
>      throw std::runtime_error("got an error message from server");
>  }
>  
> -static void read_command_from_device(void)
> +static void read_command_from_device(int streamfd)
>  {
>      StreamDevHeader hdr;
>      int n;
> @@ -150,27 +175,27 @@ static void read_command_from_device(void)
>  
>      switch (hdr.type) {
>      case STREAM_TYPE_CAPABILITIES:
> -        return handle_stream_capabilities(hdr.size);
> +        return handle_stream_capabilities(streamfd, hdr.size);
>      case STREAM_TYPE_NOTIFY_ERROR:
> -        return handle_stream_error(hdr.size);
> +        return handle_stream_error(streamfd, hdr.size);
>      case STREAM_TYPE_START_STOP:
> -        return handle_stream_start_stop(hdr.size);
> +        return handle_stream_start_stop(streamfd, hdr.size);
>      }
>      throw std::runtime_error("UNKNOWN msg of type " +
>      std::to_string(hdr.type));
>  }
>  
> -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;
>          }
> -        read_command_from_device();
> +        read_command_from_device(streamfd);
>          break;
>      }
>  
> @@ -196,7 +221,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;
> @@ -217,7 +242,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);
> @@ -294,7 +319,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 || height >=
>      STREAM_MSG_CURSOR_SET_MAX_HEIGHT) {
> @@ -328,7 +353,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;
>  
> @@ -353,26 +378,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;
>              }
>          }
>  
> @@ -415,7 +434,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");
>                  }
>              }
> @@ -428,23 +447,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__);
> @@ -540,12 +554,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 stream(streamport);
> +    std::thread cursor_th(cursor_changes, stream.file_descriptor(), display,
> event_base);
>      cursor_th.detach();
>  
>      int ret = EXIT_SUCCESS;
>      try {
> -        do_capture(streamport, f_log);
> +        do_capture(stream.file_descriptor(), streamport, f_log);
>      }
>      catch (std::runtime_error &err) {
>          syslog(LOG_ERR, "%s\n", err.what());

Rebased at
https://cgit.freedesktop.org/~fziglio/spice-streaming-agent/commit/?h=style3&id=dc5c3b3908652aabda87b5e350e845f6cf37e5eb

Otherwise looks good to me.

Frediano
> 
> From: Christophe de Dinechin <dinechin@redhat.com>
> 
> Fix a race condition, make sure we only use stream after opening
> Get rid of C-style 'goto done' in do_capture.
> Get rid of global streamfd, pass it around (cleaned up in later patch)
> 
> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> ---
>  src/spice-streaming-agent.cpp | 93
>  +++++++++++++++++++++++++------------------
>  1 file changed, 54 insertions(+), 39 deletions(-)
> 
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index 18f2b1a..7304576 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -56,14 +56,39 @@ 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);
> +    }
> +    int file_descriptor() { return fd; }
> +
> +private:
> +    int fd = -1;
> +};
> +

Why you don't define new object directly in new files instead of
having added here and then moved in the same series?

>  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};
>  
> @@ -79,7 +104,7 @@ static int have_something_to_read(int timeout)
>      return 0;
>  }
>  
> -static void handle_stream_start_stop(uint32_t len)
> +static void handle_stream_start_stop(int streamfd, uint32_t len)
>  {
>      uint8_t msg[256];
>  
> @@ -101,7 +126,7 @@ static void handle_stream_start_stop(uint32_t len)
>      }
>  }
>  
> -static void handle_stream_capabilities(uint32_t len)
> +static void handle_stream_capabilities(int streamfd, uint32_t len)
>  {
>      uint8_t caps[STREAM_MSG_CAPABILITIES_MAX_BYTES];
>  
> @@ -126,13 +151,13 @@ static void handle_stream_capabilities(uint32_t len)
>      }
>  }
>  
> -static void handle_stream_error(uint32_t len)
> +static void handle_stream_error(int streamfd, uint32_t len)
>  {
>      // TODO read message and use it
>      throw std::runtime_error("got an error message from server");
>  }
>  
> -static void read_command_from_device(void)
> +static void read_command_from_device(int streamfd)
>  {
>      StreamDevHeader hdr;
>      int n;
> @@ -150,27 +175,27 @@ static void read_command_from_device(void)
>  
>      switch (hdr.type) {
>      case STREAM_TYPE_CAPABILITIES:
> -        return handle_stream_capabilities(hdr.size);
> +        return handle_stream_capabilities(streamfd, hdr.size);
>      case STREAM_TYPE_NOTIFY_ERROR:
> -        return handle_stream_error(hdr.size);
> +        return handle_stream_error(streamfd, hdr.size);
>      case STREAM_TYPE_START_STOP:
> -        return handle_stream_start_stop(hdr.size);
> +        return handle_stream_start_stop(streamfd, hdr.size);
>      }
>      throw std::runtime_error("UNKNOWN msg of type " +
>      std::to_string(hdr.type));
>  }
>  
> -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;
>          }
> -        read_command_from_device();
> +        read_command_from_device(streamfd);
>          break;
>      }
>  
> @@ -196,7 +221,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;
> @@ -217,7 +242,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);
> @@ -294,7 +319,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 || height >=
>      STREAM_MSG_CURSOR_SET_MAX_HEIGHT) {
> @@ -328,7 +353,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;
>  
> @@ -353,26 +378,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;
>              }
>          }
>  
> @@ -415,7 +434,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");
>                  }
>              }
> @@ -428,23 +447,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__);
> @@ -540,12 +554,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 stream(streamport);
> +    std::thread cursor_th(cursor_changes, stream.file_descriptor(), display,
> event_base);
>      cursor_th.detach();
>  
>      int ret = EXIT_SUCCESS;
>      try {
> -        do_capture(streamport, f_log);
> +        do_capture(stream.file_descriptor(), streamport, f_log);
>      }
>      catch (std::runtime_error &err) {
>          syslog(LOG_ERR, "%s\n", err.what());

Frediano
> On 28 Feb 2018, at 18:16, Frediano Ziglio <fziglio@redhat.com> wrote:
> 
>> 
>> From: Christophe de Dinechin <dinechin@redhat.com>
>> 
>> Fix a race condition, make sure we only use stream after opening
>> Get rid of C-style 'goto done' in do_capture.
>> Get rid of global streamfd, pass it around (cleaned up in later patch)
>> 
>> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
>> ---
>> src/spice-streaming-agent.cpp | 93
>> +++++++++++++++++++++++++------------------
>> 1 file changed, 54 insertions(+), 39 deletions(-)
>> 
>> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
>> index 18f2b1a..7304576 100644
>> --- a/src/spice-streaming-agent.cpp
>> +++ b/src/spice-streaming-agent.cpp
>> @@ -56,14 +56,39 @@ 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);
>> +    }
>> +    int file_descriptor() { return fd; }
>> +
>> +private:
>> +    int fd = -1;
>> +};
>> +
> 
> Why you don't define new object directly in new files instead of
> having added here and then moved in the same series?

The intent was to show the evolution of the code step by step in a way that makes comparison easier.

For example, by (temporarily) keeping the Stream class in the file, I can show the lineage from a static function handle_stream_capabilities to a method Stream::handle_stream_capabilities that has practically the same body to a message-formatting class Capabilities. If I did it in one step, it would be much harder to spot mistakes along the way or bisect my way out of any issue introduced by the changes.

> 
>> 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};
>> 
>> @@ -79,7 +104,7 @@ static int have_something_to_read(int timeout)
>>     return 0;
>> }
>> 
>> -static void handle_stream_start_stop(uint32_t len)
>> +static void handle_stream_start_stop(int streamfd, uint32_t len)
>> {
>>     uint8_t msg[256];
>> 
>> @@ -101,7 +126,7 @@ static void handle_stream_start_stop(uint32_t len)
>>     }
>> }
>> 
>> -static void handle_stream_capabilities(uint32_t len)
>> +static void handle_stream_capabilities(int streamfd, uint32_t len)
>> {
>>     uint8_t caps[STREAM_MSG_CAPABILITIES_MAX_BYTES];
>> 
>> @@ -126,13 +151,13 @@ static void handle_stream_capabilities(uint32_t len)
>>     }
>> }
>> 
>> -static void handle_stream_error(uint32_t len)
>> +static void handle_stream_error(int streamfd, uint32_t len)
>> {
>>     // TODO read message and use it
>>     throw std::runtime_error("got an error message from server");
>> }
>> 
>> -static void read_command_from_device(void)
>> +static void read_command_from_device(int streamfd)
>> {
>>     StreamDevHeader hdr;
>>     int n;
>> @@ -150,27 +175,27 @@ static void read_command_from_device(void)
>> 
>>     switch (hdr.type) {
>>     case STREAM_TYPE_CAPABILITIES:
>> -        return handle_stream_capabilities(hdr.size);
>> +        return handle_stream_capabilities(streamfd, hdr.size);
>>     case STREAM_TYPE_NOTIFY_ERROR:
>> -        return handle_stream_error(hdr.size);
>> +        return handle_stream_error(streamfd, hdr.size);
>>     case STREAM_TYPE_START_STOP:
>> -        return handle_stream_start_stop(hdr.size);
>> +        return handle_stream_start_stop(streamfd, hdr.size);
>>     }
>>     throw std::runtime_error("UNKNOWN msg of type " +
>>     std::to_string(hdr.type));
>> }
>> 
>> -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;
>>         }
>> -        read_command_from_device();
>> +        read_command_from_device(streamfd);
>>         break;
>>     }
>> 
>> @@ -196,7 +221,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;
>> @@ -217,7 +242,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);
>> @@ -294,7 +319,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 || height >=
>>     STREAM_MSG_CURSOR_SET_MAX_HEIGHT) {
>> @@ -328,7 +353,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;
>> 
>> @@ -353,26 +378,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;
>>             }
>>         }
>> 
>> @@ -415,7 +434,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");
>>                 }
>>             }
>> @@ -428,23 +447,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__);
>> @@ -540,12 +554,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 stream(streamport);
>> +    std::thread cursor_th(cursor_changes, stream.file_descriptor(), display,
>> event_base);
>>     cursor_th.detach();
>> 
>>     int ret = EXIT_SUCCESS;
>>     try {
>> -        do_capture(streamport, f_log);
>> +        do_capture(stream.file_descriptor(), streamport, f_log);
>>     }
>>     catch (std::runtime_error &err) {
>>         syslog(LOG_ERR, "%s\n", err.what());
> 
> Frediano