| Message ID | 20210623184354.395316-3-adhemerval.zanella@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Add _Fork implementation | expand |
* Adhemerval Zanella via Libc-alpha: > Checked on x86_64-linux-gnu. > --- > posix/fork.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/posix/fork.c b/posix/fork.c > index 44caf8d166..9340511198 100644 > --- a/posix/fork.c > +++ b/posix/fork.c > @@ -103,6 +103,9 @@ __libc_fork (void) > } > else > { > + /* If _Fork failed, preserve its errno value. */ > + int save_errno = errno; > + > /* Release acquired locks in the multi-threaded case. */ > if (multiple_threads) > { > @@ -115,6 +118,8 @@ __libc_fork (void) > > /* Run the handlers registered for the parent. */ > __run_fork_handlers (atfork_run_parent, multiple_threads); > + > + __set_errno (save_errno); I think you should restrict the __set_errno call to pid < 0, so that errno is not 0 after a different value has been observed by the fork handlers. Thanks, Florian
On 24/06/2021 05:19, Florian Weimer wrote: > * Adhemerval Zanella via Libc-alpha: > >> Checked on x86_64-linux-gnu. >> --- >> posix/fork.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/posix/fork.c b/posix/fork.c >> index 44caf8d166..9340511198 100644 >> --- a/posix/fork.c >> +++ b/posix/fork.c >> @@ -103,6 +103,9 @@ __libc_fork (void) >> } >> else >> { >> + /* If _Fork failed, preserve its errno value. */ >> + int save_errno = errno; >> + >> /* Release acquired locks in the multi-threaded case. */ >> if (multiple_threads) >> { >> @@ -115,6 +118,8 @@ __libc_fork (void) >> >> /* Run the handlers registered for the parent. */ >> __run_fork_handlers (atfork_run_parent, multiple_threads); >> + >> + __set_errno (save_errno); > > I think you should restrict the __set_errno call to pid < 0, so that > errno is not 0 after a different value has been observed by the fork > handlers. OK, I can change it back. From the previous review iteration I understood you were ok with making it unconditional [1]. [1] https://sourceware.org/pipermail/libc-alpha/2021-March/123729.html
* Adhemerval Zanella: > On 24/06/2021 05:19, Florian Weimer wrote: >> * Adhemerval Zanella via Libc-alpha: >> >>> Checked on x86_64-linux-gnu. >>> --- >>> posix/fork.c | 5 +++++ >>> 1 file changed, 5 insertions(+) >>> >>> diff --git a/posix/fork.c b/posix/fork.c >>> index 44caf8d166..9340511198 100644 >>> --- a/posix/fork.c >>> +++ b/posix/fork.c >>> @@ -103,6 +103,9 @@ __libc_fork (void) >>> } >>> else >>> { >>> + /* If _Fork failed, preserve its errno value. */ >>> + int save_errno = errno; >>> + >>> /* Release acquired locks in the multi-threaded case. */ >>> if (multiple_threads) >>> { >>> @@ -115,6 +118,8 @@ __libc_fork (void) >>> >>> /* Run the handlers registered for the parent. */ >>> __run_fork_handlers (atfork_run_parent, multiple_threads); >>> + >>> + __set_errno (save_errno); >> >> I think you should restrict the __set_errno call to pid < 0, so that >> errno is not 0 after a different value has been observed by the fork >> handlers. > > OK, I can change it back. From the previous review iteration I understood > you were ok with making it unconditional [1]. > > [1] https://sourceware.org/pipermail/libc-alpha/2021-March/123729.html Sorry, I meant making the saving unconditional, not the restoring. Florian
On 24/06/2021 08:19, Florian Weimer wrote: > * Adhemerval Zanella: > >> On 24/06/2021 05:19, Florian Weimer wrote: >>> * Adhemerval Zanella via Libc-alpha: >>> >>>> Checked on x86_64-linux-gnu. >>>> --- >>>> posix/fork.c | 5 +++++ >>>> 1 file changed, 5 insertions(+) >>>> >>>> diff --git a/posix/fork.c b/posix/fork.c >>>> index 44caf8d166..9340511198 100644 >>>> --- a/posix/fork.c >>>> +++ b/posix/fork.c >>>> @@ -103,6 +103,9 @@ __libc_fork (void) >>>> } >>>> else >>>> { >>>> + /* If _Fork failed, preserve its errno value. */ >>>> + int save_errno = errno; >>>> + >>>> /* Release acquired locks in the multi-threaded case. */ >>>> if (multiple_threads) >>>> { >>>> @@ -115,6 +118,8 @@ __libc_fork (void) >>>> >>>> /* Run the handlers registered for the parent. */ >>>> __run_fork_handlers (atfork_run_parent, multiple_threads); >>>> + >>>> + __set_errno (save_errno); >>> >>> I think you should restrict the __set_errno call to pid < 0, so that >>> errno is not 0 after a different value has been observed by the fork >>> handlers. >> >> OK, I can change it back. From the previous review iteration I understood >> you were ok with making it unconditional [1]. >> >> [1] https://sourceware.org/pipermail/libc-alpha/2021-March/123729.html > > Sorry, I meant making the saving unconditional, not the restoring. Ok, I will fix it.
diff --git a/posix/fork.c b/posix/fork.c index 44caf8d166..9340511198 100644 --- a/posix/fork.c +++ b/posix/fork.c @@ -103,6 +103,9 @@ __libc_fork (void) } else { + /* If _Fork failed, preserve its errno value. */ + int save_errno = errno; + /* Release acquired locks in the multi-threaded case. */ if (multiple_threads) { @@ -115,6 +118,8 @@ __libc_fork (void) /* Run the handlers registered for the parent. */ __run_fork_handlers (atfork_run_parent, multiple_threads); + + __set_errno (save_errno); } return pid;