[PATCH] dlls/ntdll/file.c: Setting FileAllInformation is not 'fixable'.

[Max TenEyck Woodbury]

On 07/25/2010 02:25 PM, Reece Dunn wrote:
> On 24 July 2010 16:10, Max TenEyck Woodbury<max at 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