[Swan-dev] code for impossible situations
D. Hugh Redelmeier
hugh at mimosa.com
Mon Aug 31 14:31:17 UTC 2020
If a case is impossible, but you feel unsure about it, use passert. Then
you'll know.
Actually handling an impossible case requires or suggests that all related
code should handle it too. And that multiplies complexity and the chance
for bugs.
This code will never be exercised by our test suite. So that code might
well have bugs. It will burn cycles and create confusion in code readers.
Handling an impossible case some places and not others raises questions
and confusion too.
On the other hand, a passert reduces confusion. (If it fires, the result
is educational.)
I'm trying to understand dev_exist_check and its callers and I think this
is an example of the problem
function dev_exists_check takes a "dev_name" string parameter.
- the code handles a null dev_name by treating it a like an unknown
device. With explicit code.
- if the device is unknown (and another condition that should be
irrelevant), the code reports errno, on the assumption that the cause
for being unknown is a failed call.
- but wait: there was no call in the case of a NULL. BUG.
- but wait: I have a hunch that dev_name is never NULL. So this is only a
theoretical bug. Evidence: I put a pexpect in, ran the whole test suite,
and never once had this pexpect fire. This is not proof.
The code path for dev_name == NULL
(1) has a bug
(2) is not tested in our test suite
(3) makes the code harder to understand.
I'm going to replace it with a passert and simplify the code.
There are still mysteries. find_xfrmi_interface explicitly checks for
NULL before it calls dev_exist_check. But not all of the code in that
routine seems to handle the NULL properly. It looks to me (with less
evidence) that
(1) this too is a bug
(2) that is not tested in our test suite
(3) makes the code harder to understand.
(4) the combination of the two is magnifies the problem.
Generally: of all the callers of dev_exist_check, which ones make any
sense with a NULL dev_name?
(Please don't fix this. I've got the code on the operating table here.
I'm painfully swimming upstream.)
Null references: Tony Hoare's Billion Dollar Mistake.
<https://www.youtube.com/watch?v=ybrQvs4x0Ps>
More information about the Swan-dev
mailing list