Re: [PATCH] dlls/ntdll/file.c: Setting FileAllInformation is not 'fixable'.
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
Re: [PATCH] dlls/ntdll/file.c: Setting FileAllInformation is not 'fixable'.
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
Re: [PATCH] dlls/ntdll/file.c: Setting FileAllInformation is not 'fixable'.
On 07/25/2010 12:04 PM, Max TenEyck Woodbury wrote: On 07/25/2010 09:45 AM, James McKenzie wrote: 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. Well, you didn't point out which existing testcase you are talking about. All that your patch does is silence a FIXME. Presumably, the FIXME was placed there for a reason. Nikolay and Dmitry were pointing out that silencing that FIXME might not be appropriate, and were asking for you to demonstrate why that FIXME is invalid. Adding a testcase or pointing to an existing testcase would accomplish this. Which existing testcase demonstrates that this behavior is valid and that the FIXME is unwarranted? Does the existing testcase demonstrate the full range of behavior given that parameter? Can you expand on the tests to show that your implementation is always correct? 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. You need to take it easy, man. No one is out to get you :) A lot of folks on this list don't have English as a first language, and it can be easy to sound offensive if you haven't had the experience with the subtleties English that native speakers have had. There might also be some folks who are just abrasive, and you have to ignore or politely respond to those. In no case does accusing people like this help, even if they are doing some injustice towards you. Wine has a high barrier for entry and patches are reviewed harshly. If people are responding negatively to your patch, then it's likely because your patch was not obviously correct. The correct way to respond to this is by proving that it's correct, not asserting that it's correct. You're going to have to deal with defending your patches, and accept that sometimes you are wrong and sometimes other people are wrong. Andrew
Re: [PATCH] dlls/ntdll/file.c: Setting FileAllInformation is not 'fixable'.
Andrew Eikum wrote: On 07/25/2010 12:04 PM, Max TenEyck Woodbury wrote: On 07/25/2010 09:45 AM, James McKenzie wrote: 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. You had very well much know what Microsoft does and care very much about what they do. The goal of this project, as it has been since the mid 1990s is to fully emulate, bug and all, the Microsoft Windows32 and Windows64 (since 64 bit versions of Windows arrived) APIs. Thus we have test cases that demonstrate what the actions are of the API/ABI. That is what I've been working on with several richedit functions that I need to have for programs that I personally use. I'm 'eating my own dog food' to speak. 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. Well, you didn't point out which existing testcase you are talking about. All that your patch does is silence a FIXME. Presumably, the FIXME was placed there for a reason. Nikolay and Dmitry were pointing out that silencing that FIXME might not be appropriate, and were asking for you to demonstrate why that FIXME is invalid. Adding a testcase or pointing to an existing testcase would accomplish this. I looked at the testcase. He will need to 'beef up' the testcase to demonstrate what functions that particular test case does. If it does return not implemented then it has to be demonstrated. I tried to point this out as did Nicolay and Dmitry. Otherwise, this patch will be rejected by AJ as an attempt to silence the fixme, which he detests greatly. Which existing testcase demonstrates that this behavior is valid and that the FIXME is unwarranted? Does the existing testcase demonstrate the full range of behavior given that parameter? Can you expand on the tests to show that your implementation is always correct? 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. No. we have stated that you have no authoritative source that you are correct. State this and we all will be satisfied that the patch you propose is correct. Right now, it appears as if the patch is just a 'silence the fixme' attempt. This is why your original set of patches was beat up. We want to know, and so do our users, that implementation is not complete for functions. If this is breaking a program you are using, you are welcome to fully implement the function. That is what we are all here for. Fixmes are not the way to go, but if a stub makes things work that is the first step of many. There are many fixmes in Wine code and I'm working on three of them in the richedit dll that directly affect the ability for me to properly use programs. I've built a test case for one of them, that was rejected by AJ. I'm working on a second test case and then will build out code from that position. This is how the project moves ahead. You need to take it easy, man. No one is out to get you :) We really are not out to get you. I work, daily, as a software QA analyst. I reject perfectly good looking software because it behaves strangely and does not react properly to improper inputs. It is not easy being on that end of the process, however I also understand that writing proper code and having good/superb test cases makes all the difference. The bottom line is that there is no test case that validates your change. Create one. If your change is wrong, you then have the knowledge to state I withdraw. I've done that, several times. If it is right, then add the test case, first, then the new code and state this is based on your test case and when you ran it in the change log. Makes us all happy and it furthers the state of Wine. Wine has a high barrier for entry and patches are reviewed harshly. If people are responding negatively to your patch, then it's likely because your patch was not obviously correct. The correct way to respond to this is by proving that it's correct, not asserting that it's correct. You're going to have to deal with defending your patches, and accept that sometimes you are wrong and sometimes
Re: [PATCH] dlls/ntdll/file.c: Setting FileAllInformation is not 'fixable'.
On 24 July 2010 16:10, Max TenEyck Woodbury m...@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); 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
Re: [PATCH] dlls/ntdll/file.c: Setting FileAllInformation is not 'fixable'.
James McKenzie jjmckenzi...@earthlink.net writes: Also, strongly defending your patches without authoritative information marks you as being arrogant. After a while, your patches will be ignored. That is not a good place to be in this project. Your habit of pretending to be an authority in the project, when you clearly have no idea what you're talking about, strikes me as a lot more arrogant than Max (correctly) pointing out that there is a test case already. You are right about one thing, the patches being ignored bit. But it's not to Max that it's going to happen... -- Alexandre Julliard julli...@winehq.org
Re: [PATCH] libs/wpp: Fixed bug in preventing add_text_to_macro from handling macros over 1 kb large
Hi, Your Patch got wrapped. Try change some settings in you Mailclient please and resubmit. Thx -- Best Regards, André Hentschel
Re: [PATCH] dlls/ntdll/file.c: Setting FileAllInformation is not 'fixable'.
Max TenEyck Woodbury wrote: On 07/25/2010 01:55 PM, James McKenzie wrote: Andrew Eikum wrote: On 07/25/2010 12:04 PM, Max TenEyck Woodbury wrote: On 07/25/2010 09:45 AM, James McKenzie wrote: 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. You had very well much know what Microsoft does and care very much about what they do. The goal of this project, as it has been since the mid 1990s is to fully emulate, bug and all, the Microsoft Windows32 and Windows64 (since 64 bit versions of Windows arrived) APIs. Thus we have test cases that demonstrate what the actions are of the API/ABI. That is what I've been working on with several richedit functions that I need to have for programs that I personally use. I'm 'eating my own dog food' to speak. Since I do not have (and should never have if I work on Wine) access to Microsoft's code, I can not know what they actually do. Upon the insistance of AJ and others, I will no longer be working on or for the Wine project. Your code does have an error, it was just recently pointed out that the return value was incorrect. Thank you for sufficiently flogging me with the whip I was attempting to flog you with. James McKenzie
Re: [PATCH 2/2] dlls/ntdll/file.c: Return a different status code.
2010/7/25 Max TenEyck Woodbury m...@mtew.isa-geek.net: --- dlls/ntdll/file.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/dlls/ntdll/file.c b/dlls/ntdll/file.c index 86c200f..c32baa0 100644 --- a/dlls/ntdll/file.c +++ b/dlls/ntdll/file.c @@ -2150,7 +2150,7 @@ NTSTATUS WINAPI NtSetInformationFile(HANDLE handle, PIO_STATUS_BLOCK io, /* Invalid requests - do not need 'fixing'. */ case FileAllInformation: - io-u.Status = STATUS_NOT_IMPLEMENTED; + io-u.Status = STATUS_INVALID_INFO_CLASS; break; default: -- 1.7.1.1 your first patch has not been accepted yet. you should just make a try 2 of the first patch with the correct return code instead of a new patch correcting the first.
Re: [PATCH] dlls/ntdll/file.c: Setting FileAllInformation is not 'fixable'.
On 07/25/2010 01:55 PM, James McKenzie wrote: Andrew Eikum wrote: On 07/25/2010 12:04 PM, Max TenEyck Woodbury wrote: On 07/25/2010 09:45 AM, James McKenzie wrote: 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. You had very well much know what Microsoft does and care very much about what they do. The goal of this project, as it has been since the mid 1990s is to fully emulate, bug and all, the Microsoft Windows32 and Windows64 (since 64 bit versions of Windows arrived) APIs. Thus we have test cases that demonstrate what the actions are of the API/ABI. That is what I've been working on with several richedit functions that I need to have for programs that I personally use. I'm 'eating my own dog food' to speak. Since I do not have (and should never have if I work on Wine) access to Microsoft's code, I can not know what they actually do. The test case code is what is available. I did not write this test case code, but it has been run against Microsoft's code and has been around long enough that it would have been corrected if it did not match their results. I believe it is also safe to assume that the Microsoft code does NOT produce 'fixme' messages. Unless the test case code is wrong, there should not be a 'fixme' produced. 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. Well, you didn't point out which existing testcase you are talking about. All that your patch does is silence a FIXME. Presumably, the FIXME was placed there for a reason. Nikolay and Dmitry were pointing out that silencing that FIXME might not be appropriate, and were asking for you to demonstrate why that FIXME is invalid. Adding a testcase or pointing to an existing testcase would accomplish this. I looked at the testcase. He will need to 'beef up' the testcase to demonstrate what functions that particular test case does. If it does return not implemented then it has to be demonstrated. I tried to point this out as did Nicolay and Dmitry. Otherwise, this patch will be rejected by AJ as an attempt to silence the fixme, which he detests greatly. You obviously found the test case code. I did not write it. If it reports failures when run against Microsoft's code, it needs fixing, but I am not the one to fix it. As it stands, the expected result is a 'not implemented' status. Given that, any 'fixme' issued would be improper. Which existing testcase demonstrates that this behavior is valid and that the FIXME is unwarranted? Does the existing testcase demonstrate the full range of behavior given that parameter? Can you expand on the tests to show that your implementation is always correct? 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. No. we have stated that you have no authoritative source that you are correct. State this and we all will be satisfied that the patch you propose is correct. Right now, it appears as if the patch is just a 'silence the fixme' attempt. This is why your original set of patches was beat up. We want to know, and so do our users, that implementation is not complete for functions. If this is breaking a program you are using, you are welcome to fully implement the function. That is what we are all here for. Fixmes are not the way to go, but if a stub makes things work that is the first step of many. There are many fixmes in Wine code and I'm working on three of them in the richedit dll that directly affect the ability for me to properly use programs. I've built a test case for one of them, that was rejected by AJ. I'm working on a second test case and then will build out code from that position. This is how the project moves ahead. There is no 'authoritative source'. There are only the test cases. The 'fixme' appears to be the result of slightly sloppy coding. Since the test case shows that the proper behavior is to return an error code, the 'fixme' is an error. The patch is intended to bring Wine's behavior into line with that of the Microsoft code. It is not just a 'silence the fixme' attempt. This particular 'fixme' should never have been issued. As for
Re: [PATCH] dlls/ntdll/file.c: Setting FileAllInformation is not 'fixable'.
On 07/25/2010 01:34 PM, Andrew Eikum wrote:
On 07/25/2010 12:04 PM, Max TenEyck Woodbury wrote:
On 07/25/2010 09:45 AM, James McKenzie wrote:
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.
Well, you didn't point out which existing testcase you are talking
about. All that your patch does is silence a FIXME. Presumably, the
FIXME was placed there for a reason. Nikolay and Dmitry were pointing
out that silencing that FIXME might not be appropriate, and were asking
for you to demonstrate why that FIXME is invalid. Adding a testcase or
pointing to an existing testcase would accomplish this.
Which existing testcase demonstrates that this behavior is valid and
that the FIXME is unwarranted? Does the existing testcase demonstrate
the full range of behavior given that parameter? Can you expand on the
tests to show that your implementation is always correct?
From my notes (line breaks added to prevent wrapping)
fixme:ntdll:NtSetInformationFile Unsupported class (18)
dlls/ntdll/file.c:2152 in NtSetInformationFile
NTSTATUS WINAPI NtSetInformationFile(HANDLE handle, PIO_STATUS_BLOCK
io, PVOID ptr, ULONG len, FILE_INFORMATION_CLASS class)
switch (class) { ... default: FIXME(Unsupported class (%d)\n,
class); 18=FileAllInformation
called by dlls/ntdll/tests/file.c: in test_file_all_information
res = pNtSetInformationFile(h, io, fai_buf.fai, sizeof fai_buf,
FileAllInformation);
should not be 'FIXME'?
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.
You need to take it easy, man. No one is out to get you :)
A lot of folks on this list don't have English as a first language, and
it can be easy to sound offensive if you haven't had the experience with
the subtleties English that native speakers have had. There might also
be some folks who are just abrasive, and you have to ignore or politely
respond to those. In no case does accusing people like this help, even
if they are doing some injustice towards you.
I don't think this has anything to do with me personally. I see it as
pretty standard behaviour when an 'in-crowd' is chalanged.
Wine has a high barrier for entry and patches are reviewed harshly. If
people are responding negatively to your patch, then it's likely because
your patch was not obviously correct. The correct way to respond to this
is by proving that it's correct, not asserting that it's correct. You're
going to have to deal with defending your patches, and accept that
sometimes you are wrong and sometimes other people are wrong.
Andrew
Yes, some people are abrasive. Giving them a pass only encourages their
bad behavior. If Nicolay or anybody else was not happy with the
pointer I gave to the test case, they could have simply said so.
Proof is necessarily in the mind of the reader. If the reader is not
willing to examine the proof, they are impossible to convince.
Similarly, if they are prejudiced, it will be impossible to change
their minds no matter how much evidence is presented. On the other hand
you asked for more specific information. I am providing it, but you
will have to convince yourself that the arguments presented are correct.
If you will notice, I've already admitted to being wrong on some
things. To some people that encourages their attack behavior.
- Max
Re: [PATCH] dlls/ntdll/file.c: Setting FileAllInformation is not 'fixable'.
On 07/25/2010 02:25 PM, Reece Dunn wrote: On 24 July 2010 16:10, Max TenEyck Woodburym...@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); 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
Re: [PATCH] const variables are not compile-time constants
On Sun, Jul 25, 2010 at 11:36 PM, Mikko Rasa t...@tdb.fi wrote: - static const DWORD user_flags = clip_flags | DCX_NORESETATTRS; /* flags that can be set by user */ + static const DWORD user_flags = DCX_PARENTCLIP | DCX_CLIPSIBLINGS | DCX_CLIPCHILDREN | DCX_WINDOW + | DCX_NORESETATTRS; /* flags that can be set by user */ This should be committed first thing tomorrow imho (in order to minimize the number of potential compile errors in case of a bisect). Octavian
Re: propsys: add stub for PSPropertyKeyFromString
On Sun, Jul 25, 2010 at 2:09 PM, Louis Lenders xerox_xerox2...@yahoo.co.uk wrote: fixes http://bugs.winehq.org/show_bug.cgi?id=23528 I don't mean to step on this patch, but I've sent a fuller implementation of PSPropertyKeyFromString which, if accepted, should take precedence over this stub.
Re: [2/4] propsys/tests: Add tests for PSStringFromPropertyKey.
Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at http://testbot.winehq.org/JobDetails.pl?Key=3892 Your paranoid android. === WINEBUILD (build) === Make failed
Re: [4/4] propsys/tests: Add tests for PSPropertyKeyFromString.
Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at http://testbot.winehq.org/JobDetails.pl?Key=3893 Your paranoid android. === WINEBUILD (build) === Make failed
Re: [2/4] propsys/tests: Add tests for PSStringFromPropertyKey.
On 07/25/2010 10:46 PM, (Marvin) wrote: Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at http://testbot.winehq.org/JobDetails.pl?Key=3892 Your paranoid android. === WINEBUILD (build) === Make failed There doesn't seem to be anything wrong with the patch set, just that the bot failed to generate the import library for propsys. Previously one wasn't generated, but my patches have changed this.
