Re: [PATCH] dlls/ntdll/file.c: Setting FileAllInformation is not 'fixable'.
On 7/24/2010 18:51, Max TenEyck Woodbury wrote:
--- dlls/ntdll/file.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/dlls/ntdll/file.c b/dlls/ntdll/file.c index 0a6ee55..86c200f 100644 --- a/dlls/ntdll/file.c +++ b/dlls/ntdll/file.c @@ -2148,6 +2148,11 @@ NTSTATUS WINAPI NtSetInformationFile(HANDLE handle, PIO_STATUS_BLOCK io, io->u.Status = STATUS_INVALID_PARAMETER_3; break;
+ /* Invalid requests - do not need 'fixing'. */ + case FileAllInformation: + io->u.Status = STATUS_NOT_IMPLEMENTED; + break; + default: FIXME("Unsupported class (%d)\n", class); io->u.Status = STATUS_NOT_IMPLEMENTED; Add a test please, and a comment won't be needed with a test too.
On 07/24/2010 10:58 AM, Nikolay Sivov wrote:
On 7/24/2010 18:51, Max TenEyck Woodbury wrote:
--- dlls/ntdll/file.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/dlls/ntdll/file.c b/dlls/ntdll/file.c index 0a6ee55..86c200f 100644 --- a/dlls/ntdll/file.c +++ b/dlls/ntdll/file.c @@ -2148,6 +2148,11 @@ NTSTATUS WINAPI NtSetInformationFile(HANDLE handle, PIO_STATUS_BLOCK io, io->u.Status = STATUS_INVALID_PARAMETER_3; break;
+ /* Invalid requests - do not need 'fixing'. */ + case FileAllInformation: + io->u.Status = STATUS_NOT_IMPLEMENTED; + break; + default: FIXME("Unsupported class (%d)\n", class); io->u.Status = STATUS_NOT_IMPLEMENTED; Add a test please, and a comment won't be needed with a test too.
There already is a test in dlls/ntdll/test/file.c. It produces a 'fixme:' when it should not. This fixes that.
Max TenEyck Woodbury <max(a)mtew.isa-geek.net> wrote:
+ /* Invalid requests - do not need 'fixing'. */ + case FileAllInformation: + io->u.Status = STATUS_NOT_IMPLEMENTED; + break; + default: FIXME("Unsupported class (%d)\n", class); io->u.Status = STATUS_NOT_IMPLEMENTED; Add a test please, and a comment won't be needed with a test too.
There already is a test in dlls/ntdll/test/file.c. It produces a 'fixme:' when it should not. This fixes that.
This is not a fix. A fix is a patch that makes a test pass that previously did not. You simply silence a harmless fixme. -- Dmitry.
On 07/25/2010 12:50 AM, Dmitry Timoshkov wrote:
Max TenEyck Woodbury<max(a)mtew.isa-geek.net> wrote:
+ /* Invalid requests - do not need 'fixing'. */ + case FileAllInformation: + io->u.Status = STATUS_NOT_IMPLEMENTED; + break; + default: FIXME("Unsupported class (%d)\n", class); io->u.Status = STATUS_NOT_IMPLEMENTED; Add a test please, and a comment won't be needed with a test too.
There already is a test in dlls/ntdll/test/file.c. It produces a 'fixme:' when it should not. This fixes that.
This is not a fix. A fix is a patch that makes a test pass that previously did not. You simply silence a harmless fixme.
The 'fixme' is not 'harmless'. It gets counted. It claims that there is something wrong that needs fixing. In fact, the proper implementation for this particular request is to set an error code and ignore the request. The test code checks for exactly that condition and does NOT report a test failure when that happens. It reports a failure if the error code does NOT have the specified value. In other words this patch fixes an error in the 'work to be done count'. Please stop the sniping. There is now more text disparaging the patch than there is text in the patch itself. -Max
On 24 July 2010 16:10, Max TenEyck Woodbury <max(a)mtew.isa-geek.net> wrote:
On 07/24/2010 10:58 AM, Nikolay Sivov wrote:
On 7/24/2010 18:51, Max TenEyck Woodbury wrote:
--- dlls/ntdll/file.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/dlls/ntdll/file.c b/dlls/ntdll/file.c index 0a6ee55..86c200f 100644 --- a/dlls/ntdll/file.c +++ b/dlls/ntdll/file.c @@ -2148,6 +2148,11 @@ NTSTATUS WINAPI NtSetInformationFile(HANDLE handle, PIO_STATUS_BLOCK io, io->u.Status = STATUS_INVALID_PARAMETER_3; break;
+ /* Invalid requests - do not need 'fixing'. */ + case FileAllInformation: + io->u.Status = STATUS_NOT_IMPLEMENTED; + break; + default: FIXME("Unsupported class (%d)\n", class); io->u.Status = STATUS_NOT_IMPLEMENTED;
Add a test please, and a comment won't be needed with a test too.
There already is a test in dlls/ntdll/test/file.c. It produces a 'fixme:' when it should not. This fixes that.
The only test I can find is: 1110 res = pNtSetInformationFile(h, &io, &fai_buf.fai, sizeof fai_buf, FileAllInformation); 1111 ok ( res == STATUS_INVALID_INFO_CLASS || res == STATUS_NOT_IMPLEMENTED, "shouldn't be able to set FileAllInformation, res %x\n", res); I would say that (based on this test), STATUS_NOT_IMPLEMENTED is the wrong value to use -- it should really be STATUS_INVALID_INFO_CLASS. The check should mark the STATUS_NOT_IMPLEMENTED value as a broken value: ok ( res == STATUS_INVALID_INFO_CLASS || broken(res == STATUS_NOT_IMPLEMENTED), "shouldn't be able to set FileAllInformation, res %x\n", res); This would fix the return value for this condition. In addition to this, the Wine implementation is returning io->u.Status in all these cases, but the tests for pNtSetInformationFile do not also check they are the same. To avoid false positives or negatives, io->u.Status needs to be set before each test. Thus, you have something like: io.u.Status = 0xdeadbeef; res = pNtSetInformationFile(...); ok (res == STATUS_SUCCESS, "Expected NtSetInformationFile to return STATUS_SUCCESS, got %d\n", res); ok(res == U(io).Status, "Expected NtSetInformationFile return to match io.u.Status, got res = %d, io.u.Status = %d\n", res, U(io).Status); or: ok (res == STATUS_SUCCESS, "Expected NtSetInformationFile to return STATUS_SUCCESS, got %d\n", res); ok(U(io).Status == STATUS_SUCCESS, "Expected io.u.Status to be STATUS_SUCCESS, got %d\n", U(io).Status); This way, the tests confirm that io.u.Status and the return value of NtSetInformationFile are the same. - Reece
On 07/25/2010 02:25 PM, Reece Dunn wrote:
On 24 July 2010 16:10, Max TenEyck Woodbury<max(a)mtew.isa-geek.net> wrote:
On 07/24/2010 10:58 AM, Nikolay Sivov wrote:
On 7/24/2010 18:51, Max TenEyck Woodbury wrote:
--- dlls/ntdll/file.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/dlls/ntdll/file.c b/dlls/ntdll/file.c index 0a6ee55..86c200f 100644 --- a/dlls/ntdll/file.c +++ b/dlls/ntdll/file.c @@ -2148,6 +2148,11 @@ NTSTATUS WINAPI NtSetInformationFile(HANDLE handle, PIO_STATUS_BLOCK io, io->u.Status = STATUS_INVALID_PARAMETER_3; break;
+ /* Invalid requests - do not need 'fixing'. */ + case FileAllInformation: + io->u.Status = STATUS_NOT_IMPLEMENTED; + break; + default: FIXME("Unsupported class (%d)\n", class); io->u.Status = STATUS_NOT_IMPLEMENTED;
Add a test please, and a comment won't be needed with a test too.
There already is a test in dlls/ntdll/test/file.c. It produces a 'fixme:' when it should not. This fixes that.
The only test I can find is:
1110 res = pNtSetInformationFile(h,&io,&fai_buf.fai, sizeof fai_buf, FileAllInformation); 1111 ok ( res == STATUS_INVALID_INFO_CLASS || res == STATUS_NOT_IMPLEMENTED, "shouldn't be able to set FileAllInformation, res %x\n", res);
I would say that (based on this test), STATUS_NOT_IMPLEMENTED is the wrong value to use -- it should really be STATUS_INVALID_INFO_CLASS. The check should mark the STATUS_NOT_IMPLEMENTED value as a broken value:
ok ( res == STATUS_INVALID_INFO_CLASS || broken(res == STATUS_NOT_IMPLEMENTED), "shouldn't be able to set FileAllInformation, res %x\n", res);
This would fix the return value for this condition.
In addition to this, the Wine implementation is returning io->u.Status in all these cases, but the tests for pNtSetInformationFile do not also check they are the same. To avoid false positives or negatives, io->u.Status needs to be set before each test. Thus, you have something like:
io.u.Status = 0xdeadbeef; res = pNtSetInformationFile(...); ok (res == STATUS_SUCCESS, "Expected NtSetInformationFile to return STATUS_SUCCESS, got %d\n", res); ok(res == U(io).Status, "Expected NtSetInformationFile return to match io.u.Status, got res = %d, io.u.Status = %d\n", res, U(io).Status);
or:
ok (res == STATUS_SUCCESS, "Expected NtSetInformationFile to return STATUS_SUCCESS, got %d\n", res); ok(U(io).Status == STATUS_SUCCESS, "Expected io.u.Status to be STATUS_SUCCESS, got %d\n", U(io).Status);
This way, the tests confirm that io.u.Status and the return value of NtSetInformationFile are the same.
- Reece
Excellent critique. You are obviously better informed than I am. Please submit a patch. I will assume your logic to be correct and will change the status value generated. - Max
participants (4)
-
Dmitry Timoshkov -
Max TenEyck Woodbury -
Nikolay Sivov -
Reece Dunn