[PATCH 1/4] util: introduce shared daemon startup code

[Michal Prívozník]

On 26. 3. 2020 16:18, Rafael Fonseca wrote:
> Several daemons have similar code around general daemon startup code.
> Let's move it into a file and share it among them.
> 
> Signed-off-by: Rafael Fonseca <r4f4rfs at gmail.com>
> ---
>  src/libvirt_private.syms |   6 +
>  src/util/Makefile.inc.am |   2 +
>  src/util/virdaemon.c     | 255 +++++++++++++++++++++++++++++++++++++++
>  src/util/virdaemon.h     |  74 ++++++++++++
>  4 files changed, 337 insertions(+)
>  create mode 100644 src/util/virdaemon.c
>  create mode 100644 src/util/virdaemon.h
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 3f032c7963..e276f55bb1 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1906,6 +1906,12 @@ virCryptoHashString;
>  virCryptoHaveCipher;
>  
>  
> +# util/virdaemon.h
> +virDaemonForkIntoBackground;
> +virDaemonSetupLogging;
> +virDaemonUnixSocketPaths;
> +
> +
>  # util/virdbus.h
>  virDBusCallMethod;
>  virDBusCloseSystemBus;
> diff --git a/src/util/Makefile.inc.am b/src/util/Makefile.inc.am
> index 718b11a5f4..5bc60cb5ea 100644
> --- a/src/util/Makefile.inc.am
> +++ b/src/util/Makefile.inc.am
> @@ -42,6 +42,8 @@ UTIL_SOURCES = \
>  	util/virconf.h \
>  	util/vircrypto.c \
>  	util/vircrypto.h \
> +	util/virdaemon.c \
> +	util/virdaemon.h \
>  	util/virdbus.c \
>  	util/virdbus.h \
>  	util/virdbuspriv.h \
> diff --git a/src/util/virdaemon.c b/src/util/virdaemon.c
> new file mode 100644
> index 0000000000..4b63b44d66
> --- /dev/null
> +++ b/src/util/virdaemon.c
> @@ -0,0 +1,255 @@
> +/*
> + * virdaemon.c: shared daemon setup code
> + *
> + * Copyright (C) 2020 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/>.
> + */
> +
> +#include <config.h>
> +
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <sys/wait.h>
> +#include <fcntl.h>
> +#include <unistd.h>
> +#include <stdbool.h>
> +
> +#include "virdaemon.h"
> +#include "virutil.h"
> +#include "virfile.h"
> +#include "virlog.h"
> +#include "viralloc.h"
> +
> +#include "configmake.h"
> +
> +int
> +virDaemonForkIntoBackground(const char *argv0)
> +{
> +    int statuspipe[2];
> +    if (virPipeQuiet(statuspipe) < 0)
> +        return -1;
> +
> +    pid_t pid = fork();

This will fail 'make syntax-check' because the prohibit_fork rule
doesn't allow plain fork(). We need to white list this file, becasue
virFork() does more than just plain fork() and we really want the plain
fork(). Well, all other daemons are whitelisted there exactly because of
this code. So we will need to remove them from the whitelist as we
switch them over to this new code.

> +    switch (pid) {
> +    case 0:
> +        {
> +            /* intermediate child */
> +            int stdinfd = -1;
> +            int stdoutfd = -1;
> +            int nextpid;
> +
> +            VIR_FORCE_CLOSE(statuspipe[0]);
> +
> +            if ((stdinfd = open("/dev/null", O_RDONLY)) <= STDERR_FILENO)
> +                goto cleanup;
> +            if ((stdoutfd = open("/dev/null", O_WRONLY)) <= STDERR_FILENO)

I think this should be '< 0' because we are interested in the success of
open() really.

> +                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;
> +
> +            if (setsid() < 0)
> +                goto cleanup;
> +
> +            nextpid = fork();
> +            switch (nextpid) {
> +            case 0: /* grandchild */
> +                return statuspipe[1];
> +            case -1: /* error */
> +                goto cleanup;
> +            default: /* intermediate child succeeded */
> +                _exit(EXIT_SUCCESS);
> +            }
> +
> +        cleanup:
> +            VIR_FORCE_CLOSE(stdoutfd);
> +            VIR_FORCE_CLOSE(stdinfd);
> +            VIR_FORCE_CLOSE(statuspipe[1]);
> +            _exit(EXIT_FAILURE);
> +
> +        }
> +
> +    case -1: /* error in parent */
> +        goto error;
> +
> +    default:
> +        {
> +            /* parent */
> +            int got, exitstatus = 0;
> +            int ret;
> +            char status;
> +
> +            VIR_FORCE_CLOSE(statuspipe[1]);
> +
> +            /* We wait to make sure the first child forked successfully */
> +            if ((got = waitpid(pid, &exitstatus, 0)) < 0 ||
> +                got != pid ||
> +                exitstatus != 0) {
> +                goto error;
> +            }
> +
> +            /* If we got here, then the grandchild was spawned, so we
> +             * must exit. Block until the second child initializes
> +             * successfully */
> +        again:
> +            ret = read(statuspipe[0], &status, 1);
> +            if (ret == -1 && errno == EINTR)
> +                goto again;

I know it's pre-existing, but we have safread() which is there exactly
because of EINTR.

> +
> +            VIR_FORCE_CLOSE(statuspipe[0]);
> +
> +            if (ret != 1) {
> +                fprintf(stderr,
> +                        _("%s: error: unable to determine if daemon is "
> +                          "running: %s\n"), argv0,
> +                        g_strerror(errno));
> +                exit(EXIT_FAILURE);
> +            } else if (status != 0) {
> +                fprintf(stderr,
> +                        _("%s: error: %s. Check /var/log/messages or run without "
> +                          "--daemon for more info.\n"), argv0,
> +                        virDaemonErrTypeToString(status));

Since these strings should be translated (rightfully), the file must be
added onto the POTFILES list.

> +                exit(EXIT_FAILURE);
> +            }
> +            _exit(EXIT_SUCCESS);
> +        }
> +    }
> +
> +error:

Misaligned label ;-)

> +    VIR_FORCE_CLOSE(statuspipe[0]);
> +    VIR_FORCE_CLOSE(statuspipe[1]);
> +    return -1;
> +}
> +

Michal