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

Max TenEyck Woodbury max at mtew.isa-geek.net
Sun Jul 25 12:04:57 CDT 2010


On 07/25/2010 09:45 AM, James McKenzie wrote:
> 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;
> Max:
>
> I think you missed what Nicolay and Dmitry are trying to tell you.
> We are trying to implement, bug for bug, the functionality of what
> Windows does. Does Windows return "STATUS_NOT_IMPLEMENTED" when this
> call is made? If not, your fix is WRONG. Silencing a 'fixme' is NOT a
> fix and this will be REJECTED.
> If this is correct and is what Windows does, then state so. Otherwise,
> withdraw the patch and fix it the right way.
>
> James McKenzie
>
Frankly, I do not know what Microsoft does, but the test would fail on
their implementation if they did something else, so I think it is safe
to assume the test is implemented properly. Given that, the fixme is
wrong.

Specifically, Nicolay asked for a test case. Since I was working from
an already existing test case, his request didn't really make sense. I
pointed out that there already was a test case and that should have
been the end of it.

Dimtry's response makes even less sense. The fixme was produced by a
default branch in a switch statement. It is fairly good practice to
have such a branch and note that it is being used with a 'fixme',
though technically it should check the switch value to assure it is in
the correct range before issuing the 'fixme'. I would have added that
test if I had documentation on what the proper range is. Since I did
not have that documentation, I left the default branch alone. However,
the 'FileAllInformation' branch is obviously in range and the test case
code makes it clear that it should return STATUS_NOT_IMPLEMENTED. It
should not produce a 'fixme'. So I added a branch that returns the
proper status without an unnecessary and erroneous 'fixme'. I suspect,
but do not know, that there may be other codes that are not
implemented. Thus the comment so future additions to the list of
unimplemented codes would be put in the same place. Dimtry's response
shows that he did not look at the situation in any detail, where I had
examined what needed doing from several points of view.

Now you come along and make loud demands that the patch be rejected,
without having looked at the situation carefully. Frankly, this looks
very much like the activities of an 'in-crowd' trying to defend its
boarders. Much the same thing happend with the '_ONCE' patches. The
arguments presented by others for their rejection contained some bogus
rationalizations (along with a few points that I corrected) but did NOT
even touch the reason those patches had to be withdrawn. In other
words, I have and will withdraw patches when there are good reasons to
do so, but no such reasons have been presented for the withdrawal of
this patch so far.

- Max



More information about the wine-devel mailing list