7:45 a.m.
On 08/09/2012 09:20 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange(a)redhat.com>
The virtlockd daemon will maintain locks on behalf of libvirtd.
There are two reasons for it to be separate
- Avoid risk of other libvirtd threads accidentally
releasing fcntl() locks by opening + closing a file
that is locked
- Ensure locks can be preserved across libvirtd restarts.
virtlockd will need to be able to re-exec itself while
maintaining locks. This is simpler to achieve if its
sole job is maintaining locks
Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
---
.gitignore | 2 +
cfg.mk | 6 +-
libvirt.spec.in | 7 +
po/POTFILES.in | 1 +
src/Makefile.am | 86 ++++-
src/locking/lock_daemon.c | 750 ++++++++++++++++++++++++++++++++++++++++++
src/locking/lock_daemon.h | 43 +++
src/locking/virtlockd.init.in | 93 ++++++
src/locking/virtlockd.sysconf | 3 +
9 files changed, 987 insertions(+), 4 deletions(-)
create mode 100644 src/locking/lock_daemon.c
create mode 100644 src/locking/lock_daemon.h
create mode 100644 src/locking/virtlockd.init.in
create mode 100644 src/locking/virtlockd.sysconf
I had a merge failure trying to apply this; you'll need a minor context
rebase in cfg.mk to use this.
+++ b/cfg.mk
@@ -636,6 +636,8 @@ sc_prohibit_cross_inclusion:
@for dir in $(cross_dirs); do \
case $$dir in \
util/) safe="util";; \
+ locking/) \
+ safe="($$dir|util|conf|rpc)";; \
You added a new branch for locking...
cpu/ | locking/ | network/ | rpc/ | security/) \
...so this branch won't be reached, so you should clean it up while here.
+++ b/src/Makefile.am
+
+install-sysconfig:
+ mkdir -p $(DESTDIR)$(sysconfdir)/sysconfig
Shouldn't we be using $(MKDIR_P) instead?
+ $(INSTALL_DATA) $(srcdir)/locking/virtlockd.sysconf \
+ $(DESTDIR)$(sysconfdir)/sysconfig/virtlockd
+
+uninstall-sysconfig:
+ rm -f $(DESTDIR)$(sysconfdir)/sysconfig/virtlockd
+
+EXTRA_DIST += locking/virtlockd.init.in
+
+if WITH_LIBVIRTD
+if LIBVIRT_INIT_SCRIPT_RED_HAT
+install-init:: virtlockd.init install-sysconfig
+ mkdir -p $(DESTDIR)$(sysconfdir)/rc.d/init.d
and again.
+++ b/src/locking/lock_daemon.c
+enum {
+ VIR_LOCK_DAEMON_ERR_NONE = 0,
+ VIR_LOCK_DAEMON_ERR_PIDFILE,
+ VIR_LOCK_DAEMON_ERR_RUNDIR,
+ VIR_LOCK_DAEMON_ERR_INIT,
+ VIR_LOCK_DAEMON_ERR_SIGNAL,
+ VIR_LOCK_DAEMON_ERR_PRIVS,
+ VIR_LOCK_DAEMON_ERR_NETWORK,
+ VIR_LOCK_DAEMON_ERR_CONFIG,
+ VIR_LOCK_DAEMON_ERR_HOOKS,
Is it worth using '= 1', '= 2', and so forth, to make future extensions
aware that they must not be in the middle, since this is tied to RPC
protocol?
+static int
+virLockDaemonForkIntoBackground(const char *argv0)
+{
+ int statuspipe[2];
+ if (pipe(statuspipe) < 0)
+ return -1;
+
+ pid_t pid = fork();
+ switch (pid) {
+ case 0:
+ {
+ int stdinfd = -1;
+ int stdoutfd = -1;
+ int nextpid;
+
+ VIR_FORCE_CLOSE(statuspipe[0]);
+
+ if ((stdinfd = open("/dev/null", O_RDONLY)) < 0)
+ goto cleanup;
+ if ((stdoutfd = open("/dev/null", O_WRONLY)) < 0)
+ goto cleanup;
+ if (dup2(stdinfd, STDIN_FILENO) != STDIN_FILENO)
+ goto cleanup;
+ if (dup2(stdoutfd, STDOUT_FILENO) != STDOUT_FILENO)
+ goto cleanup;
+ if (dup2(stdoutfd, STDERR_FILENO) != STDERR_FILENO)
+ goto cleanup;
+ if (VIR_CLOSE(stdinfd) < 0)
+ goto cleanup;
+ if (VIR_CLOSE(stdoutfd) < 0)
+ goto cleanup;
Oops - this can blindly re-close stdin or stdout or stderr if the parent
process had those fds closed. It must be:
if (stdinfd > STDERR_FILENO && VIR_CLOSE(stdinfd) < 0)
goto cleanup;
stdinfd = -1;
if (stdoutfd > STDERR_FILENO && VIR_CLOSE(stdoutfd) < 0)
goto cleanup;
stdoutfd = -1;
+
+ if (setsid() < 0)
+ goto cleanup;
+
+ nextpid = fork();
+ switch (nextpid) {
+ case 0:
+ return statuspipe[1];
+ case -1:
+ return -1;
+ default:
+ _exit(0);
_exit(EXIT_SUCCESS) (I thought we had a syntax check for this - oh, I
see - that rule checked for 'exit' but not '_exit' or '_Exit'.
Gnulib
patch coming up).
+ if (ret == 1 && status != 0) {
+ fprintf(stderr,
+ _("%s: error: %s. Check /var/log/messages or run without
"
+ "--daemon for more info.\n"), argv0,
+ virDaemonErrTypeToString(status));
+ }
+ _exit(ret == 1 && status == 0 ? 0 : 1);
Another case where EXIT_{SUCCESS,FAILURE} looks better.
+
+ /*
+ * Command line override for --verbose
+ */
+ if ((verbose) && (virLogGetDefaultPriority() > VIR_LOG_INFO))
Copy and paste, but these are redundant (). Why are we copying so much
code? Should some of this live in a common file under util, to be used
by both this file and daemon/libvirtd.c?
+static int
+virLockDaemonSetupSignals(virNetServerPtr srv)
+{
+ if (virNetServerAddSignalHandler(srv, SIGINT, virLockDaemonShutdownHandler, NULL)
< 0)
+ return -1;
+ if (virNetServerAddSignalHandler(srv, SIGQUIT, virLockDaemonShutdownHandler, NULL)
< 0)
+ return -1;
+ if (virNetServerAddSignalHandler(srv, SIGTERM, virLockDaemonShutdownHandler, NULL)
< 0)
+ return -1;
Do we need to worry about setting SIGPIPE to a known state?
+static void
+virLockDaemonUsage(const char *argv0)
+{
+ fprintf (stderr,
+ _("\n\
+Usage:\n\
+ %s [options]\n\
+\n\
+Options:\n\
+ -v | --verbose Verbose messages.\n\
+ -d | --daemon Run as a daemon & write PID file.\n\
+ -t | --timeout <secs> Exit after timeout period.\n\
+ -f | --config <file> Configuration file.\n\
+ | --version Display version information.\n\
+ -p | --pid-file <file> Change name of PID file.\n\
Shouldn't we also list '--help' as an option?
+
+enum {
+ OPT_VERSION = 129
129 is still a valid character in single-byte locales, which someone
could pass in from the command line via a short option and possibly mess
up getopt_long. I think you want it to be 256 or greater.
+ struct option opts[] = {
+ { "verbose", no_argument, &verbose, 1},
+ { "daemon", no_argument, &godaemon, 1},
Why do these two use the non-NULL flag argument, instead of the more
typical "NULL, 'd'" layout...
+ { "config", required_argument, NULL,
'f'},
+ { "timeout", required_argument, NULL, 't'},
+ { "pid-file", required_argument, NULL, 'p'},
+ { "version", no_argument, NULL, OPT_VERSION },
+ { "help", no_argument, NULL, '?' },
+ {0, 0, 0, 0}
+ };
+
+ switch (c) {
+ case 0:
+ /* Got one of the flags */
+ break;
+ case 'v':
+ verbose = 1;
+ break;
+ case 'd':
+ godaemon = 1;
+ break;
...especially since here you also handle short options for the same
task, which do the same work? (I know, copy and paste from a bad
example in libvirtd.c)
+
+ case '?':
+ virLockDaemonUsage(argv[0]);
+ return 2;
That says that 'virtlockd --help' will fail with status 2, even when it
successfully printed the usage message. I think you need to route
--help slightly differently than the default handling of getopt_long
error handling, if only to control the exit status differently (again,
copying from a bad example).
Looks mostly reasonable.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org