[PATCH 0/2] signal(7) fixes

[PATCH 0/2] signal(7) fixes

[Dev Jain]

This is a follow-up to the following:
https://lore.kernel.org/all/20240607122319.768640-1-dev.jain@arm.com/

This series brings distinction between the set of blocked signals
represented in uc_sigmask, and the actual set of blocked signals. Also,
fix a wrong mention of sigprocmask(), and replace it with sigaction(). 

Dev Jain (2):
  signal.7: Clearly describe ucontext kernel dump to userspace
  signal.7: Fix wrong mention of sigprocmask

 man/man7/signal.7 | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

-- 
2.34.1

[PATCH 1/2] signal.7: Clearly describe ucontext kernel dump to userspace

[Dev Jain]

The set of blocked signals is updated from struct sigaction after the
kernel dumps ucontext. Mention this to avoid misunderstanding.

Signed-off-by: Dev Jain <dev.jain@arm.com>
---
 man/man7/signal.7 | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/man/man7/signal.7 b/man/man7/signal.7
index 4ba5306c8..17e75c86d 100644
--- a/man/man7/signal.7
+++ b/man/man7/signal.7
@@ -282,7 +282,15 @@ the thread's alternate signal stack settings.
 .B SA_SIGINFO
 flag, then the above information is accessible via the
 .I ucontext_t
-object that is pointed to by the third argument of the signal handler.)
+object that is pointed to by the third argument of the signal handler.
+We emphasize on the fact that this object contains context information
+of the thread, present before jumping into the handler; the set of
+blocked signals for the current thread would be updated from struct
+sigaction only after
+.I ucontext_t
+has been dumped to userspace. This semantically makes sense since the
+context for which the signals have been blocked, remains only during
+execution of the handler.)
 .IP (1.4)
 Any signals specified in
 .I act\->sa_mask
-- 
2.34.1

[PATCH 2/2] signal.7: Fix wrong mention of sigprocmask

[Dev Jain]

The handler is registered with sigaction(), not sigprocmask(). Even if the
purpose of writing sigprocmask() here was to mention blocked signals, the
statement currently concerns the "addition" of blocked signals; signals
blocked through sigprocmask() would already be present in the thread
context of blocked signals.

Fixes: e7a5700 (getcontext.3, signal.7: tfix)
Signed-off-by: Dev Jain <dev.jain@arm.com>
---
 man/man7/signal.7 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/man/man7/signal.7 b/man/man7/signal.7
index 17e75c86d..09d30c678 100644
--- a/man/man7/signal.7
+++ b/man/man7/signal.7
@@ -295,7 +295,7 @@ execution of the handler.)
 Any signals specified in
 .I act\->sa_mask
 when registering the handler with
-.BR sigprocmask (2)
+.BR sigaction (2)
 are added to the thread's signal mask.
 The signal being delivered is also
 added to the signal mask, unless
-- 
2.34.1

Re: [PATCH 1/2] signal.7: Clearly describe ucontext kernel dump to userspace

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 947 bytes --]

On Fri, Jun 07, 2024 at 06:01:18PM +0530, Dev Jain wrote:

>  .I ucontext_t
> -object that is pointed to by the third argument of the signal handler.)
> +object that is pointed to by the third argument of the signal handler.
> +We emphasize on the fact that this object contains context information

s/We emphasize on the fact that/Note that/

feels more idiomatic.

> +of the thread, present before jumping into the handler; the set of
> +blocked signals for the current thread would be updated from struct

s/would/will/

> +sigaction only after
> +.I ucontext_t
> +has been dumped to userspace. This semantically makes sense since the
> +context for which the signals have been blocked, remains only during
> +execution of the handler.)

I'd drop the "this semantically makes sense" and reword the last bit to
be something like "The ucontext reflects the state at the time the
signal is delivered rather than in the handler" for idiom reasons.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] signal.7: Clearly describe ucontext kernel dump to userspace

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 947 bytes --]

On Fri, Jun 07, 2024 at 06:01:18PM +0530, Dev Jain wrote:

>  .I ucontext_t
> -object that is pointed to by the third argument of the signal handler.)
> +object that is pointed to by the third argument of the signal handler.
> +We emphasize on the fact that this object contains context information

s/We emphasize on the fact that/Note that/

feels more idiomatic.

> +of the thread, present before jumping into the handler; the set of
> +blocked signals for the current thread would be updated from struct

s/would/will/

> +sigaction only after
> +.I ucontext_t
> +has been dumped to userspace. This semantically makes sense since the
> +context for which the signals have been blocked, remains only during
> +execution of the handler.)

I'd drop the "this semantically makes sense" and reword the last bit to
be something like "The ucontext reflects the state at the time the
signal is delivered rather than in the handler" for idiom reasons.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] signal.7: Clearly describe ucontext kernel dump to userspace

[Alejandro Colomar]

[-- Attachment #1: Type: text/plain, Size: 1931 bytes --]

Hi Mark, Dev,

On Fri, Jun 07, 2024 at 04:39:33PM GMT, Mark Brown wrote:
> On Fri, Jun 07, 2024 at 06:01:18PM +0530, Dev Jain wrote:
> 
> >  .I ucontext_t
> > -object that is pointed to by the third argument of the signal handler.)
> > +object that is pointed to by the third argument of the signal handler.
> > +We emphasize on the fact that this object contains context information
> 
> s/We emphasize on the fact that/Note that/
> 
> feels more idiomatic.

Even more idiomatic is to jump straight to the point, without noting it.
If it weren't notable, we wouldn't say it.  :)

> 
> > +of the thread, present before jumping into the handler; the set of
> > +blocked signals for the current thread would be updated from struct
> 
> s/would/will/
> 
> > +sigaction only after
> > +.I ucontext_t
> > +has been dumped to userspace. This semantically makes sense since the

Please use semantic newlines.  See man-pages:

$ MANWIDTH=72 man man-pages | sed -n '/Use semantic newlines/,/^$/p'
   Use semantic newlines
     In the source of a manual page, new sentences should be started on
     new lines, long sentences should be split  into  lines  at  clause
     breaks  (commas,  semicolons, colons, and so on), and long clauses
     should be split at phrase boundaries.  This convention,  sometimes
     known as "semantic newlines", makes it easier to see the effect of
     patches, which often operate at the level of individual sentences,
     clauses, or phrases.

> > +context for which the signals have been blocked, remains only during
> > +execution of the handler.)
> 
> I'd drop the "this semantically makes sense" and reword the last bit to
> be something like "The ucontext reflects the state at the time the
> signal is delivered rather than in the handler" for idiom reasons.

Thank you both.

Have a lovely day!
Alex


-- 
<https://www.alejandro-colomar.es/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]