On 3/23/23 15:29, Eric Blake wrote:
> 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.
Yes, "g_steal_pointer". (Either you or Dan has mentioned it recently.)
>
>>
>> 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 <[email protected]>
>
Thanks!
_______________________________________________
Libguestfs mailing list
[email protected]
https://listman.redhat.com/mailman/listinfo/libguestfs
