[RFC] sunrpc: Properly cleanup if tst-udp-timeout fails

[RFC] sunrpc: Properly cleanup if tst-udp-timeout fails

[Matheus Castanho]

Hi,

I recently noted some orphan processes left on a host after failed
sunrpc/tst-udp-timeout.c runs.  More info on the commit message.

Even though the test infra has support for a cleanup_function, it
is only called when the test times out.  The same happens for the code
to kill the whole process group (both on the signal handler in
support/support_test_main.c).  So I ended up adding a cleanup function
to be called right before exit() to terminate the child.

I also considered using a function with __attribute__((destructor))
to perform the cleanup, but adding a new TEST_VERIFY_* macro seemed
to be more reusable by other tests if needed.

Is this the best way to handle this issue?

----8<----

The macro TEST_VERIFY_EXIT is used several times on
sunrpc/tst-udp-timeout to exit the test if a condition evaluates to
false. The side effect is that the code to terminate the RPC server
process is not executed when the program calls exit(), so that
sub-process stays alive.

This commit adds a new TEST_VERIFY_CLEANUP macro to allow specifying
a cleanup function to be called if the check fails. TEST_VERIFY_EXIT
calls in tst-udp-timeout that may leave the orphan process behind
are replaced by this macro so a cleanup function is run before
exiting to guarantee the server is properly terminated in case of
a test failure.
---
 sunrpc/tst-udp-timeout.c | 29 ++++++++++++++++++++---------
 support/check.h          | 12 ++++++++++++
 2 files changed, 32 insertions(+), 9 deletions(-)

diff --git a/sunrpc/tst-udp-timeout.c b/sunrpc/tst-udp-timeout.c
index 8d45365b23..a936afd56c 100644
--- a/sunrpc/tst-udp-timeout.c
+++ b/sunrpc/tst-udp-timeout.c
@@ -30,6 +30,8 @@
 #include <time.h>
 #include <unistd.h>

+pid_t pid;
+
 /* Test data serialization and deserialization.   */

 struct test_query
@@ -66,6 +68,15 @@ xdr_test_response (XDR *xdrs, void *data, ...)
     && xdr_uint32_t (xdrs, &p->sum);
 }

+/* Cleanup function to be called in case of failure.  */
+static void
+cleanup (void)
+{
+  /* Kill the child process running the RPC server.  */
+  kill(pid, SIGTERM);
+  exit(1);
+}
+
 /* Implementation of the test server.  */

 enum
@@ -188,11 +199,11 @@ test_call (CLIENT *clnt, int proc, struct test_query query,
             proc, (unsigned long) timeout.tv_sec,
             (unsigned long) timeout.tv_usec);
   struct test_response response;
-  TEST_VERIFY_EXIT (clnt_call (clnt, proc,
+  TEST_VERIFY_CLEANUP (clnt_call (clnt, proc,
                                xdr_test_query, (void *) &query,
                                xdr_test_response, (void *) &response,
                                timeout)
-                    == RPC_SUCCESS);
+                    == RPC_SUCCESS, cleanup ());
   return response;
 }

@@ -218,11 +229,11 @@ test_call_flush (CLIENT *clnt)
      requested via the timeout_ms field.  */
   if (test_verbose)
     printf ("info: flushing pending queries\n");
-  TEST_VERIFY_EXIT (clnt_call (clnt, PROC_RESET_SEQ,
+  TEST_VERIFY_CLEANUP (clnt_call (clnt, PROC_RESET_SEQ,
                                (xdrproc_t) xdr_void, NULL,
                                (xdrproc_t) xdr_void, NULL,
                                ((struct timeval) { 5, 0 }))
-                    == RPC_SUCCESS);
+                    == RPC_SUCCESS, cleanup ());
 }

 /* Return the number seconds since an arbitrary point in time.  */
@@ -236,7 +247,7 @@ get_ticks (void)
   }
   {
     struct timeval tv;
-    TEST_VERIFY_EXIT (gettimeofday (&tv, NULL) == 0);
+    TEST_VERIFY_CLEANUP (gettimeofday (&tv, NULL) == 0, cleanup ());
     return tv.tv_sec + tv.tv_usec * 1e-6;
   }
 }
@@ -259,7 +270,7 @@ test_udp_server (int port)
   CLIENT *clnt = clntudp_create (&sin, PROGNUM, VERSNUM,
                                  (struct timeval) { 1, 500 * 1000 },
                                  &sock);
-  TEST_VERIFY_EXIT (clnt != NULL);
+  TEST_VERIFY_CLEANUP (clnt != NULL, cleanup ());

   /* Basic call/response test.  */
   struct test_response response = test_call
@@ -363,11 +374,11 @@ test_udp_server (int port)
       test_call_flush (clnt);
     }

-  TEST_VERIFY_EXIT (clnt_call (clnt, PROC_EXIT,
+  TEST_VERIFY_CLEANUP (clnt_call (clnt, PROC_EXIT,
                                (xdrproc_t) xdr_void, NULL,
                                (xdrproc_t) xdr_void, NULL,
                                ((struct timeval) { 5, 0 }))
-                    == RPC_SUCCESS);
+                    == RPC_SUCCESS, cleanup ());
   clnt_destroy (clnt);
 }

@@ -381,7 +392,7 @@ do_test (void)
   TEST_VERIFY_EXIT (transport != NULL);
   TEST_VERIFY (svc_register (transport, PROGNUM, VERSNUM, server_dispatch, 0));

-  pid_t pid = xfork ();
+  pid = xfork ();
   if (pid == 0)
     {
       svc_run ();
diff --git a/support/check.h b/support/check.h
index 77d1d1e14d..5f090dae3e 100644
--- a/support/check.h
+++ b/support/check.h
@@ -64,6 +64,18 @@ __BEGIN_DECLS
         (1, __FILE__, __LINE__, #expr);                         \
   })

+/* Record a test failure and run CLEANUP if EXPR evaluates to false.  */
+#define TEST_VERIFY_CLEANUP(expr, cleanup)                      \
+  ({                                                            \
+    if (expr)                                                   \
+      ;                                                         \
+    else                                                        \
+      {                                                         \
+        support_test_verify_impl (__FILE__, __LINE__, #expr);   \
+        cleanup;                                                \
+      }                                                         \
+  })
+


 int support_print_failure_impl (const char *file, int line,
--
2.21.1

Re: [RFC] sunrpc: Properly cleanup if tst-udp-timeout fails

[Lucas A. M. Magalhaes]

Seems good to me.

Lucas A. M. Magalhaes

Re: [RFC] sunrpc: Properly cleanup if tst-udp-timeout fails

[Florian Weimer]

* Matheus Castanho:

> I recently noted some orphan processes left on a host after failed
> sunrpc/tst-udp-timeout.c runs.  More info on the commit message.
>
> Even though the test infra has support for a cleanup_function, it
> is only called when the test times out.  The same happens for the code
> to kill the whole process group (both on the signal handler in
> support/support_test_main.c).  So I ended up adding a cleanup function
> to be called right before exit() to terminate the child.
>
> I also considered using a function with __attribute__((destructor))
> to perform the cleanup, but adding a new TEST_VERIFY_* macro seemed
> to be more reusable by other tests if needed.
>
> Is this the best way to handle this issue?

I'm not sure.  I think other processes have the same issue, and having
a generic solution would be nice.

We could perhaps add a fork wrapper that uses prctl with
PR_SET_PDEATHSIG, at least on Linux.  It's not possible to transfer
the cleanup to the outmost monitor process because the test failure is
only noticed after waitpid returns, at which point the process group
no longer exists.  Using a PID namespace would probably work better,
but it requires privileges (either root or at last user namespace
creation, as usual).

For the concrete patch, I think TEST_VERIFY_CLEANUP is confusing
because it does not perform the cleanup in both cases, and also the
exit function call is now obscured.  It may be better to replace
TEST_VERIFY_EXIT with TEST_VERIFY.  Or isolate the client in a
subprocess, too, so that the kill/waitpid sequence for the server
subprocess runs even if the client process failed (but that makes it
harder to debug the client code).  Just installing an atexit handler
may also be an option and acceptable in this case.

Thanks,
Florian

[PATCH] sunrpc: Properly clean up if tst-udp-timeout fails

[Matheus Castanho]

Hi Florian,

I have experimented with the options you listed above, but will probably
leave them for a future patch, as a generic solution will likely require more
careful consideration.

For the moment I dropped the extra TEST_VERIFY macro and fixed this single
test using the atexit() approach you pointed. I think it is preferable to
removing the TEST_VERIFY_EXIT calls, as without them we would have to run
the entire test (17-25 secs) even if an error ocurred in the beginning.
Also, I don't think we need to add more complexity to this test (e.g.
moving the client to a subprocess), as it already has a lot going on, so
I opted for this simpler approach.

The updated patch is below.

Thanks,
Matheus Castanho

--- 8< ---

The macro TEST_VERIFY_EXIT is used several times on
sunrpc/tst-udp-timeout to exit the test if a condition evaluates to
false. The side effect is that the code to terminate the RPC server
process is not executed when the program calls exit(), so that
sub-process stays alive.

This commit registers a clean up function with atexit() to kill the
server process before exiting the main program.
---
 sunrpc/tst-udp-timeout.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/sunrpc/tst-udp-timeout.c b/sunrpc/tst-udp-timeout.c
index 8d45365b23..de2fc3bf8c 100644
--- a/sunrpc/tst-udp-timeout.c
+++ b/sunrpc/tst-udp-timeout.c
@@ -29,6 +29,9 @@
 #include <sys/socket.h>
 #include <time.h>
 #include <unistd.h>
+#include <stdlib.h>
+
+pid_t pid;
 
 /* Test data serialization and deserialization.   */
 
@@ -177,6 +180,14 @@ server_dispatch (struct svc_req *request, SVCXPRT *transport)
     }
 }
 
+/* Function to be called before exit() to make sure the
+ * server process is properly killed.  */
+static void
+kill_server (void)
+{
+  kill(pid, SIGTERM);
+}
+
 /* Implementation of the test client.  */
 
 static struct test_response
@@ -381,12 +392,13 @@ do_test (void)
   TEST_VERIFY_EXIT (transport != NULL);
   TEST_VERIFY (svc_register (transport, PROGNUM, VERSNUM, server_dispatch, 0));
 
-  pid_t pid = xfork ();
+  pid = xfork ();
   if (pid == 0)
     {
       svc_run ();
       FAIL_EXIT1 ("supposed to be unreachable");
     }
+  atexit(kill_server);
   test_udp_server (transport->xp_port);
 
   int status;
-- 
2.21.1

Re: [PATCH] sunrpc: Properly clean up if tst-udp-timeout fails

[Florian Weimer]

* Matheus Castanho:

> Hi Florian,
>
> I have experimented with the options you listed above, but will probably
> leave them for a future patch, as a generic solution will likely require more
> careful consideration.
>
> For the moment I dropped the extra TEST_VERIFY macro and fixed this single
> test using the atexit() approach you pointed. I think it is preferable to
> removing the TEST_VERIFY_EXIT calls, as without them we would have to run
> the entire test (17-25 secs) even if an error ocurred in the beginning.
> Also, I don't think we need to add more complexity to this test (e.g.
> moving the client to a subprocess), as it already has a lot going on, so
> I opted for this simpler approach.
>
> The updated patch is below.

Thanks, I like this approach.  Some nits below.

> The macro TEST_VERIFY_EXIT is used several times on
> sunrpc/tst-udp-timeout to exit the test if a condition evaluates to
> false. The side effect is that the code to terminate the RPC server
> process is not executed when the program calls exit(), so that
> sub-process stays alive.
>
> This commit registers a clean up function with atexit() to kill the
> server process before exiting the main program.

GNU style doesn't use () after function names, so this should just be
atexit.

> ---
>  sunrpc/tst-udp-timeout.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/sunrpc/tst-udp-timeout.c b/sunrpc/tst-udp-timeout.c
> index 8d45365b23..de2fc3bf8c 100644
> --- a/sunrpc/tst-udp-timeout.c
> +++ b/sunrpc/tst-udp-timeout.c
> @@ -29,6 +29,9 @@
>  #include <sys/socket.h>
>  #include <time.h>
>  #include <unistd.h>
> +#include <stdlib.h>
> +
> +pid_t pid;

Please make this variable static.  It should also be called
server_pid, to make its purpose clear.

> +/* Function to be called before exit() to make sure the
> + * server process is properly killed.  */

Style nit: exit instead of exit(), comments should not be indented
with *, like this:

/* Function to be called before exit to make sure the
   server process is properly killed.  */

[PATCH v2] sunrpc: Properly clean up if tst-udp-timeout fails

[Matheus Castanho]

Nits fixed.

Thanks,
Matheus Castanho

--- 8< ---

The macro TEST_VERIFY_EXIT is used several times on
sunrpc/tst-udp-timeout to exit the test if a condition evaluates to
false. The side effect is that the code to terminate the RPC server
process is not executed when the program calls exit, so that
sub-process stays alive.

This commit registers a clean up function with atexit to kill the
server process before exiting the main program.
---
Changes from v1:
    - Rename pid variable and make it static
    - Fix styling issues

 sunrpc/tst-udp-timeout.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/sunrpc/tst-udp-timeout.c b/sunrpc/tst-udp-timeout.c
index 8d45365b23..36752447c0 100644
--- a/sunrpc/tst-udp-timeout.c
+++ b/sunrpc/tst-udp-timeout.c
@@ -29,6 +29,9 @@
 #include <sys/socket.h>
 #include <time.h>
 #include <unistd.h>
+#include <stdlib.h>
+
+static pid_t server_pid;
 
 /* Test data serialization and deserialization.   */
 
@@ -177,6 +180,14 @@ server_dispatch (struct svc_req *request, SVCXPRT *transport)
     }
 }
 
+/* Function to be called before exit to make sure the
+   server process is properly killed.  */
+static void
+kill_server (void)
+{
+  kill(server_pid, SIGTERM);
+}
+
 /* Implementation of the test client.  */
 
 static struct test_response
@@ -381,16 +392,17 @@ do_test (void)
   TEST_VERIFY_EXIT (transport != NULL);
   TEST_VERIFY (svc_register (transport, PROGNUM, VERSNUM, server_dispatch, 0));
 
-  pid_t pid = xfork ();
-  if (pid == 0)
+  server_pid = xfork ();
+  if (server_pid == 0)
     {
       svc_run ();
       FAIL_EXIT1 ("supposed to be unreachable");
     }
+  atexit(kill_server);
   test_udp_server (transport->xp_port);
 
   int status;
-  xwaitpid (pid, &status, 0);
+  xwaitpid (server_pid, &status, 0);
   TEST_VERIFY (WIFEXITED (status) && WEXITSTATUS (status) == EXIT_MARKER);
 
   SVC_DESTROY (transport);
-- 
2.21.1

Re: [PATCH v2] sunrpc: Properly clean up if tst-udp-timeout fails

[Florian Weimer]

* Matheus Castanho:

> Nits fixed.

This version is fine.  Do you need someone to commit it for you?

Thanks,
Florian

Re: [PATCH v2] sunrpc: Properly clean up if tst-udp-timeout fails

[Matheus Castanho]

On 2/12/20 11:14 AM, Florian Weimer wrote:
> * Matheus Castanho:
> 
>> Nits fixed.
> 
> This version is fine.  Do you need someone to commit it for you?

Yes, I do. But Tulio (cc) will do it for me. Thanks!

> 
> Thanks,
> Florian
> 

Re: [PATCH v2] sunrpc: Properly clean up if tst-udp-timeout fails

[Andreas Schwab]

On Feb 12 2020, Matheus Castanho wrote:

> @@ -177,6 +180,14 @@ server_dispatch (struct svc_req *request, SVCXPRT *transport)
>      }
>  }
>  
> +/* Function to be called before exit to make sure the
> +   server process is properly killed.  */
> +static void
> +kill_server (void)
> +{
> +  kill(server_pid, SIGTERM);

Style: space before paren.

> @@ -381,16 +392,17 @@ do_test (void)
>    TEST_VERIFY_EXIT (transport != NULL);
>    TEST_VERIFY (svc_register (transport, PROGNUM, VERSNUM, server_dispatch, 0));
>  
> -  pid_t pid = xfork ();
> -  if (pid == 0)
> +  server_pid = xfork ();
> +  if (server_pid == 0)
>      {
>        svc_run ();
>        FAIL_EXIT1 ("supposed to be unreachable");
>      }
> +  atexit(kill_server);

Likewise.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH v2] sunrpc: Properly clean up if tst-udp-timeout fails

[Florian Weimer]

* Matheus Castanho:

> On 2/12/20 11:14 AM, Florian Weimer wrote:
>> * Matheus Castanho:
>> 
>>> Nits fixed.
>> 
>> This version is fine.  Do you need someone to commit it for you?
>
> Yes, I do. But Tulio (cc) will do it for me. Thanks!

Okay, please also incorporate Andreas' feedback.  No need to post a new
version of the patch.

Thanks,
Florian

Re: [PATCH v2] sunrpc: Properly clean up if tst-udp-timeout fails

[Matheus Castanho]

On 2/12/20 11:38 AM, Florian Weimer wrote:
> * Matheus Castanho:
> 
>> On 2/12/20 11:14 AM, Florian Weimer wrote:
>>> * Matheus Castanho:
>>>
>>>> Nits fixed.
>>>
>>> This version is fine.  Do you need someone to commit it for you?
>>
>> Yes, I do. But Tulio (cc) will do it for me. Thanks!
> 
> Okay, please also incorporate Andreas' feedback.  No need to post a new
> version of the patch.

Ok, no problem.

--
Matheus Castanho
> 
> Thanks,
> Florian
> 

Re: [PATCH v2] sunrpc: Properly clean up if tst-udp-timeout fails

[Tulio Magno Quites Machado Filho]

Andreas Schwab <schwab@suse.de> writes:

> On Feb 12 2020, Matheus Castanho wrote:
>
>> @@ -177,6 +180,14 @@ server_dispatch (struct svc_req *request, SVCXPRT *transport)
>>      }
>>  }
>>  
>> +/* Function to be called before exit to make sure the
>> +   server process is properly killed.  */
>> +static void
>> +kill_server (void)
>> +{
>> +  kill(server_pid, SIGTERM);
>
> Style: space before paren.

Fixed.

>> @@ -381,16 +392,17 @@ do_test (void)
>>    TEST_VERIFY_EXIT (transport != NULL);
>>    TEST_VERIFY (svc_register (transport, PROGNUM, VERSNUM, server_dispatch, 0));
>>  
>> -  pid_t pid = xfork ();
>> -  if (pid == 0)
>> +  server_pid = xfork ();
>> +  if (server_pid == 0)
>>      {
>>        svc_run ();
>>        FAIL_EXIT1 ("supposed to be unreachable");
>>      }
>> +  atexit(kill_server);
>
> Likewise.

Fixed.

Pushed as f34c4d0f10ed09500d5f0ebd473c3f37ce4989d7.

Thanks!

-- 
Tulio Magno