[PATCH] tools: Improve signal handling in xen-vmtrace

[PATCH] tools: Improve signal handling in xen-vmtrace

[Hubert Jasudowicz]

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

Re: [PATCH] tools: Improve signal handling in xen-vmtrace

[Hubert Jasudowicz]

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

Re: [PATCH] tools: Improve signal handling in xen-vmtrace

[Andrew Cooper]

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

Re: [PATCH] tools: Improve signal handling in xen-vmtrace

[Ian Jackson]

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>

Re: [PATCH] tools: Improve signal handling in xen-vmtrace

[Ian Jackson]

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.