[Libguestfs] [libnbd PATCH v3 23/29] CONNECT_COMMAND.START: centralize resource release

[Laszlo Ersek]

This patch implements the same ideas as those presented in the prior patch

  socket activation: centralize resource release

of this same series, only for CONNECT_COMMAND.START.

The one temporary resource we have here is sv[1].

Similarly to the above-cited patch, the present patch also exposes a
resource leak on the error path. Namely, when any construction step fails
in the parent after fork(), the child process is leaked. (The child
process may well be killed and reaped once the application decides to call
nbd_close(), but until then, after the CONNECT_COMMAND.START handler
returns with failure, unusable (half-constructed) resources linger around
indefinitely. A library API should either fully succeed or fully fail.) We
are going to plug this leak in a subsequent patch.

Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---

Notes:
    context:-W

 generator/states-connect.c | 52 +++++++++++---------
 1 file changed, 30 insertions(+), 22 deletions(-)

diff --git a/generator/states-connect.c b/generator/states-connect.c
index d2476dc11c60..bd78f890a2c5 100644
--- a/generator/states-connect.c
+++ b/generator/states-connect.c
@@ -238,96 +238,104 @@  CONNECT_TCP.NEXT_ADDRESS:
   return 0;
 
  CONNECT_COMMAND.START:
+  enum state next;
   int sv[2];
   pid_t pid;
   int flags;
+  struct socket *sock;
 
   assert (!h->sock);
   assert (h->argv.ptr);
   assert (h->argv.ptr[0]);
+
+  next = %.DEAD;
+
   if (nbd_internal_socketpair (AF_UNIX, SOCK_STREAM, 0, sv) == -1) {
-    SET_NEXT_STATE (%.DEAD);
     set_error (errno, "socketpair");
-    return 0;
+    goto done;
   }
 
   /* A process is effectively in an unusable state if any of STDIN_FILENO
    * (fd#0), STDOUT_FILENO (fd#1) and STDERR_FILENO (fd#2) don't exist. If they
    * exist however, then the socket pair created above will not intersect with
    * the fd set { 0, 1, 2 }. This is relevant for the child-side dup2() logic
    * below.
    */
   assert (sv[0] > STDERR_FILENO);
   assert (sv[1] > STDERR_FILENO);
 
   pid = fork ();
   if (pid == -1) {
-    SET_NEXT_STATE (%.DEAD);
     set_error (errno, "fork");
-    close (sv[0]);
-    close (sv[1]);
-    return 0;
+    goto close_socket_pair;
   }
+
   if (pid == 0) {         /* child - run command */
     if (close (sv[0]) == -1) {
       nbd_internal_fork_safe_perror ("close");
       _exit (126);
     }
     if (dup2 (sv[1], STDIN_FILENO) == -1 ||
         dup2 (sv[1], STDOUT_FILENO) == -1) {
       nbd_internal_fork_safe_perror ("dup2");
       _exit (126);
     }
     NBD_INTERNAL_FORK_SAFE_ASSERT (sv[1] != STDIN_FILENO);
     NBD_INTERNAL_FORK_SAFE_ASSERT (sv[1] != STDOUT_FILENO);
     if (close (sv[1]) == -1) {
       nbd_internal_fork_safe_perror ("close");
       _exit (126);
     }
 
     /* Restore SIGPIPE back to SIG_DFL. */
     if (signal (SIGPIPE, SIG_DFL) == SIG_ERR) {
       nbd_internal_fork_safe_perror ("signal");
       _exit (126);
     }
 
     execvp (h->argv.ptr[0], h->argv.ptr);
     nbd_internal_fork_safe_perror (h->argv.ptr[0]);
     if (errno == ENOENT)
       _exit (127);
     else
       _exit (126);
   }
 
-  /* Parent. */
-  close (sv[1]);
-
-  h->pid = pid;
-
-  /* Only the parent-side end of the socket pair must be set to non-blocking,
+  /* Parent.
+   *
+   * Only the parent-side end of the socket pair must be set to non-blocking,
    * because the child may not be expecting a non-blocking socket.
    */
   flags = fcntl (sv[0], F_GETFL, 0);
   if (flags == -1 ||
       fcntl (sv[0], F_SETFL, flags|O_NONBLOCK) == -1) {
-    SET_NEXT_STATE (%.DEAD);
     set_error (errno, "fcntl");
-    close (sv[0]);
-    return 0;
+    goto close_socket_pair;
   }
 
-  h->sock = nbd_internal_socket_create (sv[0]);
-  if (!h->sock) {
-    SET_NEXT_STATE (%.DEAD);
+  sock = nbd_internal_socket_create (sv[0]);
+  if (!sock)
     /* nbd_internal_socket_create() calls set_error() internally */
-    close (sv[0]);
-    return 0;
-  }
+    goto close_socket_pair;
+
+  /* Commit. */
+  h->pid = pid;
+  h->sock = sock;
 
   /* The sockets are connected already, we can jump directly to
    * receiving the server magic.
    */
-  SET_NEXT_STATE (%^MAGIC.START);
+  next = %^MAGIC.START;
+
+  /* fall through, for releasing the temporaries */
+
+close_socket_pair:
+  if (next == %.DEAD)
+    close (sv[0]);
+  close (sv[1]);
+
+done:
+  SET_NEXT_STATE (next);
   return 0;
 
 } /* END STATE MACHINE */