Changeset View
Standalone View
sys/dev/nvme/nvme_sysctl.c
| Show First 20 Lines • Show All 140 Lines • ▼ Show 20 Lines | nvme_sysctl_int_coal_threshold(SYSCTL_HANDLER_ARGS) | ||||
| return (0); | return (0); | ||||
| } | } | ||||
| static int | static int | ||||
| nvme_sysctl_timeout_period(SYSCTL_HANDLER_ARGS) | nvme_sysctl_timeout_period(SYSCTL_HANDLER_ARGS) | ||||
| { | { | ||||
| struct nvme_controller *ctrlr = arg1; | struct nvme_controller *ctrlr = arg1; | ||||
| uint32_t oldval = ctrlr->timeout_period; | uint32_t newval = ctrlr->timeout_period; | ||||
| int error = sysctl_handle_int(oidp, &ctrlr->timeout_period, 0, req); | int error = sysctl_handle_int(oidp, &newval, 0, req); | ||||
vangyzen: Space before `=`.
FreeBSD style doesn't initialize variables in their declaration, but I won't… | |||||
Done Inline ActionsMy excuse is that the old code also initialized the variable in the declaration. ;-) dab: My excuse is that the old code also initialized the variable in the declaration. ;-) | |||||
| if (error) | if (error || (req->newptr == NULL)) | ||||
Done Inline ActionsYou 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. vangyzen: You should also return here if `req->newptr == NULL`. In that case, the caller is reading the… | |||||
| return (error); | return (error); | ||||
| if (ctrlr->timeout_period > NVME_MAX_TIMEOUT_PERIOD || | if (newval > NVME_MAX_TIMEOUT_PERIOD || | ||||
| ctrlr->timeout_period < NVME_MIN_TIMEOUT_PERIOD) { | newval < NVME_MIN_TIMEOUT_PERIOD) { | ||||
| ctrlr->timeout_period = oldval; | |||||
| return (EINVAL); | return (EINVAL); | ||||
| } else { | |||||
| ctrlr->timeout_period = newval; | |||||
| } | } | ||||
Done Inline ActionsCoverity 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. dab: Coverity complained here that in the old code `oldval` was restored in one path but not in… | |||||
Done Inline ActionsCoverity 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. imp: Coverity was wrong to complain about that. The old code was correct. The new code is more… | |||||
| return (0); | return (0); | ||||
| } | } | ||||
| static void | static void | ||||
| nvme_qpair_reset_stats(struct nvme_qpair *qpair) | nvme_qpair_reset_stats(struct nvme_qpair *qpair) | ||||
| { | { | ||||
| ▲ Show 20 Lines • Show All 202 Lines • Show Last 20 Lines | |||||
Space before =.
FreeBSD style doesn't initialize variables in their declaration, but I won't complain, because I disagree with that. :)