[ipxe-devel] [PATCH] rndis: register netdev with MAC filled

[Roman Kagan]

On Mon, Jul 09, 2018 at 10:53:36AM +0100, Michael Brown wrote:
> On 09/07/18 07:31, Roman Kagan wrote:
> > <snip>
> > so the cleanup needed after a failed register_netdev is the same as
> > after a failed open, and I left err_register label where it was on
> > purpose.
> > 
> > While at this, I'm also curious what the reason is to add this
> > unreachable unregister_netdev?  I think I already saw this pattern in
> > other places in iPXE so I assume you didn't do it by mistake, did you?
> 
> OK, I see the issue.
> 
> The general error handling pattern in iPXE is:
> 
>    if ( ( rc = something_that_may_fail() ) != 0 )
>        goto err_something;
> 
> paired with
> 
>    undo_something();
>  err_something:
> 
> i.e. the error handling label goes immediately _after_ the cleanup code that
> is required only if the original action succeeded.  This allows for clean
> and predictable stacking of error handling paths:
> 
>    if ( ( rc = something_that_may_fail() ) != 0 )
>        goto err_something;
>    if ( ( rc = something_else_that_may_fail() ) != 0 )
>        goto err_something_else;
> 
>    ...
> 
>    undo_something_else();
>  err_something_else:
>    undo_something();
>  err_something:
> 
> The error handling stack should thus always be a mirror ordering of the main
> code path.
> 
> This gives the following properties:
> 
> - Consistency of error handling
> 
> - Clean diffs: a patch should never need to modify another code path's error
> cleanup code, or modify existing label names.
> 
> - Cleanup code cannot be accidentally omitted by a future patch, since the
> (unreachable) cleanup code is already present.

Thanks for the explanation, I'll try to follow this pattern in further
submissions.

> I don't want to have code that looks at first glance to be following this
> pattern but actually isn't (because the cleanup order doesn't mirror the
> main code path), since that increases the cognitive load for code
> maintenance.  I think the optimal solution is probably to split
> register_rndis() into two separate functions:
> 
>   http://git.ipxe.org/ipxe.git/commitdiff/05b979146

Yes this makes it easier to follow, indeed.

Thanks,
Roman.