Re: [PATCH 2/5] wait until all data from earlier test has been written in test_waittxempty
Am Sonntag, 8. September 2013, 11:32:26 schrieb Francois Gouget: On Thu, 5 Sep 2013, Wolfgang Walter wrote: [...] To see that my later patches are needed you must modify the test a little bit: - dlls/kernel32/tests/comm.c.old 2013-09-05 13:40:10.275757373 +0200 +++ dlls/kernel32/tests/comm.c 2013-09-05 13:40:06.779074398 +0200 @@ -844,6 +844,8 @@ after - before, bytes, baud); /* don't wait for WriteFile completion */ +Sleep(2000); + S(U(ovl_wait)).Offset = 0; S(U(ovl_wait)).OffsetHigh = 0; ovl_wait.hEvent = CreateEvent(NULL, TRUE, FALSE, NULL); I did not look at the specifics of this case but I agree with the general principle of making tests reasonably independent from each other. This way we know exactly what works and what does not. It's not an absolute rule but we do try for that already when we reset LastError between tests for instance, or in some other cases when we delete some resource and recreate it for each test. I guess the above is not really the patch that you propose but I'll still comment on it: we also want the tests to run as fast as possible so Sleep() is bad. However if we were to Sleep() only if the previous test failed I think that would be ok. Indeed this patch is not the proposed one. It is a variation of a test to show a different problem. My patch to make two tests more independed from each other only sleeps if the first test fails. Regards, -- Wolfgang Walter Studentenwerk München Anstalt des öffentlichen Rechts Abteilungsleiter IT Leopoldstraße 15 80802 München
Re: [PATCH 1/5] use a correct timeout in test_waittxempty
On Thursday 05 September 2013 10:32:15 Dmitry Timoshkov wrote: Wolfgang Walter w...@stwm.de wrote: When we send 17 bytes with 150 baud, no parity, one stop bit then we need to wait at least 17*10/150 seconds 1000 ms Under Windows with both real COM-port and USB-serial cable this test takes no longer than 890 ms, usually it's even shorter like 875 ms. Testbot VMs also don't fail this test. If under Wine it takes 1000 ms for you then probably there is a bug somewhere. How can that be? If you send 17 bytes which expands to 170 raw bits and you only sent 150 bits/s? Maybe you are using a virtual machine or your com port does not support 150 baud and chooses a different speed. Or you use a UART with a large buffer and which already signals TX-EMPTY even if it is still sending from an internal buffer. But one should not rely on that. I used a real com-port, by the way, and had a com-port-analyzer adapted to it. It exactly sends 150 bits per second which means 15 data-bytes per second. Regards, -- Wolfgang Walter Studentenwerk München Anstalt des öffentlichen Recht Abteilungsleiter IT Leopoldstraße 15 80802 München
Re: [PATCH 5/5] kernel32/tests: remove several todos in test_waittxempty
On Thursday 05 September 2013 10:42:40 you wrote: Wolfgang Walter w...@stwm.de wrote: -ok(!res GetLastError() == ERROR_IO_PENDING, %d: WaitCommEvent error %d\n, i, GetLastError()); +ok(res || (!res GetLastError() == ERROR_IO_PENDING), %d: WaitCommEvent error %d\n, i, GetLastError()); This change looks spurious and unrelated. Also todos must be removed in the patch that fixes appropriate behaviour. It is allowed that WaitCommEvent returns immediately with TRUE if there is an event pending, as far as I can see. Regards, -- Wolfgang Walter Studentenwerk München Anstalt des öffentlichen Recht Abteilungsleiter IT Leopoldstraße 15 80802 München
Re: [PATCH 2/5] wait until all data from earlier test has been written in test_waittxempty
On Thursday 05 September 2013 10:37:22 you wrote: Wolfgang Walter w...@stwm.de wrote: +if (res || (!res GetLastError() == ERROR_IO_PENDING)) +/* if data has been sent: wait for termination */ +Sleep(timeout); I don't see such a problem with real COM-port and serial-USB cable under Windows or Linux here and under testbot VMs. Wine does that here (vanilla). I added this so that the NEXT test does not depend what wine exactly does. Regards, -- Wolfgang Walter Studentenwerk München Anstalt des öffentlichen Recht Abteilungsleiter IT Leopoldstraße 15 80802 München
Re: [PATCH 1/5] use a correct timeout in test_waittxempty
Am Donnerstag, 5. September 2013, 17:51:18 schrieb Dmitry Timoshkov: Wolfgang Walter w...@stwm.de wrote: When we send 17 bytes with 150 baud, no parity, one stop bit then we need to wait at least 17*10/150 seconds 1000 ms Under Windows with both real COM-port and USB-serial cable this test takes no longer than 890 ms, usually it's even shorter like 875 ms. Testbot VMs also don't fail this test. If under Wine it takes 1000 ms for you then probably there is a bug somewhere. How can that be? If you send 17 bytes which expands to 170 raw bits and you only sent 150 bits/s? Maybe you are using a virtual machine or your com port does not support 150 baud and chooses a different speed. Or you use a UART with a large buffer and which already signals TX-EMPTY even if it is still sending from an internal buffer. But one should not rely on that. I'm testing under 64-bit Windows 7 on real hardware. Then it is your UART or it's driver. As I sad: you need 1133 ms to transmit 17 bytes and I think one should not wait less. Otherwise one depends on hardware, driver internals (linux as windows) and scheduling. One should even wait longer (that is the reason I add TIMEOUT). Regards, -- Wolfgang Walter Studentenwerk München Anstalt des öffentlichen Rechts Abteilungsleiter IT Leopoldstraße 15 80802 München
Re: [PATCH 5/5] kernel32/tests: remove several todos in test_waittxempty
Am Donnerstag, 5. September 2013, 17:52:42 schrieb Dmitry Timoshkov: Wolfgang Walter w...@stwm.de wrote: On Thursday 05 September 2013 10:42:40 you wrote: Wolfgang Walter w...@stwm.de wrote: -ok(!res GetLastError() == ERROR_IO_PENDING, %d: WaitCommEvent error %d\n, i, GetLastError()); +ok(res || (!res GetLastError() == ERROR_IO_PENDING), %d: WaitCommEvent error %d\n, i, GetLastError()); This change looks spurious and unrelated. Also todos must be removed in the patch that fixes appropriate behaviour. It is allowed that WaitCommEvent returns immediately with TRUE if there is an event pending, as far as I can see. WaitCommEvent always returns FALSE when device is opened in overlapped mode. Not according to MSN documentation of WaitCommEvent(): If the overlapped operation cannot be completed immediately, the function returns FALSE and the GetLastError function returns ERROR_IO_PENDING, indicating that the operation is executing in the background. Regards. -- Wolfgang Walter Studentenwerk München Anstalt des öffentlichen Rechts Abteilungsleiter IT Leopoldstraße 15 80802 München
Re: [PATCH 2/5] wait until all data from earlier test has been written in test_waittxempty
Am Donnerstag, 5. September 2013, 17:54:58 schrieb Dmitry Timoshkov: Wolfgang Walter w...@stwm.de wrote: +if (res || (!res GetLastError() == ERROR_IO_PENDING)) +/* if data has been sent: wait for termination */ +Sleep(timeout); I don't see such a problem with real COM-port and serial-USB cable under Windows or Linux here and under testbot VMs. Wine does that here (vanilla). I added this so that the NEXT test does not depend what wine exactly does. When Wine behaviour differs from Windows one the test results need to be marked as todo_wine, and such places already have it. I don't unterstand you. I didn't change the test case. It still above and is still marked with todo. This change simply ensures that the next test is not distorted. If wine does not pass the test WriteFile on an overlapped handle without ovl structure should fail but instead sends the bytes we should wait long enough before starting the next test so that there are no remaining bytes in the tx buffer. Otherwise the next test will not test sending 17 bytes but much more. Regards, -- Wolfgang Walter Studentenwerk München Anstalt des öffentlichen Rechts Abteilungsleiter IT Leopoldstraße 15 80802 München
Re: [PATCH 5/5] kernel32/tests: remove several todos in test_waittxempty
Am Donnerstag, 5. September 2013, 18:31:45 schrieb Dmitry Timoshkov: Wolfgang Walter w...@stwm.de wrote: WaitCommEvent always returns FALSE when device is opened in overlapped mode. Not according to MSN documentation of WaitCommEvent(): If the overlapped operation cannot be completed immediately, the function returns FALSE and the GetLastError function returns ERROR_IO_PENDING, indicating that the operation is executing in the background. We have the tests for that, MSDN descriptions are proved to be often incorrect or incomplete. Then you think that wine behaviour should be changed? If yes I'll send a patch. But I wonder as wine does quit some effort to do otherwise. Regards, -- Wolfgang Walter Studentenwerk München Anstalt des öffentlichen Rechts Abteilungsleiter IT Leopoldstraße 15 80802 München
Re: [PATCH 2/5] wait until all data from earlier test has been written in test_waittxempty
Am Donnerstag, 5. September 2013, 18:55:55 schrieben Sie: Wolfgang Walter w...@stwm.de wrote: +if (res || (!res GetLastError() == ERROR_IO_PENDING)) +/* if data has been sent: wait for termination */ +Sleep(timeout); I don't see such a problem with real COM-port and serial-USB cable under Windows or Linux here and under testbot VMs. Wine does that here (vanilla). I added this so that the NEXT test does not depend what wine exactly does. When Wine behaviour differs from Windows one the test results need to be marked as todo_wine, and such places already have it. I don't unterstand you. I didn't change the test case. It still above and is still marked with todo. There is no need to add any workarounds for Wine bugs, appropriate places already have todo_wine statements. This is not a work around. todo_wine will not magically undo sending the bytes. Please explain why you think that there will be now pending bytes in the tx buffer if the tests fails. And why it does not make the next tests fail even if wine would behave correctly then. res = WriteFile(hcom, tbuf, sizeof(tbuf), bytes, NULL); todo_wine ok(!res, WriteFile on an overlapped handle without ovl structure should fail\n); todo_wine ok(GetLastError() == ERROR_INVALID_PARAMETER, expected ERROR_INVALID_PARAMETER, got %d\n, GetLastError()); both tests would fail with wine actually. Wine does write 17 bytes to com port these bytes now sit in tx buffer and need some time sending. This distorts the following tests which tests the EV_TXEMPTY behaviour: Again 17 bytes are written and then the tests assume that one waits for these 17 bytes (timeout value and messurement). But really we wait for much more bytes being sent, up to 36. So even if EV_TXEMPTY handling would work the test for it will fail with a timeout. Regards, -- Wolfgang Walter Studentenwerk München Anstalt des öffentlichen Rechts Abteilungsleiter IT Leopoldstraße 15 80802 München
Re: [PATCH 2/5] wait until all data from earlier test has been written in test_waittxempty
Am Donnerstag, 5. September 2013, 19:40:13 schrieb Dmitry Timoshkov: Wolfgang Walter w...@stwm.de wrote: This is not a work around. todo_wine will not magically undo sending the bytes. Please explain why you think that there will be now pending bytes in the tx buffer if the tests fails. And why it does not make the next tests fail even if wine would behave correctly then. res = WriteFile(hcom, tbuf, sizeof(tbuf), bytes, NULL); todo_wine ok(!res, WriteFile on an overlapped handle without ovl structure should fail\n); todo_wine ok(GetLastError() == ERROR_INVALID_PARAMETER, expected ERROR_INVALID_PARAMETER, got %d\n, GetLastError()); both tests would fail with wine actually. Wine does write 17 bytes to com port these bytes now sit in tx buffer and need some time sending. This distorts the following tests which tests the EV_TXEMPTY behaviour: Again 17 bytes are written and then the tests assume that one waits for these 17 bytes (timeout value and messurement). But really we wait for much more bytes being sent, up to 36. So even if EV_TXEMPTY handling would work the test for it will fail with a timeout. Does 'make test' pass without failures for you? Without patch 1/2 and 2/2 I get for $ ../../../tools/runtest -P wine -M kernel32.dll -T ../../.. -q -p kernel32_test.exe.so comm.c fixme:comm:set_queue_size insize 1024 outsize 1024 unimplemented stub err:comm:set_line_control ByteSize fixme:ntdll:server_ioctl_file Unsupported ioctl 1b000c (device=1b access=0 func=3 method=0) comm.c:2029: Tests skipped: interactive tests (set WINETEST_INTERACTIVE=1) With my patches 1/2 and 2/2 I get: fixme:iphlpapi:NotifyAddrChange (Handle 0x10ee8e0, overlapped 0x10ee8ec): stub wine: configuration in '/home/walterw/.wine' has been updated. fixme:comm:set_queue_size insize 1024 outsize 1024 unimplemented stub comm.c:857: Test succeeded inside todo block: WaitCommEvent failed with a timeout comm.c:881: Test succeeded inside todo block: WaitCommEvent error 997 comm.c:883: Test succeeded inside todo block: WaitCommEvent: expected EV_TXEMPTY, got 0x4 comm.c:889: Test succeeded inside todo block: WaitCommEvent used 1141 ms for waiting err:comm:set_line_control ByteSize fixme:ntdll:server_ioctl_file Unsupported ioctl 1b000c (device=1b access=0 func=3 method=0) comm.c:2036: Tests skipped: interactive tests (set WINETEST_INTERACTIVE=1) So EV_TXEMPTY handling already works partially. To see that my later patches are needed you must modify the test a little bit: - dlls/kernel32/tests/comm.c.old 2013-09-05 13:40:10.275757373 +0200 +++ dlls/kernel32/tests/comm.c 2013-09-05 13:40:06.779074398 +0200 @@ -844,6 +844,8 @@ after - before, bytes, baud); /* don't wait for WriteFile completion */ +Sleep(2000); + S(U(ovl_wait)).Offset = 0; S(U(ovl_wait)).OffsetHigh = 0; ovl_wait.hEvent = CreateEvent(NULL, TRUE, FALSE, NULL); Then the tests 857 etc. again fail. We already talked about why that is the case. Probably I should add that as an additional test. Regards, -- Wolfgang Walter Studentenwerk München Anstalt des öffentlichen Rechts Abteilungsleiter IT Leopoldstraße 15 80802 München
Re: [PATCH 2/5] wait until all data from earlier test has been written in test_waittxempty
Am Donnerstag, 5. September 2013, 21:12:37 schrieb Dmitry Timoshkov: Wolfgang Walter w...@stwm.de wrote: Does 'make test' pass without failures for you? Without patch 1/2 and 2/2 I get for $ ../../../tools/runtest -P wine -M kernel32.dll -T ../../.. -q -p kernel32_test.exe.so comm.c fixme:comm:set_queue_size insize 1024 outsize 1024 unimplemented stub err:comm:set_line_control ByteSize fixme:ntdll:server_ioctl_file Unsupported ioctl 1b000c (device=1b access=0 func=3 method=0) comm.c:2029: Tests skipped: interactive tests (set WINETEST_INTERACTIVE=1) The tests pass without any failures. With my patches 1/2 and 2/2 I get: fixme:iphlpapi:NotifyAddrChange (Handle 0x10ee8e0, overlapped 0x10ee8ec): stub wine: configuration in '/home/walterw/.wine' has been updated. fixme:comm:set_queue_size insize 1024 outsize 1024 unimplemented stub comm.c:857: Test succeeded inside todo block: WaitCommEvent failed with a timeout comm.c:881: Test succeeded inside todo block: WaitCommEvent error 997 comm.c:883: Test succeeded inside todo block: WaitCommEvent: expected EV_TXEMPTY, got 0x4 comm.c:889: Test succeeded inside todo block: WaitCommEvent used 1141 ms for waiting err:comm:set_line_control ByteSize fixme:ntdll:server_ioctl_file Unsupported ioctl 1b000c (device=1b access=0 func=3 method=0) comm.c:2036: Tests skipped: interactive tests (set WINETEST_INTERACTIVE=1) Successes inside of todo_wine blocks are treated as failres. So you think I should remove the wine_todos already here? Regards, -- Wolfgang Walter Studentenwerk München Anstalt des öffentlichen Rechts Abteilungsleiter IT Leopoldstraße 15 80802 München
Re: [PATCH 2/5] wait until all data from earlier test has been written in test_waittxempty
Am Donnerstag, 5. September 2013, 23:58:57 schrieb Dmitry Timoshkov: Jonas Maebe jonas.ma...@elis.ugent.be wrote: The tests must pass under Wine without any additional fixes as they do currently under Windows. If you add some code to the tests which suddenly makes them pass under Wine - that's not a fix, Wine implementation should be fixed instead. What I understood from Wolfgang's previous explanations is that there is currently this situation: todo_wine test1(); /* test1 fails because wine has a bug. As a side-effect, bytes will be left in the transmission buffer under wine, but not under windows. */ test2(); /* test2 also fails under wine because of the failure of test1 that influences the behaviour of test2, not because the functionality tested by test2 is broken. This make it impossible to individually evaluate test1() and test2(). */ So either * you add code in between that resets the state so that test2 can be run independently of whether test1 failed (as Wolfgang proposes), or * you put the tests in completely different executables so they don't influence each other (hopefully), or * you reorder the test so that first all the tests that currently work under wine are run (under the assumption that no regression will ever creep in), or * you require that bugs in wine routines are fixed in the order that they are called in that test (which would be strange) How about a patch set which does: 1/2. add a workaround to the tests to pass under Wine 2/2. fix Wine code and simultaneously remove the workaround added by 1/2. I don't have a fix for WaitCommEvent() does not return ERROR_INVALID_PARAMETER without ovl structure And I had no plans to do so nor did I claim. I removed the side effect that this test makes the next test fail which tests the EV_TXEMPTY handling even if that it would work perfectly. So: test A test B test A fails on wine and this influences test B in such a way that it will always fail, too. I remove this dependency. Now test B succeeds. This is not because I worked arround a wine-bug in what B should test but because wine behaves correctly in this test. And test A still fails. But that looks even more strange if the fix contained in 2/2 could be sent alone. Regards, -- Wolfgang Walter Studentenwerk München Anstalt des öffentlichen Rechts Abteilungsleiter IT Leopoldstraße 15 80802 München
Re: [PATCH 2/5] wait until all data from earlier test has been written in test_waittxempty
Am Donnerstag, 5. September 2013, 23:32:00 schrieben Sie: Wolfgang Walter w...@stwm.de wrote: With my patches 1/2 and 2/2 I get: fixme:iphlpapi:NotifyAddrChange (Handle 0x10ee8e0, overlapped 0x10ee8ec): stub wine: configuration in '/home/walterw/.wine' has been updated. fixme:comm:set_queue_size insize 1024 outsize 1024 unimplemented stub comm.c:857: Test succeeded inside todo block: WaitCommEvent failed with a timeout comm.c:881: Test succeeded inside todo block: WaitCommEvent error 997 comm.c:883: Test succeeded inside todo block: WaitCommEvent: expected EV_TXEMPTY, got 0x4 comm.c:889: Test succeeded inside todo block: WaitCommEvent used 1141 ms for waiting err:comm:set_line_control ByteSize fixme:ntdll:server_ioctl_file Unsupported ioctl 1b000c (device=1b access=0 func=3 method=0) comm.c:2036: Tests skipped: interactive tests (set WINETEST_INTERACTIVE=1) Successes inside of todo_wine blocks are treated as failres. So you think I should remove the wine_todos already here? No, the source of the failures is still there. What do you mean with that? The tests indeed do succeed now and there is a reason they do: when you call WaitCommEvent() while the tx buffer is not empty yet the wine code will detect that EV_TXEMPTY correctly: The tests must pass under Wine without any additional fixes as they do currently under Windows. If you add some code to the tests which suddenly makes them pass under Wine - that's not a fix, Wine implementation should be fixed instead. You basically say that one may not fix a bug before fixing another one which is not related only because they are tested for in a special order and these tests influence each other though they are not really related. Again: In this case not wine behaves incorrectly but the test is simply wrong. It first tests if WaitCommEvent() returns ERROR_INVALID_PARAMETER if called with NULL without without ovl structure. Then it tests the EV_TXEMPTY handling. The latter test will always fail even if it works perfectly as long as wine does not return ERROR_INVALID_PARAMETER but instead does the write in the first test. This is because the second tests does not wait long enough for both writes to complete. Regards, -- Wolfgang Walter Studentenwerk München Anstalt des öffentlichen Rechts Abteilungsleiter IT Leopoldstraße 15 80802 München
Re: [4/4] ntdll: Properly set flag which indicates buffer empty state.
Am Donnerstag, 29. August 2013, 10:47:46 schrieb Dmitry Timoshkov: Wolfgang Walter w...@stwm.de wrote: I think that happens: * application writes data to com port. * all is written, serial buffer is empty * application calls WaitCommEvent() * wait_on() is called * wait_on() calls get_irq_info() * get_irq_info() sets commio-irq_info-temt = 1 * wait_on() calls check_events() and uses sets commio-irq_info for old an new * so old-temt == new-temt and EV_TXEMPTY is not set * if there are no other events (in real world basically EV_RXCHAR): * wait_for_event() is startet with commio-irq_info-temt set to one * wait_for_event() calls get_irq_info() with new_irq_info() * get_irq_info() sets new_irq_info-temt = 1 because the buffer is still empty * wait_for_event() calls check_events() with new_irq_info and commio-irq_info * again check_events() will not set EV_TXEMPTY as both have temt == 1 It seems that we do not recover from that hang. Please correct me if I misread the code and I'm wrong. I think it's better if WaitCommEvent() returns with EV_TXEMPTY even if there has no new data been sent in between: That means that WaitCommEvent(EV_TXEMPTY) without any prior WriteFile would always return success and signal the EV_TXEMPTY event. My tests show that WaitCommEvent(EV_TXEMPTY) fails with a timeout in this case. Since the serial device is very slow it should be unlikely that after successful WriteFile() with some data WaitCommEvent() sees an already empty transmitter's output queue. Maybe for tests. But the application which fails here (with vanilla wine) hangs because it waits for EV_TXEMPTY. Basically it writes data to several serial ports, then write to files and then waits for the EV_TXEMPTYs. Regards, -- Wolfgang Walter Studentenwerk München Anstalt des öffentlichen Rechts Abteilungsleiter IT Leopoldstraße 15 80802 München
Re: [4/4] ntdll: Properly set flag which indicates buffer empty state.
On Wednesday 28 August 2013 12:41:00 Dmitry Timoshkov wrote: Dmitry Timoshkov dmi...@baikal.ru wrote: Alexandre Julliard julli...@winehq.org wrote: It doesn't work here: ../../../tools/runtest -q -P wine -M kernel32.dll -T ../../.. -p kernel32_test.exe.so comm.c touch comm.ok comm.c:835: Test failed: WriteFile took 1 ms to write 17 Bytes at 150 Baud comm.c:848: Test failed: WaitCommEvent failed with a timeout comm.c:859: Test failed: WaitCommEvent error 997 comm.c:860: Test failed: WaitCommEvent: expected EV_TXEMPTY, got 0 comm.c:865: Test failed: WaitCommEvent used 1000 ms for waiting comm.c:1895: Tests skipped: interactive tests (set WINETEST_INTERACTIVE=1) make: *** [comm.ok] Error 5 I assume it's a real hardware and not a VM? Is this with a real COM port, or USB-serial cable? If the latter one what driver is it using? Looking at the failure messages above once again, I can say that WriteFile failure is definitely not caused by this patch. Does running the test several times cause the same failures? In any case it would be interesting to see the +comm log with ntdll: Add a trace for transmitter's buffer empty flag. applied. And current logic of setting the empty buffer flag is really broken, it always sets the flag to a not zero value if ioctl(TIOCOUTQ) succeeds. Yes, but this should be no problem for the test. If you only have TIOCOUTQ it is very difficult to do otherwise. You would have to monitor any write to the serial port by any thread. The applications I know write something and then wait, so the imperfect emulation we have now is enough. Regards, -- Wolfgang Walter Studentenwerk München Anstalt des öffentlichen Recht Abteilungsleiter IT Leopoldstraße 15 80802 München
Re: [4/4] ntdll: Properly set flag which indicates buffer empty state.
Hello Dimitry,
I think I now found the difference between my patches and yours and what makes
that one application fail (with or without [4/4] ntdll: Properly set flag
which indicates buffer empty state.)
My patch removes the attempt to flag EV_TXEMPTY only once.
In check_events() EV_TXEMPTY is set:
if (mask EV_TXEMPTY)
{
if (!old-temt new-temt)
ret |= EV_TXEMPTY;
}
I think that happens:
* application writes data to com port.
* all is written, serial buffer is empty
* application calls WaitCommEvent()
* wait_on() is called
* wait_on() calls get_irq_info()
* get_irq_info() sets commio-irq_info-temt = 1
* wait_on() calls check_events() and uses sets commio-irq_info for old an new
* so old-temt == new-temt and EV_TXEMPTY is not set
* if there are no other events (in real world basically EV_RXCHAR):
* wait_for_event() is startet with commio-irq_info-temt set to one
* wait_for_event() calls get_irq_info() with new_irq_info()
* get_irq_info() sets new_irq_info-temt = 1 because the buffer is still empty
* wait_for_event() calls check_events() with new_irq_info and commio-irq_info
* again check_events() will not set EV_TXEMPTY as both have temt == 1
It seems that we do not recover from that hang.
Please correct me if I misread the code and I'm wrong.
I think it's better if WaitCommEvent() returns with EV_TXEMPTY even if there
has no new data been sent in between:
* if an application waits only for EV_TXEMPTY then there is no problem.
* if it waits for i.e. EV_TXEMPTY|EV_RXEMPTY it may busy loops. Still better
then not waiting forever.
Sending EV_TXEMPTY only once after the write buffer got empty is not easy to
implement I think. One way would be: block a write until it finished and
buffer is empty, the set a flag.
Am Mittwoch, 28. August 2013, 12:41:00 schrieb Dmitry Timoshkov:
Dmitry Timoshkov dmi...@baikal.ru wrote:
Alexandre Julliard julli...@winehq.org wrote:
It doesn't work here:
../../../tools/runtest -q -P wine -M kernel32.dll -T ../../.. -p
kernel32_test.exe.so comm.c touch comm.ok comm.c:835: Test failed:
WriteFile took 1 ms to write 17 Bytes at 150 Baud comm.c:848: Test
failed: WaitCommEvent failed with a timeout
comm.c:859: Test failed: WaitCommEvent error 997
comm.c:860: Test failed: WaitCommEvent: expected EV_TXEMPTY, got 0
comm.c:865: Test failed: WaitCommEvent used 1000 ms for waiting
comm.c:1895: Tests skipped: interactive tests (set
WINETEST_INTERACTIVE=1)
make: *** [comm.ok] Error 5
I assume it's a real hardware and not a VM? Is this with a real COM port,
or USB-serial cable? If the latter one what driver is it using?
Looking at the failure messages above once again, I can say that WriteFile
failure is definitely not caused by this patch. Does running the test
several times cause the same failures? In any case it would be interesting
to see the +comm log with ntdll: Add a trace for transmitter's buffer
empty flag. applied.
And current logic of setting the empty buffer flag is really broken, it
always sets the flag to a not zero value if ioctl(TIOCOUTQ) succeeds.
Regards,
--
Wolfgang Walter
Studentenwerk München
Anstalt des öffentlichen Rechts
Abteilungsleiter IT
Leopoldstraße 15
80802 München
Re: [1/4] ntdll: Ignore ioctl(TIOCGICOUNT) failures.
Hello,
I made similar changes so that several applications we use work. I tested your
patches, all but one do work. I don't know why one does not, though.
I attached my patch, maybe it is usefull to you.
Regards,
--
Wolfgang Walter
Studentenwerk München
Anstalt des öffentlichen Rechts
Abteilungsleiter IT
Leopoldstraße 15
80802 MünchenFrom de0a36a35e36c3d5033fbf7c6593768a8f676844 Mon Sep 17 00:00:00 2001
From: Wolfgang Walter w...@stwm.de
Date: Mon, 28 Sep 2009 15:52:32 +0200
Subject: [PATCH] improve handling of usb2serial adapters
wine does not handle most usb2serial adapters very well.
wine decides at compile time if it expects serial interfaces
to support TIOCGICOUNT and TIOCSERGETLSR.
On linux a lot of device drivers of serial use2serial-adapters
don't suport those ioctls.
This patch tries to handle those devices as if
TIOCGICOUNT and TIOCSERGETLSR were not supported by the OS.
---
dlls/ntdll/serial.c | 132 +++
1 file changed, 81 insertions(+), 51 deletions(-)
diff --git a/dlls/ntdll/serial.c b/dlls/ntdll/serial.c
index 5551b3c..c00b6a5 100644
--- a/dlls/ntdll/serial.c
+++ b/dlls/ntdll/serial.c
@@ -804,6 +804,7 @@ typedef struct async_commio
*/
static NTSTATUS get_irq_info(int fd, serial_irq_info *irq_info)
{
+NTSTATUS status = STATUS_NOT_IMPLEMENTED;
#ifdef TIOCGICOUNT
struct serial_icounter_struct einfo;
if (!ioctl(fd, TIOCGICOUNT, einfo))
@@ -815,47 +816,29 @@ static NTSTATUS get_irq_info(int fd, serial_irq_info *irq_info)
irq_info-parity = einfo.parity;
irq_info-brk = einfo.brk;
irq_info-buf_overrun = einfo.buf_overrun;
+return STATUS_SUCCESS;
}
-else
-{
-TRACE(TIOCGICOUNT err %s\n, strerror(errno));
-memset(irq_info,0, sizeof(serial_irq_info));
-return FILE_GetNtStatus();
-}
-#else
-memset(irq_info,0, sizeof(serial_irq_info));
-return STATUS_NOT_IMPLEMENTED;
-#endif
-irq_info-temt = 0;
-/* Generate a single TX_TXEMPTY event when the TX Buffer turns empty*/
-#ifdef TIOCSERGETLSR /* prefer to log the state TIOCSERGETLSR */
-if (ioctl(fd, TIOCSERGETLSR, irq_info-temt))
-{
-TRACE(TIOCSERGETLSR err %s\n, strerror(errno));
-return FILE_GetNtStatus();
-}
-#elif defined(TIOCOUTQ) /* otherwise we log when the out queue gets empty */
-if (ioctl(fd, TIOCOUTQ, irq_info-temt))
-{
-TRACE(TIOCOUTQ err %s\n, strerror(errno));
-return FILE_GetNtStatus();
-}
-else
-{
-if (irq_info-temt == 0)
-irq_info-temt = 1;
-}
+TRACE(TIOCGICOUNT err %s\n, strerror(errno));
+/* EINVAL means: TIOCGICOUNT is not supported by serial driver
+ * in this case behave as if TIOCGICOUNT is not defined by OS
+ */
+if (errno != EINVAL)
+status = FILE_GetNtStatus();
#endif
-return STATUS_SUCCESS;
+memset(irq_info,0, sizeof(serial_irq_info));
+return status;
}
-static DWORD check_events(int fd, DWORD mask,
+static NTSTATUS check_events(int fd, DWORD mask,
const serial_irq_info *new,
const serial_irq_info *old,
- DWORD new_mstat, DWORD old_mstat)
+ DWORD new_mstat, DWORD old_mstat, DWORD *ret)
{
-DWORD ret = 0, queue;
+DWORD queue;
+NTSTATUS status = STATUS_SUCCESS;
+
+*ret = 0;
TRACE(mask 0x%08x\n, mask);
TRACE(old-rx 0x%08x vs. new-rx 0x%08x\n, old-rx, new-rx);
@@ -866,28 +849,67 @@ static DWORD check_events(int fd, DWORD mask,
TRACE(old-brk 0x%08x vs. new-brk 0x%08x\n, old-brk, new-brk);
TRACE(old-buf_overrun 0x%08x vs. new-buf_overrun 0x%08x\n, old-buf_overrun, new-buf_overrun);
-if (old-brk != new-brk) ret |= EV_BREAK;
-if ((old_mstat MS_CTS_ON ) != (new_mstat MS_CTS_ON )) ret |= EV_CTS;
-if ((old_mstat MS_DSR_ON ) != (new_mstat MS_DSR_ON )) ret |= EV_DSR;
-if ((old_mstat MS_RING_ON) != (new_mstat MS_RING_ON)) ret |= EV_RING;
-if ((old_mstat MS_RLSD_ON) != (new_mstat MS_RLSD_ON)) ret |= EV_RLSD;
-if (old-frame != new-frame || old-overrun != new-overrun || old-parity != new-parity) ret |= EV_ERR;
+if (old-brk != new-brk) *ret |= EV_BREAK;
+if ((old_mstat MS_CTS_ON ) != (new_mstat MS_CTS_ON )) *ret |= EV_CTS;
+if ((old_mstat MS_DSR_ON ) != (new_mstat MS_DSR_ON )) *ret |= EV_DSR;
+if ((old_mstat MS_RING_ON) != (new_mstat MS_RING_ON)) *ret |= EV_RING;
+if ((old_mstat MS_RLSD_ON) != (new_mstat MS_RLSD_ON)) *ret |= EV_RLSD;
+if (old-frame != new-frame || old-overrun != new-overrun || old-parity != new-parity) *ret |= EV_ERR;
if (mask EV_RXCHAR)
{
queue = 0;
#ifdef TIOCINQ
if (ioctl(fd, TIOCINQ, queue))
- WARN(TIOCINQ returned error\n);
+{
+if (errno != EINVAL)
+{
+status = FILE_GetNtStatus();
+WARN(TIOCINQ
Re: [1/4] ntdll: Ignore ioctl(TIOCGICOUNT) failures.
Am Dienstag, 27. August 2013, 22:00:59 schrieben Sie: Wolfgang Walter w...@stwm.de wrote: Wolfgang Walter w...@stwm.de wrote: I made similar changes so that several applications we use work. I tested your patches, all but one do work. I don't know why one does not, though. What patch doesn't work for you? I tested all your patches together (indivually they will not really work here). Yeah, the patches need to be tested after applying all of them. They were broken into smaller parts in order to make regression test easier in case of a regression. If I don't know what doesn't work I can't make any improvement. Yes, that's clear. I'll have a closer look what's the main difference. The problematic application uses asynchronous IO. I wrote my patch 2009 (and earlier version 2008) so I have to rethink about it. I remember that TX_TXEMPTY detection was a problem, then. It could get lost. The reason I moved the output buffer empty detection from get_irq_info() into check_events() was simply because I thought handling input queue in check_events() but ouput queue in get_irq_info() seems odd. I also think that it's better to detect that get_irq_info() basicly does not work with drivers not supporting TIOCGICOUNT and it is good to have that information in the callers of get_irq_info(). So I return STATUS_NOT_IMPLEMENTED and handle the situation in the caller. And then it is better to move empty sending queue handling out of get_irq_info(). I handle the TIOCSERGETLSR wrong, as I see now (I should with TIOCSER_TEMT). Regards, -- Wolfgang Walter Studentenwerk München Anstalt des öffentlichen Rechts Abteilungsleiter IT Leopoldstraße 15 80802 München
Re: [4/4] ntdll: Properly set flag which indicates buffer empty state.
On Tuesday 27 August 2013 21:14:05 Alexandre Julliard wrote: Dmitry Timoshkov dmi...@baikal.ru writes: --- dlls/kernel32/tests/comm.c | 5 + dlls/ntdll/serial.c| 13 + 2 files changed, 6 insertions(+), 12 deletions(-) It doesn't work here: ../../../tools/runtest -q -P wine -M kernel32.dll -T ../../.. -p kernel32_test.exe.so comm.c touch comm.ok comm.c:835: Test failed: WriteFile took 1 ms to write 17 Bytes at 150 Baud comm.c:848: Test failed: WaitCommEvent failed with a timeout comm.c:859: Test failed: WaitCommEvent error 997 comm.c:860: Test failed: WaitCommEvent: expected EV_TXEMPTY, got 0 comm.c:865: Test failed: WaitCommEvent used 1000 ms for waiting comm.c:1895: Tests skipped: interactive tests (set WINETEST_INTERACTIVE=1) make: *** [comm.ok] Error 5 I think TIMEOUT (1s) in kernel32/tests/comm.c is too short and the test for 900, too. Here it tooks often 1100 milliseconds and more (with vanilla 1.6). And I checked the actual git-tree: the test fails here, too (but not always). Regards, -- Wolfgang Walter Studentenwerk München Anstalt des öffentlichen Recht Abteilungsleiter IT Leopoldstraße 15 80802 München
Re: [2/3] server: Use tcdrain() instead of tcflush() to implement FlushFileBuffers() for a COM port.
Hello Dmitry,
we had this problem, too. I sent a patch to wine-devel some time ago (I think
2008). It had the same flaw as yours. I sent a different patch after Alexandre
gave me the same answer. I then posted 2009 an different one. It did not make
it into wine, though. I don't know why as there was no answer. Therefor I keep
it since then in our own repository together with other serial fixes (i.e. to
make most USB serial adapters working better).
Attached the patch (rebased on 1.6). We use it rebased on then the actual wine
versions sincd 2009 and it works for us.
Am Montag, 26. August 2013, 10:56:29 schrieb Alexandre Julliard:
Dmitry Timoshkov dmi...@baikal.ru writes:
MSDN for FlushFileBuffers says:
Flushes the buffers of a specified file and causes all buffered data to be
written to a file.
Linux man page says:
tcdrain() waits until all output written to the object referred to by fd
has been transmitted.
It's a blocking call, you can't do that on the server side.
Regards,
--
Wolfgang Walter
Studentenwerk München
Anstalt des öffentlichen Rechts
Abteilungsleiter IT
Leopoldstraße 15
80802 MünchenFrom 2428481c2f68df5149ac3b0383579aa323246a19 Mon Sep 17 00:00:00 2001
From: Wolfgang Walter w...@stwm.de
Date: Tue, 20 Jan 2009 17:52:38 +0100
Subject: [PATCH] serial_flush() implements FlushFileBuffers incorrectly
FlushFileBuffers should write out any data not yet transmitted but
serial_flush() instead discards any data not yet transmitted
This is because it uses tcflush() instead of tcdrain().
Call tcdrain for serial handles directly from NtFlushBuffersFile()
(as it may block) and remove serial_flush() from server/serial.c
---
dlls/ntdll/file.c | 52 +---
server/serial.c | 11 +--
2 files changed, 42 insertions(+), 21 deletions(-)
diff --git a/dlls/ntdll/file.c b/dlls/ntdll/file.c
index 5147ef5..928095c 100644
--- a/dlls/ntdll/file.c
+++ b/dlls/ntdll/file.c
@@ -66,6 +66,9 @@
#ifdef HAVE_SYS_STATFS_H
# include sys/statfs.h
#endif
+#ifdef HAVE_TERMIOS_H
+#include termios.h
+#endif
#ifdef HAVE_VALGRIND_MEMCHECK_H
# include valgrind/memcheck.h
#endif
@@ -2749,19 +2752,46 @@ NTSTATUS WINAPI NtFlushBuffersFile( HANDLE hFile, IO_STATUS_BLOCK* IoStatusBlock
{
NTSTATUS ret;
HANDLE hEvent = NULL;
-
-SERVER_START_REQ( flush_file )
-{
-req-handle = wine_server_obj_handle( hFile );
-ret = wine_server_call( req );
-hEvent = wine_server_ptr_handle( reply-event );
+enum server_fd_type type;
+unsigned int options;
+int needs_close;
+int unix_handle;
+
+ret = server_get_unix_fd( hFile, FILE_WRITE_DATA, unix_handle,
+ needs_close, type, options );
+if (ret) return ret;
+
+TRACE(flush %p (type %d)\n, hFile, type);
+if (type == FD_TYPE_SERIAL) {
+TRACE(tcdrain %p (%d ; %d)\n, hFile, unix_handle, needs_close);
+while (tcdrain( unix_handle ) == -1) {
+TRACE(tcdrain %p (%d ; %d) returned -1 (%d)\n, hFile, unix_handle, needs_close, errno);
+if (errno != EINTR) {
+ret = FILE_GetNtStatus();
+break;
+}
+}
+TRACE(tcdrained %p (%d ; %d)\n, hFile, unix_handle, needs_close);
+} else {
+SERVER_START_REQ( flush_file )
+{
+req-handle = wine_server_obj_handle( hFile );
+ret = wine_server_call( req );
+hEvent = wine_server_ptr_handle( reply-event );
+}
+SERVER_END_REQ;
+if (!ret hEvent)
+{
+ret = NtWaitForSingleObject( hEvent, FALSE, NULL );
+NtClose( hEvent );
+}
}
-SERVER_END_REQ;
-if (!ret hEvent)
-{
-ret = NtWaitForSingleObject( hEvent, FALSE, NULL );
-NtClose( hEvent );
+
+if (ret == STATUS_SUCCESS IoStatusBlock) {
+IoStatusBlock-u.Status = ret;
}
+if (needs_close) close( unix_handle );
+TRACE(flushed %p (type %d) ret=%08x\n, hFile, type, ret);
return ret;
}
diff --git a/server/serial.c b/server/serial.c
index 587fee1..aac20fa 100644
--- a/server/serial.c
+++ b/server/serial.c
@@ -61,7 +61,6 @@ static struct fd *serial_get_fd( struct object *obj );
static void serial_destroy(struct object *obj);
static enum server_fd_type serial_get_fd_type( struct fd *fd );
-static void serial_flush( struct fd *fd, struct event **event );
static void serial_queue_async( struct fd *fd, const async_data_t *data, int type, int count );
struct serial
@@ -107,7 +106,7 @@ static const struct fd_ops serial_fd_ops =
{
default_fd_get_poll_events, /* get_poll_events */
default_poll_event, /* poll_event */
-serial_flush, /* flush */
+no_flush, /* flush */
serial_get_fd_type, /* get_fd_type */
default_fd_ioctl, /* ioctl */
serial_queue_async, /* queue_async
Re: Use tcdrain() instead of tcflush() to implement FlushFileBuffers() for a COM port.
Am Montag, 26. August 2013, 17:13:46 schrieb Alexandre Julliard:
Wolfgang Walter w...@stwm.de writes:
@@ -2749,19 +2752,46 @@ NTSTATUS WINAPI NtFlushBuffersFile( HANDLE hFile,
IO_STATUS_BLOCK* IoStatusBlock
{
NTSTATUS ret;
HANDLE hEvent = NULL;
-
-SERVER_START_REQ( flush_file )
-{
-req-handle = wine_server_obj_handle( hFile );
-ret = wine_server_call( req );
-hEvent = wine_server_ptr_handle( reply-event );
+enum server_fd_type type;
+unsigned int options;
+int needs_close;
+int unix_handle;
+
+ret = server_get_unix_fd( hFile, FILE_WRITE_DATA, unix_handle,
+ needs_close, type, options );
+if (ret) return ret;
You probably don't want to fail just because there's no unix fd.
What is the correct behaviour if there ist no such handle? Should I only fail
in the case FD_FILE_SERIAL ?
In this case
server_get_unix_fd()
and
if (needs_close) close( unix_handle );
could be moved there, too.
+if (ret == STATUS_SUCCESS IoStatusBlock) {
+IoStatusBlock-u.Status = ret;
}
This is an unrelated change and should be a separate patch, with tests
(also for failures cases),
Ok. I have to see how to write tests.
Regards,
--
Wolfgang Walter
Studentenwerk München
Anstalt des öffentlichen Rechts
Abteilungsleiter IT
Leopoldstraße 15
80802 München
Re: Use tcdrain() instead of tcflush() to implement FlushFileBuffers() for a COM port.
Am Montag, 26. August 2013, 19:55:04 schrieben Sie:
Wolfgang Walter w...@stwm.de writes:
Am Montag, 26. August 2013, 17:13:46 schrieb Alexandre Julliard:
You probably don't want to fail just because there's no unix fd.
What is the correct behaviour if there ist no such handle? Should I only
fail in the case FD_FILE_SERIAL ?
You'd want to still try the server call, so that the server can
implement a different behavior if needed.
So like this?
NTSTATUS WINAPI NtFlushBuffersFile( HANDLE hFile, IO_STATUS_BLOCK*
IoStatusBlock )
{
NTSTATUS ret;
HANDLE hEvent = NULL;
enum server_fd_type type;
unsigned int options;
int needs_close;
int unix_handle;
ret = server_get_unix_fd( hFile, FILE_WRITE_DATA, unix_handle,
needs_close, type, options );
if (!ret type == FD_TYPE_SERIAL) {
TRACE(tcdrain %p (%d ; %d)\n, hFile, unix_handle, needs_close);
while (tcdrain( unix_handle ) == -1) {
TRACE(tcdrain %p (%d ; %d) returned -1 (%d)\n, hFile,
unix_handle, needs_close, errno);
if (errno != EINTR) {
ret = FILE_GetNtStatus();
break;
}
}
TRACE(tcdrained %p (%d ; %d)\n, hFile, unix_handle, needs_close);
} else {
SERVER_START_REQ( flush_file )
{
req-handle = wine_server_obj_handle( hFile );
ret = wine_server_call( req );
hEvent = wine_server_ptr_handle( reply-event );
}
SERVER_END_REQ;
if (!ret hEvent)
{
ret = NtWaitForSingleObject( hEvent, FALSE, NULL );
NtClose( hEvent );
}
}
if (needs_close) close( unix_handle );
return ret;
}
Regards,
Wolfgang Walter
P.S.:
The header of NtFlushBuffersFile says:
* RETURNS
* Success: 0. IoStatusBlock is updated.
* Failure: An NTSTATUS error code describing the error.
This was the reason I added
if (ret == STATUS_SUCCESS IoStatusBlock) {
IoStatusBlock-u.Status = ret;
}
None of the applications we use seems to care, though.
--
Wolfgang Walter
Studentenwerk München
Anstalt des öffentlichen Rechts
Abteilungsleiter IT
Leopoldstraße 15
80802 München
questions to your commit wineps.drv: Allow for vertical text printing!
Hello Aric, I read commit 745e7c93c9042f62460f181daaa1f05645560b41 (wineps.drv: Allow for vertical text printing.) and I have some questions: 1) You added the parameter vertical to PSDRV_WriteSetFont() in ps.c but it remains unused. I think it isn't needed. 2) In PSDRV_WriteSetDownloadFont() in download.c you call PSDRV_WriteSetFont() with (lf.lfFaceName[0] == '@') for this new parameter vertical. I can't see why. 3) You added the parameter vertical to get_download_name() in download.c You use it to add _vertical to its name if vertical is true. I can't see why the font needs a different name. This will cause PSDRV_WriteSetDownloadFont() to download a font which is used vertically and horizontally twice. I can't see any reason why this could be necessary. I think that * PSDRV_WriteSetFont() should be changed back and of course the callers, too (in builtin.c and download.c) * get_download_name() should also be changed back (and its call in PSDRV_WriteSetDownloadFont()) Regards, -- Wolfgang Walter Studentenwerk München
regression bisected to commit: gdi32: Implement nulldrv_StretchDIBits using the PutImage gdi driver function.
Hello, I just tested version 1.3.33. With this version an application we use shows its icons with wrong colors. The background of the icons is wrong, too. I bisected it to commit c9a7bb715d2db1512db30deb11e4676e76791a07. I then checked that by replacing nulldrv_StretchDIBits in 1.3.33 with its older implementation. c9a7bb715d2db1512db30deb11e4676e76791a07 is the first bad commit commit c9a7bb715d2db1512db30deb11e4676e76791a07 Author: Huw Davies h...@codeweavers.com Date: Mon Oct 17 15:46:07 2011 +0100 gdi32: Implement nulldrv_StretchDIBits using the PutImage gdi driver function. :04 04 e4cfeee3adcb0fe2c0502909c87b2740a8de08e5 00851b246b095bdf4cafd3e6f9965c811d1c87a4 M dlls Regards, -- Wolfgang Walter Studentenwerk München Anstalt des öffentlichen Rechts Abteilungsleiter IT Leopoldstraße 15 80802 München
Re: patch for dlls/gdi32/dib.c: fixes crash
Am Donnerstag, 28. Juli 2011 schrieb Huw Davies: On Thu, Jul 28, 2011 at 04:14:56PM +0200, Wolfgang Walter wrote: I think that bitmapinfo_from_user_bitmapinfo() is the real culprit. I think colors gets to big ( 256) and therefore the size for the memcpy. Hopefully http://source.winehq.org/patches/data/77076 should fix this. Yes, it does. Thanks. -- Wolfgang Walter Studentenwerk München Anstalt des öffentlichen Rechts
Re: patch for dlls/gdi32/dib.c: fixes crash
Am Mittwoch, 27. Juli 2011 schrieben Sie: On Wed, Jul 27, 2011 at 6:44 PM, Wolfgang Walter w...@stwm.de wrote: - char src_bmibuf[FIELD_OFFSET( BITMAPINFO, bmiColors[256] )]; - BITMAPINFO *src_info = (BITMAPINFO *)src_bmibuf; - char dst_bmibuf[FIELD_OFFSET( BITMAPINFO, bmiColors[256] )]; - BITMAPINFO *dst_info = (BITMAPINFO *)dst_bmibuf; There's another instance of that in GetDIBits. Allocating 2KB+ (2 x 1064 bytes) of data on the stack is not very reasonable I guess. Hmm, allocating the structure on the stack is not really the problem. The problem is that part if the stack gets overwritten. Allocating the these structures on the heap hides the bug mostly. I think that bitmapinfo_from_user_bitmapinfo() is the real culprit. I think colors gets to big ( 256) and therefore the size for the memcpy. I just insert an test in bitmapinfo_from_user_bitmapinfo() if colors is larger then 256 and this is indeed the case. Regards, -- Wolfgang Walter Studentenwerk München Anstalt des öffentlichen Rechts
windows are incompletely drawn: bisected to commit 7864ade5a8306c0078e16ae6d7e40bdece29395b
An application we use shows incompletely painted windows with 1.3.19. I bisected it to commit 7864ade5a8306c0078e16ae6d7e40bdece29395b is the first bad commit commit 7864ade5a8306c0078e16ae6d7e40bdece29395b Author: Dmitry Timoshkov dmi...@codeweavers.com Date: Tue Sep 14 14:24:11 2010 +0900 winex11.drv: Avoid copying invalid window bits. I reverted it in 1.3.19 and that fixed indeed the problem. The wrongly painted windows are maximized windows within a MDI window: if there are serveral such windows opened and one switches between them parts of the newly active window is not painted and the content of the old one is still displayed. Regards, -- Wolfgang Walter Studentenwerk München Anstalt des öffentlichen Rechts
Re: serial: Fix race for IOCTL_SERIAL_WAIT_ON_MASK
Am Dienstag, 19. April 2011 schrieb Alexandre Julliard:
Wolfgang Walter wolfgang.wal...@stwm.de writes:
set status field of piosb to STATUS_PENDING before calling wait_on(). If
wait_on returns with STATUS_PENDING don't touch piosb.
Reason: if wait_on returns with STATUS_PENDING it started a thread which
itself modifies the status field.
In general the iosb is not modified until the operation completes.
Sorry, don't understand what you mean.
Without that patch an application of us does not work correctly (it hangs
regularly).
This is the unpatched code:
1036 static inline NTSTATUS io_control(HANDLE hDevice,
1256 case IOCTL_SERIAL_WAIT_ON_MASK:
1257 if (lpOutBuffer nOutBufferSize == sizeof(DWORD))
1258 {
1259 if (!(status = wait_on(hDevice, fd, hEvent, piosb,
lpOutBuffer)))
1260 sz = sizeof(DWORD);
1261 }
1262 else
1263 status = STATUS_INVALID_PARAMETER;
1264 break;
...
1272 }
1273 if (needs_close) close( fd );
1274 error:
1275 piosb-u.Status = status;
1276 piosb-Information = sz;
1277 if (hEvent status != STATUS_PENDING) NtSetEvent(hEvent, NULL);
1278 return status;
1279 }
So this is my theorie:
* wait_on() is called.
* wait_on() calls RtlQueueWorkItem(wait_for_event, commio, 0 /* FIXME */)
and returns with STATUS_PENDING
* wait_for_event() finishes and sets iosb before io_control() continues
* io_control sets iosb to STATUS_PENDING
= hang
I think it is not correct to manipulate iosb once
RtlQueueWorkItem(wait_for_event, commio, 0 /* FIXME */) has been called.
Regards,
--
Wolfgang Walter
Studentenwerk München
Anstalt des öffentlichen Rechts
Re: serial: Fix race for IOCTL_SERIAL_WAIT_ON_MASK
Am Dienstag, 19. April 2011 schrieb Alexandre Julliard: Wolfgang Walter wolfgang.wal...@stwm.de writes: So this is my theorie: * wait_on() is called. * wait_on() calls RtlQueueWorkItem(wait_for_event, commio, 0 /* FIXME */) and returns with STATUS_PENDING * wait_for_event() finishes and sets iosb before io_control() continues * io_control sets iosb to STATUS_PENDING The last one is a bug. iosb should only be set once we have a result. Check how the other async I/O functions do it. It is done similar elsewhere when getting asynchron, i.e. in dlls/ws2_32/socket.c: WS2_ConnectEx(): or dlls/kernel32/file.c: WriteFile() * set iosb to STATUS_PENDING, * call function * if it returns with STATUS_PENDING don't touch iosb. Regards, -- Wolfgang Walter Studentenwerk München Anstalt des öffentlichen Rechts
Re: Asynchronus serial port
Am Donnerstag, 3. September 2009 schrieb Mike Kaplinskiy:
On Wed, Sep 2, 2009 at 7:31 PM, Fenixk19fenix...@mail.ru wrote:
Hello! I've already post the
bug(http://bugs.winehq.org/show_bug.cgi?id=19713) on this subject, but i
need more help. So I've decided to write here.
There is a problem in wine. When I use asynchronous serial port read,
data never comes. Event, caused by select comes. But operation status
stays pending, and i can't do anything to this serial port anymore. In
windows it never get pending, and port can be accessed just after data
arrival. Seems to be wineserver problem, but i don't know, where to look
at. What code respond for asynchronous serial port in wineserver?
Alexander.
P.S. Test program attached.
Hi,
Alexandre would be the guy to talk to about wineserver-related things.
Sadly he's off on a long weekend. Does the attached patch help solve
the problem?
Isn't setting commio-iosb-u.Status racy?
commio-iosb may be set from wait_for_event() which is called by an extra
thread or by io_control().
Couldn't it happen that
wait_on starts the thread
wait_for_event sets the commio-iosb-u.Status to STATUS_SUCCESS
wait_on returns to io_control STATUS_PENDING
io_control overwrites commio-iosb-u.Status with STATUS_PENDING
I think io_control() should set commio-iosb-u.Status to STATUS_PENDING
before calling wait_on(). After wait_on() io_control() should not set
commio-iosb-u.Status if wait_on() returns STATUS_PENDING. Here the idea:
--
case IOCTL_SERIAL_WAIT_ON_MASK:
if (lpOutBuffer nOutBufferSize == sizeof(DWORD))
{
piosb-u.Status = STATUS_PENDING;
piosb-Information = sz;
if (!(status = wait_on(hDevice, fd, hEvent, piosb, lpOutBuffer)))
sz = sizeof(DWORD);
else if (status == STATUS_PENDING)
return status;
}
else
status = STATUS_INVALID_PARAMETER;
break;
---
Regards,
--
Wolfgang Walter
Studentenwerk München
Anstalt des öffentlichen Rechts
Leiter EDV
Leopoldstraße 15
80802 München
Re: Problem with async. comm from Wine to a USB-Serial-converter
Am Donnerstag, 28. Mai 2009 schrieb Henrik Jacobsson: Hi. I just got myself a USB-GPS (GlobalSat BU-353) and wanted to run SeaClearII, a navigation-program with it. Turns out that Wine got confused when the USB-Serial-driver wasn't acting like a real serial-device though. The app does what it's supposed to do, but Wine consumes 100% of the available CPU. After some googling I found a patch that resolved this issue perfectly, and I'm curious why it never made it into trunk. The patch: http://www.nabble.com/serial-04-04%3A-fix%3A-WaitCommEvent()-does-not-work- asynchronously--make-WaitCommEvent()-better-usable-when-TIOCGICOUNT-is-not-i mplemented-tc20980550.html I only managed to apply this patch to 1.1.17. Later releases had changed too much for my non-programmer-brain to cope with.. I would really appreciate if someone in the know could check the patch out and if possible apply it to wine-proper.. Thanks in advance.. /Henrik I posted the patch on wine-patches. But nobody commented on it so I don't know if it is ok or not. As we need it for several applications we use I have a version for newer wine versions. It is attached. The patch actually fixes 3 things: 1) Fix WaitCommEvent() when used asynchronously 2) make wait_for_event() detect when serial fd is not valid any more 3) make events work better with serial devices not supporting TIOCGICOUNT (Most drivers of usb2serial adapters do not support TIOCGICOUNT, nor do pseudo terminals) Fix for 3) could be separated from 1) und 2) but I can't test 1) and 2) without 3). Regards -- Wolfgang Walter Studentenwerk München Anstalt des öffentlichen Rechts Leiter EDV Leopoldstraße 15 80802 München From 952a344bb64a9b4c2957abd290ccda2c8118f380 Mon Sep 17 00:00:00 2001 From: Wolfgang Walter w...@stwm.de Date: Fri, 12 Dec 2008 13:21:05 +0100 Subject: [PATCH] serial: fix: WaitCommEvent() does not work asynchronously; make WaitCommEvent() better usable when TIOCGICOUNT is not implemented 1) If WaitCommEvent() is called with LPOVERLAPPED structure it returns if no event is pending. When the application then receives a the completion event it may use GetOverlappedResult() to get the status. This status is always STATUS_PENDING w ith wine even if it should be STATUS_SUCCESS. To fix it: wait_on() takes the PIO_STATUS_BLOCK as fifth argument. struct async_commio has an additional member piosb. If there are no events pending wait_on() sets the status of the PIO_STATUS_BLOCK to STATUS_PENDING , then starts a thread with RtlQueueWorkItem(wait_for_event, commio) and return with STATUS_PENDING to io_control() io_control() must not set the status of the PIO_STATUS_BLOCK when wait_on return s with STATUS_PENDING as the event check thread may change it in between. In all other cases io_control() sets the status of the PIO_STATUS_BLOCK as before. wait_for_event sets the status of the PIO_STATUS_BLOCK when either a event or an error occurs or when the application calls SetCommMask(handle,0); 2) wait_for_event() should detect when serial fd is not valid any more The application calls SetCommMask(handle,0) and then closes handle. In this case a wait_for_event() thread may be still active but its file handle is not valid any more. Probably the application should wait for the asynchronous WaitCommEvent to finis h before closing the handle. But I did not found any clear hint in MSD - and the application works fine under windows. First I check the event mask when the thread wakes up from NtDelayExecution(). This alone should probably avoid the problem. But for correctness I try to detect when get_irq_info, get_modem_status or check_events fail because the filehandle has gone. I modified check_events so that it now returns a NTSTATUS instead the found even ts. They are returned via an OUT argument *ret. I don't know if there may be still a small window for a race: app calls SetCommMask(handle,0) app closes handle app opens another file/device, unix fd is used again 3) make events work better with serial devices not supporting TIOCGICOUNT Most drivers of usb2serial adapters do not support TIOCGICOUNT nor do pseudo ter minals. I modified get_irq_info() so that it behaves in this case as if TIOCGICOUNT is n ot defined at compilation time: it returns with STATUS_NOT_IMPLEMENTED; I further modified wait_on() so that if get_irq_info() returns with STATUS_NOT_I MPLEMENTED wait_on() does not fail with an error if at least EV_RXCHAR or EV_TXE MPTY is in the waitmask. Or in other words: an applikation may use EV_BREAK or E V_ERR together with EV_RXCHAR or EV_TXEMPTY. I found that in our case all vendor dlls always set EV_BREAK|EV_ERR when they wa it on EV_RXCHAR or EV_TXEMPTY. They never expected SetCommMask(handle, EV_BREAK| EV_ERR|EV_RXCHAR) to fail. Instead there was a busy loop resetting the comm-port , calling SetCommMask(), ... wait_on() still fails when neiter EV_RXCHAR or EV_TXEMPTY is set an get_irq_info returns STATUS_NOT_IMPLEMENTED
Re: Problem with async. comm from Wine to a USB-Serial-converter
Am Mittwoch, 3. Juni 2009 schrieben Sie: 2009/6/3 Wolfgang Walter w...@stwm.de: I posted the patch on wine-patches. But nobody commented on it so I don't know if it is ok or not. As we need it for several applications we use I have a version for newer wine versions. It is attached. The patch actually fixes 3 things: 1) Fix WaitCommEvent() when used asynchronously 2) make wait_for_event() detect when serial fd is not valid any more 3) make events work better with serial devices not supporting TIOCGICOUNT (Most drivers of usb2serial adapters do not support TIOCGICOUNT, nor do pseudo terminals) Fix for 3) could be separated from 1) und 2) but I can't test 1) and 2) without 3). If it's possible, submit one patch for each one thing getting fixed. Separating 1) und 2) does not make sense. 3) needs fixes 1) and 2). Separating 3) is possible but I can't test 1) and 2) alone. Regards, -- Wolfgang Walter Studentenwerk München Anstalt des öffentlichen Rechts Leiter EDV Leopoldstraße 15 80802 München
Re: bugfix: resend: fix serial_flush
On Monday 22 December 2008, Alexandre Julliard wrote:
Wolfgang Walter w...@stwm.de writes:
@@ -201,7 +202,17 @@ static void serial_flush( struct fd *fd, struct event
**event )
/* MSDN says: If hFile is a handle to a communications device,
* the function only flushes the transmit buffer.
*/
-if (tcflush( get_unix_fd(fd), TCOFLUSH ) == -1) file_set_error();
+/* FlushFileBuffers does NOT have the semantics of tcflush.
+ * Whereas tcflush discards any data not yet transmitted
+ * FlushFileBuffers ensures they are written out.
+ * The POSIX equivalent is tcdrain
+ */
+while (tcdrain( get_unix_fd(fd)) == -1) {
+ if (errno != EINTR) {
+file_set_error();
+return;
+}
This will block, you can't do that in the server.
--
Alexandre Julliard
julli...@winehq.org
Would it be acceptable to call tcdrain directly in NtFlushBuffersFile:
=
NTSTATUS WINAPI NtFlushBuffersFile( HANDLE hFile, IO_STATUS_BLOCK*
IoStatusBlock )
{
NTSTATUS ret;
HANDLE hEvent = NULL;
enum server_fd_type type;
NTSTATUS status;
unsigned int options;
int needs_close;
int unix_handle;
status = server_get_unix_fd( hFile, FILE_WRITE_DATA, unix_handle,
needs_close, type, options );
if (type == FD_TYPE_SERIAL) {
while (tcdrain(unix_handle) == -1) {
if (errno != EINTR) {
return FILE_GetNtStatus();
}
}
return STATUS_SUCCESS;
}
SERVER_START_REQ( flush_file )
{
req-handle = wine_server_obj_handle( hFile );
ret = wine_server_call( req );
hEvent = wine_server_ptr_handle( reply-event );
}
SERVER_END_REQ;
if (!ret hEvent)
{
ret = NtWaitForSingleObject( hEvent, FALSE, NULL );
NtClose( hEvent );
}
return ret;
}
=
Regards,
--
Wolfgang Walter
Studentenwerk München
Anstalt des öffentlichen Rechts
Leiter EDV
Leopoldstraße 15
80802 München
Re: wineps: clipping bug; clipping performance
Am Dienstag, 16. Dezember 2008 11:51 schrieb Kai Blin: On Tuesday 16 December 2008 11:42:55 Wolfgang Walter wrote: I have not received any comment yet. That's probably because not too many people know much about this. Detlef Riekenberg is probably the person who should comment on this, I've CCed him. Cheers, Kai Thanks. Then I'll send him my patch concerning the use of glyphshow, too :-). Regards, -- Wolfgang Walter Studentenwerk München Anstalt des öffentlichen Rechts Leiter EDV Leopoldstraße 15 80802 München
wineps: clipping bug; clipping performance
Hello, as I wrote clipping in wineps.drv has a bug. Wine does only consider the clipping region and not the meta region. It should combine them. This is a bug which hurts us with a real windows application. I sent a patch which which corrects this. My patch also addressed a general performance problem with all postscript printers from Kyocera concerning non trivial clipping regions. Even if you use ghostscripts pxlmono/pxlcolor driver the generated PCL XL code may lead to errors on our Kyocera and HP printers because the clipping path gets to expensive. I have not received any comment yet. Regards, -- Wolfgang Walter Studentenwerk München Anstalt des öffentlichen Rechts Leiter EDV Leopoldstraße 15 80802 München
Use show instead of glyphshow: gylphshow very slow on kyocera
Hello, wineps.drv uses the postscript operator glyphshow to print glyphs. This operator is very slow on all Kyocera postscript printers. To get an idea: its generally only 1/6 to 1/10 of the performance compared to the same document printed with the windows xp postscript driver (which does not use glyphsow). I wrote an email hier where I proposed a patch. This patch changes wineps.drv such that it uses the show operator for downloaded fonts (type1 and type42). This change is not trivial as the show operator (and its relatives xshow, yshow, xyshow) operates on character codes and not on glyhpnames. So wineps.drv must generate postscript code define an appropriate encoding. As a font may only have up to 256 characters encoded and as it is not allowed to modify the encoding once defined the font must be cloned if more then 256 characters are used. This is inexpensive, though, as actually all resources but the encoding array are shared. See the patch my earlier mail for a more detailed explanation. This modification brings the performance of the postscript code generated by wine up to windows' postscript driver (if no clipping region is defined - another problem). I tested it on a wide range of kyocera printer models and two hp models. Of course I tested it with ghostscript. I would appreciate any comments and ideas. Regards, -- Wolfgang Walter Studentenwerk München Anstalt des öffentlichen Rechts Leiter EDV Leopoldstraße 15 80802 München
Use show instead of glyphshow because glyphshow is very slow on kyocera
Hello,
this is is a proposed change to the wineps driver.
Problem: wine uses the postscript command glyphshow to print characters. At
least all Postscript Printers from Kyocera are very slow when showglyph is
used. Even simple pages with only one downloaded truetype font slow down fast
printers as the Kyocera FS-4000DN or FS-9520DN so that they print not more
then 3 pages/minute - often only one to two.
I can only speculate why glyphshow is such bad for performance. The generic
windows postscript printer driver does usually not use showglyph. Nor do the
driver from Kyocera. Applications as OpenOffice don't do either.
The following patch changes wineps.drv such that it uses show (for downloaded
truetype fonts). It still contains code which inserts postscript comments in
its output for debugging purpose. It's meant for discussion.
Regards,
Wolfgang Walter
--
Wolfgang Walter
Studentenwerk München
Anstalt des öffentlichen Rechts
Leiter EDV
Leopoldstraße 15
80802 München
From e3cfd6d376bb397f8378fe65127496d7b30b4826 Mon Sep 17 00:00:00 2001
From: Wolfgang Walter [EMAIL PROTECTED]
Date: Sat, 6 Dec 2008 17:43:35 +0100
Subject: Use show instead of glyphshow
On many postscript printers glyphshow is slow. Especially all Kyocera models
from FS-1020 up to FS-4000DN or FS-9520DN are very slow when glyphshow is used
with downloaded truetype fonts. Even simple text-only pages are rendered at 1/6
to 1/10 of the usual time (i.e. reached under windows with the generic window
postscript driver).
I don't know why glyphshow is slow but I suppose that its usage is not optimised
as the eneric windows postscript printer driver seems not to use glyphshow. Nor
do the driver from Kyocera. Applications as OpenOffice don't do either.
This change modifies wineps.drv such that it uses show for downloaded fonts as
other the window printer drivers do.
Show accesses glyphs of base fonts by there character code in the encoding array
of the font. Only 256 glyphs may be encoded. Therefor I clone the font if
necessary. As the clones share all resources but their encoding array with the
original font this is not expensive.
Actually I limit the number of clones (by undefining older ones) but it's
probably not worth the code:
* the gylphs themselves are the really expensive part
* all fonts are released when a page is rendered
I had to modify how font setting works as I have to set the clone with the
correct encoding for each character.
With this modification printing perfomance is up to what one would expect. I
tested this on several Kyocera laser printers (FS-1010, FS-1030, FS-1300,
FS-C5025, FS-4000DN, FS-9520DN) and also on a HP Laserjet and a Minolta
Laserprinter. And of course with ghostscript.
This patch still contains code which creates comments in the generated
postscript file for debugging purpose.
---
dlls/wineps.drv/download.c | 337 +++-
dlls/wineps.drv/font.c |4 +-
dlls/wineps.drv/ps.c | 14 ++
dlls/wineps.drv/psdrv.h| 16 ++
dlls/wineps.drv/type1.c| 27 +---
dlls/wineps.drv/type42.c | 27 +---
6 files changed, 347 insertions(+), 78 deletions(-)
diff --git a/dlls/wineps.drv/download.c b/dlls/wineps.drv/download.c
index e794af6..bd40ef0 100644
--- a/dlls/wineps.drv/download.c
+++ b/dlls/wineps.drv/download.c
@@ -42,6 +42,23 @@ WINE_DEFAULT_DEBUG_CHANNEL(psdrv);
#define GET_BE_DWORD(ptr) ((DWORD)MAKELONG( GET_BE_WORD(((WORD *)(ptr))[1]), \
GET_BE_WORD(((WORD *)(ptr))[0]) ))
+#define GLYPH_SENT_INC 128
+#define MAX_ENC_PER_FONT 16 /* must be = 0xfe */
+#define OPT_ENC_PER_FONT 10 /* when we should start to undefine clones */
+#define MIN_ENC_PER_FONT 4 /* when we undefine: how agressive */
+
+static const char fontname[] = %s_wine_%04X;
+static const char undefinefont[] = /%s_wine_%04X undefinefont\n;
+static const char setdeffont[] = /%s findfont 40 scalefont setfont\n;
+
+
+#if DOWNLOAD_SPOOLBUF_MINPLUS = 1000
+#error DOWNLOAD_SPOOLBUF_MINPLUS must be greater then 1000
+#endif
+#if MAX_ENC_PER_FONT 0xfe
+#error MAX_ENC_PER_FONT must not exceed 0xfe
+#endif
+
/
* get_download_name
*/
@@ -192,7 +209,7 @@ static BOOL get_bbox(PSDRV_PDEVICE *physDev, RECT *rc, UINT
*emsize)
*/
BOOL PSDRV_SelectDownloadFont(PSDRV_PDEVICE *physDev)
{
-char *ps_name;
+char *ps_name = NULL;
LPOUTLINETEXTMETRICA potm;
DWORD len = GetOutlineTextMetricsA(physDev-hdc, 0, NULL);
@@ -219,6 +236,44 @@ BOOL PSDRV_SelectDownloadFont(PSDRV_PDEVICE *physDev)
}
/
+ * Download_WriteSetFont
+ *
+ * If force == TRUE or g_enc != pdl-glyph_enc_last_used:
+ * write postscript code to select the clone of font pdl
+ * where encoding g_enc is mapped as character code (g_enc 0xff)
+ *
+ */
+static BOOL Download_WriteSetFont(PSDRV_PDEVICE
