Changeset View
Standalone View
sys/dev/nvme/nvme_test.c
Show First 20 Lines • Show All 94 Lines • ▼ Show 20 Lines | nvme_ns_bio_test(void *arg) | ||||
uint64_t io_completed = 0, offset; | uint64_t io_completed = 0, offset; | ||||
uint32_t idx; | uint32_t idx; | ||||
int ref; | int ref; | ||||
buf = malloc(io_test->size, M_NVME, M_WAITOK); | buf = malloc(io_test->size, M_NVME, M_WAITOK); | ||||
idx = atomic_fetchadd_int(&io_test->td_idx, 1); | idx = atomic_fetchadd_int(&io_test->td_idx, 1); | ||||
dev = io_test->ns->cdev; | dev = io_test->ns->cdev; | ||||
offset = idx * 2048 * nvme_ns_get_sector_size(io_test->ns); | offset = (uint64_t)idx * 2048 * nvme_ns_get_sector_size(io_test->ns); | ||||
dab: There was a possibility here that the (un-casted) expression `idx * 2048` could overflow the 32… | |||||
Done Inline ActionsYou can get the same effect by add ull to 2048. imp: You can get the same effect by add ull to 2048. | |||||
Done Inline ActionsTrue. 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? dab: True. I thought the cast to uint64_t was perhaps more easily understood since the declaration… | |||||
Done Inline Actionsull is generally preferred because it's less invasive than the cast. imp: ull is generally preferred because it's less invasive than the cast. | |||||
while (1) { | while (1) { | ||||
bio = g_alloc_bio(); | bio = g_alloc_bio(); | ||||
memset(bio, 0, sizeof(*bio)); | memset(bio, 0, sizeof(*bio)); | ||||
bio->bio_cmd = (io_test->opc == NVME_OPC_READ) ? | bio->bio_cmd = (io_test->opc == NVME_OPC_READ) ? | ||||
BIO_READ : BIO_WRITE; | BIO_READ : BIO_WRITE; | ||||
bio->bio_done = nvme_ns_bio_test_cb; | bio->bio_done = nvme_ns_bio_test_cb; | ||||
bio->bio_dev = dev; | bio->bio_dev = dev; | ||||
bio->bio_offset = offset; | bio->bio_offset = offset; | ||||
bio->bio_data = buf; | bio->bio_data = buf; | ||||
bio->bio_bcount = io_test->size; | bio->bio_bcount = io_test->size; | ||||
if (io_test->flags & NVME_TEST_FLAG_REFTHREAD) { | if (io_test->flags & NVME_TEST_FLAG_REFTHREAD) { | ||||
csw = dev_refthread(dev, &ref); | csw = dev_refthread(dev, &ref); | ||||
} else | } else | ||||
csw = dev->si_devsw; | csw = dev->si_devsw; | ||||
if (csw == NULL) | |||||
panic("Unable to retrieve device switch"); | |||||
mtx = mtx_pool_find(mtxpool_sleep, bio); | mtx = mtx_pool_find(mtxpool_sleep, bio); | ||||
mtx_lock(mtx); | mtx_lock(mtx); | ||||
(*csw->d_strategy)(bio); | (*csw->d_strategy)(bio); | ||||
Done Inline ActionsWhy is this needed? If csw is NULL, that should be a panic... imp: Why is this needed? If csw is NULL, that should be a panic... | |||||
Done Inline ActionsThe 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? dab: The csw is assigned to the return from dev_refthread() on line 119. dev_refthread() can return… | |||||
Done Inline ActionsIf 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). vangyzen: If you keep the panic above, don't test for NULL here.
I believe Coverity runs on builds with… | |||||
Done Inline ActionsErrggghhh. 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. dab: Errggghhh. Forgot to take that test out.
I didn't think Coverity ran with INVARIANTS; I'm… | |||||
msleep(bio, mtx, PRIBIO, "biotestwait", 0); | msleep(bio, mtx, PRIBIO, "biotestwait", 0); | ||||
mtx_unlock(mtx); | mtx_unlock(mtx); | ||||
if (io_test->flags & NVME_TEST_FLAG_REFTHREAD) { | if (io_test->flags & NVME_TEST_FLAG_REFTHREAD) { | ||||
dev_relthread(dev, ref); | dev_relthread(dev, ref); | ||||
} | } | ||||
if ((bio->bio_flags & BIO_ERROR) || (bio->bio_resid > 0)) | if ((bio->bio_flags & BIO_ERROR) || (bio->bio_resid > 0)) | ||||
▲ Show 20 Lines • Show All 151 Lines • Show Last 20 Lines |
There was a possibility here that the (un-casted) expression idx * 2048 could overflow the 32-bits of that expression.