[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 17/23] Introduce basic infrastructure for virtlockd daemon



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

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]