July 2015 - Wine-Devel - list.winehq.org

Re: ws2_32: socket keepalive for OSX

by Charles Davis

> On Jul 29, 2015, at 7:30 PM, Matt Durgavich <mattdurgavich(a)gmail.com> wrote: > --- a/dlls/ws2_32/socket.c > +++ b/dlls/ws2_32/socket.c > @@ -4468,6 +4468,15 @@ INT WINAPI WSAIoctl(SOCKET s, DWORD code, LPVOID in_buff, DWORD in_size, LPVOID […] > + else if (setsockopt(fd, IPPROTO_TCP, TCP_KEEPALIVE, (void*)&keepintvl, sizeof(int)) == -1) > + status = WSAEINVAL; This isn’t correct. Not for Darwin, anyway. TCP_KEEPALIVE on Darwin corresponds to Linux TCP_KEEPIDLE, not TCP_KEEPINTVL. (I checked. I read the kernel source to make sure of this.) The easiest way to implement this is to define TCP_KEEPIDLE to TCP_KEEPALIVE if TCP_KEEPIDLE isn’t defined but TCP_KEEPALIVE is. As for the other BSD systems, I’m pretty sure they implement the Linux interface (if at all). I know FreeBSD does. Also, Darwin only grew support for TCP_KEEPINTVL in Darwin 13 (i.e. Mac OS 10.9), so even with my suggested change, the existing #ifdef block won’t get compiled in on lower than 10.9. If you want to support 10.6 (when TCP_KEEPALIVE was introduced) through 10.8, you might consider pulling the TCP_KEEPINTVL check into its own #ifdef block, and printing a FIXME if it isn’t defined. You know what? I already have a patch that does all these things. I think I’ll just send that. (I’ve really got to start sending more of my patches upstream…) Chip

July 30, 2015

Re: ws2_32: socket keepalive for OSX

by Bruno Jesus

Hi, Matt, thanks for the patch, here is some feedback. Please use a 4 space identation like the code style in this function. > +#elif defined(SO_KEEPALIVE) && defined(TCP_KEEPALIVE) SO_KEEPALIVE is defined for sure (already used outside #ifdef) so there is no need to test. > + /* BSDish platforms like Darwin */ > + else if (keepalive) > + { > + if (setsockopt(fd, SOL_SOCKET, SO_KEEPALIVE, (void*)&keepalive, sizeof(int)) == 1) > + status = WSAEINVAL; SO_KEEPALIVE value was already set by a previous call before the #ifdef. Is there a reason to repeat it? Also you are testing the return value for 1 instead of -1. > + else if (setsockopt(fd, IPPROTO_TCP, TCP_KEEPALIVE, (void*)&keepintvl, sizeof(int)) == -1) > + status = WSAEINVAL; > + } Please note that in the rest of the code it is used (void *), not (void*). Best wishes, Bruno On Wed, Jul 29, 2015 at 10:30 PM, Matt Durgavich <mattdurgavich(a)gmail.com> wrote: > Set socket options for keepalive on OSX tcp connections. > No tests at this time. > > dlls/ws2_32/socket.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > > >

July 30, 2015

Re: [PATCH 5/5] d3dx9/tests: Add tests for normals computation.

by Józef Kucia

On Mon, Jul 27, 2015 at 7:40 PM, Matteo Bruni <matteo.mystral(a)gmail.com> wrote: > > > + static const D3DVERTEXELEMENT9 position3f_normal1f_declaration[] = > > + { > > + {0, 0, D3DDECLTYPE_FLOAT3, D3DDECLMETHOD_DEFAULT, D3DDECLUSAGE_POSITION, 0}, > > + {0, sizeof(D3DXVECTOR3), D3DDECLTYPE_FLOAT1, D3DDECLMETHOD_DEFAULT, D3DDECLUSAGE_NORMAL, 0}, > > + D3DDECL_END() > > + }; > > + static const D3DVERTEXELEMENT9 position3f_normal2f_declaration[] = > > + { > > + {0, 0, D3DDECLTYPE_FLOAT3, D3DDECLMETHOD_DEFAULT, D3DDECLUSAGE_POSITION, 0}, > > + {0, sizeof(D3DXVECTOR3), D3DDECLTYPE_FLOAT2, D3DDECLMETHOD_DEFAULT, D3DDECLUSAGE_NORMAL, 0}, > > + D3DDECL_END() > > + }; > > + static const D3DVERTEXELEMENT9 normal4f_position3f_declaration[] = > > + { > > + {0, 0, D3DDECLTYPE_FLOAT4, D3DDECLMETHOD_DEFAULT, D3DDECLUSAGE_NORMAL, 0}, > > + {0, sizeof(D3DXVECTOR4), D3DDECLTYPE_FLOAT3, D3DDECLMETHOD_DEFAULT, D3DDECLUSAGE_POSITION, 0}, > > + D3DDECL_END() > > + }; > > Can you check that a 4-component normal placed after the position > gives the same results? It doesn't need to be in the patch if there > are no surprises (and there shouldn't be, this is me being paranoid). > No surprises.

July 29, 2015

Re: D3DRM Implementation patches upto CreateDeviceFromSurface

by Aaryaman Vasishta

I've fixed whatever leaks I could find, and also attached implementations of CreateDeviceFromSurface. Do take a look and if possible, provide feedback on any leaks that I might have missed. Jam On Sun, Jul 26, 2015 at 5:34 PM, Aaryaman Vasishta < jem456.vasishta(a)gmail.com> wrote: > > > On Sun, Jul 26, 2015 at 4:33 PM, Stefan Dösinger < > stefandoesinger(a)gmail.com> wrote: > >> -----BEGIN PGP SIGNED MESSAGE----- >> Hash: SHA1 >> >> Hi, >> >> I like the overall structure a lot more than the previous incarnation! >> >> Here are a few more comments: >> >> > + hr = DirectDrawCreate(NULL, &ddraw, NULL); >> > + if (FAILED(hr)) >> > + return hr; >> > + >> > hr = Direct3DRMDevice_create(&object); >> > if (FAILED(hr)) >> > return hr; >> You're leaking "ddraw" here. There are many more incomplete error >> handling paths in the patches. >> > I'll post another incarnation with all the leaks I can possibly find fixed > :) > >> >> > +HRESULT set_clipper_surface(struct d3drm_device *object, IDirectDraw >> *ddraw, IDirectDrawClipper *clipper, int width, int height, >> > + IDirectDrawSurface **surface) DECLSPEC_HIDDEN; >> I'm not entirely happy with this name, because it doesn't merely set any >> surfaces, it creates them. What do you think about >> d3drm_device_create_surfaces_from_clipper? Check for consistency wrt the >> "d3drm_device" part. E.g. the next function you could name >> "d3drm_device_init" or just "device_init". >> > It took me a while to figure out a name, since a name like > d3drm_device_create_surfaces_from_clipper felt too long for me initially. > But if long function names are accepted then I don't mind putting that name > up. > >> >> > +HRESULT init_device(struct d3drm_device *device, UINT version, >> IDirect3DRM *d3drm, IDirectDraw *ddraw, IDirectDrawSurface *surface, BOOL >> z_surface, IDirect3D2 *d3d2, >> > + int width, int height) >> "BOOL z_surface" is unused right now. It won't be needed until you add >> CreateDeviceFromSurface version 3. >> > Right, I'll remove this for now and re-add it in the > CreateDeviceFromSurface patches. Note that z_surface is still needed in > version 1 and 2 of CreateDeviceFromSurface in the cases where the surface > already has ds attached to it by the application (in which case native > wouldn't create its own ds internally, as seen by the tests). > >> >> You are leaking ds here in the success case. You can release it after >> calling AddAttachedSurface. >> > I'm not sure why I allowed that leak to happen, but I believe it was > causing some refcounting/segfault issues if I released the ds there. I'll > get back with more details about this. > > > > Jam >

July 29, 2015

Re: gdi32: Simplify bitwise and and xor with 0 (PVS-Studio)

by Huw Davies

On 29 Jul 2015, at 12:54, Michael Stefaniuc wrote: > On 07/29/2015 01:39 PM, Huw Davies wrote: >> On 29 Jul 2015, at 12:32, Michael Stefaniuc wrote: >>> >>> @@ -5655,8 +5655,8 @@ static void create_dither_masks_1(const dib_info *dib, int rop2, COLORREF color, >>> } >>> else >>> { >>> - rop_mask.and = (0x00 & codes.a1) ^ codes.a2; >>> - rop_mask.xor = (0x00 & codes.x1) ^ codes.x2; >>> + rop_mask.and = codes.a2; >>> + rop_mask.xor = codes.x2; >>> } >>> *and_bits |= (rop_mask.and & pixel_masks_1[x]); >>> *xor_bits |= (rop_mask.xor & pixel_masks_1[x]); >> >> >> It's clearer as it was, especially given the lines just above this hunk. > Not to me. Why do all that obfuscation if all you wanted to say is that > the codes.{a,x}1 stuff doesn't matters? The codes are normally used as pairs together. This just happens to be a special case where of course you can simplify the logic, but then you're left wondering what happened to a1/x1. Huw.

July 29, 2015

Re: gdi32: Simplify bitwise and and xor with 0 (PVS-Studio)

by Michael Stefaniuc

On 07/29/2015 01:39 PM, Huw Davies wrote: > On 29 Jul 2015, at 12:32, Michael Stefaniuc wrote: >> --- >> Same sha1 for primitives.o with and without patch. >> >> >> >> dlls/gdi32/dibdrv/primitives.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/dlls/gdi32/dibdrv/primitives.c b/dlls/gdi32/dibdrv/primitives.c >> index c7b48ed..9da08ad 100644 >> --- a/dlls/gdi32/dibdrv/primitives.c >> +++ b/dlls/gdi32/dibdrv/primitives.c >> @@ -5655,8 +5655,8 @@ static void create_dither_masks_1(const dib_info *dib, int rop2, COLORREF color, >> } >> else >> { >> - rop_mask.and = (0x00 & codes.a1) ^ codes.a2; >> - rop_mask.xor = (0x00 & codes.x1) ^ codes.x2; >> + rop_mask.and = codes.a2; >> + rop_mask.xor = codes.x2; >> } >> *and_bits |= (rop_mask.and & pixel_masks_1[x]); >> *xor_bits |= (rop_mask.xor & pixel_masks_1[x]); > > > It's clearer as it was, especially given the lines just above this hunk. Not to me. Why do all that obfuscation if all you wanted to say is that the codes.{a,x}1 stuff doesn't matters? bye michael

July 29, 2015

Re: gdi32: Simplify bitwise and and xor with 0 (PVS-Studio)

by Huw Davies

On 29 Jul 2015, at 12:32, Michael Stefaniuc wrote: > --- > Same sha1 for primitives.o with and without patch. > > > > dlls/gdi32/dibdrv/primitives.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/dlls/gdi32/dibdrv/primitives.c b/dlls/gdi32/dibdrv/primitives.c > index c7b48ed..9da08ad 100644 > --- a/dlls/gdi32/dibdrv/primitives.c > +++ b/dlls/gdi32/dibdrv/primitives.c > @@ -5655,8 +5655,8 @@ static void create_dither_masks_1(const dib_info *dib, int rop2, COLORREF color, > } > else > { > - rop_mask.and = (0x00 & codes.a1) ^ codes.a2; > - rop_mask.xor = (0x00 & codes.x1) ^ codes.x2; > + rop_mask.and = codes.a2; > + rop_mask.xor = codes.x2; > } > *and_bits |= (rop_mask.and & pixel_masks_1[x]); > *xor_bits |= (rop_mask.xor & pixel_masks_1[x]); It's clearer as it was, especially given the lines just above this hunk. Huw.

July 29, 2015

Re: [PATCH 4/5] amstream: Zero-initialize DirectDraw surface desc. (Valgrind)

by Henri Verbeet

On 28 July 2015 at 23:14, Matteo Bruni <mbruni(a)codeweavers.com> wrote: > diff --git a/dlls/amstream/mediastream.c b/dlls/amstream/mediastream.c > index 947be1e..3725c68 100644 > --- a/dlls/amstream/mediastream.c > +++ b/dlls/amstream/mediastream.c > @@ -991,6 +991,7 @@ static HRESULT ddrawstreamsample_create(IDirectDrawMediaStream *parent, IDirectD > return hr; > } > > + memset(&desc, 0, sizeof(desc)); > desc.dwSize = sizeof(desc); > desc.dwFlags = DDSD_CAPS|DDSD_HEIGHT|DDSD_WIDTH|DDSD_PIXELFORMAT; > desc.dwHeight = 100; This should not be needed. If this is for bug 38911, that's a duplicate of bug 36383, and sort of a false positive. ddraw_surface_create() clears DDSD_LPSURFACE if desc->lpSurface is NULL. The issue is that desc->lpSurface may be uninitialized if DDSD_LPSURFACE is not set, but in that case clearing the flag also wouldn't make a difference. If we want to do something about the false positive that should happen in ddraw_surface_create(). I don't personally feel strongly about it one way or the other.

July 29, 2015

Re: [2/2] shell32/tests: Check if error status is reset after successful file copy

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://testbot.winehq.org/JobDetails.pl?Key=15616 Your paranoid android. === w2000pro (32 bit shlfileop) === shlfileop.c:1833: Test failed: Expected ERROR_SUCCESS, got 6 === wvistau64 (32 bit shlfileop) === shlfileop.c:818: Test failed: Expected ERROR_CANCELLED or DE_DIFFDIR === wvistau64 (64 bit shlfileop) === shlfileop.c:818: Test failed: Expected ERROR_CANCELLED or DE_DIFFDIR

July 28, 2015

Re: [1/2] ntdll/tests: Add basic tests for RtlQueueWorkItem.

by Sebastian Lackner

On 28.07.2015 15:53, Grazvydas Ignotas wrote: > On Mon, Jul 27, 2015 at 1:33 AM, Sebastian Lackner > <sebastian(a)fds-team.de> wrote: >> On 27.07.2015 00:11, Marvin wrote: >>> Hi, >>> >>> While running your changed tests on Windows, I think I found new failures. >>> Being a bot and all I'm not very good at pattern recognition, so I might be >>> wrong, but could you please double-check? >>> Full results can be found at >>> https://testbot.winehq.org/JobDetails.pl?Key=15580 >>> >>> Your paranoid android. >>> >>> >>> === w7u (32 bit threadpool) === >>> threadpool.c:763: Test failed: WaitForSingleObject returned 258 >>> threadpool.c:777: Test failed: WaitForSingleObject returned 0 >>> threadpool.c:779: Test failed: threadpool.c:715: Running timer callback >>> >> >> This is unrelated to my tests. >> >> If noone has an idea what is causing these random failures I'll try to increase >> the timeouts a bit (in a separate patch of course) - however, these random issues >> do not occur on a local Windows 7 machine, which makes me suspect that it could >> also be related to testbot issues. > > IMHO 1s timeouts are way too low for test machines running on VMs, the > host machine may not have enough resources if multiple VMs are busy, > and the hypervisor may have it's inefficiencies too, especially when > I/O is involved. I don't have any idea how tesbot is set up bit I had > an impression there are lots of VMs running on a single host. > > If it was up to me I'd increase those timeouts a lot, to at least 10s > (or even a minute) instead of 1s. You you have to keep in mind that tests running on a testbot also have a total timeout of about 2 minutes, if I remember correctly. If a test would be stuck (for whatever reason), its still useful to run at least the rest of the tests before the job is forcefully killed. Besides that, the purpose of the tests is also to verify that the timing works correctly. Apps might rely on that, and expect the same timing when running with Wine. The tests would be pretty useless when the tolerance is too big. On my local Windows 7 machines (both on real hardware and on a KVM based virtual machine) all tests pass very reliable. :/ Does anyone familiar with the testbot setup have any idea? > > Gražvydas >

July 28, 2015