CID 1009492: Logically dead code in sys/cam/scsi/scsi_xpt.c

[dab]

Summary

In probedone(), for the PROBE_REPORT_LUNS case, all paths
that fall to the bottom of the case free() the lp variable and
set lp to NULL, so the test for a non-NULL value of lp and call
to free() if true is dead code as the test can never be true.
Fix by eliminating the whole if statement.

I will mention that I considered just eliminating the if test and
leaving the call to free() as that is harmless on a NULL pointer.
That would somewhat "future-proof" the code in case some path is
later added that doesn't free() the pointer. I happened upon
this CID after looking at some code coverage analysis that pointed
out that the call to free() was never being executed. Calling
free() unconditionally would get that coverage. Two considerations
prevented me from doing that: 1) the rest of the code in file seems
to always test for non-NULL before calling free() and 2) there
would be a bit of needless overhead from the call to free(). Any
thoughts on generally preferred practice?

Adding as subscribers people who have been in the area in the current
version of the code. I'd appreciate review from some or all of you.

Test Plan

Code review.

[cem]

Two ideas (just spitballing) re: forward compatibility.

1. Remove the other simple frees / NULL reinitialization, and keep this (unconditional) free(), or
2. Replace the removed conditional free with a KASSERT that lp is NULL

If either of those sound like better ideas to you, I'm happy with any option (including 3, this patch as-is).

[dab]

Take @cem's suggestion to use a KASSERT() to ensure that lp is indeed NULL on the affected code path.

[cem]

sys/cam/scsi/scsi_xpt.c

1401 ↗

(On Diff #53663)

I tend to print the pointer in these things because inevitably it's in some register that a panic will trample. But it's possible lp itself is uninteresting and any regression could just look backwards at the code.

[mav]

I think I would remove this code completely, together with above NULL assignments. One way or another, this check is at least incomplete, since just in if block before execution can go to out bypassing this check.

[cem]

In D19109#408718, @mav wrote:

I think I would remove this code completely, together with above NULL assignments. One way or another, this check is at least incomplete, since just in if block before execution can go to out bypassing this check.

Hm, I guess the assertion could be moved above the if block. I am also ok with removing it completely.

[dab]

In D19109#408725, @cem wrote:

In D19109#408718, @mav wrote:

I think I would remove this code completely, together with above NULL assignments. One way or another, this check is at least incomplete, since just in if block before execution can go to out bypassing this check.

Hm, I guess the assertion could be moved above the if block. I am also ok with removing it completely.

I'd be OK with moving the assertion to just after the closing brace of the if block above. For this cascaded if..then..else block, either the code does a goto out; or it falls through to the cleanup here. If it falls through to the cleanup, then it is expected that lp has already been cleaned up. So, the KASSERT() just ensures that in the future someone doesn't code up a new path where it falls through but has not done the appropriate cleanup. I think the KASSERT() serves a valid purpose.

An alternative approach would be to retain the free() call where it was and not do the free(lp, M_CAMXPT); lp = NULL; code in the fall through cases above (i.e., modify them to remove that code), letting the single test (if, indeed, we want to bother with a test at all) and free() call at the bottom take care of it. In that case, we wouldn't have the KASSERT().

I'm really open to either approach.

sys/cam/scsi/scsi_xpt.c

1401 ↗

(On Diff #53663)

Not a bad idea in general, but I think in this case it really doesn't matter. What this is verifying is that, for a path that falls through here rather than doing a goto out;, that lp has been cleaned up. It doesn't really matter what the value of lp is, just that it wasn't de-allocated.