Thanks Ben for the answers.
Few more questions.
On Thu, Jun 6, 2013 at 4:27 PM, Ben Pfaff <b...@nicira.com> wrote:
> On Thu, Jun 06, 2013 at 09:49:11AM -0700, Alex Wang wrote:
> > The 2/17. 3/17 look good to me.
>
> Thanks.
>
> > 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?
>
> A single-threaded process can ensure that a signal handler doesn't
> run during a section of code by blocking and unblocking the signal
> around that section of code. A multithreaded process can't do that,
> because the signal handler might get invoked from any thread.
Seem to me that we make a rule here. We only want single threaded process
to call "process_init() and process_start()", right?
Also, I want to ask why do you remove the sigchld_ related functions? Is
that because the "xpthread_sigmask()" is not thread safe?
> > 2. For the "process_register()", the comment still talks about blocking
> > SIGCHLD signal, which can be confusing
>
> Good catch. I deleted that sentence from the comment, since it's no
> longer correct. (It was also ungrammatical.)
>
> > 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
