On Sat, Apr 23, 2022 at 02:01:11PM -0400, Ian Cowan wrote: > Refactor the cedrus_open() function so that there is only one exit to > the function instead of 2. This prevents a future change from preventing > the mutex from being unlocked after a successful exit. Future proofing code is a complicated thing because the future is hard to predict. But one rule is that normally the success path gets tested. No one is going to forget to drop the lock on the success path. Or if we do forget, it likely will get caught quickly and it will be easy to bisect. Generally it's best to keep the success path and the failure path as separate as possible. Avoid interleaving. It's more readable. If there is no cleanup on failure then it's okay to have: unlock: unlock(foo); return ret; But once you have cleanup then put that in a separate cleanup block. As well Smatch can recognize a typical kernel cleanup block so if you write it in a weird way then you're disabling the static checker warning for missing error codes. I probably am going to write more checks which are specific to cleanup blocks in the future. regards, dan carpenter
