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