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

[Michael Brown]

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.


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

Thanks,

Michael