diff mbox series

[v2,2/6] tools/misc: rework xenwatchdogd signal handling

Message ID 20240329111056.6118-3-leigh@solinno.co.uk (mailing list archive)
State Superseded
Headers show
Series xenwatchdogd bugfixes and enhancements | expand

Commit Message

Leigh Brown March 29, 2024, 11:10 a.m. UTC 
From: Leigh Brown <leigh@solinno.co.uk>

Rework xenwatchdogd signal handling to do the minimum in the signal
handler. This is a very minor enhancement.

Signed-off-by: Leigh Brown <leigh@solinno.co.uk>
---
 tools/misc/xenwatchdogd.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

Comments

Anthony PERARD April 9, 2024, 2:30 p.m. UTC | #1 
On Fri, Mar 29, 2024 at 11:10:52AM +0000, leigh@solinno.co.uk wrote:
> From: Leigh Brown <leigh@solinno.co.uk>
> 
> Rework xenwatchdogd signal handling to do the minimum in the signal
> handler. This is a very minor enhancement.
> 
> Signed-off-by: Leigh Brown <leigh@solinno.co.uk>

Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,
Andrew Cooper April 11, 2024, 12:12 p.m. UTC | #2 
On 29/03/2024 11:10 am, leigh@solinno.co.uk wrote:
> diff --git a/tools/misc/xenwatchdogd.c b/tools/misc/xenwatchdogd.c
> index 2f7c822d61..35a0df655a 100644
> --- a/tools/misc/xenwatchdogd.c
> +++ b/tools/misc/xenwatchdogd.c
> @@ -9,9 +9,11 @@
>  #include <unistd.h>
>  #include <signal.h>
>  #include <stdio.h>
> +#include <stdbool.h>
>  
>  xc_interface *h;
> -int id = 0;
> +static bool safeexit = false;
> +static bool done = false;

It's a common bug, but these need to be volatile.  Right now, ...

> @@ -90,10 +90,14 @@ int main(int argc, char **argv)
>      if (id <= 0)
>          err(EXIT_FAILURE, "xc_watchdog setup");
>  
> -    for (;;) {
> +    while (!done) {
>          sleep(s);

... the only reason this isn't an infinite loop is because the compiler
can't prove that sleep() doesn't modify the variable.  This goes wrong
in subtle and fun ways when using LTO.

I'll fix on commit.

For the record, I've taken 1-3 which are ready.  You'll need to rework 4
and later per Anthony's feedback.

~Andrew
Leigh Brown April 11, 2024, 3:59 p.m. UTC | #3 
Hi Andrew,

On 2024-04-11 13:12, Andrew Cooper wrote:
> On 29/03/2024 11:10 am, leigh@solinno.co.uk wrote:
>> diff --git a/tools/misc/xenwatchdogd.c b/tools/misc/xenwatchdogd.c
>> index 2f7c822d61..35a0df655a 100644
>> --- a/tools/misc/xenwatchdogd.c
>> +++ b/tools/misc/xenwatchdogd.c
>> @@ -9,9 +9,11 @@
>>  #include <unistd.h>
>>  #include <signal.h>
>>  #include <stdio.h>
>> +#include <stdbool.h>
>> 
>>  xc_interface *h;
>> -int id = 0;
>> +static bool safeexit = false;
>> +static bool done = false;
> 
> It's a common bug, but these need to be volatile.  Right now, ...

I'm an idiot (I'm sure I've mentioned this a couple of times :-) )

>> @@ -90,10 +90,14 @@ int main(int argc, char **argv)
>>      if (id <= 0)
>>          err(EXIT_FAILURE, "xc_watchdog setup");
>> 
>> -    for (;;) {
>> +    while (!done) {
>>          sleep(s);
> 
> ... the only reason this isn't an infinite loop is because the compiler
> can't prove that sleep() doesn't modify the variable.  This goes wrong
> in subtle and fun ways when using LTO.
> 
> I'll fix on commit.
> 
> For the record, I've taken 1-3 which are ready.  You'll need to rework 
> 4
> and later per Anthony's feedback.
> 
> ~Andrew

Thanks, I will get those done this evening, hopefully.

Regards,

Leigh.
diff mbox series

Patch

diff --git a/tools/misc/xenwatchdogd.c b/tools/misc/xenwatchdogd.c
index 2f7c822d61..35a0df655a 100644
--- a/tools/misc/xenwatchdogd.c
+++ b/tools/misc/xenwatchdogd.c
@@ -9,9 +9,11 @@ 
 #include <unistd.h>
 #include <signal.h>
 #include <stdio.h>
+#include <stdbool.h>
 
 xc_interface *h;
-int id = 0;
+static bool safeexit = false;
+static bool done = false;
 
 void daemonize(void)
 {
@@ -38,20 +40,18 @@  void daemonize(void)
 
 void catch_exit(int sig)
 {
-    if (id)
-        xc_watchdog(h, id, 300);
-    exit(EXIT_SUCCESS);
+    done = true;
 }
 
 void catch_usr1(int sig)
 {
-    if (id)
-        xc_watchdog(h, id, 0);
-    exit(EXIT_SUCCESS);
+    safeexit = true;
+    done = true;
 }
 
 int main(int argc, char **argv)
 {
+    int id;
     int t, s;
     int ret;
 
@@ -90,10 +90,14 @@  int main(int argc, char **argv)
     if (id <= 0)
         err(EXIT_FAILURE, "xc_watchdog setup");
 
-    for (;;) {
+    while (!done) {
         sleep(s);
         ret = xc_watchdog(h, id, t);
         if (ret != 0)
             err(EXIT_FAILURE, "xc_watchdog");
     }
+
+    // Zero seconds timeout will disarm the watchdog timer
+    xc_watchdog(h, id, safeexit ? 0 : 300);
+    return 0;
 }