[Spice-devel] [PATCH spice-streaming-agent 3/5] Starts/stops the agent based on VT status
Frediano Ziglio
fziglio at redhat.com
Tue Feb 6 10:06:42 UTC 2018
>
> 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 at redhat.com>
> > ---
> > 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
More information about the Spice-devel
mailing list