9:29 a.m.
On Thu, Mar 23, 2023 at 01:10:02PM +0100, Laszlo Ersek wrote:
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.
Yes, this style has gronw on me as I've seen it in more places.
However,
--- 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;
I've also come to appreciate __attribute__((cleanup)) handling for
even less effort. If we decide to cross-port that from nbdkit (which
effectively limits libnbd to being build only by gcc and clang), we
could mark these two variables as cleanup and initialized to NULL,
then have even less code below because they get auto-freed on all
error paths where they were set.
I'm _not_ asking you do to that in this series (no need to respin and
fight even more rebase churn), so much as a question to Rich on
whether we think limiting libnbd to the same platforms as where nbdkit
currently compiles, instead of theoretically allowing libnbd to
compile in a wider set of compilers (although we haven't actually
tested that), is worth doing.
- /* Parent. */
- close (s);
- string_vector_empty (&env);
+ /* Parent -- we're done; commit. */
+ h->sact_tmpdir = tmpdir;
+ h->sact_sockpath = sockpath;
h->pid = pid;
If we do a followup series to introduce cleanup attributes, the
initial declaration of tmpdir and sockpath must be also initialized to
NULL; then here, where we transfer the values to permanent storage, we
must assign the temporaries back to NULL. Going one step further,
note that libvirt has copied ideas from the glib project of having
macros that make it obvious when you are doing transfer semantics -
changing ownership of a string from one variable to another by setting
the original to NULL, all in one line of code.
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);
With transfer semantics, you can unconditionally call free (sockpath)
here (without probing 'next') provided that 'sockpath' is initialized
to NULL, and then re-set back to NULL upon the commit portion
transferring the pointer to the permanent storage. What's more, if
you combine transfer semantics with attribute cleanup, the 'free
(sockpath)' is automatic on all exit paths without needing this goto
label or code.
+
+rmdir_tmpdir:
+ if (next == %.DEAD)
+ rmdir (tmpdir);
With transfer semantics, this code would still have to be conditional,
but the condition would change to 'if (tmpdir)' instead of depending
on 'next'.
+
+free_tmpdir:
+ if (next == %.DEAD)
+ free (tmpdir);
And with transfer semantics and attribute cleanup, this is another
label we could elide.
+
+done:
+ SET_NEXT_STATE (next);
return 0;
} /* END STATE MACHINE */
At any rate, while there are still more ways to rewrite this even more
compactly (if we decide we want to pull in attribute cleanup), your
refactor is fine for now.
Reviewed-by: Eric Blake <eblake(a)redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org