[Spice-devel] [PATCH spice-streaming-agent v2 3/5] Starts/stops the agent based on VT status
Lukáš Hrázký
lhrazky at redhat.com
Fri Jun 8 14:25:08 UTC 2018
On Mon, 2018-05-21 at 11:45 +0100, Frediano Ziglio wrote:
> Check if the current display server is active or not and stream only if
> active.
> This will allow to support multiple display servers running.
> When multiple display servers are running only one have the GPU
> associated and is writing to the screen.
> Stop capturing and release resources when the display server is not
> active and vice versa.
>
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
> src/Makefile.am | 2 ++
> src/eventfd.cpp | 38 ++++++++++++++++++++
> src/eventfd.hpp | 34 ++++++++++++++++++
> src/spice-streaming-agent.cpp | 65 +++++++++++++++++++++++++++++++----
> 4 files changed, 132 insertions(+), 7 deletions(-)
> create mode 100644 src/eventfd.cpp
> create mode 100644 src/eventfd.hpp
>
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 276478f..ffc52b2 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -62,4 +62,6 @@ spice_streaming_agent_SOURCES = \
> jpeg.hpp \
> stream-port.cpp \
> stream-port.hpp \
> + eventfd.cpp \
> + eventfd.hpp \
> $(NULL)
> diff --git a/src/eventfd.cpp b/src/eventfd.cpp
> new file mode 100644
> index 0000000..e3c1e72
> --- /dev/null
> +++ b/src/eventfd.cpp
> @@ -0,0 +1,38 @@
> +/* Linux event file descriptors
> + *
> + * \copyright
> + * Copyright 2018 Red Hat Inc. All rights reserved.
> + */
> +#include <config.h>
> +#include "eventfd.hpp"
> +
> +#include <stdexcept>
> +#include <sys/eventfd.h>
> +
> +template<typename T>
> +inline void ignore_result(T /* unused result */) {}
> +
> +namespace spice {
> +namespace streaming_agent {
> +
> +EventFD::EventFD():
> + fd(eventfd(0, EFD_CLOEXEC|EFD_NONBLOCK))
> +{
> + if (fd < 0) {
> + throw std::runtime_error("Failed to create eventfd");
Use an error derived from our error hierarchy (inheriting Error) and
include the actual error string in the exception.
> + }
> +}
> +
> +void EventFD::signal()
> +{
> + uint64_t n = 1;
> + ignore_result(write(fd, &n, sizeof(n)));
> +}
> +
> +void EventFD::ack()
> +{
> + uint64_t n;
> + ignore_result(read(fd, &n, sizeof(n)));
> +}
> +
> +}} // namespace spice::streaming_agent
> diff --git a/src/eventfd.hpp b/src/eventfd.hpp
> new file mode 100644
> index 0000000..48a175c
> --- /dev/null
> +++ b/src/eventfd.hpp
> @@ -0,0 +1,34 @@
> +/* Linux event file descriptors
The description is very vague. See also the docstring note below.
> + *
> + * \copyright
> + * Copyright 2018 Red Hat Inc. All rights reserved.
> + */
> +#ifndef SPICE_STREAMING_AGENT_EVENTFD_HPP
> +#define SPICE_STREAMING_AGENT_EVENTFD_HPP
> +
> +#include <unistd.h>
> +
> +namespace spice {
> +namespace streaming_agent {
The class could really use a docstring saying what it's about (i.e. a
portable eventfd implementation allowing to pass signals through a file
descriptor).
> +class EventFD {
> +public:
> + EventFD();
> + ~EventFD() { close(fd); }
> + void signal();
> + void ack();
> + /**
> + * Returns underlying file descriptor.
> + * Do not close, should just be used for polling.
> + */
> + int raw_fd() const { return fd; }
> +private:
> + EventFD(const EventFD&) = delete;
> + void operator=(const EventFD&) = delete;
> +
> + int fd;
> +};
> +
> +}} // namespace spice::streaming_agent
> +
> +#endif // SPICE_STREAMING_AGENT_EVENTFD_HPP
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index 6388bb2..240b9c7 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -10,6 +10,7 @@
> #include "stream-port.hpp"
> #include "error.hpp"
> #include "xorg-utils.hpp"
> +#include "eventfd.hpp"
>
> #include <spice/stream-device.h>
> #include <spice/enums.h>
> @@ -36,6 +37,7 @@
> #include <thread>
> #include <vector>
> #include <string>
> +#include <atomic>
> #include <functional>
> #include <X11/Xlib.h>
> #include <X11/extensions/Xfixes.h>
> @@ -61,12 +63,21 @@ static bool quit_requested = false;
> static bool log_binary = false;
> static bool log_frames = false;
> static std::set<SpiceVideoCodecType> client_codecs;
> +static EventFD update_fd;
> +static const char vt_property_name[] = "XFree86_has_VT";
> +static const char *stream_port_name = "/dev/virtio-ports/org.spice-space.stream.0";
Why did you move this to be static instead of local in a scope?
Limiting the scope of variables is a good thing...
> +static Atom vt_property = None;
> +static std::atomic_bool vt_active;
Adding more static variables here goes directly against the intention
to modularize the streaming agent. I think more refactoring needs to
happen before this can be implemented cleanly.
> -static bool have_something_to_read(StreamPort &stream_port, bool blocking)
> +static bool have_something_to_read(StreamPort &stream_port, bool blocking, int &fd)
> {
> - struct pollfd pollfd = {stream_port.fd, POLLIN, 0};
> + struct pollfd pollfds[2] = {
> + { stream_port.fd, POLLIN, 0 },
> + { update_fd.raw_fd(), POLLIN, 0 },
> + };
>
> - if (poll(&pollfd, 1, blocking ? -1 : 0) < 0) {
> + fd = -1;
> + if (poll(pollfds, 2, blocking ? -1 : 0) < 0) {
> if (errno == EINTR) {
> // report nothing to read, next iteration of the enclosing loop will retry
> return false;
> @@ -75,7 +86,13 @@ static bool have_something_to_read(StreamPort &stream_port, bool blocking)
> throw IOError("poll failed on the device", errno);
> }
>
> - if (pollfd.revents & POLLIN) {
> + if (pollfds[1].revents & POLLIN) {
> + fd = update_fd.raw_fd();
> + return true;
> + }
> +
> + if (pollfds[0].revents & POLLIN) {
> + fd = stream_port.fd;
> return true;
> }
>
> @@ -173,8 +190,18 @@ static void read_command_from_device(StreamPort &stream_port)
> static void read_command(StreamPort &stream_port, bool blocking)
> {
> while (!quit_requested) {
> - if (have_something_to_read(stream_port, blocking)) {
> - read_command_from_device(stream_port);
> + int fd;
> + if (have_something_to_read(stream_port, blocking, fd)) {
> + if (fd == stream_port.fd) {
> + read_command_from_device(stream_port);
> + } else if (fd == update_fd.raw_fd()) {
> + update_fd.ack();
> + bool vt_active = ::vt_active.load(std::memory_order_relaxed);
> + if (!vt_active) {
> + throw std::runtime_error("VT disabled");
Error class and a useless error message again... it holds for most of
the exceptions added in this series, so I won't mention it again...
> + }
> + continue;
> + }
Adding this second (and unrelated, besides the need to share the poll
and the control loop) fd here doesn't help with the separation of code
into modules. It seems hard, I'm thinking of ways to solve it and have
some foggy ideas, we can discuss it later or I may post some refactor
patch if I come up with something usable.
> break;
> }
>
> @@ -305,10 +332,22 @@ send_cursor(StreamPort &stream_port, unsigned width, unsigned height, int hotspo
> static void cursor_changes(StreamPort *stream_port, 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);
> + update_fd.signal();
> + continue;
> + }
> +
This looks shoehorned here, at least the cursor thread should be
renamed to a generic X thread. Not having many better ideas though,
apart from actually using logind as Christophe suggested, which might
be better altogether?
> if (event.type != event_base + 1) {
> continue;
> }
> @@ -416,7 +455,6 @@ do_capture(StreamPort &stream_port, FILE *f_log)
>
> int main(int argc, char* argv[])
> {
> - const char *stream_port_name = "/dev/virtio-ports/org.spice-space.stream.0";
> int opt;
> const char *log_filename = NULL;
> int logmask = LOG_UPTO(LOG_WARNING);
> @@ -522,6 +560,19 @@ 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;
I'd prefer to use exceptions consistently for handling errors...
> + }
> + XSelectInput(display, rootwindow, PropertyChangeMask);
> +
> + vt_active.store(get_win_prop_int(display, rootwindow, vt_property) != 0, std::memory_order_relaxed);
> + if (!::vt_active.load(std::memory_order_relaxed)) {
> + syslog(LOG_ERR, "VT is disabled");
> + return EXIT_FAILURE;
> + }
> +
> int ret = EXIT_SUCCESS;
>
> try {
More information about the Spice-devel
mailing list