Re: [PATCH] lib/cleaner_exec.c: get process ID of cleanerd through pipe

[Hitoshi Mitake <mitake.hitoshi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

At Wed, 22 Jan 2014 22:35:20 +0900,
Ryusuke Konishi wrote:
> 
> After applying commit 8ce4b524a1654ece478f54fce772cc0c16e3c559
> "cleanerd: call _exit(2) twice for ensuring not being a session
> leader", the mount helper program of nilfs2 no longer gets proper
> process ID (pid) of cleaner daemon, and umount.nilfs2 fails to kill
> cleanerd.
> 
> This fixes the issue by letting cleanerd print out the pid of spawned
> daemon and letting nilfs_launch_cleanerd() function read it through
> pipe.
> 
> Signed-off-by: Ryusuke Konishi <konishi.ryusuke-Zyj7fXuS5i5L9jVzuh4AOg@public.gmane.org>
> Cc: Hitoshi Mitake <mitake.hitoshi-Zyj7fXuS5i5L9jVzuh4AOg@public.gmane.org>

Looks good to me and sorry for my bug!
Reviewed-by: Hitoshi Mitake <mitake.hitoshi-Zyj7fXuS5i5L9jVzuh4AOg@public.gmane.org>

Thanks,
Hitoshi

> ---
>  lib/cleaner_exec.c       |   84 ++++++++++++++++++++++++++++++++++++++++++----
>  man/nilfs_cleanerd.8     |    3 ++
>  sbin/cleanerd/cleanerd.c |    3 ++
>  3 files changed, 83 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/cleaner_exec.c b/lib/cleaner_exec.c
> index edc55e3..5336c32 100644
> --- a/lib/cleaner_exec.c
> +++ b/lib/cleaner_exec.c
> @@ -114,6 +114,43 @@ static inline int process_is_alive(pid_t pid)
>  	return (kill(pid, 0) == 0);
>  }
>  
> +static int receive_pid(int fd, pid_t *ppid)
> +{
> +	char buf[100];
> +	unsigned long pid;
> +	FILE *fp;
> +	int ret;
> +
> +	fp = fdopen(fd, "r");
> +	if (!fp) {
> +		nilfs_cleaner_logger(
> +			LOG_ERR, _("Error: fdopen failed: %m"));
> +		close(fd);
> +		goto fail;
> +	}
> +
> +	/* read process ID of cleanerd through the given fd */
> +	while (fgets(buf, sizeof(buf), fp) != NULL) {
> +		/*
> +		 * fgets() is blocked during no child processes
> +		 * respond, but it will escape returning a NULL value
> +		 * and terminate the loop when all child processes
> +		 * close the given pipe (fd) including call of exit().
> +		 */
> +		ret = sscanf(buf, "NILFS_CLEANERD_PID=%lu", &pid);
> +		if (ret == 1) {
> +			*ppid = pid;
> +			fclose(fp); /* this also closes fd */
> +			return 0;
> +		}
> +	}
> +	fclose(fp);
> +fail:
> +	nilfs_cleaner_logger(
> +		LOG_WARNING, _("Warning: cannot get pid of cleanerd"));
> +	return -1;
> +}
> +
>  int nilfs_launch_cleanerd(const char *device, const char *mntdir,
>  			  unsigned long protperiod, pid_t *ppid)
>  {
> @@ -123,6 +160,7 @@ int nilfs_launch_cleanerd(const char *device, const char *mntdir,
>  	int i = 0;
>  	int ret;
>  	char buf[256];
> +	int pipes[2];
>  
>  	if (stat(cleanerd, &statbuf) != 0) {
>  		nilfs_cleaner_logger(LOG_ERR,  _("Error: %s not found"),
> @@ -130,8 +168,16 @@ int nilfs_launch_cleanerd(const char *device, const char *mntdir,
>  		return -1;
>  	}
>  
> +	ret = pipe(pipes);
> +	if (ret < 0) {
> +		nilfs_cleaner_logger(
> +			LOG_ERR, _("Error: failed to create pipe: %m"));
> +		return -1;
> +	}
> +
>  	ret = fork();
>  	if (ret == 0) {
> +		/* child */
>  		if (setgid(getgid()) < 0) {
>  			nilfs_cleaner_logger(
>  				LOG_ERR,
> @@ -159,16 +205,40 @@ int nilfs_launch_cleanerd(const char *device, const char *mntdir,
>  		sigdelset(&sigs, SIGSEGV);
>  		sigprocmask(SIG_UNBLOCK, &sigs, NULL);
>  
> +		close(pipes[0]);
> +		ret = dup2(pipes[1], STDOUT_FILENO);
> +		if (ret < 0) {
> +			nilfs_cleaner_logger(
> +				LOG_ERR,
> +				_("Error: failed to duplicate pipe: %m"));
> +			exit(1);
> +		}
> +		close(pipes[1]);
> +		
>  		execv(cleanerd, (char **)dargs);
>  		exit(1);   /* reach only if failed */
> -	} else if (ret != -1) {
> -		*ppid = ret;
> -		return 0; /* cleanerd started */
> +	} else if (ret > 0) {
> +		/* parent */
> +		close(pipes[1]);
> +
> +		/*
> +		 * fork() returns a pid of the child process, but we
> +		 * cannot use it because cleanerd internally fork()s
> +		 * and changes pid.
> +		 */
> +		ret = receive_pid(pipes[0], ppid);
> +		if (ret < 0)
> +			*ppid = 0;
> +		/*
> +		 * always return a success code because cleanerd has
> +		 * already started.
> +		 */
> +		return 0;
>  	} else {
> -		int errsv = errno;
> -		nilfs_cleaner_logger(LOG_ERR,
> -				     _("Error: could not fork: %s"),
> -				     strerror(errsv));
> +		nilfs_cleaner_logger(
> +			LOG_ERR, _("Error: could not fork: %m"));
> +		close(pipes[0]);
> +		close(pipes[1]);
>  	}
>  	return -1;
>  }
> diff --git a/man/nilfs_cleanerd.8 b/man/nilfs_cleanerd.8
> index 46caef2..fd1c48a 100644
> --- a/man/nilfs_cleanerd.8
> +++ b/man/nilfs_cleanerd.8
> @@ -24,6 +24,9 @@ users are recommended to invoke \fBnilfs_cleanerd\fP through
>  \fBmount.nilfs2\fP(8) or \fBmount\fP(8) and shutdown it through
>  \fBumount.nilfs2\fP(8) or \fBumount\fP(8) in order to avoid state
>  inconsistencies among administration tools.
> +.PP
> +\fBnilfs_cleanerd\fP displays its process ID (pid) to standard
> +output when it started.
>  .SH OPTIONS
>  .TP
>  \fB\-V\fR, \fB\-\-version\fR
> diff --git a/sbin/cleanerd/cleanerd.c b/sbin/cleanerd/cleanerd.c
> index 86dfcf7..742ab98 100644
> --- a/sbin/cleanerd/cleanerd.c
> +++ b/sbin/cleanerd/cleanerd.c
> @@ -719,6 +719,9 @@ static int daemonize(int nochdir, int noclose)
>  	if (!nochdir && (chdir(ROOTDIR) < 0))
>  		return -1;
>  
> +	printf("NILFS_CLEANERD_PID=%lu\n", (unsigned long)getpid());
> +	fflush(stdout);
> +
>  	if (!noclose) {
>  		close(0);
>  		close(1);
> -- 
> 1.7.9.3
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html