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. _______________________________________________ ipxe-devel mailing list ipxe-devel@lists.ipxe.org https://lists.ipxe.org/mailman/listinfo.cgi/ipxe-devel