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> _______________________________________________ Swan-dev mailing list [email protected] https://lists.libreswan.org/mailman/listinfo/swan-dev
