Re: [PATCH spice-streaming-agent v2 4/4] Move all stream-related functions within SpiceStream class

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 10 Nov 2017, at 14:15, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:


From: Christophe de Dinechin <dinechin@xxxxxxxxxx>

This incidentally fixes a race condition processing X events,
where we could possibly start sending cursor events to the
stream before it was actually open.

Signed-off-by: Christophe de Dinechin <dinechin@xxxxxxxxxx>
---
src/spice-streaming-agent.cpp | 68
+++++++++++++++++++++++--------------------
1 file changed, 37 insertions(+), 31 deletions(-)

diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index 8300cf2..33e0345 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -51,31 +51,39 @@ struct SpiceStreamDataMessage
    StreamMsgData msg;
};

-struct Stream
+struct SpiceStream
{

Would promote to a class.
OK


-  Stream(const char *name, int &fd): fd(fd)
+  SpiceStream(const char *name): streamfd(open(name, O_RDWR))
  {
-    fd = open(name, O_RDWR);
-    if (fd < 0)
+    if (streamfd < 0)
      throw std::runtime_error("failed to open streaming device");
  }
-  ~Stream()
+  ~SpiceStream()
  {
-    if (fd >= 0)
-      close(fd);
-    fd = -1;
+    if (streamfd >= 0)
+      close(streamfd);
+    streamfd = -1;

is the if still needed?

No


  }
-  int &fd;
+
+  int have_something_to_read(int *pfd, int timeout);
+  int read_command_from_stdin(void);
+  int read_command_from_device(void);
+  int read_command(int blocking);

Some are not strictly related to the stream.

But right now, things are a bit too interconnected because of the way read_command is structured.
Id’ like to split into another class, but I was planning to do that next.


+  size_t write_all(int fd, const void *buf, const size_t len);

this has nothing to specifically do with the structure,
mostly with some API (write) behaviour like partial write and
signals.

Made it private. Also removed fd, it’s always streamfd.


+  int send_format(unsigned w, unsigned h, unsigned c);
+  int send_frame(const void *buf, const unsigned size);
+  void send_cursor(const XFixesCursorImage &image);
+private:
+  int streamfd;
};

static int streaming_requested;
static bool quit;
-static int streamfd = -1;
static bool stdin_ok;
static int log_binary = 0;
static std::mutex stream_mtx;

-static int have_something_to_read(int *pfd, int timeout)
+int SpiceStream::have_something_to_read(int *pfd, int timeout)
{
    int nfds;
    struct pollfd pollfds[2] = {
@@ -97,7 +105,7 @@ static int have_something_to_read(int *pfd, int timeout)
    return *pfd != -1;
}

-static int read_command_from_stdin(void)
+int SpiceStream::read_command_from_stdin(void)
{
    char buffer[64], *p, *save = NULL;

@@ -121,7 +129,7 @@ static int read_command_from_stdin(void)
    return 1;
}

-static int read_command_from_device(void)
+int SpiceStream::read_command_from_device(void)
{
    StreamDevHeader hdr;
    uint8_t msg[64];
@@ -163,7 +171,7 @@ static int read_command_from_device(void)
    return 1;
}

-static int read_command(int blocking)
+int SpiceStream::read_command(int blocking)

OT: maybe should be bool blocking ?

OK


{
    int fd, n=1;
    int timeout = blocking?-1:0;
@@ -185,8 +193,7 @@ static int read_command(int blocking)
    return n;
}

-static size_t
-write_all(int fd, const void *buf, const size_t len)
+size_t SpiceStream::write_all(int fd, const void *buf, const size_t len)
{
    size_t written = 0;
    while (written < len) {
@@ -204,7 +211,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)
+int SpiceStream::send_format(unsigned w, unsigned h, unsigned c)
{

    SpiceStreamFormatMessage msg;
@@ -225,7 +232,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)
+int SpiceStream::send_frame(const void *buf, const unsigned size)
{
    SpiceStreamDataMessage msg;
    const size_t msgsize = sizeof(msg);
@@ -304,7 +311,7 @@ static void usage(const char *progname)
    exit(1);
}

-static void send_cursor(const XFixesCursorImage &image)
+void SpiceStream::send_cursor(const XFixesCursorImage &image)
{
    if (image.width >= 1024 || image.height >= 1024)
        return;
@@ -337,7 +344,7 @@ static void send_cursor(const XFixesCursorImage &image)
    write_all(streamfd, msg.get(), cursor_size);
}

-static void cursor_changes(Display *display, int event_base)
+static void cursor_changes(Display *display, int event_base, SpiceStream
*stream)
{
    unsigned long last_serial = 0;

@@ -355,23 +362,25 @@ static void cursor_changes(Display *display, int
event_base)
            continue;

        last_serial = cursor->cursor_serial;
-        send_cursor(*cursor);
+        stream->send_cursor(*cursor);
    }
}

static void
-do_capture(const char *streamport, FILE *f_log)
+do_capture(const char *streamport, FILE *f_log, Display *display, int
event_base)
{
    std::unique_ptr<FrameCapture> capture(agent.GetBestFrameCapture());
    if (!capture)
        throw std::runtime_error("cannot find a suitable capture system");

-    Stream stream(streamport, streamfd);
+    SpiceStream stream(streamport);
+    std::thread cursor_th(cursor_changes, display, event_base, &stream);
+    cursor_th.detach();

    unsigned int frame_count = 0;
    while (! quit) {
        while (!quit && !streaming_requested) {
-            if (read_command(1) < 0) {
+            if (stream.read_command(1) < 0) {
                syslog(LOG_ERR, "FAILED to read command\n");
                return;
            }
@@ -406,7 +415,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 (stream.send_format(width, height, codec) ==
EXIT_FAILURE)
                    throw std::runtime_error("FAILED to send format
                    message");
            }
            if (f_log) {
@@ -417,12 +426,12 @@ 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 (stream.send_frame(frame.buffer, frame.buffer_size) ==
EXIT_FAILURE) {
                syslog(LOG_ERR, "FAILED to send a frame\n");
                break;
            }
            //usleep(1);
-            if (read_command(0) < 0) {
+            if (stream.read_command(0) < 0) {
                syslog(LOG_ERR, "FAILED to read command\n");
                return;
            }
@@ -516,12 +525,9 @@ int main(int argc, char* argv[])
    Window rootwindow = DefaultRootWindow(display);
    XFixesSelectCursorInput(display, rootwindow,
    XFixesDisplayCursorNotifyMask);

-    std::thread cursor_th(cursor_changes, display, event_base);
-    cursor_th.detach();
-
    int ret = EXIT_SUCCESS;
    try {
-        do_capture(streamport, f_log);
+      do_capture(streamport, f_log, display, event_base);
    }
    catch (std::runtime_error &err) {
        syslog(LOG_ERR, "%s\n", err.what());

Frediano

_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]