7/10:
``` - ok(io.Status == params->iosb_status, "got %#lx\n", io.Status); - ok(io.Information == 3, "got size %Iu\n", io.Information); + todo_wine_if (NT_ERROR(params->iosb_status) && params->pending) + ok(io.Status == params->iosb_status || broken(NT_ERROR(params->iosb_status) && params->pending), "got %#lx\n", io.Status); + todo_wine_if (NT_ERROR(params->iosb_status) && params->pending) + ok(io.Information == 3 || broken(NT_ERROR(params->iosb_status) && params->pending), "got size %Iu\n", io.Information); ```
We should also be testing the actual values in those broken() blocks.
Also, can we please mark which versions have which results in a comment?
``` ret = NtDeviceIoControlFile(file, event, NULL, (void *)456, &io, ioctl, params, sizeof(*params), buffer, sizeof(buffer)); ok(ret == params->ret_status - || broken(NT_WARNING(params->ret_status) && ret == STATUS_PENDING), /* win10 */ + || (NT_WARNING(params->ret_status) && ret == STATUS_PENDING), "got %#x\n", ret); if (!params->pending && NT_ERROR(params->iosb_status)) { ```
Similarly, no objection to removing broken(), but can we please keep the comment?
``` - ok(io.Status == 0xdeadf00d, "got iosb status %#lx\n", io.Status); - ok(io.Information == 0xdeadf00d, "got information %#Ix\n", io.Information); + todo_wine ok(io.Status == 0xc00000a3 || broken(io.Status == 0xdeadf00d), /* older win10 */ + "got iosb status %#lx\n", io.Status); + todo_wine ok(io.Information == 0 || broken(io.Information == 0xdeadf00d), /* older win10 */ + "got information %#Ix\n", io.Information); ```
c00000a3 is STATUS_DEVICE_NOT_READY.
8/10:
``` + case IOCTL_WINETEST_TEST_CANCEL_UNCANCELLABLE_INIT: + KeInitializeDeviceQueue(&device_queue); + ok(IsListEmpty(&device_queue.DeviceListHead), "expected empty queue list\n"); + status = STATUS_SUCCESS; + break; ```
Or just initialize it once in DriverEntry(), and then you don't need a dedicated ioctl.
``` +#define IOCTL_WINETEST_TEST_CANCEL_UNCANCELLABLE CTL_CODE(FILE_DEVICE_UNKNOWN, 0x806, METHOD_BUFFERED, FILE_ANY_ACCESS) +#define IOCTL_WINETEST_TEST_CANCEL_COMPLETE CTL_CODE(FILE_DEVICE_UNKNOWN, 0x807, METHOD_BUFFERED, FILE_ANY_ACCESS) ```
I'd be inclined to just make these IOCTL_WINETEST_QUEUE_ASYNC and IOCTL_WINETEST_COMPLETE_ASYNC, since they don't necessarily have anything to do with cancellation, and not upend the rest of the definitions...
``` + /* put the IRP into the queue */ + ret = KeInsertDeviceQueue(&device_queue, &irp->Tail.Overlay.DeviceQueueEntry); + /* or store it elsewhere if it's the one we're supposing to be + * working on "currently" */ + if (!ret) + current_irp = irp; ```
KDEVICE_QUEUE is an odd API and I feel like it honestly makes this test more confusing. I'd just have a simple "queued_async_irps[2]" array and stash them there.
Incidentally, because there is some loose threading involved I would be inclined to validate everything we can from the kernel side. Make sure there really is an IRP queued before we complete it, etc. I know I usually say "there's no use avoiding a crash; a failure is a failure", but bluescreens are a different story, because they make iterating a pain.
``` + returned = FALSE; + ctx.file = file; + ctx.ex = FALSE; + thread = CreateThread(NULL, 0, test_cancel_complete_thread, &ctx, 0, NULL); + res = CancelIo(file); + ok(res, "CancelIo failed: %lu\n", GetLastError()); + returned = TRUE; + WaitForSingleObject(thread, INFINITE); + CloseHandle(thread); ```
The way I tend to prefer writing this kind of test is to do the blocking action from the thread, check (from the main thread) that the thread doesn't complete immediately (e.g. WaitForSingleObject(200)), and then perform the action from the main thread that should complete it and check that it completes within another reasonable amount of time. I think it's a little easier to read. If you feel this is fine though I won't object.