On Sun May 25 13:31:25 2025 +0000, Jayson Reis wrote:
> Hi there @epo thanks for the review!
> As I could not find any auto formatting tool configured, I tried
> maintaining the formatting around the code I was changing, do you mind
> point me out where did you find it different?
> I will change the calls to GetProcAddress, as there is no public header,
> this means that on console.c I cannot call the Wide function from ascii
> and just convert, right? Meaning I would need to implement the logic twice?
> About being racy, reading the docs I did not see anything special about
> it, it seems like a wrapper around the PeekInputConsole to be less
> blocking, something gives you the feel it should be reimplemented?
> Also, is it a good idea to copy the exe for tests and run on Windows to
> see how it goes?
> At least this showed something to me that my test and implementation
> seem to be wrong like when calling the function with nowait, and it is
> empty, the return should be false but GetLastError seems to be 0
You can call a function without public header, just move that prototype to console.c (either near the top, or just before ExA - I don't know what Wine code style says about that).
The race happens if two threads read from that console simultaneously, both with nowait; Peek returns for both, thread 1 calls Read, and thread 2 calls Read too and blocks because it's empty.
Tests should be run on Windows, yes. That's how we discover which behavior to implement in Wine.
We usually SetLastError(0xdeadbeef) before calling something that should (or shouldn't) set the last error, to make sure we're not seeing a leftover error from a previous operation.
If PeekW returns false, then you should return false too, not think you're in the *count=0 branch and return true.
The code style issue is probably the // comments, we use only /* in Wine. (Also it's spelled that, not tha.)
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/8137#note_104466
Hi there @epo thanks for the review!
As I could not find any auto formatting tool configured, I tried maintaining the formatting around the code I was changing, do you mind point me out where did you find it different?
I will change the calls to GetProcAddress, as there is no public header, this means that on console.c I cannot call the Wide function from ascii and just convert, right? Meaning I would need to implement the logic twice?
About being racy, reading the docs I did not see anything special about it, it seems like a wrapper around the PeekInputConsole to be less blocking, something gives you the feel it should be reimplemented?
Also, is it a good idea to copy the exe for tests and run on Windows to see how it goes?
At least this showed something to me that my test and implementation seem to be wrong like when calling the function with nowait, and it is empty, the return should be false but GetLastError seems to be 0
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/8137#note_104465
a couple of comments:
* please read and conform to [Wine Coding style](https://gitlab.winehq.org/wine/wine/-/wikis/Wine-Developer's-Guide/Coding-Practice#some-notes-about-style)
* AFAICT ReadConsoleInputEx\[AW\] isn't defined in Windows SDK so header files shouldn't be changed
* tests must reflect what Windows does, not what your implementation does (hence the generated failures in CI pipeline)
* this should be implemented using proper (new or modified) IOCTL:s; there's no way this API can be implemented on top of kernel32's one without being racy (as your proposal is). That's the reason why MS introduced this new API.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/8137#note_104458
The Aim of this MR is to fix https://bugs.winehq.org/show_bug.cgi?id=10941
In a nutshell:
- CUI app starts, and CRT inits its std streams from console,
- app calls FreeConsole() then AllocConsole() and expects the CRT I/O
functions to be able to output to newly created console.
It fails:
- when run with wineconsole, as the inherited std consoles are closed
in FreeConsole(). Added tests, but current implementation of kernelbase
is correct. So, the fix is to force wineconsole to pass unbound console
handles with standard inheritance (to they are not closed),
- when run from unix console, the std handles were also closed. There's
no unbound console in that case. The fix is not to close these
handles in FreeConsole().
- the other cases from the bug entry work because cmd was created with
unbound console handles.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/8136