On 08/09/2012 09:20 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@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@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@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org