The CONNECT_SA.START handler constructs several resources; some of those
are needed permanently if the entire handler succeeds, while others are
needed only temporarily, for constructing the permanent objects.
Currently, the various error branches in the handler deal with resource
release individually, leading to code duplication and dispersed
responsibilities; if we want to construct a new resource, then, under the
existent pattern, we'll have to modify multiple error branches to release
it as well.
In my opinion, the best pattern for dealing with this scenario is the
following structure:
- Place cascading error handling labels at the end of the function -- an
error handling slide in effect. The order of destruction on this slide
is the inverse of construction.
- Whenever an operation fails mid-construction, enter the slide such that
precisely the thus far constructed objects be freed. Note that the goto
statements and the destination labels nest properly.
- The very last construction step needs no rollback, because the last
successful step completes the entire function / handler. When this
happens, *commit* by (a) outputting the permanent resources, (b) setting
a top-level "final result" variable to "success" (aka "ownership of
permanent resources transferred to the caller"), and (c) falling through
to the slide. On the slide, use the "final result" variable to skip the
release of those resources that have been output as permanent ones.
This way, each resource is freed in exactly one location, and whenever an
error occurs, no resource is even *checked* for rollback if its
construction could not be tried, or completed, in the first place. The
introduction of a new resource needs one properly placed construction step
and one matchingly placed error handling label.
Rich notes that "state machine labels match the basic regexp pattern:
^ \\([A-Z0-9][A-Z0-9_\\.]*\\):$
(see generator/state_machine_generator.ml), and because of the
all-capitalization of state names, they won't match the lower case
ordinary C labels we're using here".
The restructuring highlights the following leak: whenever any construction
step after bind() fails, the UNIX domain socket address (i.e., the
pathname in the filesystem) is leaked. (The filesystem object may well be
removed once the application decides to call nbd_close(), but until then,
after the CONNECT_SA.START handler returns with failure, unusable
(half-constructed) resources linger around indefinitely. A library API
should either fully succeed or fully fail.) We'll plug the leak later in
this series.
Signed-off-by: Laszlo Ersek <ler...@redhat.com>
Reviewed-by: Richard W.M. Jones <rjo...@redhat.com>
---
Notes:
v4:
- pick up Rich's R-b
- remark in the commit message that the generated state machine labels,
and the manual labels introduced by the patch, belong to distinct
"namespaces" [Rich]
- cumbersome rebase conflict resolution near the
prepare_socket_activation_environment() call site, due to keeping
set_error() internal to that callee, in patch "socket activation:
clean up responsibilities of prep.sock.act.env.()"
context:-W
generator/states-connect-socket-activation.c | 85 +++++++++++---------
1 file changed, 49 insertions(+), 36 deletions(-)
diff --git a/generator/states-connect-socket-activation.c
b/generator/states-connect-socket-activation.c
index e3f7d05670b0..b20c3f1f10bb 100644
--- a/generator/states-connect-socket-activation.c
+++ b/generator/states-connect-socket-activation.c
@@ -97,132 +97,145 @@ prepare_socket_activation_environment (string_vector *env)
STATE_MACHINE {
CONNECT_SA.START:
+ enum state next;
+ char *tmpdir;
+ char *sockpath;
int s;
struct sockaddr_un addr;
string_vector env;
pid_t pid;
assert (!h->sock);
assert (h->argv.ptr);
assert (h->argv.ptr[0]);
+ next = %.DEAD;
+
/* Use /tmp instead of TMPDIR because we must ensure the path is
* short enough to store in the sockaddr_un. On some platforms this
* may cause problems so we may need to revisit it. XXX
*/
- h->sact_tmpdir = strdup ("/tmp/libnbdXXXXXX");
- if (h->sact_tmpdir == NULL) {
- SET_NEXT_STATE (%.DEAD);
+ tmpdir = strdup ("/tmp/libnbdXXXXXX");
+ if (tmpdir == NULL) {
set_error (errno, "strdup");
- return 0;
+ goto done;
}
- if (mkdtemp (h->sact_tmpdir) == NULL) {
- SET_NEXT_STATE (%.DEAD);
+
+ if (mkdtemp (tmpdir) == NULL) {
set_error (errno, "mkdtemp");
- /* Avoid cleanup in nbd_close. */
- free (h->sact_tmpdir);
- h->sact_tmpdir = NULL;
- return 0;
+ goto free_tmpdir;
}
- if (asprintf (&h->sact_sockpath, "%s/sock", h->sact_tmpdir) == -1) {
- SET_NEXT_STATE (%.DEAD);
+ if (asprintf (&sockpath, "%s/sock", tmpdir) == -1) {
set_error (errno, "asprintf");
- return 0;
+ goto rmdir_tmpdir;
}
s = nbd_internal_socket (AF_UNIX, SOCK_STREAM, 0, false);
if (s == -1) {
- SET_NEXT_STATE (%.DEAD);
set_error (errno, "socket");
- return 0;
+ goto free_sockpath;
}
addr.sun_family = AF_UNIX;
- memcpy (addr.sun_path, h->sact_sockpath, strlen (h->sact_sockpath) + 1);
+ memcpy (addr.sun_path, sockpath, strlen (sockpath) + 1);
if (bind (s, (struct sockaddr *) &addr, sizeof addr) == -1) {
- SET_NEXT_STATE (%.DEAD);
- set_error (errno, "bind: %s", h->sact_sockpath);
- close (s);
- return 0;
+ set_error (errno, "bind: %s", sockpath);
+ goto close_socket;
}
if (listen (s, SOMAXCONN) == -1) {
- SET_NEXT_STATE (%.DEAD);
set_error (errno, "listen");
- close (s);
- return 0;
+ goto close_socket;
}
- if (prepare_socket_activation_environment (&env) == -1) {
- SET_NEXT_STATE (%.DEAD);
+ if (prepare_socket_activation_environment (&env) == -1)
/* prepare_socket_activation_environment() calls set_error() internally */
- close (s);
- return 0;
- }
+ goto close_socket;
pid = fork ();
if (pid == -1) {
- SET_NEXT_STATE (%.DEAD);
set_error (errno, "fork");
- close (s);
- string_vector_empty (&env);
- return 0;
+ goto empty_env;
}
+
if (pid == 0) { /* child - run command */
if (s != FIRST_SOCKET_ACTIVATION_FD) {
if (dup2 (s, FIRST_SOCKET_ACTIVATION_FD) == -1) {
nbd_internal_fork_safe_perror ("dup2");
_exit (126);
}
if (close (s) == -1) {
nbd_internal_fork_safe_perror ("close");
_exit (126);
}
}
else {
/* We must unset CLOEXEC on the fd. (dup2 above does this
* implicitly because CLOEXEC is set on the fd, not on the
* socket).
*/
int flags = fcntl (s, F_GETFD, 0);
if (flags == -1) {
nbd_internal_fork_safe_perror ("fcntl: F_GETFD");
_exit (126);
}
if (fcntl (s, F_SETFD, (int)(flags & ~(unsigned)FD_CLOEXEC)) == -1) {
nbd_internal_fork_safe_perror ("fcntl: F_SETFD");
_exit (126);
}
}
char buf[32];
const char *v =
nbd_internal_fork_safe_itoa ((long) getpid (), buf, sizeof buf);
strcpy (&env.ptr[0][PREFIX_LENGTH], v);
/* Restore SIGPIPE back to SIG_DFL. */
if (signal (SIGPIPE, SIG_DFL) == SIG_ERR) {
nbd_internal_fork_safe_perror ("signal");
_exit (126);
}
environ = env.ptr;
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 (s);
- string_vector_empty (&env);
+ /* Parent -- we're done; commit. */
+ h->sact_tmpdir = tmpdir;
+ h->sact_sockpath = sockpath;
h->pid = pid;
h->connaddrlen = sizeof addr;
memcpy (&h->connaddr, &addr, h->connaddrlen);
- SET_NEXT_STATE (%^CONNECT.START);
+ next = %^CONNECT.START;
+
+ /* fall through, for releasing the temporaries */
+
+empty_env:
+ string_vector_empty (&env);
+
+close_socket:
+ close (s);
+
+free_sockpath:
+ if (next == %.DEAD)
+ free (sockpath);
+
+rmdir_tmpdir:
+ if (next == %.DEAD)
+ rmdir (tmpdir);
+
+free_tmpdir:
+ if (next == %.DEAD)
+ free (tmpdir);
+
+done:
+ SET_NEXT_STATE (next);
return 0;
} /* END STATE MACHINE */
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
