diff mbox series

tools: Improve signal handling in xen-vmtrace

Message ID 26720bf5c8258e1b7b4600af3648039b5b9ee18d.1614336820.git.hubert.jasudowicz@cert.pl (mailing list archive)
State New
Headers show
Series tools: Improve signal handling in xen-vmtrace | expand

Commit Message

Hubert Jasudowicz Feb. 26, 2021, 10:59 a.m. UTC
Make sure xen-vmtrace exits cleanly in case SIGPIPE is sent. This can
happen when piping the output to some other program.

Additionaly, add volatile qualifier to interrupted flag to avoid
it being optimized away by the compiler.

Signed-off-by: Hubert Jasudowicz <hubert.jasudowicz@cert.pl>
---
 tools/misc/xen-vmtrace.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Hubert Jasudowicz Feb. 26, 2021, 11:23 a.m. UTC | #1
On 2021-02-26, Hubert Jasudowicz wrote:
> Make sure xen-vmtrace exits cleanly in case SIGPIPE is sent. This can
> happen when piping the output to some other program.
> 
> Additionaly, add volatile qualifier to interrupted flag to avoid
> it being optimized away by the compiler.
> 
> Signed-off-by: Hubert Jasudowicz <hubert.jasudowicz@cert.pl>
> ---
>  tools/misc/xen-vmtrace.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/misc/xen-vmtrace.c b/tools/misc/xen-vmtrace.c
> index 7572e880c5..e2da043058 100644
> --- a/tools/misc/xen-vmtrace.c
> +++ b/tools/misc/xen-vmtrace.c
> @@ -43,7 +43,7 @@ static uint32_t domid, vcpu;
>  static size_t size;
>  static char *buf;
>  
> -static sig_atomic_t interrupted;
> +static volatile sig_atomic_t interrupted;
>  static void int_handler(int signum)
>  {
>      interrupted = 1;
> @@ -81,6 +81,9 @@ int main(int argc, char **argv)
>      if ( signal(SIGINT, int_handler) == SIG_ERR )
>          err(1, "Failed to register signal handler\n");
>  
> +    if ( signal(SIGPIPE, int_handler) == SIG_ERR )
> +        err(1, "Failed to register signal handler\n");
> +
>      if ( argc != 3 )
>      {
>          fprintf(stderr, "Usage: %s <domid> <vcpu_id>\n", argv[0]);
> -- 
> 2.30.0
> 
> 

Oops, forgot 4.15 tag. But IMO this should be included.

Thanks
Hubert Jasudowicz
CERT Polska
Andrew Cooper Feb. 26, 2021, 5:44 p.m. UTC | #2
On 26/02/2021 10:59, Hubert Jasudowicz wrote:
> Make sure xen-vmtrace exits cleanly in case SIGPIPE is sent. This can
> happen when piping the output to some other program.
>
> Additionaly, add volatile qualifier to interrupted flag to avoid
> it being optimized away by the compiler.
>
> Signed-off-by: Hubert Jasudowicz <hubert.jasudowicz@cert.pl>

Ok, so this is being used in production.

In which case, what other signals potentially need dealing with?  Lets
get them all fixed in one go.

When that's done, we should make it installed by default, to match its
expected usecase.

~Andrew
Ian Jackson March 1, 2021, 5:34 p.m. UTC | #3
Hubert Jasudowicz writes ("[PATCH] tools: Improve signal handling in xen-vmtrace"):
> Make sure xen-vmtrace exits cleanly in case SIGPIPE is sent. This can
> happen when piping the output to some other program.
> 
> Additionaly, add volatile qualifier to interrupted flag to avoid
> it being optimized away by the compiler.

Release-Acked-by: Ian Jackson <iwj@xenproject.org>
Ian Jackson March 1, 2021, 5:39 p.m. UTC | #4
Andrew Cooper writes ("Re: [PATCH] tools: Improve signal handling in xen-vmtrace"):
> In which case, what other signals potentially need dealing with?  Lets
> get them all fixed in one go.
> 
> When that's done, we should make it installed by default, to match its
> expected usecase.

With my tools maintainer hat on:

TERM INT HUP PIPE QUIT

Not sure if we can be bothered with SIGTSTP.

If you want to be nice, when a signal occurs. arrange to re-raise it
after cleanup.  After all, exiting with stderr blather and a non-zero
exit status, merely for SIGPIPE, is rather unfriendly.

This means writing the signal number to the volatile.


With my release manager hat on:

I do not intend to give a release ack to install this by default, at
this stage.  It would have been better to have made this program a
proper utility from the start, but it has now missed the boat for
being a supported feature for 4.15.

OTOH given that it is not installed by default, nor supported, I would
welcome impreovements to it that I don't think will break the build.


Ian.
diff mbox series

Patch

diff --git a/tools/misc/xen-vmtrace.c b/tools/misc/xen-vmtrace.c
index 7572e880c5..e2da043058 100644
--- a/tools/misc/xen-vmtrace.c
+++ b/tools/misc/xen-vmtrace.c
@@ -43,7 +43,7 @@  static uint32_t domid, vcpu;
 static size_t size;
 static char *buf;
 
-static sig_atomic_t interrupted;
+static volatile sig_atomic_t interrupted;
 static void int_handler(int signum)
 {
     interrupted = 1;
@@ -81,6 +81,9 @@  int main(int argc, char **argv)
     if ( signal(SIGINT, int_handler) == SIG_ERR )
         err(1, "Failed to register signal handler\n");
 
+    if ( signal(SIGPIPE, int_handler) == SIG_ERR )
+        err(1, "Failed to register signal handler\n");
+
     if ( argc != 3 )
     {
         fprintf(stderr, "Usage: %s <domid> <vcpu_id>\n", argv[0]);