[v2,05/24] Use RAII to cleanup stream in case of exception or return

Submitted by Christophe de Dinechin on Feb. 21, 2018, 5:46 p.m.

Details

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

Not browsing as part of any series.

Commit Message

Christophe de Dinechin Feb. 21, 2018, 5:46 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 | 91 +++++++++++++++++++++++++------------------
 1 file changed, 53 insertions(+), 38 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index acec001..8508f9b 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -56,14 +56,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};
 
@@ -79,7 +103,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 +125,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 +150,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 +174,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 +220,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 +241,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);
@@ -293,7 +317,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 ||
@@ -327,7 +351,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;
 
@@ -349,25 +373,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;
             }
         }
 
@@ -409,7 +428,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) {
@@ -421,23 +440,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__);
@@ -523,12 +537,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());