diff mbox

sunrpc: Properly clean up if tst-udp-timeout fails

Message ID 20200212131423.27742-1-msc@linux.ibm.com
State Superseded
Delegated to: Florian Weimer
Headers show

Commit Message

Matheus Castanho Feb. 12, 2020, 1:14 p.m. UTC
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(-)

Comments

Florian Weimer Feb. 12, 2020, 1:16 p.m. UTC | #1
* 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.  */
diff mbox

Patch

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;