12:22 a.m.
On Thu, May 21, 2015 at 05:46:54PM +0200, Michal Privoznik wrote:
On 20.05.2015 07:19, Martin Kletzander wrote:
> For this to pe properly separated from other protocols used by the
> server, there is second server added which allows access to the whole
> virNetDaemon to its clients.
>
> Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
> ---
> cfg.mk | 3 ++
> daemon/Makefile.am | 34 ++++++++++++++-
> daemon/admin_server.c | 116 ++++++++++++++++++++++++++++++++++++++++++++++++++
> daemon/admin_server.h | 36 ++++++++++++++++
> daemon/libvirtd.c | 104 +++++++++++++++++++++++++++++++++++++++-----
> daemon/libvirtd.h | 14 +++++-
> po/POTFILES.in | 1 +
> 7 files changed, 295 insertions(+), 13 deletions(-)
> create mode 100644 daemon/admin_server.c
> create mode 100644 daemon/admin_server.h
>
> diff --git a/daemon/Makefile.am b/daemon/Makefile.am
> index 42dec5d..45c6e9b 100644
> --- a/daemon/Makefile.am
> +++ b/daemon/Makefile.am
> @@ -1,6 +1,6 @@
> ## Process this file with automake to produce Makefile.in
>
> -## Copyright (C) 2005-2014 Red Hat, Inc.
> +## Copyright (C) 2005-2015 Red Hat, Inc.
> ##
> ## This library is free software; you can redistribute it and/or
> ## modify it under the terms of the GNU Lesser General Public
> @@ -25,6 +25,7 @@ INCLUDES = \
> -I$(top_srcdir)/src/conf \
> -I$(top_srcdir)/src/rpc \
> -I$(top_srcdir)/src/remote \
> + -I$(top_srcdir)/src/admin \
> -I$(top_srcdir)/src/access \
> $(GETTEXT_CPPFLAGS)
>
> @@ -34,6 +35,7 @@ DAEMON_GENERATED = \
> remote_dispatch.h \
> lxc_dispatch.h \
> qemu_dispatch.h \
> + admin_dispatch.h \
> $(NULL)
>
> DAEMON_SOURCES = \
> @@ -49,6 +51,7 @@ EXTRA_DIST = \
> remote_dispatch.h \
> lxc_dispatch.h \
> qemu_dispatch.h \
> + admin_dispatch.h \
> libvirtd.conf \
> libvirtd.init.in \
> libvirtd.upstart \
> @@ -78,6 +81,9 @@ BUILT_SOURCES =
> REMOTE_PROTOCOL = $(top_srcdir)/src/remote/remote_protocol.x
> LXC_PROTOCOL = $(top_srcdir)/src/remote/lxc_protocol.x
> QEMU_PROTOCOL = $(top_srcdir)/src/remote/qemu_protocol.x
> +ADMIN_PROTOCOL = $(top_srcdir)/src/admin/admin_protocol.x
> +
> +BUILT_SOURCES += admin_dispatch.h
No. This one is not really a built source. Into BUILT_SOURCES should go
only those files which are built from the .tar.gz. This one is built
from git. I mean, you need whole set of developer tools (e.g. rpcgen in
this case) to generate some files which are part of released package.
For instance, previously unpacking the package, and running 'make clean'
would not clean anything. Now it cleans admin_dispatch.h. Worse, in
order to generate it you need rpcgen, which you previously didn't.
>
> remote_dispatch.h: $(top_srcdir)/src/rpc/gendispatch.pl \
> $(REMOTE_PROTOCOL)
> @@ -97,6 +103,12 @@ qemu_dispatch.h: $(top_srcdir)/src/rpc/gendispatch.pl \
> --mode=server qemu QEMU $(QEMU_PROTOCOL) \
> > $(srcdir)/qemu_dispatch.h
>
> +admin_dispatch.h: $(srcdir)/../src/rpc/gendispatch.pl \
> + $(ADMIN_PROTOCOL)
> + $(AM_V_GEN)$(PERL) -w $(srcdir)/../src/rpc/gendispatch.pl \
> + --mode=server admin ADMIN $(ADMIN_PROTOCOL) \
> + > $(srcdir)/admin_dispatch.h
> +
> if WITH_LIBVIRTD
>
> # Build a convenience library, for reuse in tests/libvirtdconftest
> diff --git a/daemon/admin_server.c b/daemon/admin_server.c
> new file mode 100644
> index 0000000..810908b
> --- /dev/null
> +++ b/daemon/admin_server.c
> @@ -0,0 +1,116 @@
> +/*
> + * admin_server.c:
> + *
> + * Copyright (C) 2014-2015 Red Hat, Inc.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library. If not, see
> + * <http://www.gnu.org/licenses/>;.
> + *
> + * Author: Martin Kletzander <mkletzan(a)redhat.com>
> + */
> +
> +#include <config.h>
> +
> +#include "internal.h"
> +#include "libvirtd.h"
> +#include "libvirt_internal.h"
> +
> +#include "admin_protocol.h"
> +#include "admin_server.h"
> +#include "datatypes.h"
> +#include "viralloc.h"
> +#include "virerror.h"
> +#include "virlog.h"
> +#include "virnetdaemon.h"
> +#include "virnetserver.h"
> +#include "virstring.h"
> +#include "virthreadjob.h"
> +
> +#define VIR_FROM_THIS VIR_FROM_ADMIN
> +
> +VIR_LOG_INIT("daemon.admin");
> +
> +
> +void
> +remoteAdmClientFreeFunc(void *data)
> +{
> + struct daemonAdmClientPrivate *priv = data;
> +
> + virObjectUnref(priv->dmn);
virMutexDestroy(&priv->lock);
> + VIR_FREE(data);
> +}
> +
> +void *
> +remoteAdmClientInitHook(virNetServerClientPtr client ATTRIBUTE_UNUSED,
> + void *opaque)
> +{
> + struct daemonAdmClientPrivate *priv;
> +
> + if (VIR_ALLOC(priv) < 0)
> + return NULL;
> +
> + if (virMutexInit(&priv->lock) < 0) {
> + VIR_FREE(priv);
> + virReportSystemError(errno, "%s", _("unable to init
mutex"));
> + return NULL;
> + }
> +
> + /*
> + * We don't necessarily need to ref this object right now as there
> + * must be one ref being held throughout the life of the daemon,
> + * but let's just be safe for future.
> + */
> + priv->dmn = virObjectRef(opaque);
> +
> + return priv;
> +}
> diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
> index 22ba6cb..1205671 100644
> --- a/daemon/libvirtd.c
> +++ b/daemon/libvirtd.c
> @@ -44,6 +44,7 @@
> #include "libvirtd.h"
> #include "libvirtd-config.h"
>
> +#include "admin_server.h"
> #include "viruuid.h"
> #include "remote_driver.h"
> #include "viralloc.h"
> @@ -112,6 +113,7 @@ VIR_LOG_INIT("daemon.libvirtd");
> virNetSASLContextPtr saslCtxt = NULL;
> #endif
> virNetServerProgramPtr remoteProgram = NULL;
> +virNetServerProgramPtr adminProgram = NULL;
> virNetServerProgramPtr qemuProgram = NULL;
> virNetServerProgramPtr lxcProgram = NULL;
>
> @@ -253,18 +255,24 @@ static int
> daemonUnixSocketPaths(struct daemonConfig *config,
> bool privileged,
> char **sockfile,
> - char **rosockfile)
> + char **rosockfile,
> + char **admsockfile)
> {
> if (config->unix_sock_dir) {
> if (virAsprintf(sockfile, "%s/libvirt-sock",
config->unix_sock_dir) < 0)
> goto error;
> - if (privileged &&
> - virAsprintf(rosockfile, "%s/libvirt-sock-ro",
config->unix_sock_dir) < 0)
> - goto error;
> +
> + if (privileged) {
> + if (virAsprintf(rosockfile, "%s/libvirt-sock-ro",
config->unix_sock_dir) < 0)
> + goto error;
> + if (virAsprintf(admsockfile, "%s/libvirt-admin-sock",
config->unix_sock_dir) < 0)
> + goto error;
> + }
> } else {
> if (privileged) {
> if (VIR_STRDUP(*sockfile, LOCALSTATEDIR
"/run/libvirt/libvirt-sock") < 0 ||
> - VIR_STRDUP(*rosockfile, LOCALSTATEDIR
"/run/libvirt/libvirt-sock-ro") < 0)
> + VIR_STRDUP(*rosockfile, LOCALSTATEDIR
"/run/libvirt/libvirt-sock-ro") < 0 ||
> + VIR_STRDUP(*admsockfile, LOCALSTATEDIR
"/run/libvirt/libvirt-admin-sock") < 0)
> goto error;
> } else {
> char *rundir = NULL;
> @@ -280,7 +288,8 @@ daemonUnixSocketPaths(struct daemonConfig *config,
> }
> umask(old_umask);
>
> - if (virAsprintf(sockfile, "%s/libvirt-sock", rundir) < 0)
{
> + if (virAsprintf(sockfile, "%s/libvirt-sock", rundir) < 0
||
> + virAsprintf(admsockfile, "%s/libvirt-admin-sock", rundir)
< 0) {
> VIR_FREE(rundir);
> goto error;
> }
> @@ -427,13 +436,16 @@ static void daemonInitialize(void)
>
> static int ATTRIBUTE_NONNULL(3)
> daemonSetupNetworking(virNetServerPtr srv,
> + virNetServerPtr srvAdm,
> struct daemonConfig *config,
> const char *sock_path,
> const char *sock_path_ro,
> + const char *sock_path_adm,
> bool ipsock,
> bool privileged)
> {
> virNetServerServicePtr svc = NULL;
> + virNetServerServicePtr svcAdm = NULL;
> virNetServerServicePtr svcRO = NULL;
> virNetServerServicePtr svcTCP = NULL;
> #if WITH_GNUTLS
> @@ -442,6 +454,7 @@ daemonSetupNetworking(virNetServerPtr srv,
> gid_t unix_sock_gid = 0;
> int unix_sock_ro_mask = 0;
> int unix_sock_rw_mask = 0;
> + int unix_sock_adm_mask = 0;
>
> unsigned int cur_fd = STDERR_FILENO + 1;
> unsigned int nfds = virGetListenFDs();
> @@ -461,6 +474,11 @@ daemonSetupNetworking(virNetServerPtr srv,
> goto error;
> }
>
> + if (virStrToLong_i(config->unix_sock_admin_perms, NULL, 8,
&unix_sock_adm_mask) != 0) {
> + VIR_ERROR(_("Failed to parse mode '%s'"),
config->unix_sock_admin_perms);
> + goto error;
> + }
> +
> if (virStrToLong_i(config->unix_sock_rw_perms, NULL, 8,
&unix_sock_rw_mask) != 0) {
> VIR_ERROR(_("Failed to parse mode '%s'"),
config->unix_sock_rw_perms);
> goto error;
> @@ -503,6 +521,24 @@ daemonSetupNetworking(virNetServerPtr srv,
> virNetServerAddService(srv, svcRO, NULL) < 0)
> goto error;
>
> + if (sock_path_adm) {
> + VIR_DEBUG("Registering unix socket %s", sock_path_adm);
> + if (!(svcAdm = virNetServerServiceNewUNIX(sock_path_adm,
> + unix_sock_adm_mask,
> + unix_sock_gid,
> + REMOTE_AUTH_NONE,
> +#if WITH_GNUTLS
> + NULL,
> +#endif
> + true,
> + config->max_queued_clients,
> + config->max_client_requests)))
> + goto error;
> + }
> +
> + if (virNetServerAddService(srvAdm, svcAdm, NULL) < 0)
> + goto error;
> +
> if (ipsock) {
> if (config->listen_tcp) {
> VIR_DEBUG("Registering TCP socket %s:%s",
> @@ -600,6 +636,7 @@ daemonSetupNetworking(virNetServerPtr srv,
> virObjectUnref(svcTCP);
> virObjectUnref(svc);
> virObjectUnref(svcRO);
> + virObjectUnref(svcAdm);
> return -1;
> }
>
> @@ -1100,6 +1137,7 @@ daemonUsage(const char *argv0, bool privileged)
> int main(int argc, char **argv) {
> virNetDaemonPtr dmn = NULL;
> virNetServerPtr srv = NULL;
> + virNetServerPtr srvAdm = NULL;
> char *remote_config_file = NULL;
> int statuswrite = -1;
> int ret = 1;
> @@ -1107,6 +1145,7 @@ int main(int argc, char **argv) {
> char *pid_file = NULL;
> char *sock_file = NULL;
> char *sock_file_ro = NULL;
> + char *sock_file_adm = NULL;
> int timeout = -1; /* -t: Shutdown timeout */
> int verbose = 0;
> int godaemon = 0;
> @@ -1274,12 +1313,15 @@ int main(int argc, char **argv) {
> if (daemonUnixSocketPaths(config,
> privileged,
> &sock_file,
> - &sock_file_ro) < 0) {
> + &sock_file_ro,
> + &sock_file_adm) < 0) {
> VIR_ERROR(_("Can't determine socket paths"));
> exit(EXIT_FAILURE);
> }
> - VIR_DEBUG("Decided on socket paths '%s' and '%s'",
> - sock_file, NULLSTR(sock_file_ro));
> + VIR_DEBUG("Decided on socket paths '%s', '%s' and
'%s'",
> + sock_file,
> + NULLSTR(sock_file_ro),
> + NULLSTR(sock_file_adm));
>
> if (godaemon) {
> char ebuf[1024];
> @@ -1411,6 +1453,40 @@ int main(int argc, char **argv) {
> goto cleanup;
> }
>
> + if (!(srvAdm = virNetServerNew(config->min_workers,
> + config->max_workers,
> + config->prio_workers,
> + config->max_clients,
> + config->max_anonymous_clients,
Hm.. This may work for now, but I guess we want different limits here.
Users running libvirt in production may have really big worker pool,
while admin API will suffice one or two, like 640 kilo of RAM (TM).
Yes, makes sense, I forgot this was a trial part as well. I'll add
the configuration and different defaults.
> +
config->keepalive_interval,
> + config->keepalive_count,
I guess keepalive on a socket unix is overkill. The eventloop will see
EOF immediately as client closes the socket.
It's not about seeing EOF, but about *not* seeing EOF. We want to
disconnect from clients that are caught in an endless loop.
> +
!!config->keepalive_required,
> + NULL,
> + remoteAdmClientInitHook,
> + NULL,
> + remoteAdmClientFreeFunc,
> + dmn))) {
> + ret = VIR_DAEMON_ERR_INIT;
> + goto cleanup;
> + }
> +
> + if (virNetDaemonAddServer(dmn, srvAdm) < 0) {
> + ret = VIR_DAEMON_ERR_INIT;
> + goto cleanup;
> + }
This is what I was talking about in reply to 02/13. Imagine the limits
for domain worker pool and sockets were set really low on daemon
startup. No new client can be accepted there. I would expect mgmt
application to connect to admin API and size up the limits. However, due
to bug in 2/13 all servers are suspended. I'll leave it up to you where
you'd like to solve it.
There's nothing to solve. The client is being added in a particular
server and that server will call virNetServerUpdateServicesLocked()
only on itself, not virNetDaemonUpdateServices() on the daemon.
> +
> + if (!(adminProgram = virNetServerProgramNew(ADMIN_PROGRAM,
> + ADMIN_PROTOCOL_VERSION,
> + adminProcs,
> + adminNProcs))) {
> + ret = VIR_DAEMON_ERR_INIT;
> + goto cleanup;
> + }
> + if (virNetServerAddProgram(srvAdm, adminProgram) < 0) {
> + ret = VIR_DAEMON_ERR_INIT;
> + goto cleanup;
> + }
> +
Otherwise looking good.
Michal