[Spice-devel] [PATCH spice-streaming-agent 3/5] Starts/stops the agent based on VT status
Lukáš Hrázký
lhrazky at redhat.com
Tue Feb 6 13:33:48 UTC 2018
On Tue, 2018-02-06 at 05:06 -0500, Frediano Ziglio wrote:
> >
> > 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.
Yeah, but for thread safety no global variables (or locking thereof) is
a requirement, cardinality is a design decision. Yes, none of those
apply here (though one day you may wanna add a thread and then off you
go refactoring everything).
But good practice here is not for the above reasons, but also because
it enables modularity and locality and makes you write cleaner code.
Here you have everything in one global context, accessing it from
whereever you want. That is not a good design. Good for a quick
prototype, but not good for production code (imho).
> > > 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.
Well, I'm thinking that you would see if it's failing sometime. Not
sure what the attempts are here for, is this a TEMP_FAILURE_RETRY
analogy? (and if so, why not use the macro?)
> > > + 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