Page MenuHomeFreeBSD
Differential D24532 Diff 71222 sys/dev/nvme/nvme_ctrlr.c

Changeset View

Standalone View

View Options

sys/dev/nvme/nvme_ctrlr.c

Show First 20 Lines • Show All 1,291 Lines • ▼ Show 20 Lines case NVME_RESET_CONTROLLER:
break; break;
case NVME_PASSTHROUGH_CMD: case NVME_PASSTHROUGH_CMD:
pt = (struct nvme_pt_command *)arg; pt = (struct nvme_pt_command *)arg;
return (nvme_ctrlr_passthrough_cmd(ctrlr, pt, le32toh(pt->cmd.nsid), return (nvme_ctrlr_passthrough_cmd(ctrlr, pt, le32toh(pt->cmd.nsid),
1 /* is_user_buffer */, 1 /* is_admin_cmd */)); 1 /* is_user_buffer */, 1 /* is_admin_cmd */));
case NVME_GET_NSID: case NVME_GET_NSID:
{ {
struct nvme_get_nsid *gnsid = (struct nvme_get_nsid *)arg; struct nvme_get_nsid *gnsid = (struct nvme_get_nsid *)arg;
strncpy(gnsid->cdev, device_get_nameunit(ctrlr->dev), strncpy(gnsid->cdev, device_get_nameunit(ctrlr->dev),
impUnsubmitted
Done
Inline Actions

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

imp: strncpy ensures all the bytes in the returned structure are initialized. strlcpy only ensures…
dabAuthorUnsubmitted
Done
Inline Actions

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?

dab: True. Coverity was complaining because strncpy() doesn't guarantee NUL termination. I thought I…
sizeof(gnsid->cdev)); sizeof(gnsid->cdev));
gnsid->cdev[sizeof(gnsid->cdev) - 1] = '\0';
vangyzenUnsubmitted
Done
Inline Actions

Missing closing paren.

vangyzen: Missing closing paren.
dabAuthorUnsubmitted
Done
Inline Actions

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

dab: Also not the only mistake there on that line. Careless.
gnsid->nsid = 0; gnsid->nsid = 0;
break; break;
} }
default: default:
return (ENOTTY); return (ENOTTY);
} }
return (0); return (0);
▲ Show 20 Lines • Show All 268 Lines • ▼ Show 20 Linesnvme_ctrlr_resume(struct nvme_controller *ctrlr)
* nmve_attach() for why. * nmve_attach() for why.
*/ */
if (nvme_ctrlr_hw_reset(ctrlr) != 0) if (nvme_ctrlr_hw_reset(ctrlr) != 0)
goto fail; goto fail;
if (nvme_ctrlr_hw_reset(ctrlr) != 0) if (nvme_ctrlr_hw_reset(ctrlr) != 0)
goto fail; goto fail;
/* /*
* Now that we're reset the hardware, we can restart the controller. Any * Now that we've reset the hardware, we can restart the controller. Any
* I/O that was pending is requeued. Any admin commands are aborted with * I/O that was pending is requeued. Any admin commands are aborted with
* an error. Once we've restarted, take the controller out of reset. * an error. Once we've restarted, take the controller out of reset.
*/ */
nvme_ctrlr_start(ctrlr, true); nvme_ctrlr_start(ctrlr, true);
atomic_cmpset_32(&ctrlr->is_resetting, 1, 0); (void)atomic_cmpset_32(&ctrlr->is_resetting, 1, 0);
dabAuthorUnsubmitted
Done
Inline Actions

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.

dab: There is some precedent for this in the kernel, but not a whole lot. It does make clear to both…
return (0); return (0);
fail: fail:
/* /*
* Since we can't bring the controller out of reset, announce and fail * Since we can't bring the controller out of reset, announce and fail
* the controller. However, we have to return success for the resume * the controller. However, we have to return success for the resume
* itself, due to questionable APIs. * itself, due to questionable APIs.
*/ */
nvme_printf(ctrlr, "Failed to reset on resume, failing.\n"); nvme_printf(ctrlr, "Failed to reset on resume, failing.\n");
nvme_ctrlr_fail(ctrlr); nvme_ctrlr_fail(ctrlr);
atomic_cmpset_32(&ctrlr->is_resetting, 1, 0); (void)atomic_cmpset_32(&ctrlr->is_resetting, 1, 0);
return (0); return (0);
} }