On 19.03.2015 16:22, Andrew Eikum wrote:
dlls/kernel32/kernel32.spec | 4 +- dlls/kernel32/path.c | 97 +++++++++++++++++++++++++++++++++++++++++++++ dlls/kernel32/tests/file.c | 88 ++++++++++++++++++++++++++++++++++++++++ include/winbase.h | 2 + 4 files changed, 189 insertions(+), 2 deletions(-)
Why do you do the work twice? It would have been much easier if you would have just helped us to review and improve our Staging patchset which is there since about half a year ago, and supports a lot more flags.
https://github.com/wine-compholio/wine-staging/blob/master/patches/kernel32-... https://github.com/wine-compholio/wine-staging/blob/master/patches/kernel32-...
Why does noone bother anymore to test those patches, the author Michael Müller asked for feedback there long time ago: https://bugs.winehq.org/show_bug.cgi?id=36073#c4
I'll submit the patches in a few minutes so that you can decide if its better/worse than the current approach.
Regards, Sebastian
On Thu, Mar 19, 2015 at 06:30:34PM +0100, Sebastian Lackner wrote:
On 19.03.2015 16:22, Andrew Eikum wrote:
dlls/kernel32/kernel32.spec | 4 +- dlls/kernel32/path.c | 97 +++++++++++++++++++++++++++++++++++++++++++++ dlls/kernel32/tests/file.c | 88 ++++++++++++++++++++++++++++++++++++++++ include/winbase.h | 2 + 4 files changed, 189 insertions(+), 2 deletions(-)
Why do you do the work twice? It would have been much easier if you would have just helped us to review and improve our Staging patchset which is there since about half a year ago, and supports a lot more flags.
https://github.com/wine-compholio/wine-staging/blob/master/patches/kernel32-... https://github.com/wine-compholio/wine-staging/blob/master/patches/kernel32-...
Why does noone bother anymore to test those patches, the author Michael Müller asked for feedback there long time ago: https://bugs.winehq.org/show_bug.cgi?id=36073#c4
I didn't know that existed. I don't watch the bugs mailing list and I didn't think to google for it. I don't remember seeing it come across wine-patches, though maybe I missed it.
Andrew
On Thu, Mar 19, 2015 at 06:30:34PM +0100, Sebastian Lackner wrote:
On 19.03.2015 16:22, Andrew Eikum wrote:
dlls/kernel32/kernel32.spec | 4 +- dlls/kernel32/path.c | 97 +++++++++++++++++++++++++++++++++++++++++++++ dlls/kernel32/tests/file.c | 88 ++++++++++++++++++++++++++++++++++++++++ include/winbase.h | 2 + 4 files changed, 189 insertions(+), 2 deletions(-)
Why do you do the work twice? It would have been much easier if you would have just helped us to review and improve our Staging patchset which is there since about half a year ago, and supports a lot more flags.
https://github.com/wine-compholio/wine-staging/blob/master/patches/kernel32-... https://github.com/wine-compholio/wine-staging/blob/master/patches/kernel32-...
Why does noone bother anymore to test those patches, the author Michael Müller asked for feedback there long time ago: https://bugs.winehq.org/show_bug.cgi?id=36073#c4
I'll submit the patches in a few minutes so that you can decide if its better/worse than the current approach.
The patch you linked certainly looks better, though note that the A version seems to have the same problem that Nikolay pointed out.
Andrew
On 19.03.2015 18:45, Andrew Eikum wrote:
On Thu, Mar 19, 2015 at 06:30:34PM +0100, Sebastian Lackner wrote:
On 19.03.2015 16:22, Andrew Eikum wrote:
dlls/kernel32/kernel32.spec | 4 +- dlls/kernel32/path.c | 97 +++++++++++++++++++++++++++++++++++++++++++++ dlls/kernel32/tests/file.c | 88 ++++++++++++++++++++++++++++++++++++++++ include/winbase.h | 2 + 4 files changed, 189 insertions(+), 2 deletions(-)
Why do you do the work twice? It would have been much easier if you would have just helped us to review and improve our Staging patchset which is there since about half a year ago, and supports a lot more flags.
https://github.com/wine-compholio/wine-staging/blob/master/patches/kernel32-... https://github.com/wine-compholio/wine-staging/blob/master/patches/kernel32-...
Why does noone bother anymore to test those patches, the author Michael Müller asked for feedback there long time ago: https://bugs.winehq.org/show_bug.cgi?id=36073#c4
I'll submit the patches in a few minutes so that you can decide if its better/worse than the current approach.
The patch you linked certainly looks better, though note that the A version seems to have the same problem that Nikolay pointed out.
Andrew
Yes, this part should probably be changed a bit to make the code a bit more clean. Nevertheless I am wondering if this is a real problem. Under which circumstances would the WCHAR representation be longer than the ASCII one? Can anyone given an example? Wine uses the same assumption also in a couple of other functions, for example:
- GetEnvironmentVariableA - ExpandEnvironmentStringsA - ...
Are these all wrong?
Sebastian
On 19.03.2015 21:03, Sebastian Lackner wrote:
On 19.03.2015 18:45, Andrew Eikum wrote:
On Thu, Mar 19, 2015 at 06:30:34PM +0100, Sebastian Lackner wrote:
On 19.03.2015 16:22, Andrew Eikum wrote:
dlls/kernel32/kernel32.spec | 4 +- dlls/kernel32/path.c | 97 +++++++++++++++++++++++++++++++++++++++++++++ dlls/kernel32/tests/file.c | 88 ++++++++++++++++++++++++++++++++++++++++ include/winbase.h | 2 + 4 files changed, 189 insertions(+), 2 deletions(-)
Why do you do the work twice? It would have been much easier if you would have just helped us to review and improve our Staging patchset which is there since about half a year ago, and supports a lot more flags.
https://github.com/wine-compholio/wine-staging/blob/master/patches/kernel32-... https://github.com/wine-compholio/wine-staging/blob/master/patches/kernel32-...
Why does noone bother anymore to test those patches, the author Michael Müller asked for feedback there long time ago: https://bugs.winehq.org/show_bug.cgi?id=36073#c4
I'll submit the patches in a few minutes so that you can decide if its better/worse than the current approach.
The patch you linked certainly looks better, though note that the A version seems to have the same problem that Nikolay pointed out.
Andrew
Yes, this part should probably be changed a bit to make the code a bit more clean. Nevertheless I am wondering if this is a real problem. Under which circumstances would the WCHAR representation be longer than the ASCII one?
WCHAR representation probably can't be longer in characters than ansii one. But that's not an overallocation that I was talking about. From your repo:
--- +DWORD WINAPI GetFinalPathNameByHandleA(HANDLE file, LPSTR path, DWORD charcount, DWORD flags) +{ + WCHAR *str; + DWORD result; + + TRACE( "(%p,%p,%d,%x)\n", file, path, charcount, flags ); + + if (!path || !charcount) + return GetFinalPathNameByHandleW(file, (LPWSTR)path, charcount, flags); ---
Here you return W-length in characters as A-length in characters, and that doesn't look right to me, because you can't know A-length until you run WideCharToMultiByte(). At least that's my understanding of how DBCS behave.
Can anyone given an example? Wine uses the same assumption also in
a couple of other functions, for example:
- GetEnvironmentVariableA
- ExpandEnvironmentStringsA
- ...
Are these all wrong?
Sebastian
On 19.03.2015 19:32, Nikolay Sivov wrote:
On 19.03.2015 21:03, Sebastian Lackner wrote:
On 19.03.2015 18:45, Andrew Eikum wrote:
On Thu, Mar 19, 2015 at 06:30:34PM +0100, Sebastian Lackner wrote:
On 19.03.2015 16:22, Andrew Eikum wrote:
dlls/kernel32/kernel32.spec | 4 +- dlls/kernel32/path.c | 97 +++++++++++++++++++++++++++++++++++++++++++++ dlls/kernel32/tests/file.c | 88 ++++++++++++++++++++++++++++++++++++++++ include/winbase.h | 2 + 4 files changed, 189 insertions(+), 2 deletions(-)
Why do you do the work twice? It would have been much easier if you would have just helped us to review and improve our Staging patchset which is there since about half a year ago, and supports a lot more flags.
https://github.com/wine-compholio/wine-staging/blob/master/patches/kernel32-... https://github.com/wine-compholio/wine-staging/blob/master/patches/kernel32-...
Why does noone bother anymore to test those patches, the author Michael Müller asked for feedback there long time ago: https://bugs.winehq.org/show_bug.cgi?id=36073#c4
I'll submit the patches in a few minutes so that you can decide if its better/worse than the current approach.
The patch you linked certainly looks better, though note that the A version seems to have the same problem that Nikolay pointed out.
Andrew
Yes, this part should probably be changed a bit to make the code a bit more clean. Nevertheless I am wondering if this is a real problem. Under which circumstances would the WCHAR representation be longer than the ASCII one?
WCHAR representation probably can't be longer in characters than ansii one. But that's not an overallocation that I was talking about. From your repo:
+DWORD WINAPI GetFinalPathNameByHandleA(HANDLE file, LPSTR path, DWORD charcount, DWORD flags) +{
- WCHAR *str;
- DWORD result;
- TRACE( "(%p,%p,%d,%x)\n", file, path, charcount, flags );
- if (!path || !charcount)
return GetFinalPathNameByHandleW(file, (LPWSTR)path, charcount, flags);
Here you return W-length in characters as A-length in characters, and that doesn't look right to me, because you can't know A-length until you run WideCharToMultiByte(). At least that's my understanding of how DBCS behave.
Can anyone given an example? Wine uses the same assumption also in
a couple of other functions, for example:
- GetEnvironmentVariableA
- ExpandEnvironmentStringsA
- ...
Are these all wrong?
Sebastian
Ah, sure. Was looking at the other code part all the time. ^^ Will fix that. Any other feedback while we're just discussing this version?
Sebastian
On 19.03.2015 21:42, Sebastian Lackner wrote:
Ah, sure. Was looking at the other code part all the time. ^^ Will fix that. Any other feedback while we're just discussing this version?
Sure.
return GetFinalPathNameByHandleW(file, (LPWSTR)path, charcount, flags);
I don't see why you're trying to pass A buffer as a W one here. But this will change probably.
- result = GetFinalPathNameByHandleW(file, (LPWSTR)str, charcount, flags);
Do you really need a cast?
- if (result)
- {
if (result < charcount){result = FILE_name_WtoA( str, result, path, charcount - 1 );path[result] = 0;}
else result--; /* Why does Windows do this? */
In this case when 'charcount <= result' (equality applies here because of include/don't include NULL thing?) you're still return a W-length instead of A one. So as I said on first reply to Andrew's path it seems easier to use W call in a clean way to get proper A-length at the end.
- }
Some comments regarding functional part (note that I didn't try to run test on windows or write more tests), obvious things only:
- else if (file == INVALID_HANDLE_VALUE)
- {
SetLastError( ERROR_INVALID_HANDLE );return 0;- }
Won't NtQueryObject() fail in this case returning proper error for you?
- else if (flags & ~(FILE_NAME_OPENED | VOLUME_NAME_GUID | VOLUME_NAME_NONE | VOLUME_NAME_NT))
- {
WARN("Invalid or unsupported flags: %x\n", flags);SetLastError( ERROR_INVALID_PARAMETER );return 0;- }
Invalid and unsupported is important distinction. For invalid WARN is fine, for unsupported FIXME is probably better, unless you tried that and got a flood of those in output.
Sebastian
-
Andrew Eikum -
Nikolay Sivov -
Sebastian Lackner