Fixes and unit tests for Get/Set Features functions #717
Conversation
Several nvme_{g,s}et_features_*() functions use the wrong feature ID.
Correct them so the intended feature is requested.
Signed-off-by: Caleb Sander <[email protected]>
A few nvme_set_features_*() functions are setting the wrong bits in CDW11 based on their inputs. Correct the fields passed to NVME_SET(). Signed-off-by: Caleb Sander <[email protected]>
Several nvme_{g,s}et_features_*() functions take a data buffer
but don't provide it to the Get/Set Features commands.
Fix them to pass the data buffer to nvme_{g,s}et_features().
Getting the Host Memory Buffer feature also returns
a struct nvme_host_mem_buf_attrs data structure,
so make a nvme_get_features_host_mem_buf2() function
that takes in a data buffer to receive it.
Signed-off-by: Caleb Sander <[email protected]>
The last 2 bytes of the struct nvme_timestamp data are not initialized in nvme_set_features_timestamp(). Add an initializer to avoid sending uninitialized stack memory to the controller. Signed-off-by: Caleb Sander <[email protected]>
According to the NVMe spec, the Host Behavior and Write Protection features are not saveable. Setting the SAVE bit may cause the Set Features command to be rejected, so don't set it for these features. Signed-off-by: Caleb Sander <[email protected]>
The ENDGID parameter of nvme_get_features_endurance_event_cfg() is currently unused. According to the NVMe spec, it should be passed in CDW 11. Signed-off-by: Caleb Sander <[email protected]>
nvme_get_features_iocs_profile() and nvme_set_features_iocs_profile() are defined but not exorted in libnvme.so. nvme_set_features_iocs_profile()'s prototype was also removed, so add it back. IOCSI is a 9-bit value, so fix its bitmask and change its type to u16. Signed-off-by: Caleb Sander <[email protected]>
nvme_set_features_host_id()'s prototype says the parameter EXHID comes before SAVE, but its definition does the opposite, causing the parameters to be misinterpreted. Match the definition to the declaration. Signed-off-by: Caleb Sander <[email protected]>
Several features are configured on a per-namespace basis
by setting the NSID in the Set Features and Get Features commands.
But the corresponding nvme_{g,s}et_features_*() functions
aren't passing the NSID in the commands.
For the functions missing a NSID parameter, define new variants
with the NSID argument added and mark the old functions as deprecated.
Signed-off-by: Caleb Sander <[email protected]>
nvme_set_features_lba_range() was missing an implementation, so add one. Change nr_ranges to a u8 since its maximum value is 64. Signed-off-by: Caleb Sander <[email protected]>
Use the mock ioctl() infrastructure to test the functions in ioctl.h that issue Set Features and Get Features commands. Signed-off-by: Caleb Sander <[email protected]>
A few of the feature related function have been deprecated. Update the test to use the new APIs instead of the old one. Signed-off-by: Daniel Wagner <[email protected]>
|
The compiler did complain for ../test/test.c:201:9: error: ‘nvme_get_features_err_recovery’ is deprecated [-Werror=deprecated-declarations]
201 | ret = nvme_get_features_err_recovery(fd, sel, &result);
| ^~~
In file included from ../src/nvme/linux.h:14,
from ../src/libnvme.h:18,
from ../test/test.c:23:
../src/nvme/ioctl.h:2762:5: note: declared here
2762 | int nvme_get_features_err_recovery(int fd, enum nvme_get_features_sel sel,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../test/test.c:261:9: error: ‘nvme_get_features_resv_mask’ is deprecated [-Werror=deprecated-declarations]
261 | ret = nvme_get_features_resv_mask(fd, sel, &result);
| ^~~
../src/nvme/ioctl.h:3068:5: note: declared here
3068 | int nvme_get_features_resv_mask(int fd, enum nvme_get_features_sel sel,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
../test/test.c:266:9: error: ‘nvme_get_features_resv_persist’ is deprecated [-Werror=deprecated-declarations]
266 | ret = nvme_get_features_resv_persist(fd, sel, &result);
| ^~~
../src/nvme/ioctl.h:3097:5: note: declared here
3097 | int nvme_get_features_resv_persist(int fd, enum nvme_get_features_sel sel,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~Anyway, I fixed up the test myself. The rest looks good. Thanks! |
|
Hmm, specifying a NSID of 0 doesn't really make sense, probably those usages should be updated to use an actual NSID. But thanks for taking a look at the code! |
|
I see. I could not test it, no hardware which supports these features. Don't know if anyone is actually running this test anymore. It really looks like it was there to get some testing done. Your unit tests are way better :). Maybe we could just ditch it. We can't run in a CI context anyway, it as it needs root rights and real hardware. |
|
Well there is probably some value in testing against real hardware. My unit tests are only as bug-free as my reading/understanding of the spec. And it's always possible devices have non-spec-compliant implementations. |
|
I still hope to find some time to work on getting a test setup where we run some tests, e.g. blktests against real hardware. blktests is exercising some parts of the nvme-cli/libnvme code base. I suppose if I get this working we could run this test also. On a second thought let's keep it and rather figure out how to get CI tests against real hardware. |
The functions for sending Set Features and Get Features commands are pretty buggy.
That's probably because
nvme-clionly callsnvme_set_features()andnvme_get_features()directly.I have no idea if anyone is actually using these functions, but may as well fix them if they exist and add tests.