Re: [PATCH spice-streaming-agent 3/5] Starts/stops the agent based on VT status

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

 



> 
> On Thu, 2018-02-01 at 16:40 +0000, Frediano Ziglio wrote:
> > Check if the current server is active or not and stream only if active.
> > This will allow to support multiple servers running.
> > When multiple servers are running only one have the GPU associated
> > and is writing to the screen.
> > Stop capturing and release resources when the server is not active
> > and vice versa.
> 
> In every place in this patch series where you talk about 'server', you
> should replace it with 'display server' (or possibly just 'X server',
> not sure if Wayland is ever going to apply here?). I was confused for
> quite some time thinking you mean the SPICE server.
> 

Agreed

> > Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> > ---
> >  src/spice-streaming-agent.cpp | 99
> >  ++++++++++++++++++++++++++++++++++---------
> >  1 file changed, 80 insertions(+), 19 deletions(-)
> > 
> > diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> > index a688d1f..2c32070 100644
> > --- a/src/spice-streaming-agent.cpp
> > +++ b/src/spice-streaming-agent.cpp
> > @@ -16,12 +16,14 @@
> >  #include <poll.h>
> >  #include <syslog.h>
> >  #include <signal.h>
> > +#include <sys/eventfd.h>
> >  #include <exception>
> >  #include <stdexcept>
> >  #include <memory>
> >  #include <mutex>
> >  #include <thread>
> >  #include <vector>
> > +#include <atomic>
> >  #include <functional>
> >  #include <X11/Xlib.h>
> >  #include <X11/extensions/Xfixes.h>
> > @@ -57,19 +59,25 @@ static int streaming_requested;
> >  static std::set<SpiceVideoCodecType> client_codecs;
> >  static bool quit;
> >  static int streamfd = -1;
> > +static int update_fd = -1;
> >  static bool stdin_ok;
> >  static int log_binary = 0;
> >  static std::mutex stream_mtx;
> > +static const char vt_property_name[] = "XFree86_has_VT";
> 
> const char[] here, std::string on the line below :) Just use
> std::string...
> 

Well, is quite different. One is a variable, the other is basically
a label for a constant.

> > +static string streamport = "/dev/virtio-ports/com.redhat.stream.0";
> > +static Atom vt_property = None;
> > +static std::atomic_bool vt_active;
> 
> These static variables... Not too sure about C, in C++ it is considered
> bad practice. Not an objection here at the moment, since there are some
> already present, but in the future the way to go is to create a class
> (or more) to hold them and properly encapsulate them.
> 

Yes, better option would be to encapsulate, surely.
There are different reason why static and globals are not a good practise
in either C and C++. One is thread safety, another is cardinality (they
basically create a singleton), they can be implemented with class members.
Currently none of the above apply.

> >  static int have_something_to_read(int *pfd, int timeout)
> >  {
> >      int nfds;
> > -    struct pollfd pollfds[2] = {
> > +    struct pollfd pollfds[3] = {
> >          {streamfd, POLLIN, 0},
> > +        {update_fd, POLLIN, 0},
> >          {0, POLLIN, 0}
> >      };
> >      *pfd = -1;
> > -    nfds = (stdin_ok ? 2 : 1);
> > +    nfds = (stdin_ok ? 3 : 2);
> >      if (poll(pollfds, nfds, timeout) < 0) {
> >          syslog(LOG_ERR, "poll FAILED\n");
> >          return -1;
> > @@ -78,6 +86,9 @@ static int have_something_to_read(int *pfd, int timeout)
> >          *pfd = streamfd;
> >      }
> >      if (pollfds[1].revents == POLLIN) {
> > +        *pfd = update_fd;
> > +    }
> > +    if (pollfds[2].revents == POLLIN) {
> >          *pfd = 0;
> >      }
> >      return *pfd != -1;
> > @@ -152,6 +163,28 @@ static int read_command_from_device(void)
> >      return 1;
> >  }
> >  
> > +static void start_capture()
> > +{
> > +    for (int attempts = 0; ; ) {
> > +        streamfd = open(streamport.c_str(), O_RDWR);
> > +        if (streamfd >= 0)
> > +            break;
> > +        if (++attempts > 10)
> > +            throw std::runtime_error("failed to open the streaming device
> > (" +
> > +                                     streamport + "): " +
> > strerror(errno));
> 
> It would be good to log the failed attempts. As a side note, we should
> look for a logging library for C++.
> 

Why not only the last? I would put only in case of debugging.

> > +        usleep(50 * 1000);
> > +    }
> > +}
> > +
> > +static void stop_capture()
> > +{
> > +    if (streamfd) {
> > +        close(streamfd);
> > +        streamfd = -1;
> > +    }
> > +    streaming_requested = 0;
> > +}
> > +
> >  static int read_command(bool blocking)
> >  {
> >      int fd, n=1;
> > @@ -164,8 +197,17 @@ static int read_command(bool blocking)
> >              sleep(1);
> >              continue;
> >          }
> > -        if (fd) {
> > +        if (fd == streamfd) {
> >              n = read_command_from_device();
> > +        } else if (fd == update_fd) {
> > +            uint64_t n;
> > +            read(update_fd, &n, sizeof(n));
> > +            bool vt_active = ::vt_active.load(std::memory_order_relaxed);
> > +            if (vt_active) {
> > +                start_capture();
> > +            } else {
> > +                stop_capture();
> > +            }
> >          } else {
> >              n = read_command_from_stdin();
> >          }
> > @@ -328,10 +370,23 @@ send_cursor(unsigned width, unsigned height, int
> > hotspot_x, int hotspot_y,
> >  static void cursor_changes(Display *display, int event_base)
> >  {
> >      unsigned long last_serial = 0;
> > +    Window rootwindow = DefaultRootWindow(display);
> >  
> >      while (1) {
> >          XEvent event;
> >          XNextEvent(display, &event);
> > +
> > +        if (event.type == PropertyNotify) {
> > +            if (vt_property == None || event.xproperty.atom !=
> > vt_property)
> > +                continue;
> > +            // update vt property, activate screen read if needed
> > +            vt_active.store(get_win_prop_int(display, rootwindow,
> > vt_property) != 0, std::memory_order_relaxed);
> > +            std::atomic_thread_fence(std::memory_order_acquire);
> > +            uint64_t n = 1;
> > +            write(update_fd, &n, sizeof(n));
> 
> What does writing '1' to update_fd mean? (rhetorical question :)) You
> do not even check the value in the place you read it from update_fd. I
> understand this is sufficient and works atm., but this way you include
> random communication details in deferent places of the code, it is not
> clean.
> 

The 1 is a 1. Yes, having an helper function would help to understand.
The check for value is not necessary in this case, is just used to
reset the kernel signal, no matter the content.

> This communication should be encapsulated, in C++ for example in a
> class that would have a method like toggle_streaming(), which would
> send the '1' and in the place where you read the command, you would use
> the same class to do so.
> 

Yes, make sense.

> > +            continue;
> > +        }
> > +
> >          if (event.type != event_base + 1)
> >              continue;
> >  
> > @@ -352,19 +407,14 @@ static void cursor_changes(Display *display, int
> > event_base)
> >  }
> >  
> >  static void
> > -do_capture(const string &streamport, FILE *f_log)
> > +do_capture(FILE *f_log)
> >  {
> > -    streamfd = open(streamport.c_str(), O_RDWR);
> > -    if (streamfd < 0)
> > -        throw std::runtime_error("failed to open the streaming device (" +
> > -                                 streamport + "): " + strerror(errno));
> > -
> >      unsigned int frame_count = 0;
> >      while (! quit) {
> >          while (!quit && !streaming_requested) {
> >              if (read_command(true) < 0) {
> >                  syslog(LOG_ERR, "FAILED to read command\n");
> > -                goto done;
> > +                return;
> >              }
> >          }
> >  
> > @@ -422,23 +472,16 @@ do_capture(const string &streamport, FILE *f_log)
> >              //usleep(1);
> >              if (read_command(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__);
> >  
> >  int main(int argc, char* argv[])
> >  {
> > -    string streamport = "/dev/virtio-ports/com.redhat.stream.0";
> >      char opt;
> >      const char *log_filename = NULL;
> >      int logmask = LOG_UPTO(LOG_WARNING);
> > @@ -492,6 +535,12 @@ int main(int argc, char* argv[])
> >  
> >      agent.LoadPlugins(PLUGINSDIR);
> >  
> > +    update_fd = eventfd(0, EFD_CLOEXEC|EFD_NONBLOCK);
> > +    if (update_fd < 0) {
> > +        syslog(LOG_ERR, "FAILED to create eventfd\n");
> > +        return EXIT_FAILURE;
> > +    }
> > +
> >      register_interrupts();
> >  
> >      FILE *f_log = NULL;
> > @@ -517,12 +566,24 @@ int main(int argc, char* argv[])
> >      Window rootwindow = DefaultRootWindow(display);
> >      XFixesSelectCursorInput(display, rootwindow,
> >      XFixesDisplayCursorNotifyMask);
> >  
> > +    vt_property = XInternAtom(display, vt_property_name, True);
> > +    if (vt_property == None) {
> > +        syslog(LOG_ERR, "VT property not found\n");
> > +        return EXIT_FAILURE;
> > +    }
> > +    XSelectInput(display, rootwindow, PropertyChangeMask);
> > +
> > +    vt_active.store(get_win_prop_int(display, rootwindow, vt_property) !=
> > 0, std::memory_order_relaxed);
> > +
> >      std::thread cursor_th(cursor_changes, display, event_base);
> >      cursor_th.detach();
> >  
> >      int ret = EXIT_SUCCESS;
> >      try {
> > -        do_capture(streamport, f_log);
> > +        if (::vt_active.load(std::memory_order_relaxed))
> > +            start_capture();
> > +        do_capture(f_log);
> > +        stop_capture();
> >      }
> >      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]