Hey Ben,
The 2/17. 3/17 look good to me.
For this patch, want to ask few questions:
1. why does the previous implementation cannot guarantee thread safety (An
example?)? Is this related to the sigchld_ related functions?
2. For the "process_register()", the comment still talks about blocking
SIGCHLD signal, which can be confusing
Thanks
On Wed, Jun 5, 2013 at 1:05 PM, Ben Pfaff <b...@nicira.com> wrote:
> Signed-off-by: Ben Pfaff <b...@nicira.com>
> ---
> lib/process.c | 103
> ++++++++++++++++----------------------------------
> lib/process.h | 1 +
> ovsdb/ovsdb-server.c | 7 ++-
> 3 files changed, 39 insertions(+), 72 deletions(-)
>
> diff --git a/lib/process.c b/lib/process.c
> index 4102308..22dd266 100644
> --- a/lib/process.c
> +++ b/lib/process.c
> @@ -44,9 +44,9 @@ struct process {
> char *name;
> pid_t pid;
>
> - /* Modified by signal handler. */
> - volatile bool exited;
> - volatile int status;
> + /* State. */
> + bool exited;
> + int status;
> };
>
> /* Pipe used to signal child termination. */
> @@ -55,9 +55,6 @@ static int fds[2];
> /* All processes. */
> static struct list all_processes = LIST_INITIALIZER(&all_processes);
>
> -static bool sigchld_is_blocked(void);
> -static void block_sigchld(sigset_t *);
> -static void unblock_sigchld(const sigset_t *);
> static void sigchld_handler(int signr OVS_UNUSED);
>
> /* Initializes the process subsystem (if it is not already initialized).
> Calls
> @@ -155,8 +152,6 @@ process_register(const char *name, pid_t pid)
> struct process *p;
> const char *slash;
>
> - ovs_assert(sigchld_is_blocked());
> -
> p = xzalloc(sizeof *p);
> p->pid = pid;
> slash = strrchr(name, '/');
> @@ -181,7 +176,6 @@ process_register(const char *name, pid_t pid)
> int
> process_start(char **argv, struct process **pp)
> {
> - sigset_t oldsigs;
> pid_t pid;
> int error;
>
> @@ -192,16 +186,13 @@ process_start(char **argv, struct process **pp)
> return error;
> }
>
> - block_sigchld(&oldsigs);
> pid = fork();
> if (pid < 0) {
> - unblock_sigchld(&oldsigs);
> VLOG_WARN("fork failed: %s", strerror(errno));
> return errno;
> } else if (pid) {
> /* Running in parent process. */
> *pp = process_register(argv[0], pid);
> - unblock_sigchld(&oldsigs);
> return 0;
> } else {
> /* Running in child process. */
> @@ -209,7 +200,6 @@ process_start(char **argv, struct process **pp)
> int fd;
>
> fatal_signal_fork();
> - unblock_sigchld(&oldsigs);
> for (fd = 3; fd < fd_max; fd++) {
> close(fd);
> }
> @@ -225,12 +215,7 @@ void
> process_destroy(struct process *p)
> {
> if (p) {
> - sigset_t oldsigs;
> -
> - block_sigchld(&oldsigs);
> list_remove(&p->node);
> - unblock_sigchld(&oldsigs);
> -
> free(p->name);
> free(p);
> }
> @@ -265,13 +250,7 @@ process_name(const struct process *p)
> bool
> process_exited(struct process *p)
> {
> - if (p->exited) {
> - return true;
> - } else {
> - char buf[_POSIX_PIPE_BUF];
> - ignore(read(fds[0], buf, sizeof buf));
> - return false;
> - }
> + return p->exited;
> }
>
> /* Returns process 'p''s exit status, as reported by waitpid(2).
> @@ -313,6 +292,35 @@ process_status_msg(int status)
> return ds_cstr(&ds);
> }
>
> +/* Executes periodic maintenance activities required by the process
> module. */
> +void
> +process_run(void)
> +{
> + char buf[_POSIX_PIPE_BUF];
> +
> + if (!list_is_empty(&all_processes) && read(fds[0], buf, sizeof buf) >
> 0) {
> + struct process *p;
> +
> + LIST_FOR_EACH (p, node, &all_processes) {
> + if (!p->exited) {
> + int retval, status;
> + do {
> + retval = waitpid(p->pid, &status, WNOHANG);
> + } while (retval == -1 && errno == EINTR);
> + if (retval == p->pid) {
> + p->exited = true;
> + p->status = status;
> + } else if (retval < 0) {
> + VLOG_WARN("waitpid: %s", strerror(errno));
> + p->exited = true;
> + p->status = -1;
> + }
> + }
> + }
> + }
> +}
> +
> +
> /* Causes the next call to poll_block() to wake up when process 'p' has
> * exited. */
> void
> @@ -353,50 +361,5 @@ process_search_path(const char *name)
> static void
> sigchld_handler(int signr OVS_UNUSED)
> {
> - struct process *p;
> -
> - COVERAGE_INC(process_sigchld);
> - LIST_FOR_EACH (p, node, &all_processes) {
> - if (!p->exited) {
> - int retval, status;
> - do {
> - retval = waitpid(p->pid, &status, WNOHANG);
> - } while (retval == -1 && errno == EINTR);
> - if (retval == p->pid) {
> - p->exited = true;
> - p->status = status;
> - } else if (retval < 0) {
> - /* XXX We want to log something but we're in a signal
> - * handler. */
> - p->exited = true;
> - p->status = -1;
> - }
> - }
> - }
> ignore(write(fds[1], "", 1));
> }
> -
> -static bool
> -sigchld_is_blocked(void)
> -{
> - sigset_t sigs;
> -
> - xpthread_sigmask(SIG_SETMASK, NULL, &sigs);
> - return sigismember(&sigs, SIGCHLD);
> -}
> -
> -static void
> -block_sigchld(sigset_t *oldsigs)
> -{
> - sigset_t sigchld;
> -
> - sigemptyset(&sigchld);
> - sigaddset(&sigchld, SIGCHLD);
> - xpthread_sigmask(SIG_BLOCK, &sigchld, oldsigs);
> -}
> -
> -static void
> -unblock_sigchld(const sigset_t *oldsigs)
> -{
> - xpthread_sigmask(SIG_SETMASK, oldsigs, NULL);
> -}
> diff --git a/lib/process.h b/lib/process.h
> index 6d1410b..d17737d 100644
> --- a/lib/process.h
> +++ b/lib/process.h
> @@ -33,6 +33,7 @@ bool process_exited(struct process *);
> int process_status(const struct process *);
> char *process_status_msg(int);
>
> +void process_run(void);
> void process_wait(struct process *);
>
> char *process_search_path(const char *);
> diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c
> index b9afd8c..20e1964 100644
> --- a/ovsdb/ovsdb-server.c
> +++ b/ovsdb/ovsdb-server.c
> @@ -230,8 +230,11 @@ main(int argc, char *argv[])
> for (i = 0; i < n_dbs; i++) {
> ovsdb_trigger_run(dbs[i].db, time_msec());
> }
> - if (run_process && process_exited(run_process)) {
> - exiting = true;
> + if (run_process) {
> + process_run();
> + if (process_exited(run_process)) {
> + exiting = true;
> + }
> }
>
> /* update Manager status(es) every 5 seconds */
> --
> 1.7.2.5
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
