February 2014 - Wine-Devel - list.winehq.org

Re: ddraw/tests: Simplify the 'NT4 testbot' special case a bit.

by Marvin

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 https://newtestbot.winehq.org/JobDetails.pl?Key=5209 Your paranoid android. === w2000pro (32 bit ddraw2) === ddraw2: unhandled exception c0000005 at 004267A5 === w8 (32 bit ddraw2) === Timeout === w8 (32 bit ddraw1) === ddraw1.c:4113: Test failed: Got unexpected refcount 3. ddraw1.c:4132: Test failed: Got unexpected refcount 3. ddraw1.c:4139: Test failed: Got unexpected hr 0. ddraw1.c:4142: Test failed: Got unexpected refcount 3. ddraw1.c:4144: Test failed: Got unexpected refcount 2. ddraw1.c:4150: Test failed: Got unexpected hr 0. ddraw1.c:4153: Test failed: Got unexpected refcount 2. ddraw1.c:4155: Test failed: Got unexpected refcount 1.

Feb. 27, 2014

Re: ddraw: Add some more D3DDEVCAPS constants.

by Francois Gouget

On Wed, 26 Feb 2014, Henri Verbeet wrote: > > --- a/include/d3dcaps.h > > +++ b/include/d3dcaps.h > > @@ -302,6 +302,11 @@ typedef struct _D3DDeviceDesc7 { > > #define D3DDEVCAPS_HWTRANSFORMANDLIGHT 0x00010000 > > #define D3DDEVCAPS_CANBLTSYSTONONLOCAL 0x00020000 > > #define D3DDEVCAPS_HWRASTERIZATION 0x00080000 > > +#define D3DDEVCAPS_PUREDEVICE 0x00100000 > > +#define D3DDEVCAPS_QUINTICRTPATCHES 0x00200000 > > +#define D3DDEVCAPS_RTPATCHES 0x00400000 > > +#define D3DDEVCAPS_RTPATCHHANDLEZERO 0x00800000 > > +#define D3DDEVCAPS_NPATCHES 0x01000000 > Where did you find these? They're not in any SDK I have. My mistake. They're supposed to be in d3d8caps.h and d3d9caps.h (and they are already there in Wine), not in plain d3dcaps.h. -- Francois Gouget <fgouget(a)free.fr> http://fgouget.free.fr/ Demander si un ordinateur peut penser revient à demander si un sous-marin peut nager.

Feb. 27, 2014

ddraw/tests: Don't crash after a failed CreateSurface() call.

by Francois Gouget

So there are a number of CreateSurface() calls that fail and these are causing the tests to crash. For instance for ddraw2: http://test.winehq.org/data/tests/ddraw:ddraw2.html Is there a way to check what surface types are supported? create_device() calls EnumDevices() but finding a match clearly does not guarantee we'll be able to create a surface given the error we get there (but presumably that check was more for the following CreateDevice() call). I tried using EnumSurfaces() but without much luck. I'm also a bit worried that DDENUMSURFACES_MATCH is not used anywhere in Wine (*). Alternatively we can just react to failing CreateSurface() calls and clean up + abort a bit like is done in the following patch. (*) Also the DDENUMSURFACES_{SEARCH,MATCH}TYPE macros found in dlls/ddraw/ddraw.c are not used anymore since c8901d6f. diff --git a/dlls/ddraw/tests/ddraw2.c b/dlls/ddraw/tests/ddraw2.c index 5a3bd77..2a5a9c4 100644 --- a/dlls/ddraw/tests/ddraw2.c +++ b/dlls/ddraw/tests/ddraw2.c @@ -227,7 +227,7 @@ static IDirect3DDevice2 *create_device(IDirectDraw2 *ddraw, HWND window, DWORD c surface_desc.dwWidth = 640; surface_desc.dwHeight = 480; hr = IDirectDraw2_CreateSurface(ddraw, &surface_desc, &ds, NULL); - ok(SUCCEEDED(hr), "Failed to create depth buffer, hr %#x.\n", hr); + ok(SUCCEEDED(hr) || broken(hr == DDERR_INVALIDPIXELFORMAT), "Failed to create depth buffer, hr %#x.\n", hr); if (FAILED(hr)) { IDirect3D2_Release(d3d); @@ -4451,6 +4451,8 @@ static void test_surface_lock(void) hr = IDirectDraw2_CreateSurface(ddraw, &ddsd, &surface, NULL); ok(SUCCEEDED(hr), "Failed to create surface, type %s, hr %#x.\n", tests[i].name, hr); + if (FAILED(hr)) + continue; memset(&ddsd, 0, sizeof(ddsd)); ddsd.dwSize = sizeof(ddsd); @@ -4817,6 +4819,8 @@ static void test_set_surface_desc(void) hr = IDirectDraw2_CreateSurface(ddraw, &ddsd, &surface, NULL); ok(SUCCEEDED(hr), "Failed to create surface, hr %#x.\n", hr); + if (FAILED(hr)) + goto done; hr = IDirectDrawSurface_QueryInterface(surface, &IID_IDirectDrawSurface3, (void **)&surface3); ok(SUCCEEDED(hr), "Failed to get IDirectDrawSurface3 interface, hr %#x.\n", hr); @@ -5113,6 +5117,12 @@ static void test_user_memory_getdc(void) ddsd.ddsCaps.dwCaps = DDSCAPS_SYSTEMMEMORY; hr = IDirectDraw2_CreateSurface(ddraw, &ddsd, &surface, NULL); ok(SUCCEEDED(hr), "Failed to create surface, hr %#x.\n", hr); + if (FAILED(hr)) + { + IDirectDraw2_Release(ddraw); + DestroyWindow(window); + return; + } hr = IDirectDrawSurface_QueryInterface(surface, &IID_IDirectDrawSurface3, (void **)&surface3); ok(SUCCEEDED(hr), "Failed to get IDirectDrawSurface3 interface, hr %#x.\n", hr); diff --git a/dlls/ddraw/tests/ddraw4.c b/dlls/ddraw/tests/ddraw4.c index 81c043c..5da4a71 100644 --- a/dlls/ddraw/tests/ddraw4.c +++ b/dlls/ddraw/tests/ddraw4.c @@ -5423,6 +5423,12 @@ static void test_set_surface_desc(void) hr = IDirectDraw4_CreateSurface(ddraw, &ddsd, &surface, NULL); ok(SUCCEEDED(hr), "Failed to create surface, hr %#x.\n", hr); + if (FAILED(hr)) + { + IDirectDraw4_Release(ddraw); + DestroyWindow(window); + return; + } reset_ddsd(&ddsd); ddsd.dwFlags = DDSD_LPSURFACE; @@ -5715,6 +5721,12 @@ static void test_user_memory_getdc(void) ddsd.ddsCaps.dwCaps = DDSCAPS_SYSTEMMEMORY; hr = IDirectDraw4_CreateSurface(ddraw, &ddsd, &surface, NULL); ok(SUCCEEDED(hr), "Failed to create surface, hr %#x.\n", hr); + if (FAILED(hr)) + { + IDirectDraw4_Release(ddraw); + DestroyWindow(window); + return; + } memset(data, 0xaa, sizeof(data)); reset_ddsd(&ddsd); diff --git a/dlls/ddraw/tests/ddraw7.c b/dlls/ddraw/tests/ddraw7.c index 8e01238..4735fca 100644 --- a/dlls/ddraw/tests/ddraw7.c +++ b/dlls/ddraw/tests/ddraw7.c @@ -5301,6 +5301,12 @@ static void test_set_surface_desc(void) hr = IDirectDraw7_CreateSurface(ddraw, &ddsd, &surface, NULL); ok(SUCCEEDED(hr), "Failed to create surface, hr %#x.\n", hr); + if (FAILED(hr)) + { + IDirectDraw7_Release(ddraw); + DestroyWindow(window); + return; + } reset_ddsd(&ddsd); ddsd.dwFlags = DDSD_LPSURFACE; @@ -5594,6 +5600,12 @@ static void test_user_memory_getdc(void) ddsd.ddsCaps.dwCaps = DDSCAPS_SYSTEMMEMORY; hr = IDirectDraw7_CreateSurface(ddraw, &ddsd, &surface, NULL); ok(SUCCEEDED(hr), "Failed to create surface, hr %#x.\n", hr); + if (FAILED(hr)) + { + IDirectDraw7_Release(ddraw); + DestroyWindow(window); + return; + } memset(data, 0xaa, sizeof(data)); reset_ddsd(&ddsd);

Feb. 27, 2014

Re: [PATCH 2/2] Quickly retry sendmsg or recvmsg if errno is EAGAIN or EINTR.

by Erich E. Hoover

On Wed, Feb 26, 2014 at 6:10 PM, Max Woodbury <mtewoodbury(a)gmail.com> wrote: > ... > So it's mainly on style: > > - I've been reading other peoples code for 50 years and that aspect > of K&R is much easier to handle in my opinion. Same with the > extra blank line. Other parts of wine use the style and are much > easier to read as a result. > > - And the miscount can't simply be ignored? Very little of Wine uses a blank line after an if, and I've never seen it with a single line statement. That doesn't really matter though, the rest of the function (and really the whole file) is in Allman style and our current guidelines require matching the surrounding style. (Note: I am not in charge of this, so debating me on the topic is pointless) > If you read kernel code, you'll find that just returning to > application level is sometimes enough to free the missing resource. > That happens often enough that a single quick retry on EAGAIN is > warranted. I'm not familiar enough with what you're trying to do, but if the buffer is full and the kernel throws back EAGAIN then it would seem that you're not in such a time crunch that waiting for the higher level to handle it is going to be a problem. I could be wrong though, I don't do a lot of high-throughput socket programming. > Bringing the relevant part of the retry code down and applying it to > only those parts where is has a strong likelihood of doing the most > good is exactly what the patch does. If one retry is not enough, then > higher level strategy is needed so no changes are called for on the > higher level. A loop at this level is _not_ a good idea since there > would have to be something to break the loop if it runs too long. I believe that Ken was suggesting that there are ways to do this such that the higher level can handle EGAIN and the lower level EINTR. In my, admittedly limited, experience with this issue his suggestion seems reasonable, but I could be wrong. However, I am sure that if you put together a convincing test case that it will make getting the patch accepted a lot easier (though not guaranteed). > In other words, this fiddle is based on several decades of experience with > kernel and bottom level application debugging experience. At > least look at it and test it before rejecting it on superficial grounds. Please do not be confused, I am not in charge of acceptance/rejection. I have, however, been involved in Wine long enough to know that if you don't address the issues mentioned on this list (either with an explanation, the addition of new tests, or changing the code to handle the case mentioned) then AJ will usually reject the patch. Best, Erich

Feb. 27, 2014

Re: [PATCH 2/2] Quickly retry sendmsg or recvmsg if errno is EAGAIN or EINTR.

by Max Woodbury

On 02/26/2014 08:10 PM, Bruno Jesus wrote: > On Wed, Feb 26, 2014 at 6:30 PM, Max Woodbury <mtewoodbury(a)gmail.com> wrote: >> Guild Wars 2. See other sub-thread. > > Can you share the URL to the other sub-thread, please? > > Ken pointed out that I had forgotten to CC the list. I hope that I've fixed that now... max

Feb. 27, 2014

Re: [PATCH 2/2] Quickly retry sendmsg or recvmsg if errno is EAGAIN or EINTR.

by Max Woodbury

On 02/26/2014 06:36 PM, Erich E. Hoover wrote: > On Wed, Feb 26, 2014 at 4:26 PM, Max Woodbury <mtewoodbury(a)gmail.com> wrote: >> Please explain in words of one syllable why this has been rejected? > > Probably for the reasons Ken already mentioned: > 1) Strange blank line after "if (n == -1)" > 2) Unnecessary style change to K&R style ("if (ret >= 0) {") > 3) "[PATCH 2/2]" with no "[PATCH 1/2]" > 4) No explanation for his comment that EAGAIN is probably not needed. > EINTR might want this behavior, but EGAIN should not. > 5) If you bring this code "down" to where you have then you need to > remove it from the "higher" level: > http://source.winehq.org/source/dlls/ws2_32/socket.c#L4494 > http://source.winehq.org/source/dlls/ws2_32/socket.c#L6500 > 6) It should be in a loop > > Does that help? > > Best, > Erich > > So it's mainly on style: - I've been reading other peoples code for 50 years and that aspect of K&R is much easier to handle in my opinion. Same with the extra blank line. Other parts of wine use the style and are much easier to read as a result. - And the miscount can't simply be ignored? If you read kernel code, you'll find that just returning to application level is sometimes enough to free the missing resource. That happens often enough that a single quick retry on EAGAIN is warranted. Bringing the relevant part of the retry code down and applying it to only those parts where is has a strong likelihood of doing the most good is exactly what the patch does. If one retry is not enough, then higher level strategy is needed so no changes are called for on the higher level. A loop at this level is _not_ a good idea since there would have to be something to break the loop if it runs too long. In other words, this fiddle is based on several decades of experience with kernel and bottom level application debugging experience. At least look at it and test it before rejecting it on superficial grounds. max

Feb. 27, 2014

Re: [PATCH 2/2] Quickly retry sendmsg or recvmsg if errno is EAGAIN or EINTR.

by Bruno Jesus

On Wed, Feb 26, 2014 at 6:30 PM, Max Woodbury <mtewoodbury(a)gmail.com> wrote: > Guild Wars 2. See other sub-thread. Can you share the URL to the other sub-thread, please?

Feb. 27, 2014

Re: [PATCH 2/2] Quickly retry sendmsg or recvmsg if errno is EAGAIN or EINTR.

by Erich E. Hoover

On Wed, Feb 26, 2014 at 4:26 PM, Max Woodbury <mtewoodbury(a)gmail.com> wrote: > Please explain in words of one syllable why this has been rejected? Probably for the reasons Ken already mentioned: 1) Strange blank line after "if (n == -1)" 2) Unnecessary style change to K&R style ("if (ret >= 0) {") 3) "[PATCH 2/2]" with no "[PATCH 1/2]" 4) No explanation for his comment that EAGAIN is probably not needed. EINTR might want this behavior, but EGAIN should not. 5) If you bring this code "down" to where you have then you need to remove it from the "higher" level: http://source.winehq.org/source/dlls/ws2_32/socket.c#L4494 http://source.winehq.org/source/dlls/ws2_32/socket.c#L6500 6) It should be in a loop Does that help? Best, Erich

Feb. 26, 2014

Fwd: Re: [PATCH 2/2] Quickly retry sendmsg or recvmsg if errno is EAGAIN or EINTR.

by Max TenEyck Woodbury

-------- Original Message -------- Subject: Re: [PATCH 2/2] Quickly retry sendmsg or recvmsg if errno is EAGAIN or EINTR. Date: Wed, 26 Feb 2014 13:33:22 -0500 From: Max Woodbury <mtewoodbury(a)gmail.com> To: Ken Thomases <ken(a)codeweavers.com> On 02/23/2014 05:23 AM, Ken Thomases wrote: > On Feb 21, 2014, at 6:35 PM, mtewoodbury(a)gmail.com wrote: > >> Note that this greatly improves network reliability. > > How did you determine that? How could we confirm that? Feature of Guild Wars 2 (trading post) that failed to work on wine now works. Lower Game latency. Fewer login failures. > > >> - if ( (n = recvmsg(fd, &hdr, wsa->flags)) == -1 ) >> + n = recvmsg(fd, &hdr, wsa->flags); >> + if (n == -1 && (errno == EINTR || errno == EAGAIN)) >> + n = recvmsg(fd, &hdr, wsa->flags); >> + if (n == -1) >> + >> return -1; > > What's with that blank line between the last "if" and its corresponding statement? To emphasize the exit from the function. I find that I often want to know where such exits are and this makes them easier to fined. Personal preference... > >> - if (ret >= 0) >> - { >> + if (ret == -1 && (errno == EINTR || errno == EAGAIN)) >> + ret = sendmsg(fd, &hdr, wsa->flags); /* Try it again quickly */ >> + if (ret >= 0) { > > Why did you change the existing lines? You could have just added your two lines. Mostly just personal preference... > > In general: > > Is this actually the second patch of a two-patch series as indicated by the "[PATCH 2/2]" > in the subject? I didn't receive the first patch. No, my dyslexia strikes again. > > Have you tested to see if just retrying on EINTR is sufficient to improve the > reliability? EAGAIN is not typically a situation where an immediate retry helps. EAGAIN says specifically resources _temporarily_ unavailable. How temporary is not specified, so one quick retry could fix the problem. > > The callers of WS2_recv() and WS2_send() already have tests for EINTR and EAGAIN. > The strategy for dealing with those should probably exist in just one place. > For my part, I think that all interruptible syscalls should be in a loop that > immediately retries for EINTR, but EAGAIN should usually go back to some select(), > poll(), kevent() or similar wait function. I'd more or less agree except that there is, if I recall correctly, some other stuff that has to happen before the retry at the higher levels and I was getting game feature failures and latency waiting for those fixes. By all means, something like this could and should be done for most system calls. This one just annoyed me enough to try fiddling with it. Since it seemed to make a difference, I posted it... > > -Ken

Feb. 26, 2014

Re: [PATCH 2/2] Quickly retry sendmsg or recvmsg if errno is EAGAIN or EINTR.

by Max Woodbury

Please explain in words of one syllable why this has been rejected?

Feb. 26, 2014