<div dir="ltr">Thanks Ben for the answers.<div><br></div><div>Few more questions.<br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jun 6, 2013 at 4:27 PM, Ben Pfaff <span dir="ltr"><<a href="mailto:blp@nicira.com" target="_blank">blp@nicira.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="im">On Thu, Jun 06, 2013 at 09:49:11AM -0700, Alex Wang wrote:<br>
> The 2/17. 3/17 look good to me.<br>
<br>
</div>Thanks.<br>
<div class="im"><br>
> For this patch, want to ask few questions:<br>
><br>
> 1. why does the previous implementation cannot guarantee thread safety (An<br>
> example?)? Is this related to the sigchld_ related functions?<br>
<br>
</div>A single-threaded process can ensure that a signal handler doesn't<br>
run during a section of code by blocking and unblocking the signal<br>
around that section of code. A multithreaded process can't do that,<br>
because the signal handler might get invoked from any thread. </blockquote><div><br></div><div><br></div><div style>Seem to me that we make a rule here. We only want single threaded process to call "process_init() and process_start()", right?</div>
<div style><br></div><div style><br></div><div style>Also, I want to ask why do you remove the sigchld_ related functions? Is that because the "<span style="color:rgb(0,0,0)">xpthread_sigmask()</span>" is not thread safe?</div>
<div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="im">
> 2. For the "process_register()", the comment still talks about blocking<br>
> SIGCHLD signal, which can be confusing<br>
<br>
</div>Good catch. I deleted that sentence from the comment, since it's no<br>
longer correct. (It was also ungrammatical.)<br>
<div class=""><div class="h5"><br>
> On Wed, Jun 5, 2013 at 1:05 PM, Ben Pfaff <<a href="mailto:blp@nicira.com">blp@nicira.com</a>> wrote:<br>
><br>
> > Signed-off-by: Ben Pfaff <<a href="mailto:blp@nicira.com">blp@nicira.com</a>><br>
> > ---<br>
> > lib/process.c | 103<br>
> > ++++++++++++++++----------------------------------<br>
> > lib/process.h | 1 +<br>
> > ovsdb/ovsdb-server.c | 7 ++-<br>
> > 3 files changed, 39 insertions(+), 72 deletions(-)<br>
> ><br>
> > diff --git a/lib/process.c b/lib/process.c<br>
> > index 4102308..22dd266 100644<br>
> > --- a/lib/process.c<br>
> > +++ b/lib/process.c<br>
> > @@ -44,9 +44,9 @@ struct process {<br>
> > char *name;<br>
> > pid_t pid;<br>
> ><br>
> > - /* Modified by signal handler. */<br>
> > - volatile bool exited;<br>
> > - volatile int status;<br>
> > + /* State. */<br>
> > + bool exited;<br>
> > + int status;<br>
> > };<br>
> ><br>
> > /* Pipe used to signal child termination. */<br>
> > @@ -55,9 +55,6 @@ static int fds[2];<br>
> > /* All processes. */<br>
> > static struct list all_processes = LIST_INITIALIZER(&all_processes);<br>
> ><br>
> > -static bool sigchld_is_blocked(void);<br>
> > -static void block_sigchld(sigset_t *);<br>
> > -static void unblock_sigchld(const sigset_t *);<br>
> > static void sigchld_handler(int signr OVS_UNUSED);<br>
> ><br>
> > /* Initializes the process subsystem (if it is not already initialized).<br>
> > Calls<br>
> > @@ -155,8 +152,6 @@ process_register(const char *name, pid_t pid)<br>
> > struct process *p;<br>
> > const char *slash;<br>
> ><br>
> > - ovs_assert(sigchld_is_blocked());<br>
> > -<br>
> > p = xzalloc(sizeof *p);<br>
> > p->pid = pid;<br>
> > slash = strrchr(name, '/');<br>
> > @@ -181,7 +176,6 @@ process_register(const char *name, pid_t pid)<br>
> > int<br>
> > process_start(char **argv, struct process **pp)<br>
> > {<br>
> > - sigset_t oldsigs;<br>
> > pid_t pid;<br>
> > int error;<br>
> ><br>
> > @@ -192,16 +186,13 @@ process_start(char **argv, struct process **pp)<br>
> > return error;<br>
> > }<br>
> ><br>
> > - block_sigchld(&oldsigs);<br>
> > pid = fork();<br>
> > if (pid < 0) {<br>
> > - unblock_sigchld(&oldsigs);<br>
> > VLOG_WARN("fork failed: %s", strerror(errno));<br>
> > return errno;<br>
> > } else if (pid) {<br>
> > /* Running in parent process. */<br>
> > *pp = process_register(argv[0], pid);<br>
> > - unblock_sigchld(&oldsigs);<br>
> > return 0;<br>
> > } else {<br>
> > /* Running in child process. */<br>
> > @@ -209,7 +200,6 @@ process_start(char **argv, struct process **pp)<br>
> > int fd;<br>
> ><br>
> > fatal_signal_fork();<br>
> > - unblock_sigchld(&oldsigs);<br>
> > for (fd = 3; fd < fd_max; fd++) {<br>
> > close(fd);<br>
> > }<br>
> > @@ -225,12 +215,7 @@ void<br>
> > process_destroy(struct process *p)<br>
> > {<br>
> > if (p) {<br>
> > - sigset_t oldsigs;<br>
> > -<br>
> > - block_sigchld(&oldsigs);<br>
> > list_remove(&p->node);<br>
> > - unblock_sigchld(&oldsigs);<br>
> > -<br>
> > free(p->name);<br>
> > free(p);<br>
> > }<br>
> > @@ -265,13 +250,7 @@ process_name(const struct process *p)<br>
> > bool<br>
> > process_exited(struct process *p)<br>
> > {<br>
> > - if (p->exited) {<br>
> > - return true;<br>
> > - } else {<br>
> > - char buf[_POSIX_PIPE_BUF];<br>
> > - ignore(read(fds[0], buf, sizeof buf));<br>
> > - return false;<br>
> > - }<br>
> > + return p->exited;<br>
> > }<br>
> ><br>
> > /* Returns process 'p''s exit status, as reported by waitpid(2).<br>
> > @@ -313,6 +292,35 @@ process_status_msg(int status)<br>
> > return ds_cstr(&ds);<br>
> > }<br>
> ><br>
> > +/* Executes periodic maintenance activities required by the process<br>
> > module. */<br>
> > +void<br>
> > +process_run(void)<br>
> > +{<br>
> > + char buf[_POSIX_PIPE_BUF];<br>
> > +<br>
> > + if (!list_is_empty(&all_processes) && read(fds[0], buf, sizeof buf) ><br>
> > 0) {<br>
> > + struct process *p;<br>
> > +<br>
> > + LIST_FOR_EACH (p, node, &all_processes) {<br>
> > + if (!p->exited) {<br>
> > + int retval, status;<br>
> > + do {<br>
> > + retval = waitpid(p->pid, &status, WNOHANG);<br>
> > + } while (retval == -1 && errno == EINTR);<br>
> > + if (retval == p->pid) {<br>
> > + p->exited = true;<br>
> > + p->status = status;<br>
> > + } else if (retval < 0) {<br>
> > + VLOG_WARN("waitpid: %s", strerror(errno));<br>
> > + p->exited = true;<br>
> > + p->status = -1;<br>
> > + }<br>
> > + }<br>
> > + }<br>
> > + }<br>
> > +}<br>
> > +<br>
> > +<br>
> > /* Causes the next call to poll_block() to wake up when process 'p' has<br>
> > * exited. */<br>
> > void<br>
> > @@ -353,50 +361,5 @@ process_search_path(const char *name)<br>
> > static void<br>
> > sigchld_handler(int signr OVS_UNUSED)<br>
> > {<br>
> > - struct process *p;<br>
> > -<br>
> > - COVERAGE_INC(process_sigchld);<br>
> > - LIST_FOR_EACH (p, node, &all_processes) {<br>
> > - if (!p->exited) {<br>
> > - int retval, status;<br>
> > - do {<br>
> > - retval = waitpid(p->pid, &status, WNOHANG);<br>
> > - } while (retval == -1 && errno == EINTR);<br>
> > - if (retval == p->pid) {<br>
> > - p->exited = true;<br>
> > - p->status = status;<br>
> > - } else if (retval < 0) {<br>
> > - /* XXX We want to log something but we're in a signal<br>
> > - * handler. */<br>
> > - p->exited = true;<br>
> > - p->status = -1;<br>
> > - }<br>
> > - }<br>
> > - }<br>
> > ignore(write(fds[1], "", 1));<br>
> > }<br>
> > -<br>
> > -static bool<br>
> > -sigchld_is_blocked(void)<br>
> > -{<br>
> > - sigset_t sigs;<br>
> > -<br>
> > - xpthread_sigmask(SIG_SETMASK, NULL, &sigs);<br>
> > - return sigismember(&sigs, SIGCHLD);<br>
> > -}<br>
> > -<br>
> > -static void<br>
> > -block_sigchld(sigset_t *oldsigs)<br>
> > -{<br>
> > - sigset_t sigchld;<br>
> > -<br>
> > - sigemptyset(&sigchld);<br>
> > - sigaddset(&sigchld, SIGCHLD);<br>
> > - xpthread_sigmask(SIG_BLOCK, &sigchld, oldsigs);<br>
> > -}<br>
> > -<br>
> > -static void<br>
> > -unblock_sigchld(const sigset_t *oldsigs)<br>
> > -{<br>
> > - xpthread_sigmask(SIG_SETMASK, oldsigs, NULL);<br>
> > -}<br>
> > diff --git a/lib/process.h b/lib/process.h<br>
> > index 6d1410b..d17737d 100644<br>
> > --- a/lib/process.h<br>
> > +++ b/lib/process.h<br>
> > @@ -33,6 +33,7 @@ bool process_exited(struct process *);<br>
> > int process_status(const struct process *);<br>
> > char *process_status_msg(int);<br>
> ><br>
> > +void process_run(void);<br>
> > void process_wait(struct process *);<br>
> ><br>
> > char *process_search_path(const char *);<br>
> > diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c<br>
> > index b9afd8c..20e1964 100644<br>
> > --- a/ovsdb/ovsdb-server.c<br>
> > +++ b/ovsdb/ovsdb-server.c<br>
> > @@ -230,8 +230,11 @@ main(int argc, char *argv[])<br>
> > for (i = 0; i < n_dbs; i++) {<br>
> > ovsdb_trigger_run(dbs[i].db, time_msec());<br>
> > }<br>
> > - if (run_process && process_exited(run_process)) {<br>
> > - exiting = true;<br>
> > + if (run_process) {<br>
> > + process_run();<br>
> > + if (process_exited(run_process)) {<br>
> > + exiting = true;<br>
> > + }<br>
> > }<br>
> ><br>
> > /* update Manager status(es) every 5 seconds */<br>
> > --<br>
> > 1.7.2.5<br>
> ><br>
> > _______________________________________________<br>
> > dev mailing list<br>
> > <a href="mailto:dev@openvswitch.org">dev@openvswitch.org</a><br>
> > <a href="http://openvswitch.org/mailman/listinfo/dev" target="_blank">http://openvswitch.org/mailman/listinfo/dev</a><br>
> ><br>
</div></div></blockquote></div><br></div></div></div>