March 2017 - Wine-devel - list.winehq.org

Re: [2/8] webservices: Protect messages with a critical section.
by Sebastian Lackner 08 Mar '17

08 Mar '17

On 08.03.2017 11:25, Hans Leidekker wrote: > + } > } > > msg->is_addressed = TRUE; Not sure if it matters, but previously this was skipped in case of a failure. Also please note that depending on how many objects are used by an application, a separate CS for each might significantly increase memory usage. > - return S_OK; > + > + LeaveCriticalSection( &msg->cs ); > + return hr; [...] > - msg->init = init; > - msg->state = WS_MESSAGE_STATE_INITIALIZED; > - return write_envelope( msg ); > + if ((hr = write_envelope( msg )) == S_OK) > + { > + msg->init = init; > + msg->state = WS_MESSAGE_STATE_INITIALIZED; > + } Here, the new code also does something else than the old one. Previously the msg fields were changed unconditionally.

2 1

Re: [5/8] webservices: Protect writers with a critical section.
by Sebastian Lackner 08 Mar '17

08 Mar '17

On 08.03.2017 11:25, Hans Leidekker wrote: > + if ((hr = write_set_element_namespace( writer )) != S_OK) goto done; > + if ((hr = write_startelement( writer )) != S_OK) goto done; > + if ((hr = write_endstartelement( writer )) != S_OK) goto done; > writer->state = WRITER_STATE_ENDSTARTELEMENT; > + > +done: > + LeaveCriticalSection( &writer->cs ); > return S_OK; You should return hr here. > for (i = offset; i < count; i++) > { > const char *ptr = (const char *)array + (offset + i) * type_size; > - if ((hr = write_element_node( writer, NULL, localname, ns )) != S_OK) return hr; > + if ((hr = write_element_node( writer, NULL, localname, ns )) != S_OK) goto done; > if ((hr = write_type( writer, WS_ELEMENT_TYPE_MAPPING, type, NULL, WS_WRITE_REQUIRED_POINTER, > - &ptr, sizeof(ptr) )) != S_OK) return hr; > - if ((hr = write_endelement_node( writer )) != S_OK) return hr; > + &ptr, sizeof(ptr) )) != S_OK) goto done; > + if ((hr = write_endelement_node( writer )) != S_OK) goto done; > } > > - return S_OK; > +done: > + LeaveCriticalSection( &writer->cs ); > + return hr; It looks like hr will be undefined here if count == 0 > } > > /************************************************************************** > @@ -2392,9 +2662,20 @@ HRESULT WINAPI WsWriteXmlBuffer( WS_XML_WRITER *handle, WS_XML_BUFFER *buffer, W > > if (!writer || !xmlbuf) return E_INVALIDARG; > > - if ((hr = write_flush( writer )) != S_OK) return hr; > - if ((hr = write_grow_buffer( writer, xmlbuf->size )) != S_OK) return hr; > + EnterCriticalSection( &writer->cs ); > + > + if (writer->magic != WRITER_MAGIC) > + { > + LeaveCriticalSection( &writer->cs ); > + return E_INVALIDARG; > + } > + > + if ((hr = write_flush( writer )) != S_OK) goto done; > + if ((hr = write_grow_buffer( writer, xmlbuf->size )) != S_OK) goto done; > write_bytes( writer, xmlbuf->ptr, xmlbuf->size ); > + > +done: > + LeaveCriticalSection( &writer->cs ); > return S_OK; Here you should also use hr.

1 0

Re: [PATCH] kernel32/tests: Add 64bit number tests for FormatMessage[A|W] (try 2)
by Sebastian Lackner 08 Mar '17

08 Mar '17

On 08.03.2017 15:42, Bruno Jesus wrote: > + { -1, "-1", 2}, > + { ULL(0xFFFFFFFF,0xFFFFFFFF), "-1", 2}, You can remove the second test. 0xffffffffffffffff and -1 are just different representations for the same number. Best regards, Sebastian

2 1

Re: bcrypt: Add a builtin sha384/sha512 implementation. (v2)
by Sebastian Lackner 08 Mar '17

08 Mar '17

On 08.03.2017 12:17, Jacek Caban wrote: > On 08.03.2017 12:03, Sebastian Lackner wrote: >> +void sha512_init(SHA512_CTX *ctx) >> +{ >> + ctx->len = 0; >> + ctx->h[0] = 0x6a09e667f3bcc908; >> + ctx->h[1] = 0xbb67ae8584caa73b; >> + ctx->h[2] = 0x3c6ef372fe94f82b; >> + ctx->h[3] = 0xa54ff53a5f1d36f1; >> + ctx->h[4] = 0x510e527fade682d1; >> + ctx->h[5] = 0x9b05688c2b3e6c1f; >> + ctx->h[6] = 0x1f83d9abfb41bd6b; >> + ctx->h[7] = 0x5be0cd19137e2179; > > > I think those are still not portable, because those constants are too > large. You may use something like ((ULONG64)0x6a09e667<<32)|0xf3bcc908. > > > Jacek I was wondering about that too, but it looks like we have already various places where 64-bit consts are used without any ugly tricks. Examples are in tests, include files, and also implementation code. Is it really necessary? Or is it only the suffix which causes trouble? Best regards, Sebastian

2 1

Re: [4/5] bcrypt: Add a builtin sha384/sha512 implementation.
by Hans Leidekker 08 Mar '17

08 Mar '17

On Tue, 2017-03-07 at 23:16 +0100, Sebastian Lackner wrote: > +void sha384_init(SHA512_CTX *ctx) > +{ > + ctx->len = 0; > + ctx->h[0] = 0xcbbb9d5dc1059ed8ULL; > + ctx->h[1] = 0x629a292a367cd507ULL; > + ctx->h[2] = 0x9159015a3070dd17ULL; > + ctx->h[3] = 0x152fecd8f70e5939ULL; > + ctx->h[4] = 0x67332667ffc00b31ULL; > + ctx->h[5] = 0x8eb44a8768581511ULL; > + ctx->h[6] = 0xdb0c2e0d64f98fa7ULL; > + ctx->h[7] = 0x47b5481dbefa4fa4ULL; We try to avoid the (U)LL suffix. The patch set looks good otherwise.

2 1

Re: [PATCH] d3dx9/tests: Add test for out of bound array selector in effect.
by Matteo Bruni 07 Mar '17

07 Mar '17

2017-03-07 18:47 GMT+01:00 Paul Gofman <gofmanp(a)gmail.com>: > Signed-off-by: Paul Gofman <gofmanp(a)gmail.com> Hi Paul! Patch looks mostly good to me BUT I have a couple comments anyway... > --- > dlls/d3dx9_36/tests/effect.c | 159 ++++++++++++++++++++++++++++++++++++++----- > 1 file changed, 141 insertions(+), 18 deletions(-) > > diff --git a/dlls/d3dx9_36/tests/effect.c b/dlls/d3dx9_36/tests/effect.c > index b3845c5..fe84173 100644 > --- a/dlls/d3dx9_36/tests/effect.c > +++ b/dlls/d3dx9_36/tests/effect.c > @@ -3813,6 +3813,33 @@ static const DWORD test_effect_preshader_effect_blob[] = > #define TEST_EFFECT_PRESHADER_VSHADER_POS 2458 > #define TEST_EFFECT_PRESHADER_VSHADER_LEN 13 > > +static BOOL test_effect_preshader_compare_shader(IDirect3DDevice9 *device, int expected_shader_index) > +{ > + IDirect3DVertexShader9 *vshader; > + void *byte_code; > + unsigned int byte_code_size; > + HRESULT hr; > + BOOL ret; > + > + if (FAILED(hr = IDirect3DDevice9_GetVertexShader(device, &vshader)) || !vshader) > + return FALSE; It seems preferable to keep the detailed ok() calls from the old test instead of a generic ok() message on the return value of test_effect_preshader_compare_shader(). I guess you want to avoid to have ambiguous ok() calls in this helper. Until we get something like the with_context() thing from https://source.winehq.org/patches/data/131137 just passing a string with the test name to the helper function and using that in the ok() messages should do the trick. > + hr = effect->lpVtbl->BeginPass(effect, 1); > + todo_wine ok(hr == E_FAIL, "Got result %#x.\n", hr); > + if (SUCCEEDED(hr)) > + effect->lpVtbl->EndPass(effect); > + > + hr = effect->lpVtbl->BeginPass(effect, 1); > + ok(hr == D3D_OK, "Got result %#x.\n", hr); That reminds me, do we have tests for two BeginPass() calls without an EndPass() in between? Might be interesting to see what happens both with the same or different pass index. Other somewhat interesting cases might be BeginPass() outside of Begin() / End() and multiple Begin() / End() / EndPass() calls in a row. It should probably be in a separate patch though. BTW, I think it's somewhat idiomatic to call it "out of bounds" even when technically there is a single bound involved.

2 3

Interested in Wine Outreachy Application
by Sreya Mittal 07 Mar '17

07 Mar '17

Hi, I am Sreya Mittal pursuing my Btech in Computer Science at International Institute of Information Technology, Hyderabad. I am interested in getting started with open-source contributions and looking forward to becoming a part of Wine and Outreachy community. I am proficient in C. This would help in the work. I would be grateful if I could get a few pointers regarding how to get started. Thank you, Sreya

4 5

Re: kernel32: add stub for GetCurrentPackageFullName
by Sebastian Lackner 07 Mar '17

07 Mar '17

On 04.03.2017 11:05, Louis Lenders wrote: > --- a/dlls/kernel32/kernel32.spec > +++ b/dlls/kernel32/kernel32.spec > @@ -629,6 +629,7 @@ > @ stdcall GetCurrentDirectoryA(long ptr) > @ stdcall GetCurrentDirectoryW(long ptr) > @ stdcall GetCurrentPackageId(ptr ptr) > +@ stdcall GetCurrentPackageFullName(ptr ptr) > @ stdcall -norelay GetCurrentProcess() > @ stdcall -norelay GetCurrentProcessId() > @ stdcall GetCurrentProcessorNumber() ntdll.NtGetCurrentProcessorNumber In this case its too late, but please make sure to keep the specfile ordered alphabetically in the future.

2 1

Re: [PATCH v2 2/5] winebus.sys: Build device reports for linux event devices
by Roderick Colenbrander 07 Mar '17

07 Mar '17

Hi Aric, I have had a quick look over this patch and have some initial feedback, maybe more later if time permits. For the absolute and relative axes don't hard code the minimum and maximum values for the report descriptors. These are not correct for many devices e.g. Xbox controllers use 16-bit absolute axes, while DS3/DS4 for example use 8-bit. In addition xbox controllers use signed data, while DS3/DS4 use unsigned (0-255)Also many devices use signed data. You should rely on the absinfo data returned by EVIOCGABS and base your input ranges on that. Thanks, Roderick On Mon, Feb 27, 2017 at 9:32 AM, Aric Stewart <aric(a)codeweavers.com> wrote: > > v2: Updates from Sebastian Lackner > > Signed-off-by: Aric Stewart <aric(a)codeweavers.com> > --- > dlls/winebus.sys/bus_udev.c | 438 +++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 436 insertions(+), 2 deletions(-) > > > > >

2 2

Re: qcap: Fix compile error when O_CLOEXEC is not defined.
by Alexandre Julliard 07 Mar '17

07 Mar '17

Sebastian Lackner <sebastian(a)fds-team.de> writes: > Signed-off-by: Sebastian Lackner <sebastian(a)fds-team.de> > --- > > Defining to 0 ensures that it doesn't do any harm. If we don't > care about ancient systems, we can probably also remove the fcntl call. > On modern systems, setting CLOEXEC twice (via open and fcntl) shouldn't > hurt. I'm too lazy to check, but I'm wondering if passing O_CLOEXEC would cause the open to fail on an ancient system, and whether we should be checking for EINVAL and retrying... -- Alexandre Julliard julliard(a)winehq.org

2 2