libnvme/tests/ioctl: run the mock ioctl tests on Windows - #3815
libnvme/tests/ioctl: run the mock ioctl tests on Windows#3815bbusacker wants to merge 7 commits into
Conversation
| .cdw11 = (TEST_FIDX << 0) | (TEST_CSI << 24), | ||
| .cdw14 = TEST_UUID, | ||
| .out_data = &expected_id, | ||
| WIN_CSI_UNSUPPORTED, |
There was a problem hiding this comment.
.err = WIN_CSI_UNSUPPORTED ?
There was a problem hiding this comment.
WIN_CSI_UNSUPPORTED is a macro that sets .win_err = -ENOTSUP and .win_no_ioctl = true (defined in mock.h). This pattern is used for 8 different tests, which is probably why it was implemented as a macro. Would you prefer the settings to be explicit instead of using the macro?
| (((sizeof(expected_log) >> 2) - 1) << 16), | ||
| .cdw14 = (TEST_CSI << 24), | ||
| .out_data = &expected_log, | ||
| WIN_CSI_UNSUPPORTED, |
There was a problem hiding this comment.
same here is it .err or .win_err?
| @@ -0,0 +1,83 @@ | |||
| # ioctl mock tests | |||
There was a problem hiding this comment.
There is libnvme/design folder. I suggest to add a sub folder tests and move this document there, with a different name obviously.
There was a problem hiding this comment.
Thanks for the feedback. The README.md file is now design/tests/ioctl-mock.md.
|
Thank for the effort. IMO the ioctl tests are not really bringing a lot to the table, they are mostly exercising the serializing/deserializing path which is platform neutral. So I am wondering if a Windows port is worth having. Don't get me wrong, I don't mind it as long it doesn't add additional maintenance burden. WDYT? |
The Linux passthru ioctls always write the CQE result back, so callers can read cmd->result after the call regardless of what it held on entry. The Windows implementation only assigns cmd->result on the paths that manage to retrieve completion data, which leaves whatever the caller happened to have in the field for every command that fails early -- an unsupported opcode, a rejected Command Set Identifier, or a failed IOCTL. Zero it in both entry points so the two platforms agree: after a failed command, cmd->result reads back as 0 rather than as stale caller data. Signed-off-by: Brandon Busacker <bbusacker@micron.com>
sndk_do_sn861_drive_resize() copies admin_cmd.result out unconditionally, including on the error paths where no completion data was retrieved, and its caller declares the receiving variable uninitialised. Copy the result only when the command succeeded, and initialise the caller's variable, so a failed resize cannot be reported with a fabricated result. Signed-off-by: Brandon Busacker <bbusacker@micron.com>
The %m conversion is a glibc printf extension. It is not in C, and mingw's printf does not implement it, so these diagnostics would emit a literal "%m" once these tests are built for Windows. Call strerror(errno) explicitly and include <string.h> (and <errno.h> in logs.c) for it. Signed-off-by: Brandon Busacker <bbusacker@micron.com>
Several of these tests assert an exact error or result value that a non-Linux implementation cannot reproduce. Windows has no passthru ioctl that carries a Command Set Identifier, so libnvme rejects those commands before issuing any IOCTL; the Windows IOCTLs report a failed command as EIO rather than the NVMe status code; get-features sets DNR where Linux does not; and no completion data is available for a failed command, so the result reads back as 0. Rather than #ifdef the assertions, carry the difference in the mock: win_err overrides the expected error on Windows, win_no_ioctl says the command never reaches the mock layer at all, and mock_err(), mock_ok() and mock_result() read the expectation for the platform under test. WIN_CSI_UNSUPPORTED names the common case of a rejected Command Set. On Linux every one of these resolves to the value the test asserted before, so behaviour there is unchanged. The one new test, set_error_clears_stale_result, checks that a failed command does not leave a caller-supplied result in place; it seeds cmd->result after the init helper precisely so it is testing the passthru entry point and not nvme_init_set_features_async_event(). Signed-off-by: Brandon Busacker <bbusacker@micron.com>
The ioctl mock is injected with LD_PRELOAD on Linux, which Windows has no equivalent of. ld --wrap does not substitute for it either: libnvme is built as a DLL, so its call to DeviceIoControl is bound to its own import thunk at load time and never passes through a symbol the test executable could wrap. Overwriting that thunk does work. Every PE image reaches an imported function through a slot in its Import Address Table, so rewriting libnvme's IAT slot redirects the call without modifying any code, and the patch is confined to the one module -- which is what a test wants. iat_patch() finds the slot for a named import and swaps in a replacement, optionally handing back the original so it can be called through or restored. Nothing builds this yet; the mock that uses it follows. Signed-off-by: Brandon Busacker <bbusacker@micron.com>
…dows The ioctl tests have been Linux-only because both halves of the harness are Linux-specific: the mock is injected with LD_PRELOAD and it intercepts the NVMe passthru ioctl. Windows offers neither. What it does offer is a small, fixed set of NVMe-specific IOCTLs that libnvme drives through DeviceIoControl, which is interceptable by patching libnvme's import thunk. mock-win.c is that interception. It patches DeviceIoControl in the libnvme module, decodes the Windows IOCTL back into the NVMe command it represents, matches it against the next expected mock_cmd exactly as mock.c does, and synthesises the reply the driver would have returned. The decoders cover the three command families libnvme implements on Windows today: identify, get/set features, and get log page. The remaining test programs stay off Windows. They exercise families that reach the driver by other routes -- SCSI CDBs for read, write and flush, dedicated firmware, format and sanitize IOCTLs -- or that libnvme does not implement on Windows at all. Each needs its own decoder, so enabling them is future work rather than a line in this file, and they are left disabled instead of reporting a pass they did not earn. With the mock chosen inside ioctl/meson.build the host gate in the parent meson.build has nothing left to decide, so it goes away. README.md records how the two mocks differ and what a new decoder has to do. Signed-off-by: Brandon Busacker <bbusacker@micron.com>
I agree that most of the value from the mock tests is gained by just running them on Linux, since the serializing/deserializing path is shared. The main benefits received from having separate Windows ioctl tests are code coverage of ioctl-win.c and a check against unexpected changes to the ioctl-win implementation without the need for end-to-end testing. Writing the tests has also helped highlight some of the differences in behavior vs Linux, and has provided an opportunity to better evaluate those differences and see if different design decisions could be made. The commands that are currently being mocked and tested on Windows have been well tested on hardware and should be stable, so maintenance shouldn't be an issue from that side. We will need to make sure that any intended changes to ioctl-win.c that would change the expected behavior include a matching change to the tests. Do you have any particular concerns regarding the maintenance of these tests? If you don't think the benefits received merit the complexity of the tests, we can drop this and rely on end-to-end testing. |
7bc73d6 to
c52e03d
Compare
Adds a tests folder under design, and moves the readme file for the ioctl mock tests under that folder as ioctl-mock.md. Signed-off-by: Broc Going <bgoing@micron.com>
3 participants
The
libnvme/tests/ioctlsuite is the only place command-building is checkedwithout a device, and it has been Linux-only because both halves of the harness
are: the mock is injected with
LD_PRELOAD, and it intercepts the NVMe passthruioctl. Windows has neither.
This series adds a Windows interception layer so the existing
identify,featuresandlogstests run there against the same expectations, and fixesthe two result-handling bugs the new coverage exposed.
How it works
libnvme reaches the driver through
DeviceIoControl.--wrapcannot interceptthat -- libnvme is a DLL, so the call is bound through its own import thunk at
load time and never passes through a symbol the test executable links. What does
work is rewriting the thunk:
iat-hook.cpatches libnvme's Import Address Tableslot, which redirects the call without touching any code and leaves other modules
(including libnvme's own device enumeration) alone.
mock-win.cthen decodes the Windows IOCTL back into the NVMe command itrepresents and matches it against the next expected
struct mock_cmd, exactly asmock.cdoes. That forward translation inioctl-win.cis lossy, so each decoderrecords which fields it actually recovered -- and a bit mask where a field
survives only partly, e.g. Windows' 4-bit
LogSpecificFieldagainst NVMe's 7.Only recovered fields are compared; a discarded field is skipped rather than
compared against zero, which would turn a real mismatch into a false pass.
Where Windows genuinely cannot behave identically, the test says so instead of
being skipped:
win_errandwin_no_ioctlonstruct mock_cmd, read throughmock_err()/mock_ok()/mock_result(). On Linux every one of those resolvesto the value the test asserted before. The mock rejects an expectation libnvme
could not have produced, so an undeclared divergence fails loudly.
libnvme/tests/ioctl/README.mddocuments all of this, including what a newdecoder has to do.
Two fixes
cmd->resulton paths thatretrieved completion data, leaving stale caller data in the field for any
command that failed early. Linux always writes it back. Now both do.
sndk_do_sn861_drive_resize()copiedadmin_cmd.resultout unconditionally,including on error paths, and its caller left the receiving variable
uninitialised.
Scope
The other ioctl tests stay off Windows. They exercise families that reach the
driver by other routes -- SCSI CDBs for read, write and flush, dedicated
firmware, format and sanitize IOCTLs -- or that libnvme does not implement on
Windows at all. Each needs its own decoder, so they are left disabled rather than
reporting a pass they didn't earn.
Verification
identical to
upstream/master-- the only difference in the list isregistration order, since
subdir('ioctl')moved ahead ofsubdir('sysfs').warnings, with
libnvme - identify,libnvme - featuresandlibnvme - logspassing.
last are all in
mock-win.cand deliberate -- a Windows API function-pointertypedef (explained in a comment at the declaration), long
fail()messagestrings, the
-ENOSYS->ERROR_CALL_NOT_IMPLEMENTEDerrno mapping, and a"falls through" phrase in prose that isn't a switch fallthrough.