Message ID | 4f3ca170-c8d2-f684-4922-a799413dddda@redhat.com |
---|---|
State | New |
Headers | show |
Series | nptl: Document AS-safe functions in cancellation.c. | expand |
On Fri, Oct 4, 2019 at 4:01 PM Carlos O'Donell <carlos@redhat.com> wrote: > > In a recent conversation with Mathieu Desnoyer he pointed out that > because write is AS-safe all the wrappers around write should be > also AS-safe. We don't spell that out explicitly in the comments > for __pthread_enable_asynccancel and __pthread_disable_asynccancel > so I added them here. > > > /* The next two functions are similar to pthread_setcanceltype() but > more specialized for the use in the cancelable functions like write(). > - They do not need to check parameters etc. */ > + They do not need to check parameters etc. This function must be > + AS-safe, with the exception of the actual cancellation, because they > + are called by wrappers around AS-safe functions like write().*/ Grammar nit: Either "_These functions_ must be AS-safe, ..." or "because _it is_ called." The former is probably better because consistent with the first sentence in this comment... > - > +/* This function must be AS-safe, with the exception of the actual > + cancellation, because they are called by wrappers around AS-safe > + functions like write().*/ ... but then maybe you _don't_ want to add this comment as well. zw
On 10/4/19 4:45 PM, Zack Weinberg wrote: > On Fri, Oct 4, 2019 at 4:01 PM Carlos O'Donell <carlos@redhat.com> wrote: >> >> In a recent conversation with Mathieu Desnoyer he pointed out that >> because write is AS-safe all the wrappers around write should be >> also AS-safe. We don't spell that out explicitly in the comments >> for __pthread_enable_asynccancel and __pthread_disable_asynccancel >> so I added them here. >> >> >> /* The next two functions are similar to pthread_setcanceltype() but >> more specialized for the use in the cancelable functions like write(). >> - They do not need to check parameters etc. */ >> + They do not need to check parameters etc. This function must be >> + AS-safe, with the exception of the actual cancellation, because they >> + are called by wrappers around AS-safe functions like write().*/ > > Grammar nit: Either "_These functions_ must be AS-safe, ..." or > "because _it is_ called." The former is probably better because > consistent with the first sentence in this comment... Yes, I actually wrote "These" first, but then cut-and-pasted it into both functions, and rewrote "These" to "This" and forgot to clean it up. Thanks for the review! >> - >> +/* This function must be AS-safe, with the exception of the actual >> + cancellation, because they are called by wrappers around AS-safe >> + functions like write().*/ > > ... but then maybe you _don't_ want to add this comment as well. v2 - Rework comments based on Zack's review. How about v2? 8< --- 8< --- 8< Document in comments that __pthread_enable_asynccancel and __pthread_disable_asynccancel must be AS-safe in general with the exception of the act of cancellation. --- ChangeLog | 5 +++++ nptl/cancellation.c | 7 +++++-- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/ChangeLog b/ChangeLog index cfabdaddf4..2ce48223f4 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,8 @@ +2019-10-07 Carlos O'Donell <carlos@redhat.com> + + * nptl/cancellation.c: Document that all functions here must be + AS-safe because they wrap AS-safe functions. + 2019-10-07 Leandro Pereira <leandro.pereira@microsoft.com> * elf/dl-load.c: Use __pread64_nocancel() instead of __lseek()+ diff --git a/nptl/cancellation.c b/nptl/cancellation.c index 7712845561..23d7b2b34c 100644 --- a/nptl/cancellation.c +++ b/nptl/cancellation.c @@ -24,7 +24,9 @@ /* The next two functions are similar to pthread_setcanceltype() but more specialized for the use in the cancelable functions like write(). - They do not need to check parameters etc. */ + They do not need to check parameters etc. These functions must be + AS-safe, with the exception of the actual cancellation, because they + are called by wrappers around AS-safe functions like write().*/ int attribute_hidden __pthread_enable_asynccancel (void) @@ -59,7 +61,8 @@ __pthread_enable_asynccancel (void) return oldval; } - +/* See the comment for __pthread_enable_asynccancel regarding + the AS-safety of this function. */ void attribute_hidden __pthread_disable_asynccancel (int oldtype)
On Mon, Oct 7, 2019 at 1:35 AM Carlos O'Donell <carlos@redhat.com> wrote: > > How about v2? Revised patch LGTM. zw
On 10/7/19 12:58 PM, Zack Weinberg wrote: > On Mon, Oct 7, 2019 at 1:35 AM Carlos O'Donell <carlos@redhat.com> wrote: >> >> How about v2? > > Revised patch LGTM. Pushed. Thanks.
diff --git a/ChangeLog b/ChangeLog index bdd97f0606..716f912c40 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,8 @@ +2019-10-04 Carlos O'Donell <carlos@redhat.com> + + * nptl/cancellation.c: Document that all functions here must be + AS-safe because they wrap AS-safe functions. + 2019-10-03 Leandro Pereira <leandro.pereira@microsoft.com> * elf/dl-load.c: Use __pread64_nocancel() instead of __lseek()+ diff --git a/nptl/cancellation.c b/nptl/cancellation.c index 7712845561..ffe5b78b18 100644 --- a/nptl/cancellation.c +++ b/nptl/cancellation.c @@ -24,7 +24,9 @@ /* The next two functions are similar to pthread_setcanceltype() but more specialized for the use in the cancelable functions like write(). - They do not need to check parameters etc. */ + They do not need to check parameters etc. This function must be + AS-safe, with the exception of the actual cancellation, because they + are called by wrappers around AS-safe functions like write().*/ int attribute_hidden __pthread_enable_asynccancel (void) @@ -59,7 +61,9 @@ __pthread_enable_asynccancel (void) return oldval; } - +/* This function must be AS-safe, with the exception of the actual + cancellation, because they are called by wrappers around AS-safe + functions like write().*/ void attribute_hidden __pthread_disable_asynccancel (int oldtype)