There is some precedent for this in the kernel, but not a whole lot. It does make clear to both the human and machine (e.g., Coverity) reader that ignoring the return value is intentional.

Coverity complained here that in the old code oldval was restored in one path but not in another. This revised code does have the advantage (besides quieting Coverity) that at no time is an invalid value for the timeout stored in the sysctl variable.

There was a possibility here that the (un-casted) expression idx * 2048 could overflow the 32-bits of that expression.

strncpy ensures all the bytes in the returned structure are initialized. strlcpy only ensures that the string is NUL terminated.

Same as above. I think strncpy is more correct.

Coverity was wrong to complain about that. The old code was correct. The new code is more correct, though, for the reason you noted. We use the timeout_period for new I/Os. It's unlikely to make any difference for really long timeouts and nvme drives are fast enough it would be hard to set one too slow, but it's better to have the code be more correct.

Why is this needed? If csw is NULL, that should be a panic...

True. Coverity was complaining because strncpy() doesn't guarantee NUL termination. I thought I had seen it treated as a NUL terminated string in other parts of the code, so I thought strlcpy() would be appropriate. Upon rechecking, I see that it is used as a string within nvmecontrol, but it is copied with a strndup(), thus allowing for the possibility of non-NUL termination. Restoring the strncpy() and explicitly storing a NUL in the last byte would both quiet Coverity and be correct, as the field is over-allocated by 1 byte.

Would you agree?

The csw is assigned to the return from dev_refthread() on line 119. dev_refthread() can return NULL and in fact that is checked for in other places within the kernel, so Coverity kindly suggested that the it should also be done here.

I could explicitly panic after that assignment if the value is NULL. I know some prefer to panic on the dereference rather than put in the extra code of a test and explicit panic.

Thoughts?

Missing closing paren.

Missing closing paren.

Space before =.

FreeBSD style doesn't initialize variables in their declaration, but I won't complain, because I disagree with that. :)

You should also return here if req->newptr == NULL. In that case, the caller is reading the current value, but not setting a new one. For that to work, initialize newval with ctrlr->timeout_period, as it was before.prefer this.

If you keep the panic above, don't test for NULL here.

I believe Coverity runs on builds with INVARIANTS and therefore KASSERTs. Maybe KASSERT(csw != NULL, ("insert message")); would appease Coverity while keeping the code clean (i.e. simply panic on dereference).

Also not the only mistake there on that line. Careless.

My excuse is that the old code also initialized the variable in the declaration. ;-)

Errggghhh. Forgot to take that test out.

I didn't think Coverity ran with INVARIANTS; I'm almost positive I've seen issues flagged that would not have been had a KASSERT been in effect. Although, that might be a difference between Coverity runs internal to $JOB and the FreeBSD scans.

You can get the same effect by add ull to 2048.

True. I thought the cast to uint64_t was perhaps more easily understood since the declaration of offset is just a few lines above and one wouldn't have to do the mental equivalence check of ULL == uint64_t. Is there a preference for one method over the other?

ull is generally preferred because it's less invasive than the cast.