On Thu, Jul 3, 2014 at 1:23 PM, Ben Pfaff <b...@nicira.com> wrote:
> On Sun, Jun 29, 2014 at 08:37:25PM -0700, Ansis Atteka wrote:
>> From: Ansis <ansisatt...@gmail.com>
>
> Fix up the patch author?
>
>> The child process (the one being monitored) could die before it was able
>> to call fork_notify_startup() function. If such situation arises, then
>> parent process (the one monitoring child process) would also terminate
>> with a fatal log message:
>>
>> ...|EMER|fork child died before signaling startup (killed (...))
>>
>> This patch changes that beahvior by always restarting child process
>> if it was able to start up at least once in the past. However, if
>> child was not able to start up even the first time, then monitor process
>> would still terminate, because that would most likely indicate a
>> persistent programming error.
>>
>> To reproduce use following script:
>>
>> while : ; do kill -SIGSEGV `cat /var/run/openvswitch/ovs-vswitchd.pid`; done
>>
>> Signed-Off-By: Ansis Atteka <aatt...@nicira.com>
>> Issue: 1273550
>
> Clang does not like this:
>
> ../lib/daemon-unix.c:338:13: error: variable 'retval' is used
> uninitialized
> whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
> if (child_ready) {
> ^~~~~~~~~~~
> ../lib/daemon-unix.c:347:13: note: uninitialized use occurs here
> if (retval == daemon_pid || !child_ready) {
> ^~~~~~
> ../lib/daemon-unix.c:338:9: note: remove the 'if' if its condition is
> always
> true
> if (child_ready) {
> ^~~~~~~~~~~~~~~~~
> ../lib/daemon-unix.c:332:19: note: initialize the variable 'retval' to
> silence
> this warning
> int retval;
> ^
> = 0
> 1 error generated.
>
> I can kind of see its point. This use of uninitialized data will
> never be a real problem, because !child_ready will always be true if
> retval is uninitialized, but it still reads better (and pleases Clang)
> if the test condition is reversed:
> if (!child_ready || retval == daemon_pid) {
>
> I think that the previous code logged something at VLL_EMER level if
> the monitor process exited. I think the new version does not; it
> would be nice if it did. You might use VLOG_FATAL instead of exit(1)
> to get that effect.
I sent V2 patch that addresses all your comments. Thanks for review!
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
