[Spice-devel] [PATCH linux vdagent] Add systemd socket activation
Fabiano Fidêncio
fabiano at fidencio.org
Wed Jul 19 22:48:31 UTC 2017
On Thu, Jul 13, 2017 at 10:35 PM, Jonathon Jongsma <jjongsma at redhat.com> wrote:
> If we are configured to use the systemd init script, also add support
> for systemd socket activation. systemd will listen on the socket that is
> used to communicate between the session agent and the system daemon.
> When the session agent connects, the system daemon will automatically be
> started. The socket will be enabled only if the required virtio-port
> device exists. The socket is disabled when the device is removed.
>
> This has a couple minor advantages to the previous approach:
> - For VMS that are not running a graphical desktop (and thus no
> session agents are running), the system vdagent daemon won't get
> started at all even if the spice virtio port is configured. Only the
> socket will be enabled. In the previous approach, the system daemon
> was started when the virtio device was added regardless of whether
> it was needed or not.
> - Solves issues related to switching between systemd targets. With the
> previous approach, when a user switches to a different target
> ("systemctl isolate multi-user.target"), spice-vdagentd.target was
> stopped by systemd (since "isolate" by definition stops all targets
> except the one specified). This meant that if the user subsequently
> switched back to graphical.target, the spice-vdagentd.target would
> still be disabled and vdagent functionality would not work. With
> this change, the socket will still be listening after switching to a
> different target, so as soon as a session agent tries to connect to
> the socket, the system daemon will get restarted.
>
> Fixes: rhbz#1340160
>
> Signed-off-by: Jonathon Jongsma <jjongsma at redhat.com>
> ---
> NOTES:
> - this patch piggybacks onto the --with-init-script configure option to
> determine whether to use systemd socket activation. I'm not sure whether
> that's the correct choice or not, but it seems to work ok. If the init
> script is specified as "systemd" or "systemd+redhat" and we have a new
> enough libsystemd, we attempt to use socket activation.
> - Even if the agent is configured with systemd socket activation, it should
> still fall back to opening its own socket if it fails to get a socket from
> systemd (for example, if it is running under a different init system).
> - In theory, we could use socket activation with systemd < 209, but then we'd
> have to link against the separate libsystemd-daemon library. Since this is a
> new feature, I decided to only support more recent versions of systemd. But
> support for older versions could be added pretty easily.
>
>
> Makefile.am | 5 +++-
> configure.ac | 8 +++++++
> data/70-spice-vdagentd.rules | 2 +-
> data/spice-vdagentd.service | 8 +------
> data/spice-vdagentd.socket | 10 ++++++++
> data/spice-vdagentd.target | 2 --
> src/udscs.c | 45 ++++++++++++++++++++++++------------
> src/udscs.h | 11 +++++++++
> src/vdagentd/vdagentd.c | 55 +++++++++++++++++++++++++++++++++++---------
> 9 files changed, 109 insertions(+), 37 deletions(-)
> create mode 100644 data/spice-vdagentd.socket
> delete mode 100644 data/spice-vdagentd.target
>
> diff --git a/Makefile.am b/Makefile.am
> index 7755f09..f70d514 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -42,6 +42,7 @@ src_spice_vdagent_SOURCES = \
>
> src_spice_vdagentd_CFLAGS = \
> $(DBUS_CFLAGS) \
> + $(LIBSYSTEMD_DAEMON_CFLAGS) \
> $(LIBSYSTEMD_LOGIN_CFLAGS) \
> $(PCIACCESS_CFLAGS) \
> $(SPICE_CFLAGS) \
> @@ -52,6 +53,7 @@ src_spice_vdagentd_CFLAGS = \
>
> src_spice_vdagentd_LDADD = \
> $(DBUS_LIBS) \
> + $(LIBSYSTEMD_DAEMON_LIBS) \
> $(LIBSYSTEMD_LOGIN_LIBS) \
> $(PCIACCESS_LIBS) \
> $(SPICE_LIBS) \
> @@ -102,7 +104,7 @@ if INIT_SCRIPT_SYSTEMD
> systemdunitdir = $(SYSTEMDSYSTEMUNITDIR)
> systemdunit_DATA = \
> $(top_srcdir)/data/spice-vdagentd.service \
> - $(top_srcdir)/data/spice-vdagentd.target
> + $(top_srcdir)/data/spice-vdagentd.socket
>
> udevrulesdir = /lib/udev/rules.d
> udevrules_DATA = $(top_srcdir)/data/70-spice-vdagentd.rules
> @@ -124,6 +126,7 @@ EXTRA_DIST = \
> data/spice-vdagent.desktop \
> data/spice-vdagentd \
> data/spice-vdagentd.service \
> + data/spice-vdagentd.socket \
> data/spice-vdagentd.target \
> data/tmpfiles.d/spice-vdagentd.conf \
> data/xorg.conf.RHEL-5 \
> diff --git a/configure.ac b/configure.ac
> index f207240..fbc20a9 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -65,6 +65,14 @@ AC_MSG_RESULT($with_init_script)
> if test "x$init_systemd" = "xyes"; then
> SYSTEMDSYSTEMUNITDIR=`${PKG_CONFIG} systemd --variable=systemdsystemunitdir`
> AC_SUBST(SYSTEMDSYSTEMUNITDIR)
> + # earlier versions of systemd require a separate libsystemd-daemon library
> + PKG_CHECK_MODULES([LIBSYSTEMD_DAEMON],
> + [libsystemd >= 209],
> + [have_libsystemd_daemon="yes"],
> + [have_libsystemd_daemon="no"])
> + if test "$have_libsystemd_daemon" = "yes"; then
> + AC_DEFINE([WITH_SYSTEMD_SOCKET_ACTIVATION], [1], [If defined, vdagentd will support socket activation with systemd] )
> + fi
> fi
>
> AC_ARG_ENABLE([pciaccess],
> diff --git a/data/70-spice-vdagentd.rules b/data/70-spice-vdagentd.rules
> index a1785ba..64b4166 100644
> --- a/data/70-spice-vdagentd.rules
> +++ b/data/70-spice-vdagentd.rules
> @@ -1 +1 @@
> -ACTION=="add", SUBSYSTEM=="virtio-ports", ENV{DEVLINKS}=="/dev/virtio-ports/com.redhat.spice.0", ENV{SYSTEMD_WANTS}="spice-vdagentd.target"
> +ACTION=="add", SUBSYSTEM=="virtio-ports", ENV{DEVLINKS}=="/dev/virtio-ports/com.redhat.spice.0", ENV{SYSTEMD_WANTS}="spice-vdagentd.socket"
> diff --git a/data/spice-vdagentd.service b/data/spice-vdagentd.service
> index 8c5effe..c6a890a 100644
> --- a/data/spice-vdagentd.service
> +++ b/data/spice-vdagentd.service
> @@ -1,17 +1,11 @@
> [Unit]
> Description=Agent daemon for Spice guests
> After=dbus.target
> -
> -# TODO we should use:
> -#Requires=spice-vdagentd.socket
> +Requires=spice-vdagentd.socket
Is this really necessary? IIRC it's not when .service and the .socket
have the very same name.
>
> [Service]
> Type=forking
> EnvironmentFile=-/etc/sysconfig/spice-vdagentd
> -ExecStartPre=/bin/rm -f /var/run/spice-vdagentd/spice-vdagent-sock
> ExecStart=/usr/sbin/spice-vdagentd $SPICE_VDAGENTD_EXTRA_ARGS
> PIDFile=/var/run/spice-vdagentd/spice-vdagentd.pid
> PrivateTmp=true
Don't we want to have the service restared on failure as well?
Restart=on-failure
> -
> -[Install]
> -WantedBy=spice-vdagentd.target
It's a quite common practice to add "Also=<unit's name>.socket in the
Install part, to ensure proper installation/uninstallation when using
systemctl enable/disable.
> diff --git a/data/spice-vdagentd.socket b/data/spice-vdagentd.socket
> new file mode 100644
> index 0000000..e34a188
> --- /dev/null
> +++ b/data/spice-vdagentd.socket
> @@ -0,0 +1,10 @@
> +[Unit]
> +Description=Activation socket for spice guest agent daemon
> +# only start the socket if the virtio port device exists
> +Requisite=dev-virtio\x2dports-com.redhat.spice.0.device
> +
> +[Socket]
> +ListenStream=/var/run/spice-vdagentd/spice-vdagent-sock
> +
> +[Install]
> +WantedBy=sockets.target
> diff --git a/data/spice-vdagentd.target b/data/spice-vdagentd.target
> deleted file mode 100644
> index 1f74931..0000000
> --- a/data/spice-vdagentd.target
> +++ /dev/null
> @@ -1,2 +0,0 @@
> -[Unit]
> -Description=Agent daemon for Spice guests
> diff --git a/src/udscs.c b/src/udscs.c
> index fdd75a4..8b16f89 100644
> --- a/src/udscs.c
> +++ b/src/udscs.c
> @@ -369,16 +369,19 @@ struct udscs_server {
> udscs_disconnect_callback disconnect_callback;
> };
>
> -struct udscs_server *udscs_create_server(const char *socketname,
> +struct udscs_server *udscs_create_server_for_fd(int fd,
> udscs_connect_callback connect_callback,
> udscs_read_callback read_callback,
> udscs_disconnect_callback disconnect_callback,
> const char * const type_to_string[], int no_types, int debug)
> {
> - int c;
> - struct sockaddr_un address;
> struct udscs_server *server;
>
> + if (fd <= 0) {
> + syslog(LOG_ERR, "Invalid file descriptor: %i", fd);
> + return NULL;
> + }
> +
> server = calloc(1, sizeof(*server));
> if (!server)
> return NULL;
> @@ -386,35 +389,47 @@ struct udscs_server *udscs_create_server(const char *socketname,
> server->type_to_string = type_to_string;
> server->no_types = no_types;
> server->debug = debug;
> + server->fd = fd;
> + server->connect_callback = connect_callback;
> + server->read_callback = read_callback;
> + server->disconnect_callback = disconnect_callback;
> +
> + return server;
> +}
> +
> +struct udscs_server *udscs_create_server(const char *socketname,
> + udscs_connect_callback connect_callback,
> + udscs_read_callback read_callback,
> + udscs_disconnect_callback disconnect_callback,
> + const char * const type_to_string[], int no_types, int debug)
> +{
> + int c;
> + int fd;
> + struct sockaddr_un address;
>
> - server->fd = socket(PF_UNIX, SOCK_STREAM, 0);
> - if (server->fd == -1) {
> + fd = socket(PF_UNIX, SOCK_STREAM, 0);
> + if (fd == -1) {
> syslog(LOG_ERR, "creating unix domain socket: %m");
> - free(server);
> return NULL;
> }
>
> address.sun_family = AF_UNIX;
> snprintf(address.sun_path, sizeof(address.sun_path), "%s", socketname);
> - c = bind(server->fd, (struct sockaddr *)&address, sizeof(address));
> + c = bind(fd, (struct sockaddr *)&address, sizeof(address));
> if (c != 0) {
> syslog(LOG_ERR, "bind %s: %m", socketname);
> - free(server);
> return NULL;
> }
>
> - c = listen(server->fd, 5);
> + c = listen(fd, 5);
> if (c != 0) {
> syslog(LOG_ERR, "listen: %m");
> - free(server);
> return NULL;
> }
>
> - server->connect_callback = connect_callback;
> - server->read_callback = read_callback;
> - server->disconnect_callback = disconnect_callback;
> -
> - return server;
> + return udscs_create_server_for_fd(fd, connect_callback, read_callback,
> + disconnect_callback, type_to_string,
> + no_types, debug);
> }
>
> void udscs_destroy_server(struct udscs_server *server)
> diff --git a/src/udscs.h b/src/udscs.h
> index 7b6bff0..30a96db 100644
> --- a/src/udscs.h
> +++ b/src/udscs.h
> @@ -120,6 +120,17 @@ struct udscs_server;
> */
> typedef void (*udscs_connect_callback)(struct udscs_connection *conn);
>
> +/* Create a server for the given file descriptor. This allows us to use
> + * pre-configured sockets for use with systemd socket activation, etc.
> + *
> + * See udscs_create_server() for more information
> + */
> +struct udscs_server *udscs_create_server_for_fd(int fd,
> + udscs_connect_callback connect_callback,
> + udscs_read_callback read_callback,
> + udscs_disconnect_callback disconnect_callback,
> + const char * const type_to_string[], int no_types, int debug);
> +
> /* Create the unix domain socket specified by socketname and
> * start listening on it.
> * Only sockets bound to a pathname are supported.
> diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
> index 1f10f1c..eb6834a 100644
> --- a/src/vdagentd/vdagentd.c
> +++ b/src/vdagentd/vdagentd.c
> @@ -36,6 +36,10 @@
> #include <spice/vd_agent.h>
> #include <glib.h>
>
> +#ifdef WITH_SYSTEMD_SOCKET_ACTIVATION
> +#include <systemd/sd-daemon.h>
> +#endif /* WITH_SYSTEMD_SOCKET_ACTIVATION */
> +
> #include "udscs.h"
> #include "vdagentd-proto.h"
> #include "vdagentd-proto-strings.h"
> @@ -1083,6 +1087,7 @@ int main(int argc, char *argv[])
> int do_daemonize = 1;
> int want_session_info = 1;
> struct sigaction act;
> + gboolean own_socket = TRUE;
>
> for (;;) {
> if (-1 == (c = getopt(argc, argv, "-dhxXfos:u:S:")))
> @@ -1133,10 +1138,30 @@ int main(int argc, char *argv[])
> openlog("spice-vdagentd", do_daemonize ? 0 : LOG_PERROR, LOG_USER);
>
> /* Setup communication with vdagent process(es) */
> - server = udscs_create_server(vdagentd_socket, agent_connect,
> - agent_read_complete, agent_disconnect,
> - vdagentd_messages, VDAGENTD_NO_MESSAGES,
> - debug);
> +#ifdef WITH_SYSTEMD_SOCKET_ACTIVATION
> + int n_fds;
> + /* try to retrieve pre-configured sockets from systemd */
> + n_fds = sd_listen_fds(0);
> + if (n_fds > 1) {
> + syslog(LOG_CRIT, "Recieved too many sockets from systemd (%i)", n_fds);
> + return 1;
> + } else if (n_fds == 1) {
> + server = udscs_create_server_for_fd(SD_LISTEN_FDS_START, agent_connect,
> + agent_read_complete,
> + agent_disconnect,
> + vdagentd_messages,
> + VDAGENTD_NO_MESSAGES, debug);
> + own_socket = FALSE;
> + } else
> + /* systemd socket activation not enabled, create our own */
> +#endif /* WITH_SYSTEMD_SOCKET_ACTIVATION */
> + {
> + server = udscs_create_server(vdagentd_socket, agent_connect,
> + agent_read_complete, agent_disconnect,
> + vdagentd_messages, VDAGENTD_NO_MESSAGES,
> + debug);
> + }
> +
> if (!server) {
> if (errno == EADDRINUSE) {
> syslog(LOG_CRIT, "Fatal the server socket %s exists already. Delete it?",
> @@ -1147,11 +1172,15 @@ int main(int argc, char *argv[])
> }
> return 1;
> }
> - if (chmod(vdagentd_socket, 0666)) {
> - syslog(LOG_CRIT, "Fatal could not change permissions on %s: %m",
> - vdagentd_socket);
> - udscs_destroy_server(server);
> - return 1;
> +
> + /* no need to set permissions on a socket that was provided by systemd */
> + if (own_socket) {
> + if (chmod(vdagentd_socket, 0666)) {
> + syslog(LOG_CRIT, "Fatal could not change permissions on %s: %m",
> + vdagentd_socket);
> + udscs_destroy_server(server);
> + return 1;
> + }
> }
>
> if (do_daemonize)
> @@ -1181,8 +1210,12 @@ int main(int argc, char *argv[])
> vdagent_virtio_port_destroy(&virtio_port);
> session_info_destroy(session_info);
> udscs_destroy_server(server);
> - if (unlink(vdagentd_socket) != 0)
> - syslog(LOG_ERR, "unlink %s: %s", vdagentd_socket, strerror(errno));
> +
> + /* leave the socket around if it was provided by systemd */
> + if (own_socket) {
> + if (unlink(vdagentd_socket) != 0)
> + syslog(LOG_ERR, "unlink %s: %s", vdagentd_socket, strerror(errno));
> + }
> syslog(LOG_INFO, "vdagentd quitting, returning status %d", retval);
>
> if (do_daemonize)
> --
> 2.9.4
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
--
Fabiano Fidêncio
More information about the Spice-devel
mailing list