Message ID | 20210202151134.2123748-5-adhemerval.zanella@linaro.org |
---|---|
State | Superseded |
Delegated to: | Adhemerval Zanella Netto |
Headers |
Return-Path: <libc-alpha-bounces@sourceware.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 90A5F398B166; Tue, 2 Feb 2021 15:11:50 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 90A5F398B166 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1612278710; bh=rN7qZxCRG5QszL9noYacvE6M74GjyFOWWn+yuqUSEug=; h=To:Subject:Date:In-Reply-To:References:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=qn8r9q8qVcGsHgDj3PsZI8mz93gVK6rgPzMPxwehOpuIoZwrG2CkgIbtnK2LHHjk2 pXADHWkB6D4xKyGXXBb16Z4Gr78VFDs+dDa/RpKiA3lbSaMBG0pYsT5W4C87PTrkvC U5repkpt3AL6O5+ZEj/DN6G0PiglZP8jqInDEZhA= X-Original-To: libc-alpha@sourceware.org Delivered-To: libc-alpha@sourceware.org Received: from mail-qt1-x82f.google.com (mail-qt1-x82f.google.com [IPv6:2607:f8b0:4864:20::82f]) by sourceware.org (Postfix) with ESMTPS id ACB26398B162 for <libc-alpha@sourceware.org>; Tue, 2 Feb 2021 15:11:47 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org ACB26398B162 Received: by mail-qt1-x82f.google.com with SMTP id l23so15082598qtq.13 for <libc-alpha@sourceware.org>; Tue, 02 Feb 2021 07:11:47 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=rN7qZxCRG5QszL9noYacvE6M74GjyFOWWn+yuqUSEug=; b=rVkrkF/2MJt4evEO+XSPJTzEhD6MsOeJWk/nlRkr9b/3pG+YUdmDno0ErS5LP6tWAd fV90QXKAJDLgXXdfhfAydY+n5OatdLwZhigUxvbeW6lccVKvnHmykUKNF11K4stz7Xyo nZfyQTaRy7j2eCbIvElz3wz1YmtluIHt+vJWNs3QRivEt/Aphnq8PtvLEl4cHDbzfVS1 mS9+e1Xo5wsRA3krJyw0OsiWPUchl9oI14kkvtquXdy5K76671/sIfK+OUnuLOKyE44A FWmw2qSBW4bBopN++omjC9QbVZS9P5vbRW60QCYT5tF9ONFVyuJnr+9BnZu91dWchfJt kQWw== X-Gm-Message-State: AOAM531cH0mRvf1HfLVBNfQEFf6cWmVry+J5PItuDjlHV0RPZ6q7rh7A Z0DtlRomDVp8jGIQsxKQizJejOpmcZhGvg== X-Google-Smtp-Source: ABdhPJyClQETAoo6v60zzbmFkxRUG5UcXqAfNsNrjwFPKCpWaEG2bl94NLSt0IKd2PeRQLdoib7BZQ== X-Received: by 2002:ac8:7b52:: with SMTP id m18mr20680075qtu.312.1612278707103; Tue, 02 Feb 2021 07:11:47 -0800 (PST) Received: from localhost.localdomain ([177.194.48.209]) by smtp.googlemail.com with ESMTPSA id z20sm16617267qki.93.2021.02.02.07.11.46 for <libc-alpha@sourceware.org> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 02 Feb 2021 07:11:46 -0800 (PST) To: libc-alpha@sourceware.org Subject: [PATCH 5/8] posix: Do not clobber errno by atfork handlers Date: Tue, 2 Feb 2021 12:11:31 -0300 Message-Id: <20210202151134.2123748-5-adhemerval.zanella@linaro.org> X-Mailer: git-send-email 2.25.1 In-Reply-To: <20210202151134.2123748-1-adhemerval.zanella@linaro.org> References: <20210202151134.2123748-1-adhemerval.zanella@linaro.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-13.5 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list <libc-alpha.sourceware.org> List-Unsubscribe: <https://sourceware.org/mailman/options/libc-alpha>, <mailto:libc-alpha-request@sourceware.org?subject=unsubscribe> List-Archive: <https://sourceware.org/pipermail/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-request@sourceware.org?subject=help> List-Subscribe: <https://sourceware.org/mailman/listinfo/libc-alpha>, <mailto:libc-alpha-request@sourceware.org?subject=subscribe> From: Adhemerval Zanella via Libc-alpha <libc-alpha@sourceware.org> Reply-To: Adhemerval Zanella <adhemerval.zanella@linaro.org> Errors-To: libc-alpha-bounces@sourceware.org Sender: "Libc-alpha" <libc-alpha-bounces@sourceware.org> |
Series |
[1/8] posix: Consolidate register-atfork
|
|
Commit Message
Adhemerval Zanella Netto
Feb. 2, 2021, 3:11 p.m. UTC
Checked on x86_64-linux-gnu. --- posix/fork.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
Comments
* Adhemerval Zanella via Libc-alpha: > Checked on x86_64-linux-gnu. > --- > posix/fork.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/posix/fork.c b/posix/fork.c > index 4c9e60f187..7f27fb8338 100644 > --- a/posix/fork.c > +++ b/posix/fork.c > @@ -68,7 +68,7 @@ __libc_fork (void) > } > > pid_t pid = _Fork (); > - > + int save_errno = errno; > if (pid == 0) > { > /* Reset the lock state in the multi-threaded case. */ > @@ -107,6 +107,8 @@ __libc_fork (void) > __run_fork_handlers (pid == 0 ? atfork_run_child : atfork_run_parent, > multiple_threads); > > + if (pid < 0) > + __set_errno (save_errno); > return pid; > } > weak_alias (__libc_fork, __fork) Why is this condition correct? Shouldn't it be pid >= 0? I wonder if this should be part of __run_fork_handlers, so that fork handlers always observe the original errno value. Thanks, Florian
On 09/03/2021 08:01, Florian Weimer wrote: > * Adhemerval Zanella via Libc-alpha: > >> Checked on x86_64-linux-gnu. >> --- >> posix/fork.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/posix/fork.c b/posix/fork.c >> index 4c9e60f187..7f27fb8338 100644 >> --- a/posix/fork.c >> +++ b/posix/fork.c >> @@ -68,7 +68,7 @@ __libc_fork (void) >> } >> >> pid_t pid = _Fork (); >> - >> + int save_errno = errno; >> if (pid == 0) >> { >> /* Reset the lock state in the multi-threaded case. */ >> @@ -107,6 +107,8 @@ __libc_fork (void) >> __run_fork_handlers (pid == 0 ? atfork_run_child : atfork_run_parent, >> multiple_threads); >> >> + if (pid < 0) >> + __set_errno (save_errno); >> return pid; >> } >> weak_alias (__libc_fork, __fork) > > Why is this condition correct? Shouldn't it be pid >= 0? But pid >= 0 is a valid call which should not set errno. The idea is to return the _Fork error value even if some atfork handler in the parent does clobber it. > > I wonder if this should be part of __run_fork_handlers, so that fork > handlers always observe the original errno value. I am not sure about it, checking the errno without actually calling a function that might setting it is error-prone imho.
* Adhemerval Zanella: > On 09/03/2021 08:01, Florian Weimer wrote: >> * Adhemerval Zanella via Libc-alpha: >> >>> Checked on x86_64-linux-gnu. >>> --- >>> posix/fork.c | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/posix/fork.c b/posix/fork.c >>> index 4c9e60f187..7f27fb8338 100644 >>> --- a/posix/fork.c >>> +++ b/posix/fork.c >>> @@ -68,7 +68,7 @@ __libc_fork (void) >>> } >>> >>> pid_t pid = _Fork (); >>> - >>> + int save_errno = errno; >>> if (pid == 0) >>> { >>> /* Reset the lock state in the multi-threaded case. */ >>> @@ -107,6 +107,8 @@ __libc_fork (void) >>> __run_fork_handlers (pid == 0 ? atfork_run_child : atfork_run_parent, >>> multiple_threads); >>> >>> + if (pid < 0) >>> + __set_errno (save_errno); >>> return pid; >>> } >>> weak_alias (__libc_fork, __fork) >> >> Why is this condition correct? Shouldn't it be pid >= 0? > > But pid >= 0 is a valid call which should not set errno. The idea is > to return the _Fork error value even if some atfork handler in the > parent does clobber it. Then maybe move that into the else part for the parent (on fork error there is no subprocess) and add a comment? Thanks, Florian
On 11/03/2021 10:25, Florian Weimer wrote: > * Adhemerval Zanella: > >> On 09/03/2021 08:01, Florian Weimer wrote: >>> * Adhemerval Zanella via Libc-alpha: >>> >>>> Checked on x86_64-linux-gnu. >>>> --- >>>> posix/fork.c | 4 +++- >>>> 1 file changed, 3 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/posix/fork.c b/posix/fork.c >>>> index 4c9e60f187..7f27fb8338 100644 >>>> --- a/posix/fork.c >>>> +++ b/posix/fork.c >>>> @@ -68,7 +68,7 @@ __libc_fork (void) >>>> } >>>> >>>> pid_t pid = _Fork (); >>>> - >>>> + int save_errno = errno; >>>> if (pid == 0) >>>> { >>>> /* Reset the lock state in the multi-threaded case. */ >>>> @@ -107,6 +107,8 @@ __libc_fork (void) >>>> __run_fork_handlers (pid == 0 ? atfork_run_child : atfork_run_parent, >>>> multiple_threads); >>>> >>>> + if (pid < 0) >>>> + __set_errno (save_errno); >>>> return pid; >>>> } >>>> weak_alias (__libc_fork, __fork) >>> >>> Why is this condition correct? Shouldn't it be pid >= 0? >> >> But pid >= 0 is a valid call which should not set errno. The idea is >> to return the _Fork error value even if some atfork handler in the >> parent does clobber it. > > Then maybe move that into the else part for the parent (on fork error > there is no subprocess) and add a comment? But we need to restore it *after* __run_fork_handlers calls. I will add the comment to make it explicit: /* Restore the errno value even if some atfork handler in the parent clobber it. */ if (pid < 0) __set_errno (save_errno)
On Mär 11 2021, Adhemerval Zanella via Libc-alpha wrote: > But we need to restore it *after* __run_fork_handlers calls. I will > add the comment to make it explicit: > > /* Restore the errno value even if some atfork handler in the parent > clobber it. */ Perhaps: "If _Fork failed, preserve its errno value." > if (pid < 0) > __set_errno (save_errno) Andreas.
* Andreas Schwab: > On Mär 11 2021, Adhemerval Zanella via Libc-alpha wrote: > >> But we need to restore it *after* __run_fork_handlers calls. I will >> add the comment to make it explicit: >> >> /* Restore the errno value even if some atfork handler in the parent >> clobber it. */ > > Perhaps: "If _Fork failed, preserve its errno value." > >> if (pid < 0) >> __set_errno (save_errno) Agreed. And then it's clear it should be moved to the parent branch of the if statement above. Thanks, Florian
On 11/03/2021 11:25, Florian Weimer wrote: > * Andreas Schwab: > >> On Mär 11 2021, Adhemerval Zanella via Libc-alpha wrote: >> >>> But we need to restore it *after* __run_fork_handlers calls. I will >>> add the comment to make it explicit: >>> >>> /* Restore the errno value even if some atfork handler in the parent >>> clobber it. */ >> >> Perhaps: "If _Fork failed, preserve its errno value." Ack. >> >>> if (pid < 0) >>> __set_errno (save_errno) > > Agreed. And then it's clear it should be moved to the parent branch of > the if statement above. > But moving it before __run_fork_handlers in the the previous if (which handles the pid != 0) will both 1. reset the errno even for success (pid > 0) and 2. not fixing the issue since __run_fork_handlers will be issue later. What I am missing in your suggestion?
* Adhemerval Zanella: >>>> if (pid < 0) >>>> __set_errno (save_errno) >> >> Agreed. And then it's clear it should be moved to the parent branch of >> the if statement above. >> > > But moving it before __run_fork_handlers in the the previous if (which > handles the pid != 0) will both 1. reset the errno even for success > (pid > 0) and 2. not fixing the issue since __run_fork_handlers will > be issue later. What I am missing in your suggestion? After the __run_fork_handlers call on the parent branch. Thanks, Florian
On 11/03/2021 12:32, Florian Weimer wrote: > * Adhemerval Zanella: > >>>>> if (pid < 0) >>>>> __set_errno (save_errno) >>> >>> Agreed. And then it's clear it should be moved to the parent branch of >>> the if statement above. >>> >> >> But moving it before __run_fork_handlers in the the previous if (which >> handles the pid != 0) will both 1. reset the errno even for success >> (pid > 0) and 2. not fixing the issue since __run_fork_handlers will >> be issue later. What I am missing in your suggestion? > > After the __run_fork_handlers call on the parent branch. Why is this an improvement over doing on fork.c, now that with this change is the only caller of __run_fork_handlers and the routine with the information (pid result) to restore the errno?
* Adhemerval Zanella: > On 11/03/2021 12:32, Florian Weimer wrote: >> * Adhemerval Zanella: >> >>>>>> if (pid < 0) >>>>>> __set_errno (save_errno) >>>> >>>> Agreed. And then it's clear it should be moved to the parent branch of >>>> the if statement above. >>>> >>> >>> But moving it before __run_fork_handlers in the the previous if (which >>> handles the pid != 0) will both 1. reset the errno even for success >>> (pid > 0) and 2. not fixing the issue since __run_fork_handlers will >>> be issue later. What I am missing in your suggestion? >> >> After the __run_fork_handlers call on the parent branch. > > Why is this an improvement over doing on fork.c, now that with > this change is the only caller of __run_fork_handlers and the > routine with the information (pid result) to restore the > errno? I suggest to leave it in fork.c, but put it in the parent branch, instead after the if statement. The code only runs if fork fails, so it's something the parent process needs to do. It's not cleanup that has to happen on both paths. The compiler will likely optimize it to identical machine code, but I think it's helpful to the reader to put it on the parent execution path. Thanks, Florian
On 11/03/2021 16:23, Florian Weimer wrote: > * Adhemerval Zanella: > >> On 11/03/2021 12:32, Florian Weimer wrote: >>> * Adhemerval Zanella: >>> >>>>>>> if (pid < 0) >>>>>>> __set_errno (save_errno) >>>>> >>>>> Agreed. And then it's clear it should be moved to the parent branch of >>>>> the if statement above. >>>>> >>>> >>>> But moving it before __run_fork_handlers in the the previous if (which >>>> handles the pid != 0) will both 1. reset the errno even for success >>>> (pid > 0) and 2. not fixing the issue since __run_fork_handlers will >>>> be issue later. What I am missing in your suggestion? >>> >>> After the __run_fork_handlers call on the parent branch. >> >> Why is this an improvement over doing on fork.c, now that with >> this change is the only caller of __run_fork_handlers and the >> routine with the information (pid result) to restore the >> errno? > > I suggest to leave it in fork.c, but put it in the parent branch, > instead after the if statement. The code only runs if fork fails, so > it's something the parent process needs to do. It's not cleanup that > has to happen on both paths. The compiler will likely optimize it to > identical machine code, but I think it's helpful to the reader to put it > on the parent execution path. Right, I got it now your suggestion.
diff --git a/posix/fork.c b/posix/fork.c index 4c9e60f187..7f27fb8338 100644 --- a/posix/fork.c +++ b/posix/fork.c @@ -68,7 +68,7 @@ __libc_fork (void) } pid_t pid = _Fork (); - + int save_errno = errno; if (pid == 0) { /* Reset the lock state in the multi-threaded case. */ @@ -107,6 +107,8 @@ __libc_fork (void) __run_fork_handlers (pid == 0 ? atfork_run_child : atfork_run_parent, multiple_threads); + if (pid < 0) + __set_errno (save_errno); return pid; } weak_alias (__libc_fork, __fork)