On 08/09/2012 09:20 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
> .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@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list