Re: [Libguestfs] [libnbd PATCH v3 05/19] socket activation: centralize resource release

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&gt;