October 2016 - Wine-Devel - list.winehq.org

Re: d3dx9: d3dx9_base_effect_get_pass_desc

by Paul Gofman

No, I didn't test GetPassDesc shader return behaviour and did not meet any application requiring it. But there is test_effect_preshader_effect_blob in tests/effect.c which has a shader chosen by selector which probably can be used for testing. I suppose the checks for GetPassDesc could be inserted right into test_effect_preshader() test which already tests shader switching through selector. There is already a code which identifies a selected shader, I hope the same can be used for checking GetPassDesc return. I don't know if Matteo will like this idea but the simplest way I see to determine the shader actually selected for pass is to store pointer to it right from d3dx9_apply_state function (in case SC_VERTEXSHADER: and case SC_PIXELSHADER:), as at this place correct shader is determined based on selector value (if selector is used). I can't be sure without testing though what GetPassDesc should return before BeginPass() was called (that is, before any apply_state was called), and then after BeginPass/EndPass was called (will it be last selected? Or first shaders' array element? Or based on the current selector value??). Probably this needs to be tested before deciding on implementation. On 10/31/2016 06:29 PM, Matteo Bruni wrote: > 2016-10-29 21:11 GMT+02:00 Luis C. Busquets Pérez <luis.busquets(a)ilidium.com>: >> Could anyone help improving this patch so that it makes it to the wine >> tree? >> >> >>> --- >>> dlls/d3dx9_36/effect.c | 46 >>> ++++++++++++++++++++++++++++++++++++++++++++-- >>> 1 file changed, 44 insertions(+), 2 deletions(-) >>> >>> diff --git a/dlls/d3dx9_36/effect.c b/dlls/d3dx9_36/effect.c >>> index 42f5aea..c69b73c 100644 >>> --- a/dlls/d3dx9_36/effect.c >>> +++ b/dlls/d3dx9_36/effect.c >>> @@ -150,6 +150,7 @@ struct d3dx9_base_effect >>> UINT parameter_count; >>> UINT technique_count; >>> UINT object_count; >>> + BOOL clonable; >>> >>> struct d3dx_parameter *parameters; >>> struct d3dx_technique *techniques; >>> @@ -999,6 +1000,11 @@ static HRESULT >>> d3dx9_base_effect_get_pass_desc(struct d3dx9_base_effect *base, >>> D3DXHANDLE pass, D3DXPASS_DESC *desc) >>> { >>> struct d3dx_pass *p = get_valid_pass(base, pass); >>> + struct d3dx_state *s; >>> + struct d3dx_parameter *param; >>> + IDirect3DVertexShader9 *vshader; >>> + IDirect3DPixelShader9 *pshader; >>> + UINT i, size; >>> >>> if (!desc || !p) >>> { >>> @@ -1008,11 +1014,45 @@ static HRESULT >>> d3dx9_base_effect_get_pass_desc(struct d3dx9_base_effect *base, >>> >>> desc->Name = p->name; >>> desc->Annotations = p->annotation_count; >>> - >>> - FIXME("Pixel shader and vertex shader are not supported, >>> yet.\n"); >>> desc->pVertexShaderFunction = NULL; >>> desc->pPixelShaderFunction = NULL; >>> >>> + if (base->clonable) >>> + { >>> + for (i=0; i < p->state_count; i++) >>> + { >>> + s = p->states + i; >>> + if (state_table[s->operation].class == SC_VERTEXSHADER) >>> + { >>> + param = &(s->parameter); >>> + vshader = *(struct IDirect3DVertexShader9 **)param- >>>> data; >>> + if (vshader) >>> + { >>> + IDirect3DVertexShader9_GetFunction(vshader, >>> NULL, >>> &size); >>> + if (desc->pVertexShaderFunction) >>> + free((void *)desc->pVertexShaderFunction); >>> + desc->pVertexShaderFunction = malloc(size); >>> + IDirect3DVertexShader9_GetFunction(vshader, >>> (void >>> *)(desc->pVertexShaderFunction), &size); >>> + } >>> + } >>> + if (state_table[s->operation].class == SC_PIXELSHADER) >>> + { >>> + param = &(s->parameter); >>> + pshader = *(struct IDirect3DPixelShader9 **)param- >>>> data; >>> + if (pshader) >>> + { >>> + IDirect3DPixelShader9_GetFunction(pshader, NULL, >>> &size); >>> + if (desc->pPixelShaderFunction) >>> + free((void *)desc->pPixelShaderFunction); >>> + desc->pPixelShaderFunction = malloc(size); >>> + IDirect3DPixelShader9_GetFunction(pshader, (void >>> *)(desc->pPixelShaderFunction), &size); >>> + } >>> + } >>> + } >>> + } > You probably want to take the shader code from the "objects" array instead. > Also that doesn't work for ST_ARRAY_SELECTOR vertex / pixel shader > states. It isn't even obvious what should be the returned value in > that case i.e. it needs tests. > > I think Paul had a look at this at some point although I don't know if > there is any relevant patch attached to a bug or something. > >

Oct. 31, 2016

Re: d3dx9: d3dx9_base_effect_get_pass_desc

by Matteo Bruni

2016-10-29 21:11 GMT+02:00 Luis C. Busquets Pérez <luis.busquets(a)ilidium.com>: > Could anyone help improving this patch so that it makes it to the wine > tree? > > >> --- >> dlls/d3dx9_36/effect.c | 46 >> ++++++++++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 44 insertions(+), 2 deletions(-) >> >> diff --git a/dlls/d3dx9_36/effect.c b/dlls/d3dx9_36/effect.c >> index 42f5aea..c69b73c 100644 >> --- a/dlls/d3dx9_36/effect.c >> +++ b/dlls/d3dx9_36/effect.c >> @@ -150,6 +150,7 @@ struct d3dx9_base_effect >> UINT parameter_count; >> UINT technique_count; >> UINT object_count; >> + BOOL clonable; >> >> struct d3dx_parameter *parameters; >> struct d3dx_technique *techniques; >> @@ -999,6 +1000,11 @@ static HRESULT >> d3dx9_base_effect_get_pass_desc(struct d3dx9_base_effect *base, >> D3DXHANDLE pass, D3DXPASS_DESC *desc) >> { >> struct d3dx_pass *p = get_valid_pass(base, pass); >> + struct d3dx_state *s; >> + struct d3dx_parameter *param; >> + IDirect3DVertexShader9 *vshader; >> + IDirect3DPixelShader9 *pshader; >> + UINT i, size; >> >> if (!desc || !p) >> { >> @@ -1008,11 +1014,45 @@ static HRESULT >> d3dx9_base_effect_get_pass_desc(struct d3dx9_base_effect *base, >> >> desc->Name = p->name; >> desc->Annotations = p->annotation_count; >> - >> - FIXME("Pixel shader and vertex shader are not supported, >> yet.\n"); >> desc->pVertexShaderFunction = NULL; >> desc->pPixelShaderFunction = NULL; >> >> + if (base->clonable) >> + { >> + for (i=0; i < p->state_count; i++) >> + { >> + s = p->states + i; >> + if (state_table[s->operation].class == SC_VERTEXSHADER) >> + { >> + param = &(s->parameter); >> + vshader = *(struct IDirect3DVertexShader9 **)param- >> > >> > data; >> + if (vshader) >> + { >> + IDirect3DVertexShader9_GetFunction(vshader, >> NULL, >> &size); >> + if (desc->pVertexShaderFunction) >> + free((void *)desc->pVertexShaderFunction); >> + desc->pVertexShaderFunction = malloc(size); >> + IDirect3DVertexShader9_GetFunction(vshader, >> (void >> *)(desc->pVertexShaderFunction), &size); >> + } >> + } >> + if (state_table[s->operation].class == SC_PIXELSHADER) >> + { >> + param = &(s->parameter); >> + pshader = *(struct IDirect3DPixelShader9 **)param- >> > >> > data; >> + if (pshader) >> + { >> + IDirect3DPixelShader9_GetFunction(pshader, NULL, >> &size); >> + if (desc->pPixelShaderFunction) >> + free((void *)desc->pPixelShaderFunction); >> + desc->pPixelShaderFunction = malloc(size); >> + IDirect3DPixelShader9_GetFunction(pshader, (void >> *)(desc->pPixelShaderFunction), &size); >> + } >> + } >> + } >> + } You probably want to take the shader code from the "objects" array instead. Also that doesn't work for ST_ARRAY_SELECTOR vertex / pixel shader states. It isn't even obvious what should be the returned value in that case i.e. it needs tests. I think Paul had a look at this at some point although I don't know if there is any relevant patch attached to a bug or something.

Oct. 31, 2016

Re: [PATCH 7/7] ddraw: Fix remaining style issues in executebuffer.c

by Henri Verbeet

On 30 October 2016 at 15:02, Stefan Dösinger <stefandoesinger(a)gmx.at> wrote: > Feel free to ignore this. With this the file follows the wined3d style, > unless I missed something... > +static void _dump_executedata(const D3DEXECUTEDATA *lpData) Naming conventions for "_dump_executedata" and "lpData". > TRACE("dwSize : %d\n", lpData->dwSize); %u for unsigned integers, punctuation. > -static void _dump_D3DEXECUTEBUFFERDESC(const D3DEXECUTEBUFFERDESC *lpDesc) { > +static void _dump_D3DEXECUTEBUFFERDESC(const D3DEXECUTEBUFFERDESC *lpDesc) > +{ > TRACE("dwSize : %d\n", lpDesc->dwSize); Likewise. > TRACE("dwFlags : %x\n", lpDesc->dwFlags); %#x for hexadecimal values. > @@ -76,12 +78,12 @@ HRESULT d3d_execute_buffer_execute(struct d3d_execute_buffer *buffer, > for (;;) > { > D3DINSTRUCTION *current = (D3DINSTRUCTION *)instr; Should probably be const. > - BYTE size; > - WORD count; > - > - count = current->wCount; > - size = current->bSize; > - instr += sizeof(D3DINSTRUCTION); > + BYTE size; > + WORD count; > + > + count = current->wCount; > + size = current->bSize; > + instr += sizeof(D3DINSTRUCTION); "sizeof(*current)" > instr = (char*)current + ci->dwOffset; "(char *)" > + default: > + ERR("Unhandled OpCode %d !!!\n",current->bOpcode); Should probably use %#x. Punctuation. Some of these probably deserve a patch on their own, and there may very well be more issues.

Oct. 31, 2016

Re: [PATCH 6/7] ddraw: Implement executebuffer point and line draws.

by Henri Verbeet

On 30 October 2016 at 15:02, Stefan Dösinger <stefandoesinger(a)gmx.at> wrote: > case D3DOP_TRIANGLE: > { > WORD *indices; > - unsigned int index_pos = buffer->index_pos; > + unsigned int index_pos = buffer->index_pos, num_indices; Since you're doing style cleanups in the next patch, "index_count".

Oct. 31, 2016

Re: [PATCH 5/7] ddraw: Discard the dst vertex buffer if possible.

by Henri Verbeet

On 30 October 2016 at 15:02, Stefan Dösinger <stefandoesinger(a)gmx.at> wrote: > Once we have buffer->buffer copies working we can load the source data > into a buffer in SetExecuteData and never have to lock the destination > buffer. In general I think wined3d_device_copy_sub_resource_region() is supposed to work for buffers. Is there something in particular that's broken?

Oct. 31, 2016

Re: [PATCH] wined3d: Invalidate INDEXBUFFER device state when contents of bound index buffer is changed.

by Józef Kucia

On Mon, Oct 31, 2016 at 1:27 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote: > On 31 October 2016 at 12:51, Józef Kucia <joseph.kucia(a)gmail.com> wrote: >> On Fri, Oct 28, 2016 at 3:55 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote: >>> On 28 October 2016 at 12:20, Józef Kucia <jkucia(a)codeweavers.com> wrote: >>>> + if (buffer->resource.bind_count) >>>> + { >>>> + if (buffer->buffer_type_hint == GL_ELEMENT_ARRAY_BUFFER) >>>> + device_invalidate_state(buffer->resource.device, STATE_INDEXBUFFER); >>>> + } >>> You can merge those if-conditions. >>> >>> What happens exactly that makes this required? >> >> The bound index buffer is unloaded and the game crashes on the next >> draw call because we use indexed drawing with zeroish offset and >> GL_ELEMENT_ARRAY_BUFFER_BINDING is 0. >> > I figured it was something like that, but the question is why that > happens exactly. In theory context_apply_draw_state() will load the > index buffer into a specific location based on "stream_info.all_vbo", > and context_update_stream_info() should make sure STATE_INDEXBUFFER is > invalidated if that changes between draws. Invalidating it in > buffer_unload() makes more sense, if the issue is that > glDeleteBuffers() gets called for an index buffer that's still bound. > (And we already have a similar invalidate for vertex buffers there.) > An argument could probably be made that the invalidate belongs in > delete_gl_buffer() instead of buffer_unload(), and that > STATE_STREAMSRC doesn't need to be invalidated for anything other than > vertex buffers. Insane 2 uses a single index buffer for all draw calls when rendering game menus. This index buffer is still bound when it's unloaded. glDeleteBuffers() unbinds the deleted buffer object in the current context. After the buffer is unloaded the application still uses this deleted buffer object in other threads, but when it tries to render again in the thread in which the buffer object was deleted the crash occurs.

Oct. 31, 2016

Re: [PATCH] wined3d: Invalidate INDEXBUFFER device state when contents of bound index buffer is changed.

by Henri Verbeet

On 31 October 2016 at 12:51, Józef Kucia <joseph.kucia(a)gmail.com> wrote: > On Fri, Oct 28, 2016 at 3:55 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote: >> On 28 October 2016 at 12:20, Józef Kucia <jkucia(a)codeweavers.com> wrote: >>> + if (buffer->resource.bind_count) >>> + { >>> + if (buffer->buffer_type_hint == GL_ELEMENT_ARRAY_BUFFER) >>> + device_invalidate_state(buffer->resource.device, STATE_INDEXBUFFER); >>> + } >> You can merge those if-conditions. >> >> What happens exactly that makes this required? > > The bound index buffer is unloaded and the game crashes on the next > draw call because we use indexed drawing with zeroish offset and > GL_ELEMENT_ARRAY_BUFFER_BINDING is 0. > I figured it was something like that, but the question is why that happens exactly. In theory context_apply_draw_state() will load the index buffer into a specific location based on "stream_info.all_vbo", and context_update_stream_info() should make sure STATE_INDEXBUFFER is invalidated if that changes between draws. Invalidating it in buffer_unload() makes more sense, if the issue is that glDeleteBuffers() gets called for an index buffer that's still bound. (And we already have a similar invalidate for vertex buffers there.) An argument could probably be made that the invalidate belongs in delete_gl_buffer() instead of buffer_unload(), and that STATE_STREAMSRC doesn't need to be invalidated for anything other than vertex buffers. > The other thing is that, in theory, when a buffer is modified in the > current context we should should invalidate buffer bindings for this > buffer in other contexts in order to make the contents changes visible > in other contexts. That's the reason why I was trying to restore > device_invalidate_state() call in wined3d_buffer_load(). This would > fix the regression as well. The quote from GL spec: > "If the contents of an object T are changed in a context other than the cur- > rent context, T must be attached or re-attached to at least one > binding point in the > current context, or at least one attachment point of a currently bound container > object C, in order to guarantee that the new contents of T are visible > in the current > context." Yeah, but it's pretty hopeless since you'd also need to flush, and then you get into "StrictDrawOrdering" territory. This was actually the kind of thing (among others) that CSMT was intended to fix, but of course people in general care more about fps than correctness.

Oct. 31, 2016

Re: [PATCH] wined3d: Invalidate INDEXBUFFER device state when contents of bound index buffer is changed.

by Józef Kucia

On Fri, Oct 28, 2016 at 3:55 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote: > On 28 October 2016 at 12:20, Józef Kucia <jkucia(a)codeweavers.com> wrote: >> + if (buffer->resource.bind_count) >> + { >> + if (buffer->buffer_type_hint == GL_ELEMENT_ARRAY_BUFFER) >> + device_invalidate_state(buffer->resource.device, STATE_INDEXBUFFER); >> + } > You can merge those if-conditions. > > What happens exactly that makes this required? The bound index buffer is unloaded and the game crashes on the next draw call because we use indexed drawing with zeroish offset and GL_ELEMENT_ARRAY_BUFFER_BINDING is 0. I've sent another patch which targets this issue. The other thing is that, in theory, when a buffer is modified in the current context we should should invalidate buffer bindings for this buffer in other contexts in order to make the contents changes visible in other contexts. That's the reason why I was trying to restore device_invalidate_state() call in wined3d_buffer_load(). This would fix the regression as well. The quote from GL spec: "If the contents of an object T are changed in a context other than the cur- rent context, T must be attached or re-attached to at least one binding point in the current context, or at least one attachment point of a currently bound container object C, in order to guarantee that the new contents of T are visible in the current context."

Oct. 31, 2016

Re: [PATCH 2/7] ddraw: Use wined3d_device_process_vertices for execute buffers.

by Henri Verbeet

On 30 October 2016 at 15:02, Stefan Dösinger <stefandoesinger(a)gmx.at> wrote: > @@ -318,65 +277,36 @@ HRESULT d3d_execute_buffer_execute(struct d3d_execute_buffer *buffer, > + hr = wined3d_resource_map(wined3d_buffer_get_resource(buffer->src_vertex_buffer), 0, > + &map_desc, &box, WINED3D_MAP_DISCARD); > if (FAILED(hr)) > return hr; > - dst = map_desc.data; > + memcpy(map_desc.data, src, ci->dwCount * sizeof(*src)); > + wined3d_resource_unmap(wined3d_buffer_get_resource(buffer->dst_vertex_buffer), 0); That looks wrong.

Oct. 31, 2016

Re: [PATCH] xinput1_3: Prefer builtin DLL

by Filip Frackiewicz

Ahh I see what you mean now, thank you for clarifying :) Filip On Sun, Oct 30, 2016 at 4:04 PM, Bruno Jesus <00cpxxx(a)gmail.com> wrote: > On Sun, Oct 30, 2016 at 12:37 PM, Filip Frackiewicz > <fjfrackiewicz(a)gmail.com> wrote: >> Hi Bruno, > > Hello, thanks for the email. > >> I hope I am not intruding or stating something already known,,, >> >>>- case DLL_WINE_PREATTACH: >>>- return FALSE; /* prefer native version */ >> case DLL_PROCESS_ATTACH: >> DisableThreadLibraryCalls(inst); >> break; >> >> This code looks different than the patch >> https://bugs.winehq.org/attachment.cgi?id=55851 attached to >> https://bugs.winehq.org/show_bug.cgi?id=41428 > > If you take a look at the whole function [1] you will see that it > returns TRUE in the end, so there is no need for the "case > DLL_WINE_PREATTACH" any longer. > > [1] http://source.winehq.org/source/dlls/xinput1_3/xinput1_3_main.c#0042

Oct. 30, 2016

Re: [PATCH] xinput1_3: Prefer builtin DLL

by Bruno Jesus

On Sun, Oct 30, 2016 at 12:37 PM, Filip Frackiewicz <fjfrackiewicz(a)gmail.com> wrote: > Hi Bruno, Hello, thanks for the email. > I hope I am not intruding or stating something already known,,, > >>- case DLL_WINE_PREATTACH: >>- return FALSE; /* prefer native version */ > case DLL_PROCESS_ATTACH: > DisableThreadLibraryCalls(inst); > break; > > This code looks different than the patch > https://bugs.winehq.org/attachment.cgi?id=55851 attached to > https://bugs.winehq.org/show_bug.cgi?id=41428 If you take a look at the whole function [1] you will see that it returns TRUE in the end, so there is no need for the "case DLL_WINE_PREATTACH" any longer. [1] http://source.winehq.org/source/dlls/xinput1_3/xinput1_3_main.c#0042

Oct. 30, 2016

RE: [PATCH] xinput1_3: Prefer builtin DLL

by Filip Frackiewicz

Hi Bruno, I hope I am not intruding or stating something already known,,, >- case DLL_WINE_PREATTACH: >- return FALSE; /* prefer native version */ case DLL_PROCESS_ATTACH: DisableThreadLibraryCalls(inst); break; This code looks different than the patch https://bugs.winehq.org/attachment.cgi?id=55851 attached to https://bugs.winehq.org/show_bug.cgi?id=41428 The patch attached to the bug did fix the issue in another game for me. Just thought I'd mention this just in case :) Cheers, Filip

Oct. 30, 2016

Re: [PATCH 6/7] ddraw: Implement executebuffer point and line draws.

by Stefan Dösinger

Am 2016-10-30 um 07:02 schrieb Stefan Dösinger: > + case D3DOP_LINE: > + primitive_size = 2; > + wined3d_device_set_primitive_type(device->wined3d_device, WINED3D_PT_LINELIST); I should probably mention the purpose of this patch: It fixes the minimap and bike performance display in Moto Racer 2.

Oct. 30, 2016

Re: [PATCH] ws2_32: Fix the default behavior of IPV6_V6ONLY

by Bruno Jesus

On Sat, Oct 29, 2016 at 10:37 PM, Marvin <testbot(a)winehq.org> wrote: > === w2008s64 (64 bit sock) === > sock: unhandled exception c0000005 at 000007FEFF126F94 Unfortunately a well-known issue. Clean run https://testbot.winehq.org/JobDetails.pl?Key=26344 I just started wondering if there is a difference between testing the patches manually and letting Marvin test them, because it is quite common for people to send a reference to a "clean run" whenever Marvin sends such emails.

Oct. 29, 2016

Re: [PATCH] ws2_32: Fix the default behavior of IPV6_V6ONLY

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=26345 Your paranoid android. === w2008s64 (64 bit sock) === sock: unhandled exception c0000005 at 000007FEFF126F94

Oct. 29, 2016

Re: d3dx9: d3dx9_base_effect_get_pass_desc

by Henri Verbeet

On 29 October 2016 at 21:11, Luis C. Busquets Pérez <luis.busquets(a)ilidium.com> wrote: > Could anyone help improving this patch so that it makes it to the wine > tree? At first sight: >> @@ -5955,6 +5995,8 @@ static HRESULT d3dx9_base_effect_init(struct >> d3dx9_base_effect *base, >> >> base->effect = effect; >> >> + base->clonable = TRUE; >> + Did you perhaps intend to store "eflags" instead here? >> + if (base->clonable) >> + { And then check "base->flags" against D3DXFX_NOT_CLONEABLE here. >> + if (vshader) >> + { >> + IDirect3DVertexShader9_GetFunction(vshader, >> NULL, >> &size); >> + if (desc->pVertexShaderFunction) >> + free((void *)desc->pVertexShaderFunction); >> + desc->pVertexShaderFunction = malloc(size); >> + IDirect3DVertexShader9_GetFunction(vshader, >> (void >> *)(desc->pVertexShaderFunction), &size); >> + } Why not move this out of the loop? >> + if (pshader) >> + { >> + IDirect3DPixelShader9_GetFunction(pshader, NULL, >> &size); >> + if (desc->pPixelShaderFunction) >> + free((void *)desc->pPixelShaderFunction); >> + desc->pPixelShaderFunction = malloc(size); >> + IDirect3DPixelShader9_GetFunction(pshader, (void >> *)(desc->pPixelShaderFunction), &size); >> + } Likewise.

Oct. 29, 2016

Re: d3dx9: d3dx9_base_effect_get_pass_desc

by Luis C. Busquets Pérez

Could anyone help improving this patch so that it makes it to the wine tree? > --- > dlls/d3dx9_36/effect.c | 46 > ++++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 44 insertions(+), 2 deletions(-) > > diff --git a/dlls/d3dx9_36/effect.c b/dlls/d3dx9_36/effect.c > index 42f5aea..c69b73c 100644 > --- a/dlls/d3dx9_36/effect.c > +++ b/dlls/d3dx9_36/effect.c > @@ -150,6 +150,7 @@ struct d3dx9_base_effect > UINT parameter_count; > UINT technique_count; > UINT object_count; > + BOOL clonable; > > struct d3dx_parameter *parameters; > struct d3dx_technique *techniques; > @@ -999,6 +1000,11 @@ static HRESULT > d3dx9_base_effect_get_pass_desc(struct d3dx9_base_effect *base, > D3DXHANDLE pass, D3DXPASS_DESC *desc) > { > struct d3dx_pass *p = get_valid_pass(base, pass); > + struct d3dx_state *s; > + struct d3dx_parameter *param; > + IDirect3DVertexShader9 *vshader; > + IDirect3DPixelShader9 *pshader; > + UINT i, size; > > if (!desc || !p) > { > @@ -1008,11 +1014,45 @@ static HRESULT > d3dx9_base_effect_get_pass_desc(struct d3dx9_base_effect *base, > > desc->Name = p->name; > desc->Annotations = p->annotation_count; > - > - FIXME("Pixel shader and vertex shader are not supported, > yet.\n"); > desc->pVertexShaderFunction = NULL; > desc->pPixelShaderFunction = NULL; > > + if (base->clonable) > + { > + for (i=0; i < p->state_count; i++) > + { > + s = p->states + i; > + if (state_table[s->operation].class == SC_VERTEXSHADER) > + { > + param = &(s->parameter); > + vshader = *(struct IDirect3DVertexShader9 **)param- > > > > data; > + if (vshader) > + { > + IDirect3DVertexShader9_GetFunction(vshader, > NULL, > &size); > + if (desc->pVertexShaderFunction) > + free((void *)desc->pVertexShaderFunction); > + desc->pVertexShaderFunction = malloc(size); > + IDirect3DVertexShader9_GetFunction(vshader, > (void > *)(desc->pVertexShaderFunction), &size); > + } > + } > + if (state_table[s->operation].class == SC_PIXELSHADER) > + { > + param = &(s->parameter); > + pshader = *(struct IDirect3DPixelShader9 **)param- > > > > data; > + if (pshader) > + { > + IDirect3DPixelShader9_GetFunction(pshader, NULL, > &size); > + if (desc->pPixelShaderFunction) > + free((void *)desc->pPixelShaderFunction); > + desc->pPixelShaderFunction = malloc(size); > + IDirect3DPixelShader9_GetFunction(pshader, (void > *)(desc->pPixelShaderFunction), &size); > + } > + } > + } > + } > + > + TRACE("Pass '%s': %i annots. Vertexshader %p and Pixelshader > %p\n", desc->Name, desc->Annotations, desc->pVertexShaderFunction, > desc->pPixelShaderFunction); > + > return D3D_OK; > } > > @@ -5955,6 +5995,8 @@ static HRESULT d3dx9_base_effect_init(struct > d3dx9_base_effect *base, > > base->effect = effect; > > + base->clonable = TRUE; > + > read_dword(&ptr, &tag); > TRACE("Tag: %x\n", tag); > > -- > 2.7.4

Oct. 29, 2016

Re: Imort system certificates in crypt32@Wine

by Donat Enikeev

Hi Jacek, I've applied your recommendations and submitted new version to @wine-patches > Also, it seems that we don't really have tests using root store. It > would be nice to have some. Generic test for this and other common stores are submitted as well, it took slightly more time, as appeared that certs for HKCU\MY are not saved in the registry, but in %AppData%, even though it is explicitly declared to use registry physical store. However, this test doesn't cover expressing of system certs into registry, as I hasn't figured out yet, how to do that in a portable way, so that tests are reliably working in a) the read-only context, b) across distributions and c) in the future, considering certificates revocation and expiration (ftr I have 173 certificates in /etc/ssl/certs/ca-certificates.crt shipped with distribution, not counting another hundreds of Mozilla) Re-implementing certs import from scratch for the reference, so tests are well-independent, would have taken notable amount of time, while putting hacks into the code for the test-purposes only - looks redundantly (and such patches will not be accepted, I suppose :). I am still thinking on this, but probably you would have (or already have) ideas how to do that in more elegant way Looking forward Best, Donnie On Чт, окт 27, 2016 at 2:15 , Jacek Caban <jacek(a)codeweavers.com> wrote: > Hi Donnie, > > On 26.10.2016 21:29, Donat Enikeev wrote: >> Hi Jacek, >> >> Following your proposal over the previous patch, that it should be >> a step forward to the implementation closer to the original >> behaviour, >> >> I've prepared different version of patch: >> >> 1. sligthly changed existing functions saving certs to the registry >> (to be able to pass the REG_OPTION_VOLATILE flag) >> 2. replaced the RootStore entity and everything related with new >> CRYPT_ImportSystemRootCertsToReg() that does actual import (once) >> into REG_OPTION_VOLATILE keys >> 3. start calling this import whenever an application is reaching >> necessity to work with HKLM\Root certs store as previously, however >> the latter call could be moved to the DllMain >> >> Does it look better? > > Yes, it looks better. I have a few comments from a quick review. > >> static const char * const CRYPT_knownLocations[] = { >> "/etc/ssl/certs/ca-certificates.crt", >> "/etc/ssl/certs", >> @@ -492,6 +445,7 @@ static const char * const >> CRYPT_knownLocations[] = { >> "/etc/security/cacerts", /* Android */ >> }; >> >> +/* This cert had been expired on 31.12.1999, shoult we delete it ? >> */ >> static const BYTE authenticode[] = { >> >> 0x30,0x82,0x03,0xd6,0x30,0x82,0x02,0xbe,0xa0,0x03,0x02,0x01,0x02,0x02,0x01,0x01, >> >> 0x30,0x0d,0x06,0x09,0x2a,0x86,0x48,0x86,0xf7,0x0d,0x01,0x01,0x04,0x05,0x00,0x30, >> @@ -555,6 +509,10 @@ static const BYTE authenticode[] = { >> >> 0x60,0xdc,0x82,0x1f,0xfb,0xce,0x74,0x06,0x7e,0xd6,0xf1,0xac,0x19,0x6a,0x4f,0x74, >> >> 0x5c,0xc5,0x15,0x66,0x31,0x6c,0xc1,0x62,0x71,0x91,0x0f,0x59,0x5b,0x7d,0x2a,0x82, >> 0x1a,0xdf,0xb1,0xb4,0xd8,0x1d,0x37,0xde,0x0d,0x0f }; >> + >> +/* Microsoft Root Authority >> + * Valid to 31.12.2020, please consider update after this date >> + */ >> static const BYTE rootauthority[] = { >> >> 0x30,0x82,0x04,0x12,0x30,0x82,0x02,0xfa,0xa0,0x03,0x02,0x01,0x02,0x02,0x0f,0x00, >> >> 0xc1,0x00,0x8b,0x3c,0x3c,0x88,0x11,0xd1,0x3e,0xf6,0x63,0xec,0xdf,0x40,0x30,0x0d, >> @@ -622,6 +580,10 @@ static const BYTE rootauthority[] = { >> >> 0x04,0xe4,0xa8,0x45,0x04,0xc8,0x5a,0x33,0x38,0x6e,0x4d,0x1c,0x0d,0x62,0xb7,0x0a, >> >> 0xa2,0x8c,0xd3,0xd5,0x54,0x3f,0x46,0xcd,0x1c,0x55,0xa6,0x70,0xdb,0x12,0x3a,0x87, >> 0x93,0x75,0x9f,0xa7,0xd2,0xa0 }; >> + >> +/* Microsoft Root Authority Certificate >> + * Valid to 09.05.2021, please consider update after this date >> + */ > > Please avoid unrelated changes in part of code that you don't touch. > The > patch changes enough places already. > >> static const BYTE rootcertauthority[] = { >> >> 0x30,0x82,0x05,0x99,0x30,0x82,0x03,0x81,0xa0,0x03,0x02,0x01,0x02,0x02,0x10,0x79, >> >> 0xad,0x16,0xa1,0x4a,0xa0,0xa5,0xad,0x4c,0x73,0x58,0xf4,0x07,0x13,0x2e,0x65,0x30, >> @@ -790,55 +752,48 @@ static void >> read_trusted_roots_from_known_locations(HCERTSTORE store) >> >> static HCERTSTORE create_root_store(void) >> { >> - HCERTSTORE root = NULL; >> HCERTSTORE memStore = CertOpenStore(CERT_STORE_PROV_MEMORY, >> X509_ASN_ENCODING, 0, CERT_STORE_CREATE_NEW_FLAG, NULL); >> >> if (memStore) >> { >> - CERT_STORE_PROV_INFO provInfo = { >> - sizeof(CERT_STORE_PROV_INFO), >> - sizeof(rootProvFuncs) / sizeof(rootProvFuncs[0]), >> - rootProvFuncs, >> - NULL, >> - 0, >> - NULL >> - }; >> - >> read_trusted_roots_from_known_locations(memStore); >> add_ms_root_certs(memStore); >> - root = CRYPT_ProvCreateStore(0, memStore, &provInfo); >> } >> - TRACE("returning %p\n", root); >> - return root; >> + TRACE("returning %p\n", memStore); >> + return memStore; >> } >> >> -static WINECRYPT_CERTSTORE *CRYPT_rootStore; >> +static LONG root_certs_imported = 0; >> >> -WINECRYPT_CERTSTORE *CRYPT_RootOpenStore(HCRYPTPROV hCryptProv, >> DWORD dwFlags) >> +static const WCHAR certs_root_path[] = >> + >> {'S','o','f','t','w','a','r','e','\\','M','i','c','r','o','s','o','f','t','\\', >> + >> 'S','y','s','t','e','m','C','e','r','t','i','f','i','c','a','t','e','s','\\', >> + 'R','o','o','t','\\', >> 'C','e','r','t','i','f','i','c','a','t','e','s', 0}; >> + >> +BOOL CRYPT_ImportSystemRootCertsToReg(void) >> { >> - TRACE("(%ld, %08x)\n", hCryptProv, dwFlags); >> + HCERTSTORE store = NULL; >> + HKEY key; >> + BOOL ret = FALSE; >> + LONG rc; >> >> - if (dwFlags & CERT_STORE_DELETE_FLAG) >> - { >> - WARN("root store can't be deleted\n"); >> - SetLastError(ERROR_ACCESS_DENIED); >> - return NULL; >> - } >> - if (!CRYPT_rootStore) >> - { >> - HCERTSTORE root = create_root_store(); >> + TRACE("\n"); >> + >> + if (root_certs_imported) return FALSE; > > To guarantee that it's called only once per process, INIT_ONCE would > be > nicer. However, in this case, we'd like to have it done only once per > wineserver session. For that CreateMutex seems like a good choice. > > Also, there is no need to return BOOL if since return value is not > used > anyway. > > Also, it seems that we don't really have tests using root store. It > would be nice to have some. > > Thanks, > Jacek

Oct. 28, 2016

Re: [PATCH] d2d1: Use transformed geometry to translate glyph run outline

by Nikolay Sivov

On 28.10.2016 16:55, Henri Verbeet wrote: > On 28 October 2016 at 13:23, Nikolay Sivov <nsivov(a)codeweavers.com> wrote: >> Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> >> --- >> dlls/d2d1/render_target.c | 27 +++++++++++++++++++-------- >> 1 file changed, 19 insertions(+), 8 deletions(-) >> > This is probably fine, but why would you want to do that? > > Only to make it work as a single geometry filling command, instead of three - adjusting target transform/filling/reverting transform. But as you pointed out until major issue with resources is resolved this is premature.

Oct. 28, 2016

Re: [PATCH] wined3d: Invalidate INDEXBUFFER device state when contents of bound index buffer is changed.

by Henri Verbeet

On 28 October 2016 at 12:20, Józef Kucia <jkucia(a)codeweavers.com> wrote: > + if (buffer->resource.bind_count) > + { > + if (buffer->buffer_type_hint == GL_ELEMENT_ARRAY_BUFFER) > + device_invalidate_state(buffer->resource.device, STATE_INDEXBUFFER); > + } You can merge those if-conditions. What happens exactly that makes this required?

Oct. 28, 2016

Re: [PATCH] d2d1: Use transformed geometry to translate glyph run outline

by Henri Verbeet

On 28 October 2016 at 13:23, Nikolay Sivov <nsivov(a)codeweavers.com> wrote: > Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> > --- > dlls/d2d1/render_target.c | 27 +++++++++++++++++++-------- > 1 file changed, 19 insertions(+), 8 deletions(-) > This is probably fine, but why would you want to do that?

Oct. 28, 2016

Re: [v2 PATCH] ucrtbase: Added _sopen_dispatch/_wsopen_dispatch

by Piotr Caban

On 10/28/16 13:49, Nikolay Sivov wrote: > On 28.10.2016 14:53, Piotr Caban wrote: >> A FIXME message would be probably also useful since the secure parameter >> is ignored. > > I'm not sure about that, every *sopen_s call will trigger it. I don't know what the secure parameter is meant to do without doing some testing but I don't like the fact that it's ignored without any sign in logs. I guess you have some more information about it, e.g. I don't know why *_s function are passing 1 there. Did you find any documentation for this function? Thanks, Piotr

Oct. 28, 2016

Re: [v3 PATCH] ucrtbase: Added _sopen_dispatch/_wsopen_dispatch

by Huw Davies

Signed-off-by: Huw Davies <huw(a)codeweavers.com>

Oct. 28, 2016

Re: [v2 PATCH] ucrtbase: Added _sopen_dispatch/_wsopen_dispatch

by Piotr Caban

On 10/28/16 13:41, Huw Davies wrote: > On Fri, Oct 28, 2016 at 01:43:19PM +0300, Nikolay Sivov wrote: >> diff --git a/dlls/msvcrt/file.c b/dlls/msvcrt/file.c >> index 432d5c7..37e5ece 100644 >> --- a/dlls/msvcrt/file.c >> +++ b/dlls/msvcrt/file.c >> @@ -2144,9 +2144,10 @@ static int check_bom(HANDLE h, int oflags, BOOL seek) >> } >> >> /********************************************************************* >> - * _wsopen_s (MSVCRT.@) >> + * _wsopen_dispatch (UCRTBASE.@) >> */ >> -int CDECL MSVCRT__wsopen_s( int *fd, const MSVCRT_wchar_t* path, int oflags, int shflags, int pmode ) >> +int CDECL MSVCRT__wsopen_dispatch( const MSVCRT_wchar_t* path, int oflags, int shflags, int pmode, >> + int *fd, int secure ) >> { >> DWORD access = 0, creation = 0, attrib; >> SECURITY_ATTRIBUTES sa; > > You probably want to add 'secure' to the TRACE() just below here and > change the order of the parameters in that TRACE() to match the new > function. A FIXME message would be probably also useful since the secure parameter is ignored. Thanks, Piotr

Oct. 28, 2016

Re: [v2 PATCH] ucrtbase: Added _sopen_dispatch/_wsopen_dispatch

by Nikolay Sivov

On 28.10.2016 14:53, Piotr Caban wrote: > On 10/28/16 13:41, Huw Davies wrote: >> On Fri, Oct 28, 2016 at 01:43:19PM +0300, Nikolay Sivov wrote: >>> diff --git a/dlls/msvcrt/file.c b/dlls/msvcrt/file.c >>> index 432d5c7..37e5ece 100644 >>> --- a/dlls/msvcrt/file.c >>> +++ b/dlls/msvcrt/file.c >>> @@ -2144,9 +2144,10 @@ static int check_bom(HANDLE h, int oflags, >>> BOOL seek) >>> } >>> >>> /********************************************************************* >>> - * _wsopen_s (MSVCRT.@) >>> + * _wsopen_dispatch (UCRTBASE.@) >>> */ >>> -int CDECL MSVCRT__wsopen_s( int *fd, const MSVCRT_wchar_t* path, int >>> oflags, int shflags, int pmode ) >>> +int CDECL MSVCRT__wsopen_dispatch( const MSVCRT_wchar_t* path, int >>> oflags, int shflags, int pmode, >>> + int *fd, int secure ) >>> { >>> DWORD access = 0, creation = 0, attrib; >>> SECURITY_ATTRIBUTES sa; >> >> You probably want to add 'secure' to the TRACE() just below here and >> change the order of the parameters in that TRACE() to match the new >> function. > > A FIXME message would be probably also useful since the secure parameter > is ignored. I'm not sure about that, every *sopen_s call will trigger it. > > Thanks, > Piotr > > >

Oct. 28, 2016

Re: [PATCH] winebus.sys: Remove dead code

by Aric Stewart

On 10/28/16 4:41 AM, Sebastian Lackner wrote: > It is dead code, but I don't think putting it in a comment makes it > better. In general, commented out code should be avoided because it is > not checked by the compiler. In addition, we might want to implement > driver unloading in the near future, and will have to add the code > back then (including the "if" because failures to create the monitor > are not critical). I am not sure what you are advocating for here. Leaving it as it is hoping that driver unloading is implemented soon? If so then I feel like I should write a full and proper unload function. Fully removing the code without comment bugs me because then we are loosing knowledge. In 1 month, 6 months, years when driver unloading is implemented the need to release the monitor will have to be remember/rediscovered. -aric > > 2016-10-28 3:00 GMT+02:00 Aric Stewart <aric(a)codeweavers.com>: >> >> Coverity 1373717 >> >> Signed-off-by: Aric Stewart <aric(a)codeweavers.com> >> --- >> dlls/winebus.sys/bus_udev.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> >> >> >> > >

Oct. 28, 2016

Re: [v2 PATCH] ucrtbase: Added _sopen_dispatch/_wsopen_dispatch

by Huw Davies

On Fri, Oct 28, 2016 at 01:43:19PM +0300, Nikolay Sivov wrote: > diff --git a/dlls/msvcrt/file.c b/dlls/msvcrt/file.c > index 432d5c7..37e5ece 100644 > --- a/dlls/msvcrt/file.c > +++ b/dlls/msvcrt/file.c > @@ -2144,9 +2144,10 @@ static int check_bom(HANDLE h, int oflags, BOOL seek) > } > > /********************************************************************* > - * _wsopen_s (MSVCRT.@) > + * _wsopen_dispatch (UCRTBASE.@) > */ > -int CDECL MSVCRT__wsopen_s( int *fd, const MSVCRT_wchar_t* path, int oflags, int shflags, int pmode ) > +int CDECL MSVCRT__wsopen_dispatch( const MSVCRT_wchar_t* path, int oflags, int shflags, int pmode, > + int *fd, int secure ) > { > DWORD access = 0, creation = 0, attrib; > SECURITY_ATTRIBUTES sa; You probably want to add 'secure' to the TRACE() just below here and change the order of the parameters in that TRACE() to match the new function. Huw.

Oct. 28, 2016

Re: [PATCH] winebus.sys: Remove dead code

by Sebastian Lackner

It is dead code, but I don't think putting it in a comment makes it better. In general, commented out code should be avoided because it is not checked by the compiler. In addition, we might want to implement driver unloading in the near future, and will have to add the code back then (including the "if" because failures to create the monitor are not critical). 2016-10-28 3:00 GMT+02:00 Aric Stewart <aric(a)codeweavers.com>: > > Coverity 1373717 > > Signed-off-by: Aric Stewart <aric(a)codeweavers.com> > --- > dlls/winebus.sys/bus_udev.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > >

Oct. 28, 2016

Re: Cmd.exe test cases

by Frédéric Delanoy

On Fri, Oct 28, 2016 at 2:51 AM, Carlos Rafael Ramirez <crramirez(a)gmail.com> wrote: > Hello, > > I want to add some test cases to cmd program. But I don't understand the > structure of them. Where to put the command and where to put the assertion > or expected output? > > I am coding a patch and I want to make it like TDD, first the test fail and > after the fix the test passes. > > Thank you Hi Carlos, Tests for builtins go to test_builitins.cmd (commands) and test_builtins.cmd.exp (expected results) test_cmdline.cmd[.exp] is for command line cmd usage, so not what you want here. make crosstest can be used to create a win executable for testing on native. For more information, see https://wiki.winehq.org/Cmd_Conformance_Tests Regards, Frédéric Delanoy

Oct. 28, 2016

Cmd.exe test cases

by Carlos Rafael Ramirez

Hello, I want to add some test cases to cmd program. But I don't understand the structure of them. Where to put the command and where to put the assertion or expected output? I am coding a patch and I want to make it like TDD, first the test fail and after the fix the test passes. Thank you

Oct. 27, 2016

Re: [3/3] hidclass.sys: Only return new reports when reading data from ringbuffer.

by Aric Stewart

On 10/27/16 9:45 AM, Sebastian Lackner wrote: > Signed-off-by: Sebastian Lackner <sebastian(a)fds-team.de> > --- > > Depending on the buffer size, we might get a huge amount of old reports otherwise. > > @ Aric: I believe there are some more things wrong with the ringbuffer implementation. > We shouldn't really need a ->start pointer, this is exactly what the ring->pointers[] > are used for. Also, the current implementation never uses all slots. > Reviews on the ringbuffer code is very welcome! Thanks! It has been a while since I did that implementation but if I recall the purpose for start and end is about tracking the writing positions, It was more important before this patch, but after this patch you are correct that start is no longer needed. 2 -aric > dlls/hidclass.sys/buffer.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/dlls/hidclass.sys/buffer.c b/dlls/hidclass.sys/buffer.c > index 14e83e4..0b29f97 100644 > --- a/dlls/hidclass.sys/buffer.c > +++ b/dlls/hidclass.sys/buffer.c > @@ -170,7 +170,7 @@ UINT RingBuffer_AddPointer(struct ReportRingBuffer *ring) > for( ;count < ring->pointer_alloc; count++) > ring->pointers[count] = POINTER_UNUSED; > } > - ring->pointers[idx] = ring->start; > + ring->pointers[idx] = ring->end; > LeaveCriticalSection(&ring->lock); > return idx; > } >

Oct. 27, 2016

Re: [PATCH] ucrtbase: Added _sopen_dispatch/_wsopen_dispatch

by Huw Davies

On 27 Oct 2016, at 17:17, Nikolay Sivov <nsivov(a)codeweavers.com> wrote: > diff --git a/dlls/msvcrt/file.c b/dlls/msvcrt/file.c > index 432d5c7..9097598 100644 > --- a/dlls/msvcrt/file.c > +++ b/dlls/msvcrt/file.c > @@ -2330,6 +2330,18 @@ int CDECL MSVCRT__wsopen( const MSVCRT_wchar_t *path, int oflags, int shflags, . > } > > /********************************************************************* > + * _wsopen_dispatch (UCRTBASE.@) > + */ > +int CDECL MSVCRT__wsopen_dispatch( const MSVCRT_wchar_t *path, int oflags, int shflags, > + int pmode, int *filehandle, int secure) > +{ > + if (secure) > + FIXME("secure argument ignored.\n"); > + > + return MSVCRT__wsopen_s( filehandle, path, oflags, shflags, pmode ); > +} Wouldn’t it make more sense to have _wsopen() and _wsopen_s() call _wsopen_dispatch() (and likewise for the char* version)? Huw.

Oct. 27, 2016

Re: ntdll: Add ApiSetQueryApiSetPresence stub.

by Nikolay Sivov

Do you have a program that needs this?

Oct. 27, 2016

Re: ntdll: Forward EtwEventRegister() from EventRegister().

by Nikolay Sivov

> --- a/dlls/ntdll/ntdll.spec > +++ b/dlls/ntdll/ntdll.spec > @@ -41,6 +41,7 @@ > # @ stub DbgUiStopDebugging > @ stub DbgUiWaitStateChange > @ stdcall DbgUserBreakPoint() > +@ stdcall EtwEventRegister(ptr ptr ptr ptr) advapi32.EventRegister This won't do it. Advapi32 should forward to ntdll, not the other way around.

Oct. 27, 2016

Re: Imort system certificates in crypt32@Wine

by Jacek Caban

Hi Donnie, On 26.10.2016 21:29, Donat Enikeev wrote: > Hi Jacek, > > Following your proposal over the previous patch, that it should be a step forward to the implementation closer to the original behaviour, > > I've prepared different version of patch: > > 1. sligthly changed existing functions saving certs to the registry (to be able to pass the REG_OPTION_VOLATILE flag) > 2. replaced the RootStore entity and everything related with new CRYPT_ImportSystemRootCertsToReg() that does actual import (once) into REG_OPTION_VOLATILE keys > 3. start calling this import whenever an application is reaching necessity to work with HKLM\Root certs store as previously, however the latter call could be moved to the DllMain > > Does it look better? Yes, it looks better. I have a few comments from a quick review. > static const char * const CRYPT_knownLocations[] = { > "/etc/ssl/certs/ca-certificates.crt", > "/etc/ssl/certs", > @@ -492,6 +445,7 @@ static const char * const CRYPT_knownLocations[] = { > "/etc/security/cacerts", /* Android */ > }; > > +/* This cert had been expired on 31.12.1999, shoult we delete it ? */ > static const BYTE authenticode[] = { > 0x30,0x82,0x03,0xd6,0x30,0x82,0x02,0xbe,0xa0,0x03,0x02,0x01,0x02,0x02,0x01,0x01, > 0x30,0x0d,0x06,0x09,0x2a,0x86,0x48,0x86,0xf7,0x0d,0x01,0x01,0x04,0x05,0x00,0x30, > @@ -555,6 +509,10 @@ static const BYTE authenticode[] = { > 0x60,0xdc,0x82,0x1f,0xfb,0xce,0x74,0x06,0x7e,0xd6,0xf1,0xac,0x19,0x6a,0x4f,0x74, > 0x5c,0xc5,0x15,0x66,0x31,0x6c,0xc1,0x62,0x71,0x91,0x0f,0x59,0x5b,0x7d,0x2a,0x82, > 0x1a,0xdf,0xb1,0xb4,0xd8,0x1d,0x37,0xde,0x0d,0x0f }; > + > +/* Microsoft Root Authority > + * Valid to 31.12.2020, please consider update after this date > + */ > static const BYTE rootauthority[] = { > 0x30,0x82,0x04,0x12,0x30,0x82,0x02,0xfa,0xa0,0x03,0x02,0x01,0x02,0x02,0x0f,0x00, > 0xc1,0x00,0x8b,0x3c,0x3c,0x88,0x11,0xd1,0x3e,0xf6,0x63,0xec,0xdf,0x40,0x30,0x0d, > @@ -622,6 +580,10 @@ static const BYTE rootauthority[] = { > 0x04,0xe4,0xa8,0x45,0x04,0xc8,0x5a,0x33,0x38,0x6e,0x4d,0x1c,0x0d,0x62,0xb7,0x0a, > 0xa2,0x8c,0xd3,0xd5,0x54,0x3f,0x46,0xcd,0x1c,0x55,0xa6,0x70,0xdb,0x12,0x3a,0x87, > 0x93,0x75,0x9f,0xa7,0xd2,0xa0 }; > + > +/* Microsoft Root Authority Certificate > + * Valid to 09.05.2021, please consider update after this date > + */ Please avoid unrelated changes in part of code that you don't touch. The patch changes enough places already. > static const BYTE rootcertauthority[] = { > 0x30,0x82,0x05,0x99,0x30,0x82,0x03,0x81,0xa0,0x03,0x02,0x01,0x02,0x02,0x10,0x79, > 0xad,0x16,0xa1,0x4a,0xa0,0xa5,0xad,0x4c,0x73,0x58,0xf4,0x07,0x13,0x2e,0x65,0x30, > @@ -790,55 +752,48 @@ static void read_trusted_roots_from_known_locations(HCERTSTORE store) > > static HCERTSTORE create_root_store(void) > { > - HCERTSTORE root = NULL; > HCERTSTORE memStore = CertOpenStore(CERT_STORE_PROV_MEMORY, > X509_ASN_ENCODING, 0, CERT_STORE_CREATE_NEW_FLAG, NULL); > > if (memStore) > { > - CERT_STORE_PROV_INFO provInfo = { > - sizeof(CERT_STORE_PROV_INFO), > - sizeof(rootProvFuncs) / sizeof(rootProvFuncs[0]), > - rootProvFuncs, > - NULL, > - 0, > - NULL > - }; > - > read_trusted_roots_from_known_locations(memStore); > add_ms_root_certs(memStore); > - root = CRYPT_ProvCreateStore(0, memStore, &provInfo); > } > - TRACE("returning %p\n", root); > - return root; > + TRACE("returning %p\n", memStore); > + return memStore; > } > > -static WINECRYPT_CERTSTORE *CRYPT_rootStore; > +static LONG root_certs_imported = 0; > > -WINECRYPT_CERTSTORE *CRYPT_RootOpenStore(HCRYPTPROV hCryptProv, DWORD dwFlags) > +static const WCHAR certs_root_path[] = > + {'S','o','f','t','w','a','r','e','\\','M','i','c','r','o','s','o','f','t','\\', > + 'S','y','s','t','e','m','C','e','r','t','i','f','i','c','a','t','e','s','\\', > + 'R','o','o','t','\\', 'C','e','r','t','i','f','i','c','a','t','e','s', 0}; > + > +BOOL CRYPT_ImportSystemRootCertsToReg(void) > { > - TRACE("(%ld, %08x)\n", hCryptProv, dwFlags); > + HCERTSTORE store = NULL; > + HKEY key; > + BOOL ret = FALSE; > + LONG rc; > > - if (dwFlags & CERT_STORE_DELETE_FLAG) > - { > - WARN("root store can't be deleted\n"); > - SetLastError(ERROR_ACCESS_DENIED); > - return NULL; > - } > - if (!CRYPT_rootStore) > - { > - HCERTSTORE root = create_root_store(); > + TRACE("\n"); > + > + if (root_certs_imported) return FALSE; To guarantee that it's called only once per process, INIT_ONCE would be nicer. However, in this case, we'd like to have it done only once per wineserver session. For that CreateMutex seems like a good choice. Also, there is no need to return BOOL if since return value is not used anyway. Also, it seems that we don't really have tests using root store. It would be nice to have some. Thanks, Jacek

Oct. 27, 2016

Re: [PATCH 1/2] edit: Test that EM_REPLACESEL works after EM_GETHANDLE.

by Sebastian Lackner

On 27.10.2016 00:41, Roman Pišl wrote: > Signed-off-by: Roman Pišl <rpisl(a)seznam.cz> > --- > dlls/user32/tests/edit.c | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) I haven't reviewed the patch yet, but please note that the subject should start with the component (here user32), not with the filename. It was also wrong in http://source.winehq.org/git/wine.git/patch/ddfc5275af0081cba6e834bf9d9e35d…, but seems like Alexandre didn't notice it. ;) Best regards, Sebastian

Oct. 26, 2016

Re: Imort system certificates in crypt32@Wine

by Donat Enikeev

Hi Jacek, Following your proposal over the previous patch, that it should be a step forward to the implementation closer to the original behaviour, I've prepared different version of patch: 1. sligthly changed existing functions saving certs to the registry (to be able to pass the REG_OPTION_VOLATILE flag) 2. replaced the RootStore entity and everything related with new CRYPT_ImportSystemRootCertsToReg() that does actual import (once) into REG_OPTION_VOLATILE keys 3. start calling this import whenever an application is reaching necessity to work with HKLM\Root certs store as previously, however the latter call could be moved to the DllMain Does it look better? P.s. The CiscoIP and certmgr apps are working without problems, registry keys of system certs are not saved with the wine unload as expected; though tests are not ready yet Best, Donnie Signed-off-by: Donat Enikeev <donat(a)enikeev.net> --- dlls/crypt32/crypt32_private.h | 6 +- dlls/crypt32/main.c | 3 +- dlls/crypt32/regstore.c | 10 ++-- dlls/crypt32/rootstore.c | 121 +++++++++++++---------------------------- dlls/crypt32/store.c | 13 ++--- 5 files changed, 53 insertions(+), 100 deletions(-) diff --git a/dlls/crypt32/crypt32_private.h b/dlls/crypt32/crypt32_private.h index ad4a827..8d415dc 100644 --- a/dlls/crypt32/crypt32_private.h +++ b/dlls/crypt32/crypt32_private.h @@ -336,7 +336,11 @@ WINECRYPT_CERTSTORE *CRYPT_FileNameOpenStoreA(HCRYPTPROV hCryptProv, DWORD dwFlags, const void *pvPara) DECLSPEC_HIDDEN; WINECRYPT_CERTSTORE *CRYPT_FileNameOpenStoreW(HCRYPTPROV hCryptProv, DWORD dwFlags, const void *pvPara) DECLSPEC_HIDDEN; -WINECRYPT_CERTSTORE *CRYPT_RootOpenStore(HCRYPTPROV hCryptProv, DWORD dwFlags) DECLSPEC_HIDDEN; + +BOOL CRYPT_ImportSystemRootCertsToReg(void) DECLSPEC_HIDDEN; +BOOL CRYPT_SerializeContextsToReg(HKEY key, DWORD dwFlags, const WINE_CONTEXT_INTERFACE *contextInterface, + HCERTSTORE memStore) DECLSPEC_HIDDEN; + BOOL CRYPT_IsCertificateSelfSigned(PCCERT_CONTEXT cert) DECLSPEC_HIDDEN; /* Allocates and initializes a certificate chain engine, but without creating diff --git a/dlls/crypt32/main.c b/dlls/crypt32/main.c index 241a1d9..cf6c66a 100644 --- a/dlls/crypt32/main.c +++ b/dlls/crypt32/main.c @@ -49,7 +49,6 @@ BOOL WINAPI DllMain(HINSTANCE hInst, DWORD fdwReason, PVOID pvReserved) if (pvReserved) break; crypt_oid_free(); crypt_sip_free(); - root_store_free(); default_chain_engine_free(); if (hDefProv) CryptReleaseContext(hDefProv, 0); break; @@ -194,7 +193,7 @@ HCRYPTPROV WINAPI I_CryptGetDefaultCryptProv(DWORD reserved) BOOL WINAPI I_CryptReadTrustedPublisherDWORDValueFromRegistry(LPCWSTR name, DWORD *value) { - static const WCHAR safer[] = { + static const WCHAR safer[] = { 'S','o','f','t','w','a','r','e','\\','P','o','l','i','c','i','e','s','\\', 'M','i','c','r','o','s','o','f','t','\\','S','y','s','t','e','m', 'C','e','r','t','i','f','i','c','a','t','e','s','\\', diff --git a/dlls/crypt32/regstore.c b/dlls/crypt32/regstore.c index b79387f..750f315 100644 --- a/dlls/crypt32/regstore.c +++ b/dlls/crypt32/regstore.c @@ -179,7 +179,7 @@ static void CRYPT_RegReadFromReg(HKEY key, HCERTSTORE store) } /* Hash is assumed to be 20 bytes in length (a SHA-1 hash) */ -static BOOL CRYPT_WriteSerializedToReg(HKEY key, const BYTE *hash, const BYTE *buf, +static BOOL CRYPT_WriteSerializedToReg(HKEY key, DWORD dwFlags, const BYTE *hash, const BYTE *buf, DWORD len) { WCHAR asciiHash[20 * 2 + 1]; @@ -188,7 +188,7 @@ static BOOL CRYPT_WriteSerializedToReg(HKEY key, const BYTE *hash, const BYTE *b BOOL ret; CRYPT_HashToStr(hash, asciiHash); - rc = RegCreateKeyExW(key, asciiHash, 0, NULL, 0, KEY_ALL_ACCESS, NULL, + rc = RegCreateKeyExW(key, asciiHash, 0, NULL, dwFlags, KEY_ALL_ACCESS, NULL, &subKey, NULL); if (!rc) { @@ -205,7 +205,7 @@ static BOOL CRYPT_WriteSerializedToReg(HKEY key, const BYTE *hash, const BYTE *b return ret; } -static BOOL CRYPT_SerializeContextsToReg(HKEY key, +BOOL CRYPT_SerializeContextsToReg(HKEY key, DWORD dwFlags, const WINE_CONTEXT_INTERFACE *contextInterface, HCERTSTORE memStore) { const void *context = NULL; @@ -232,7 +232,7 @@ static BOOL CRYPT_SerializeContextsToReg(HKEY key, { ret = contextInterface->serialize(context, 0, buf, &size); if (ret) - ret = CRYPT_WriteSerializedToReg(key, hash, buf, size); + ret = CRYPT_WriteSerializedToReg(key, dwFlags, hash, buf, size); } CryptMemFree(buf); } @@ -287,7 +287,7 @@ static BOOL CRYPT_RegWriteToReg(WINE_REGSTOREINFO *store) } LeaveCriticalSection(&store->cs); } - ret = CRYPT_SerializeContextsToReg(key, interfaces[i], + ret = CRYPT_SerializeContextsToReg(key, 0, interfaces[i], store->memStore); RegCloseKey(key); } diff --git a/dlls/crypt32/rootstore.c b/dlls/crypt32/rootstore.c index 5cee235..9ad330c 100644 --- a/dlls/crypt32/rootstore.c +++ b/dlls/crypt32/rootstore.c @@ -435,53 +435,6 @@ static BOOL import_certs_from_path(LPCSTR path, HCERTSTORE store, return ret; } -static BOOL WINAPI CRYPT_RootWriteCert(HCERTSTORE hCertStore, - PCCERT_CONTEXT cert, DWORD dwFlags) -{ - /* The root store can't have certs added */ - return FALSE; -} - -static BOOL WINAPI CRYPT_RootDeleteCert(HCERTSTORE hCertStore, - PCCERT_CONTEXT cert, DWORD dwFlags) -{ - /* The root store can't have certs deleted */ - return FALSE; -} - -static BOOL WINAPI CRYPT_RootWriteCRL(HCERTSTORE hCertStore, - PCCRL_CONTEXT crl, DWORD dwFlags) -{ - /* The root store can have CRLs added. At worst, a malicious application - * can DoS itself, as the changes aren't persisted in any way. - */ - return TRUE; -} - -static BOOL WINAPI CRYPT_RootDeleteCRL(HCERTSTORE hCertStore, - PCCRL_CONTEXT crl, DWORD dwFlags) -{ - /* The root store can't have CRLs deleted */ - return FALSE; -} - -static void *rootProvFuncs[] = { - NULL, /* CERT_STORE_PROV_CLOSE_FUNC */ - NULL, /* CERT_STORE_PROV_READ_CERT_FUNC */ - CRYPT_RootWriteCert, - CRYPT_RootDeleteCert, - NULL, /* CERT_STORE_PROV_SET_CERT_PROPERTY_FUNC */ - NULL, /* CERT_STORE_PROV_READ_CRL_FUNC */ - CRYPT_RootWriteCRL, - CRYPT_RootDeleteCRL, - NULL, /* CERT_STORE_PROV_SET_CRL_PROPERTY_FUNC */ - NULL, /* CERT_STORE_PROV_READ_CTL_FUNC */ - NULL, /* CERT_STORE_PROV_WRITE_CTL_FUNC */ - NULL, /* CERT_STORE_PROV_DELETE_CTL_FUNC */ - NULL, /* CERT_STORE_PROV_SET_CTL_PROPERTY_FUNC */ - NULL, /* CERT_STORE_PROV_CONTROL_FUNC */ -}; - static const char * const CRYPT_knownLocations[] = { "/etc/ssl/certs/ca-certificates.crt", "/etc/ssl/certs", @@ -492,6 +445,7 @@ static const char * const CRYPT_knownLocations[] = { "/etc/security/cacerts", /* Android */ }; +/* This cert had been expired on 31.12.1999, shoult we delete it ? */ static const BYTE authenticode[] = { 0x30,0x82,0x03,0xd6,0x30,0x82,0x02,0xbe,0xa0,0x03,0x02,0x01,0x02,0x02,0x01,0x01, 0x30,0x0d,0x06,0x09,0x2a,0x86,0x48,0x86,0xf7,0x0d,0x01,0x01,0x04,0x05,0x00,0x30, @@ -555,6 +509,10 @@ static const BYTE authenticode[] = { 0x60,0xdc,0x82,0x1f,0xfb,0xce,0x74,0x06,0x7e,0xd6,0xf1,0xac,0x19,0x6a,0x4f,0x74, 0x5c,0xc5,0x15,0x66,0x31,0x6c,0xc1,0x62,0x71,0x91,0x0f,0x59,0x5b,0x7d,0x2a,0x82, 0x1a,0xdf,0xb1,0xb4,0xd8,0x1d,0x37,0xde,0x0d,0x0f }; + +/* Microsoft Root Authority + * Valid to 31.12.2020, please consider update after this date + */ static const BYTE rootauthority[] = { 0x30,0x82,0x04,0x12,0x30,0x82,0x02,0xfa,0xa0,0x03,0x02,0x01,0x02,0x02,0x0f,0x00, 0xc1,0x00,0x8b,0x3c,0x3c,0x88,0x11,0xd1,0x3e,0xf6,0x63,0xec,0xdf,0x40,0x30,0x0d, @@ -622,6 +580,10 @@ static const BYTE rootauthority[] = { 0x04,0xe4,0xa8,0x45,0x04,0xc8,0x5a,0x33,0x38,0x6e,0x4d,0x1c,0x0d,0x62,0xb7,0x0a, 0xa2,0x8c,0xd3,0xd5,0x54,0x3f,0x46,0xcd,0x1c,0x55,0xa6,0x70,0xdb,0x12,0x3a,0x87, 0x93,0x75,0x9f,0xa7,0xd2,0xa0 }; + +/* Microsoft Root Authority Certificate + * Valid to 09.05.2021, please consider update after this date + */ static const BYTE rootcertauthority[] = { 0x30,0x82,0x05,0x99,0x30,0x82,0x03,0x81,0xa0,0x03,0x02,0x01,0x02,0x02,0x10,0x79, 0xad,0x16,0xa1,0x4a,0xa0,0xa5,0xad,0x4c,0x73,0x58,0xf4,0x07,0x13,0x2e,0x65,0x30, @@ -790,55 +752,48 @@ static void read_trusted_roots_from_known_locations(HCERTSTORE store) static HCERTSTORE create_root_store(void) { - HCERTSTORE root = NULL; HCERTSTORE memStore = CertOpenStore(CERT_STORE_PROV_MEMORY, X509_ASN_ENCODING, 0, CERT_STORE_CREATE_NEW_FLAG, NULL); if (memStore) { - CERT_STORE_PROV_INFO provInfo = { - sizeof(CERT_STORE_PROV_INFO), - sizeof(rootProvFuncs) / sizeof(rootProvFuncs[0]), - rootProvFuncs, - NULL, - 0, - NULL - }; - read_trusted_roots_from_known_locations(memStore); add_ms_root_certs(memStore); - root = CRYPT_ProvCreateStore(0, memStore, &provInfo); } - TRACE("returning %p\n", root); - return root; + TRACE("returning %p\n", memStore); + return memStore; } -static WINECRYPT_CERTSTORE *CRYPT_rootStore; +static LONG root_certs_imported = 0; -WINECRYPT_CERTSTORE *CRYPT_RootOpenStore(HCRYPTPROV hCryptProv, DWORD dwFlags) +static const WCHAR certs_root_path[] = + {'S','o','f','t','w','a','r','e','\\','M','i','c','r','o','s','o','f','t','\\', + 'S','y','s','t','e','m','C','e','r','t','i','f','i','c','a','t','e','s','\\', + 'R','o','o','t','\\', 'C','e','r','t','i','f','i','c','a','t','e','s', 0}; + +BOOL CRYPT_ImportSystemRootCertsToReg(void) { - TRACE("(%ld, %08x)\n", hCryptProv, dwFlags); + HCERTSTORE store = NULL; + HKEY key; + BOOL ret = FALSE; + LONG rc; - if (dwFlags & CERT_STORE_DELETE_FLAG) - { - WARN("root store can't be deleted\n"); - SetLastError(ERROR_ACCESS_DENIED); - return NULL; - } - if (!CRYPT_rootStore) - { - HCERTSTORE root = create_root_store(); + TRACE("\n"); + + if (root_certs_imported) return FALSE; - InterlockedCompareExchangePointer((PVOID *)&CRYPT_rootStore, root, - NULL); - if (CRYPT_rootStore != root) - CertCloseStore(root, 0); + store = create_root_store(); + if (!store) return FALSE; + + rc = RegCreateKeyExW(HKEY_LOCAL_MACHINE, certs_root_path, 0, NULL, 0, KEY_ALL_ACCESS, NULL, &key, 0); + if (!rc) + { + ret = CRYPT_SerializeContextsToReg(key, REG_OPTION_VOLATILE, pCertInterface, store); + if (ret) + InterlockedIncrement(&root_certs_imported); + RegCloseKey(key); } - CRYPT_rootStore->vtbl->addref(CRYPT_rootStore); - return CRYPT_rootStore; -} -void root_store_free(void) -{ - CertCloseStore(CRYPT_rootStore, 0); -} + CertCloseStore(store, 0); + return ret; +} \ No newline at end of file diff --git a/dlls/crypt32/store.c b/dlls/crypt32/store.c index 356712b..9e1980e 100644 --- a/dlls/crypt32/store.c +++ b/dlls/crypt32/store.c @@ -424,21 +424,15 @@ static WINECRYPT_CERTSTORE *CRYPT_SysRegOpenStoreW(HCRYPTPROV hCryptProv, SetLastError(E_INVALIDARG); return NULL; } - /* FIXME: In Windows, the root store (even the current user location) is - * protected: adding to it or removing from it present a user interface, - * and the keys are owned by the system process, not the current user. - * Wine's registry doesn't implement access controls, so a similar - * mechanism isn't possible yet. - */ - if ((dwFlags & CERT_SYSTEM_STORE_LOCATION_MASK) == - CERT_SYSTEM_STORE_LOCAL_MACHINE && !lstrcmpiW(storeName, rootW)) - return CRYPT_RootOpenStore(hCryptProv, dwFlags); switch (dwFlags & CERT_SYSTEM_STORE_LOCATION_MASK) { case CERT_SYSTEM_STORE_LOCAL_MACHINE: root = HKEY_LOCAL_MACHINE; base = CERT_LOCAL_MACHINE_SYSTEM_STORE_REGPATH; + /* If the HKLM\Root certs are requested, expressing system certs into the registry */ + if (!lstrcmpiW(storeName, rootW)) + CRYPT_ImportSystemRootCertsToReg(); break; case CERT_SYSTEM_STORE_CURRENT_USER: root = HKEY_CURRENT_USER; @@ -491,6 +485,7 @@ static WINECRYPT_CERTSTORE *CRYPT_SysRegOpenStoreW(HCRYPTPROV hCryptProv, KEY_ALL_ACCESS; wsprintfW(storePath, fmt, base, storeName); + if (dwFlags & CERT_STORE_OPEN_EXISTING_FLAG) rc = RegOpenKeyExW(root, storePath, 0, sam, &key); else -- 2.7.4

Oct. 26, 2016

d3dx9: d3dx9_base_effect_get_pass_desc

by Luis C. Busquets Pérez

Dear developers, I have tried to run Civ4BeyondtheSword.exe with the internal d3dx9_33.dll I can run the programme but when I start a game, it hangs. I saw that a lot of FIXME's with the message: fixme:d3dx:d3dx9_base_effect_get_pass_desc Pixel shader and vertex shader are not supported, yet. Therefore, I tried to finishing implementing the function. Find below a patch that works. With this patch, the programme does not hang. Fell free to study, redo it or rewrite it but please I look forward to somebody who corrects it and manages to integrate that function in the wine tree so that I do not need to compile the wine. On the other hand, the settler image seems to something in the middle. Furthermore, the leaders also seem to have something in the middle. Actually, it seems that the image that should be behind the leader in the bottom is in the middle of them and bend vertically when they bend. I assume this is not because of the patch but I guess that this information could serve to locate and correct another bug. --- dlls/d3dx9_36/effect.c | 46 ++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 44 insertions(+), 2 deletions(-) diff --git a/dlls/d3dx9_36/effect.c b/dlls/d3dx9_36/effect.c index 42f5aea..c69b73c 100644 --- a/dlls/d3dx9_36/effect.c +++ b/dlls/d3dx9_36/effect.c @@ -150,6 +150,7 @@ struct d3dx9_base_effect UINT parameter_count; UINT technique_count; UINT object_count; + BOOL clonable; struct d3dx_parameter *parameters; struct d3dx_technique *techniques; @@ -999,6 +1000,11 @@ static HRESULT d3dx9_base_effect_get_pass_desc(struct d3dx9_base_effect *base, D3DXHANDLE pass, D3DXPASS_DESC *desc) { struct d3dx_pass *p = get_valid_pass(base, pass); + struct d3dx_state *s; + struct d3dx_parameter *param; + IDirect3DVertexShader9 *vshader; + IDirect3DPixelShader9 *pshader; + UINT i, size; if (!desc || !p) { @@ -1008,11 +1014,45 @@ static HRESULT d3dx9_base_effect_get_pass_desc(struct d3dx9_base_effect *base, desc->Name = p->name; desc->Annotations = p->annotation_count; - - FIXME("Pixel shader and vertex shader are not supported, yet.\n"); desc->pVertexShaderFunction = NULL; desc->pPixelShaderFunction = NULL; + if (base->clonable) + { + for (i=0; i < p->state_count; i++) + { + s = p->states + i; + if (state_table[s->operation].class == SC_VERTEXSHADER) + { + param = &(s->parameter); + vshader = *(struct IDirect3DVertexShader9 **)param- >data; + if (vshader) + { + IDirect3DVertexShader9_GetFunction(vshader, NULL, &size); + if (desc->pVertexShaderFunction) + free((void *)desc->pVertexShaderFunction); + desc->pVertexShaderFunction = malloc(size); + IDirect3DVertexShader9_GetFunction(vshader, (void *)(desc->pVertexShaderFunction), &size); + } + } + if (state_table[s->operation].class == SC_PIXELSHADER) + { + param = &(s->parameter); + pshader = *(struct IDirect3DPixelShader9 **)param- >data; + if (pshader) + { + IDirect3DPixelShader9_GetFunction(pshader, NULL, &size); + if (desc->pPixelShaderFunction) + free((void *)desc->pPixelShaderFunction); + desc->pPixelShaderFunction = malloc(size); + IDirect3DPixelShader9_GetFunction(pshader, (void *)(desc->pPixelShaderFunction), &size); + } + } + } + } + + TRACE("Pass '%s': %i annots. Vertexshader %p and Pixelshader %p\n", desc->Name, desc->Annotations, desc->pVertexShaderFunction, desc->pPixelShaderFunction); + return D3D_OK; } @@ -5955,6 +5995,8 @@ static HRESULT d3dx9_base_effect_init(struct d3dx9_base_effect *base, base->effect = effect; + base->clonable = TRUE; + read_dword(&ptr, &tag); TRACE("Tag: %x\n", tag); -- 2.7.4

Oct. 26, 2016

d3dx9: d3dx9_base_effect_get_pass_desc

by Luis C. Busquets Pérez

Dear developers, I have tried to run Civ4BeyondtheSword.exe with the internal d3dx9_33.dll I can run the programme but when I start a game, it hangs. I saw that a lot of FIXME's with the message: fixme:d3dx:d3dx9_base_effect_get_pass_desc Pixel shader and vertex shader are not supported, yet. Therefore, I tried to finishing implementing the function. Find attached a patch that works. With this patch, the programme does not hang. Fell free to study, redo it or rewrite it but please I look forward to somebody who corrects it and manages to integrate that function in the wine tree so that I do not need to compile the wine. On the other hand, find attached an image, where you can see that the settler image seems to something in the middle. Furthermore, the leaders also seem to have something in the middle. Actually, it seems that the image that should be behind the leader in the bottom is in the middle of them and bend vertically when they bend. I assume this is not because of the patch but I guess that this information could serve to locate and correct another bug.

Oct. 26, 2016

Re: [PATCH] hid: implement HidD_SetOutputReport

by Sebastian Lackner

On 24.10.2016 16:06, Aric Stewart wrote: > Also fixes an issue with IOCTL_HID_SET_FEATURE where the wrong buffers where being sent. > > Signed-off-by: Aric Stewart <aric(a)codeweavers.com> > --- > dlls/hid/hid.spec | 2 +- > dlls/hid/hidd.c | 6 ++++++ > dlls/hidclass.sys/device.c | 11 +++++++---- > include/ddk/hidsdi.h | 1 + > 4 files changed, 15 insertions(+), 5 deletions(-) > While you are just at it, shouldn't the first char in SystemBuffer be skipped? "packet.reportBuffer = irp->AssociatedIrp.SystemBuffer;" Also, it looks like the first assignment of "irp->IoStatus.Information = 0;" is not really required, its also assigned later. Regards, Sebastian

Oct. 26, 2016

Re: [6/6] webservices: Implement WsCall.

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=26297 Your paranoid android. === build (build) === Patch failed to apply

Oct. 26, 2016

Re: [PATCH 1/4] d2d1: Implement FillContainsPoint for rectangles

by Henri Verbeet

On 21 October 2016 at 18:40, Nikolay Sivov <bunglehead(a)gmail.com> wrote: > On Fri, Oct 21, 2016 at 7:28 PM, Andrew Eikum <aeikum(a)codeweavers.com> > wrote: >> + *contains = probe.x > geometry->u.rectangle.rect.left - tolerance && >> + probe.x < geometry->u.rectangle.rect.right + tolerance && >> + probe.y > geometry->u.rectangle.rect.top - tolerance && >> + probe.y < geometry->u.rectangle.rect.bottom + tolerance; >> + > Does it really work like that? I'd expect tolerance to be irrelevant for > paths constructed purely from lines. > So basically in general case path is approximated with lines (or enclosed > area with triangles) with given tolerance, > and then point is tested against this flattened path. While in rectangular > case it's already flat to begin with. > It would certainly be nice to have some tests for how "tolerance" is handled in general, even if those tests just end up as todo_wine. Similarly, it would be nice to have some tests for how problematic transformation matrices are handled.

Oct. 26, 2016

Product Registration Bug

by Jonathan Allen

Dear List, Do we have any movement on bug 29354? I'm trying to move to an all-Linux environment and am blocked on two (old, venerable) XP (but not MS) applications that go to sleep in the ... fixme:msi:msi_validate_product_id partial stud: template ... ... code and can't move beyond that point. Completely stuck - help! Jonathan

Oct. 25, 2016

Re: [PATCH 01/10 v2] server: Introduced iosb struct for server-side IO_STATUS_BLOCK representation and use it in irp_call.

by Sebastian Lackner

2016-10-25 13:26 GMT+02:00 Jacek Caban <jacek(a)codeweavers.com>: > On 25.10.2016 06:03, Sebastian Lackner wrote: >> On 24.10.2016 15:46, Jacek Caban wrote: >>> BTW, did you expect staging patches to be so much faster in overlapped >>> case than plain Wine? I didn't look too deeply in the code, maybe there >>> is some optimization, but that's a bit suspicious. >> Thanks, these are exactly the stats I was looking for. I'm not aware of >> any special optimizations, so not sure how Staging can be faster. >> If you used a prebuilt Staging version with compiler optimization enabled >> that could also be the reason. > > I should be more precise, sorry. In my tests I used plain Wine with > staging named pipe patches applied (some trivial conflicts needed to be > resolved AFAIR). I can try it other way if you think it would be valuable. Hm, I am not sure what causes it then. I will probably do some more investigations, but it shouldn't really block your patchset. It could also be a side effect of using a different socket type (SOCK_SEQPACKET vs SOCK_STREAM) for example. > >> BTW; I assume you are aware of it already, but your current >> implementation >> lacks handling of PIPE_NOWAIT, which is also implemented in the Staging >> patchset. > > Yes, this patchset was intended to be as small as makes sense and > PIPE_NOWAIT is not implemented in current Wine anyway. PIPE_NOWAIT > should be easy to implement on top of my patches, preferably after > patches for immediate read/write returns are in (that's one of mentioned > optimization). I expect this to be very simple then. > > Thanks for the feedback. > > Best regards, > Jacek

Oct. 25, 2016

Re: [PATCH 01/10 v2] server: Introduced iosb struct for server-side IO_STATUS_BLOCK representation and use it in irp_call.

by Jacek Caban

On 25.10.2016 06:03, Sebastian Lackner wrote: > On 24.10.2016 15:46, Jacek Caban wrote: >> BTW, did you expect staging patches to be so much faster in overlapped >> case than plain Wine? I didn't look too deeply in the code, maybe there >> is some optimization, but that's a bit suspicious. > Thanks, these are exactly the stats I was looking for. I'm not aware of > any special optimizations, so not sure how Staging can be faster. > If you used a prebuilt Staging version with compiler optimization enabled > that could also be the reason. I should be more precise, sorry. In my tests I used plain Wine with staging named pipe patches applied (some trivial conflicts needed to be resolved AFAIR). I can try it other way if you think it would be valuable. > BTW; I assume you are aware of it already, but your current > implementation > lacks handling of PIPE_NOWAIT, which is also implemented in the Staging > patchset. Yes, this patchset was intended to be as small as makes sense and PIPE_NOWAIT is not implemented in current Wine anyway. PIPE_NOWAIT should be easy to implement on top of my patches, preferably after patches for immediate read/write returns are in (that's one of mentioned optimization). I expect this to be very simple then. Thanks for the feedback. Best regards, Jacek

Oct. 25, 2016

Re: Polish translation

by Nikolay Sivov

On 24.10.2016 21:55, m4sk1n wrote: > Here is translated .po file from my GlotPress. Hope it will be used. > Hi. Thank you for your contribution. Translations should be submitted as regular patches, generated using git format-patch or similar. Also make sure to use your real name when submitting.

Oct. 25, 2016

Re: [PATCH 01/10 v2] server: Introduced iosb struct for server-side IO_STATUS_BLOCK representation and use it in irp_call.

by Sebastian Lackner

On 24.10.2016 15:46, Jacek Caban wrote: > On 20.10.2016 19:17, Sebastian Lackner wrote: >> On 19.10.2016 19:05, Jacek Caban wrote: >>> v2: Don't store async queues in a list. >>> >>> Signed-off-by: Jacek Caban <jacek(a)codeweavers.com> >>> --- >>> server/async.c | 34 +++++++++++++++++++++++++++++++++ >>> server/device.c | 58 >>> +++++++++++++++++++++++++-------------------------------- >>> server/file.h | 14 ++++++++++++++ >>> 3 files changed, 73 insertions(+), 33 deletions(-) >>> >> I'm not sure if Alexandre has already agreed to this approach, but from my >> point of view it would make sense to do some measurements. How fast is the >> proposed named pipe approach compared to alternatives (unpatched Wine, or >> the Linux-only implementation included in Wine Staging)? > > As Alexandre said, I think we should concentrate on correctness first, > but I did some benchmarking that should give us overview. That is correct, but of course there isn't just one way to implement named pipes. It might also be possible to implement it in a portable way without going through wineserver or using Linux-specific functions. It would require more complexity on the ntdll side though. In fact, having object handling in ntdll (at least as fast-path for process- local objects) is something people have been thinking about since a long time, and it probably could also be used to solve the message mode problem. Nevertheless, I have to admit that reusing the driver functions is also very elegant. ;) > Proper > measuring depends on many things and different implementations have > various weaknesses. Here are a few things to keep in mind: > > - My patchset leaves room for improvements as explained in [1]. I have > prototyped patches for those optimizations and tested both with and > without them. > > - Calls with immediate results have very different characteristics than > ones that return immediately. If we need to wait, then server round > trips either using overlapped calls or blocking select require server > round trips, for which extra data that needs to be transferred with my > patchset (if not too big), shouldn't cause significant difference. When > using my tests with large buffer, most calls should have immediate > results, so I expect it to tests worst-case scenario. > > - Current Wine implementation uses blocking socket calls for > non-overlapped pipes. This significantly affects results, so I tested > both in overlapped and non-overlapped mode (although calls themselves > are not overlapped). > > - Small buffer sizes would be interesting to test, because they cause > mode blocking and thus would show differences in such case. Sadly, > staging implementation doesn't take that into account, so results would > be meaningless. > > I used the attached test with parameters: > npbench.exe 1024 10000 100000 (for non-overlapped) > npbench.exe 1024 10000 100000 1 (for overlapped) > > Results are also attached. Proper benchmarking would require a lot more > care, but I think it shows what you asked for. This implementation is > 3.3 times slower in overlapped case, 7.4 times slower in non-overlapped > tests (3.1 and 2.5 times compared to Windows in VM). I'd say that's not > too bad. Overlapped case is even faster than I expected. > > BTW, did you expect staging patches to be so much faster in overlapped > case than plain Wine? I didn't look too deeply in the code, maybe there > is some optimization, but that's a bit suspicious. Thanks, these are exactly the stats I was looking for. I'm not aware of any special optimizations, so not sure how Staging can be faster. If you used a prebuilt Staging version with compiler optimization enabled that could also be the reason. Nevertheless, the values for "plain wine+msgmode patchset+optimizations" actually look pretty good when taking into account the wineserver roundtrip. > >> In general I like the idea, but I fear that adding significant optimizations >> afterwards will be very difficult. > > FWIW, it should be fairly easy to rebase staging patches on top of my > series. It leaves the whole previous implementation mostly untouched for > byte mode case. That, however, may not be true after further changes in > the future, esp. if we decided to use server I/O implementation for byte > mode case as well. Well, I would probably keep it for some more time in Staging, but I don't think it would make sense to merge it into the development branch. Issues are easier to debug when it behaves the same on all platforms. BTW; I assume you are aware of it already, but your current implementation lacks handling of PIPE_NOWAIT, which is also implemented in the Staging patchset. Best regards, Sebastian

Oct. 24, 2016

Re: Testing: gcc -foutline-msabi-xlogues patch

by Daniel Santos

On 10/23/2016 02:34 PM, Daniel Santos wrote: > I've found a flaw in the realignment case, so nevermind about testing > that, but the normally aligned case should still be fine. Actually, I was wrong about the flaw (it was fine), but it is now more efficient with the forced-realign case and ready for testing. Also, I accidentally did the numbers wrong on how much the .text shrunk, inflating them slightly. The current delta is 19.56% for the aligned case and 18.21% for the realigned case. repo: https://github.com/daniel-santos/gcc branch: outline-msabi-xlogues-5_4_0 Thanks, Daniel

Oct. 24, 2016

Re: [PATCH 09/10] kernel32/tests: Added more named pipe tests.

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=26285 Your paranoid android. === w864 (64 bit pipe) === pipe.c:2575: Test failed: FlushFileBuffers failed: 87 pipe.c:2575: Test failed: FlushFileBuffers failed: 87

Oct. 24, 2016

Re: [PATCH 01/10 v2] server: Introduced iosb struct for server-side IO_STATUS_BLOCK representation and use it in irp_call.

by Jacek Caban

On 24.10.2016 15:46, Jacek Caban wrote: > - My patchset leaves room for improvements as explained in [1]. [1] https://www.winehq.org/pipermail/wine-patches/2016-October/154803.html

Oct. 24, 2016

Re: [PATCH 01/10 v2] server: Introduced iosb struct for server-side IO_STATUS_BLOCK representation and use it in irp_call.

by Jacek Caban

On 20.10.2016 19:17, Sebastian Lackner wrote: > On 19.10.2016 19:05, Jacek Caban wrote: >> v2: Don't store async queues in a list. >> >> Signed-off-by: Jacek Caban <jacek(a)codeweavers.com> >> --- >> server/async.c | 34 +++++++++++++++++++++++++++++++++ >> server/device.c | 58 >> +++++++++++++++++++++++++-------------------------------- >> server/file.h | 14 ++++++++++++++ >> 3 files changed, 73 insertions(+), 33 deletions(-) >> > I'm not sure if Alexandre has already agreed to this approach, but from my > point of view it would make sense to do some measurements. How fast is the > proposed named pipe approach compared to alternatives (unpatched Wine, or > the Linux-only implementation included in Wine Staging)? As Alexandre said, I think we should concentrate on correctness first, but I did some benchmarking that should give us overview. Proper measuring depends on many things and different implementations have various weaknesses. Here are a few things to keep in mind: - My patchset leaves room for improvements as explained in [1]. I have prototyped patches for those optimizations and tested both with and without them. - Calls with immediate results have very different characteristics than ones that return immediately. If we need to wait, then server round trips either using overlapped calls or blocking select require server round trips, for which extra data that needs to be transferred with my patchset (if not too big), shouldn't cause significant difference. When using my tests with large buffer, most calls should have immediate results, so I expect it to tests worst-case scenario. - Current Wine implementation uses blocking socket calls for non-overlapped pipes. This significantly affects results, so I tested both in overlapped and non-overlapped mode (although calls themselves are not overlapped). - Small buffer sizes would be interesting to test, because they cause mode blocking and thus would show differences in such case. Sadly, staging implementation doesn't take that into account, so results would be meaningless. I used the attached test with parameters: npbench.exe 1024 10000 100000 (for non-overlapped) npbench.exe 1024 10000 100000 1 (for overlapped) Results are also attached. Proper benchmarking would require a lot more care, but I think it shows what you asked for. This implementation is 3.3 times slower in overlapped case, 7.4 times slower in non-overlapped tests (3.1 and 2.5 times compared to Windows in VM). I'd say that's not too bad. Overlapped case is even faster than I expected. BTW, did you expect staging patches to be so much faster in overlapped case than plain Wine? I didn't look too deeply in the code, maybe there is some optimization, but that's a bit suspicious. > In general I like the idea, but I fear that adding significant optimizations > afterwards will be very difficult. FWIW, it should be fairly easy to rebase staging patches on top of my series. It leaves the whole previous implementation mostly untouched for byte mode case. That, however, may not be true after further changes in the future, esp. if we decided to use server I/O implementation for byte mode case as well. Thanks, Jacek

Oct. 24, 2016

Re: Testing: gcc -foutline-msabi-xlogues patch

by Daniel Santos

I've found a flaw in the realignment case, so nevermind about testing that, but the normally aligned case should still be fine.

Oct. 23, 2016

Re: [PATCH 01/10 v2] server: Introduced iosb struct for server-side IO_STATUS_BLOCK representation and use it in irp_call.

by Alexandre Julliard

Sebastian Lackner <sebastian(a)fds-team.de> writes: > I'm not sure if Alexandre has already agreed to this approach, but from my > point of view it would make sense to do some measurements. How fast is the > proposed named pipe approach compared to alternatives (unpatched Wine, or > the Linux-only implementation included in Wine Staging)? > In general I like the idea, but I fear that adding significant optimizations > afterwards will be very difficult. Performance is of course an interesting question, but correctness comes first. Unpatched Wine does not provide correct semantics, so it doesn't really matter how fast it would be. A Linux-only hack could potentially be used as an optimization, but we need a working portable implementation first. -- Alexandre Julliard julliard(a)winehq.org

Oct. 23, 2016

Testing: gcc -foutline-msabi-xlogues patch

by Daniel Santos

I've been working on a gcc patch targeting 64-bit Wine to reduce the impact of the implicit register clobbers when a Microsoft's x64 function calls a System V ABI function and I appear to finally have a working implementation. However, I cannot get recent Wine tests to complete with an unpatched compiler (I have 30 failures, 4 which are crashes and I even get some dmesgs from drivers.) While the same Wine tests pass both with and without the new optimization, I could use some external testing to make sure there's no flaws that I've missed (especially a full test with no failures). The patch set is not actually complete as my unit tests didn't catch many of the flaws (now fixed) that I caught when building real Wine, so I'm re-working them. I have my current version up on github: https://github.com/daniel-santos/gcc/tree/gcc-5_4_0-outline-msabi-xlogues. Background For those unfamiliar, the differences between the two ABIs requires an ms_abi function to save and restore 12 registers: RSI, RDI and XMM6-15. The size of pushing/popping RSI and RDI is insignificant, but saving and restoring 10 SSE registers takes between 94 and 174 bytes each function (dependent upon the SP offset), thereby bloating ".text" size quite a bit. Details This patch uses out-of-line stubs for register for saves/restores to reduce .text size. The *theory* (not yet proven) is that the reduction of instruction cache misses will offset the extra few instructions required to facilitate this. But while we're going this far, the optimization also saves/restores an additional 6 non-volatile ms_abi registers if they are clobbered in the function: RBX, RBP and R12-15. This extra step actually saves very little ".text" code, but the stub implements them using MOVs which the CPU is better able to parallelize. While MOV instructions are larger (4 bytes vs 1 or 2), they only appear in the stubs, theoretically gaining the performance benefits of MOVs without paying for the cost of the extra size. This optimization works both with and without forced stack realignment (https://bugs.winehq.org/show_bug.cgi?id=27680) although more can be done to produce better code in the realigned case. In the forced realignment case, this optimization reduces .text size Wine 64 by 19% and in the normal (aligned stack) case by 22%. As an example, here is a test function (with stubs) built with -foutline-msabi-xlogues (but no realignment): __attribute__ ((noinline)) long sysv_fn0 () { return 42; } __attribute__ ((ms_abi, noinline)) long msabifn0_6 () { __asm__ __volatile__ ("" ::: "rbx", "rbp", "r12", "r13", "r14", "r15"); return sysv_fn0 (); } 0000000000401000 <msabifn0_6>: 401000: 48 8d 44 24 88 lea -0x78(%rsp),%rax 401005: 48 81 ec f8 00 00 00 sub $0xf8,%rsp 40100c: e8 09 e8 01 00 callq 41f81a <__msabi_save_18> 401011: 31 c0 xor %eax,%eax 401013: e8 48 fc ff ff callq 400c60 <sysv_fn0> 401018: 48 8d b4 24 80 00 00 lea 0x80(%rsp),%rsi 40101f: 00 401020: 4c 8d 56 78 lea 0x78(%rsi),%r10 401024: e9 41 e8 01 00 jmpq 41f86a <__msabi_restore_ret_18> 000000000041f81a <__msabi_save_18>: 41f81a: 4c 89 78 90 mov %r15,-0x70(%rax) 000000000041f81e <__msabi_save_17>: 41f81e: 4c 89 70 98 mov %r14,-0x68(%rax) 000000000041f822 <__msabi_save_16>: 41f822: 4c 89 68 a0 mov %r13,-0x60(%rax) 000000000041f826 <__msabi_save_15>: 41f826: 4c 89 60 a8 mov %r12,-0x58(%rax) 000000000041f82a <__msabi_save_14>: 41f82a: 48 89 68 b0 mov %rbp,-0x50(%rax) 000000000041f82e <__msabi_save_13>: 41f82e: 48 89 58 b8 mov %rbx,-0x48(%rax) 000000000041f832 <__msabi_save_12>: 41f832: 48 89 78 c0 mov %rdi,-0x40(%rax) 41f836: 48 89 70 c8 mov %rsi,-0x38(%rax) 41f83a: 44 0f 29 78 d0 movaps %xmm15,-0x30(%rax) 41f83f: 44 0f 29 70 e0 movaps %xmm14,-0x20(%rax) 41f844: 44 0f 29 68 f0 movaps %xmm13,-0x10(%rax) 41f849: 44 0f 29 20 movaps %xmm12,(%rax) 41f84d: 44 0f 29 58 10 movaps %xmm11,0x10(%rax) 41f852: 44 0f 29 50 20 movaps %xmm10,0x20(%rax) 41f857: 44 0f 29 48 30 movaps %xmm9,0x30(%rax) 41f85c: 44 0f 29 40 40 movaps %xmm8,0x40(%rax) 41f861: 0f 29 78 50 movaps %xmm7,0x50(%rax) 41f865: 0f 29 70 60 movaps %xmm6,0x60(%rax) 41f869: c3 retq 000000000041f86a <__msabi_restore_ret_18>: 41f86a: 4c 8b 7e 90 mov -0x70(%rsi),%r15 000000000041f86e <__msabi_restore_ret_17>: 41f86e: 4c 8b 76 98 mov -0x68(%rsi),%r14 000000000041f872 <__msabi_restore_ret_16>: 41f872: 4c 8b 6e a0 mov -0x60(%rsi),%r13 000000000041f876 <__msabi_restore_ret_15>: 41f876: 4c 8b 66 a8 mov -0x58(%rsi),%r12 000000000041f87a <__msabi_restore_ret_14>: 41f87a: 48 8b 6e b0 mov -0x50(%rsi),%rbp 000000000041f87e <__msabi_restore_ret_13>: 41f87e: 48 8b 5e b8 mov -0x48(%rsi),%rbx 000000000041f882 <__msabi_restore_ret_12>: 41f882: 48 8b 7e c0 mov -0x40(%rsi),%rdi 41f886: 44 0f 28 7e d0 movaps -0x30(%rsi),%xmm15 41f88b: 44 0f 28 76 e0 movaps -0x20(%rsi),%xmm14 41f890: 44 0f 28 6e f0 movaps -0x10(%rsi),%xmm13 41f895: 44 0f 28 26 movaps (%rsi),%xmm12 41f899: 44 0f 28 5e 10 movaps 0x10(%rsi),%xmm11 41f89e: 44 0f 28 56 20 movaps 0x20(%rsi),%xmm10 41f8a3: 44 0f 28 4e 30 movaps 0x30(%rsi),%xmm9 41f8a8: 44 0f 28 46 40 movaps 0x40(%rsi),%xmm8 41f8ad: 0f 28 7e 50 movaps 0x50(%rsi),%xmm7 41f8b1: 0f 28 76 60 movaps 0x60(%rsi),%xmm6 41f8b5: 48 8b 76 c8 mov -0x38(%rsi),%rsi 41f8b9: 4c 89 d4 mov %r10,%rsp 41f8bc: c3 retq Here is a similar function with forced realignment. This can be improved with an additional "have frame pointer" set of stubs. __attribute__ ((ms_abi, noinline, force_align_arg_pointer)) long msabifn0_5 () { __asm__ __volatile__ ( "" ::: "rbx", "r12", "r13", "r14", "r15" ); return sysv_fn0 (); } 0000000000400fc0 <msabifn0_5>: 400fc0: 55 push %rbp 400fc1: 48 8d 44 24 90 lea -0x70(%rsp),%rax 400fc6: 48 89 e5 mov %rsp,%rbp 400fc9: 48 83 e0 f0 and $0xfffffffffffffff0,%rax 400fcd: 48 8d 60 b0 lea -0x50(%rax),%rsp 400fd1: e8 78 e8 01 00 callq 41f84e <__msabi_save_13> 400fd6: 41 57 push %r15 400fd8: 41 56 push %r14 400fda: 41 55 push %r13 400fdc: 41 54 push %r12 400fde: 31 c0 xor %eax,%eax 400fe0: e8 7b fc ff ff callq 400c60 <sysv_fn0> 400fe5: 48 8d a5 20 ff ff ff lea -0xe0(%rbp),%rsp 400fec: 41 5c pop %r12 400fee: 41 5d pop %r13 400ff0: 41 5e pop %r14 400ff2: 41 5f pop %r15 400ff4: 48 8d 74 24 50 lea 0x50(%rsp),%rsi 400ff9: e8 a0 e8 01 00 callq 41f89e <__msabi_restore_13> 400ffe: 48 81 c4 c0 00 00 00 add $0xc0,%rsp 401005: 5d pop %rbp 401006: c3 retq Daniel

Oct. 22, 2016

Re: Stack realignment configure option?

by Daniel Santos

Sorry about the generated configure script in the patch, I didn't mean to include that.

Oct. 22, 2016

Stack realignment configure option?

by Daniel Santos

I've read up a bit on the issues with realignment on Wine 64 and all of the programs that crash because they luckily get away with violating the ABI on Windows. While not crashing is a pretty good thing, forcing stack realignment also affects optimizations in some less obvious ways, inhibiting sibling calls, ret from function bodies, adding a frame pointer, etc. I don't actually have any bright ideas for this atm, but can we at least add a configure option to disable this for those who don't happen to have such programs crashing? Probably only Gentoo users and geeks will care for it.

Oct. 22, 2016

Re: TRACE in wined3d_check_device_format()

by Henri Verbeet

On 22 October 2016 at 02:02, <wylda(a)volny.cz> wrote: > Hi, > isn't that middle \n in TRACE some kind of leftover? > > TRACE("wined3d %p, adapter_idx %u, device_type %s, adapter_format %s, usage %s, %s,\n" > "resource_type %s, check_format %s.\n", > wined3d, adapter_idx, debug_d3ddevicetype(device_type), debug_d3dformat(adapter_format_id), > debug_d3dusage(usage), debug_d3dusagequery(usage), debug_d3dresourcetype(resource_type), > debug_d3dformat(check_format_id)); > Yeah, that shouldn't be there.

Oct. 22, 2016

TRACE in wined3d_check_device_format()

by wylda＠volny.cz

Hi, isn't that middle \n in TRACE some kind of leftover? TRACE("wined3d %p, adapter_idx %u, device_type %s, adapter_format %s, usage %s, %s,\n" "resource_type %s, check_format %s.\n", wined3d, adapter_idx, debug_d3ddevicetype(device_type), debug_d3dformat(adapter_format_id), debug_d3dusage(usage), debug_d3dusagequery(usage), debug_d3dresourcetype(resource_type), debug_d3dformat(check_format_id)); Regards, W.

Oct. 21, 2016

Re: How to make winedbg listen to same port every time?

by Bruno Jesus

On Fri, Oct 21, 2016 at 3:15 PM, Bruno Jesus <00cpxxx(a)gmail.com> wrote: > On Wed, Oct 19, 2016 at 1:07 PM, A B <dianaxxyyzz(a)gmail.com> wrote: >> >> If i run : >> winedbg --gdb --no-start test.exe >> >> it say : >> target remote localhost:55304 >> >> >> But if I run it second time , it say a different port , for example >> target remote localhost:111111 >> >> is there a command line argument for winedbg so it to listen to same port >> every time , and to do not change the port ,so I can specity to witch port >> it to listen , or I have to recompile winedbg ? >> Thank you! > > Judging by the wine source code at file gdbproxy.c (function > gdb_startup) [1] it is not possible, the port is always chosen by the > OS. It could be changed to support port selection, but I'm curious > about why you have such need. > > [1] http://source.winehq.org/source/programs/winedbg/gdbproxy.c Actually there is bug https://bugs.winehq.org/show_bug.cgi?id=21404

Oct. 21, 2016

Re: How to make winedbg listen to same port every time?

by Bruno Jesus

On Wed, Oct 19, 2016 at 1:07 PM, A B <dianaxxyyzz(a)gmail.com> wrote: > > If i run : > winedbg --gdb --no-start test.exe > > it say : > target remote localhost:55304 > > > But if I run it second time , it say a different port , for example > target remote localhost:111111 > > is there a command line argument for winedbg so it to listen to same port > every time , and to do not change the port ,so I can specity to witch port > it to listen , or I have to recompile winedbg ? > Thank you! Judging by the wine source code at file gdbproxy.c (function gdb_startup) [1] it is not possible, the port is always chosen by the OS. It could be changed to support port selection, but I'm curious about why you have such need. [1] http://source.winehq.org/source/programs/winedbg/gdbproxy.c Best wishes, Bruno

Oct. 21, 2016

Re: [PATCH 1/4] d2d1: Implement FillContainsPoint for rectangles

by Nikolay Sivov

On Fri, Oct 21, 2016 at 7:28 PM, Andrew Eikum <aeikum(a)codeweavers.com> wrote: > > + *contains = probe.x > geometry->u.rectangle.rect.left - tolerance && > + probe.x < geometry->u.rectangle.rect.right + tolerance && > + probe.y > geometry->u.rectangle.rect.top - tolerance && > + probe.y < geometry->u.rectangle.rect.bottom + tolerance; > + > Does it really work like that? I'd expect tolerance to be irrelevant for paths constructed purely from lines. So basically in general case path is approximated with lines (or enclosed area with triangles) with given tolerance, and then point is tested against this flattened path. While in rectangular case it's already flat to begin with.

Oct. 21, 2016

Re: [PATCH 6/9] wined3d: Replace wined3d_buffer_load_sysmem() calls with wined3d_buffer_load_location().

by Henri Verbeet

On 20 October 2016 at 12:50, Józef Kucia <jkucia(a)codeweavers.com> wrote: > - /* Download the buffer, but don't permanently enable double buffering */ > - if (!(buffer->flags & WINED3D_BUFFER_DOUBLEBUFFER)) > - { > - wined3d_buffer_load_sysmem(buffer, context); > - buffer->flags &= ~WINED3D_BUFFER_DOUBLEBUFFER; > - } > + /* Download the buffer, but don't permanently enable double buffering. */ > + wined3d_buffer_load_location(buffer, context, WINED3D_LOCATION_SYSMEM); > + buffer->flags &= ~WINED3D_BUFFER_DOUBLEBUFFER; Wouldn't this also clear WINED3D_BUFFER_DOUBLEBUFFER if it was previously set? Longer term, it's probably inappropriate for wined3d_buffer_load_location() to set WINED3D_BUFFER_DOUBLEBUFFER to begin with.

Oct. 21, 2016

Re: [PATCH 5/9] wined3d: Implement wined3d_buffer_prepare_location() for WINED3D_LOCATION_BUFFER.

by Henri Verbeet

On 20 October 2016 at 12:50, Józef Kucia <jkucia(a)codeweavers.com> wrote: > @@ -612,7 +619,7 @@ void buffer_get_memory(struct wined3d_buffer *buffer, struct wined3d_context *co > { > if ((buffer->flags & WINED3D_BUFFER_USE_BO) && !buffer->resource.map_count) > { > - buffer_create_buffer_object(buffer, context); > + wined3d_buffer_prepare_location(buffer, context, WINED3D_LOCATION_BUFFER); > if (buffer->buffer_object) Note that that should be equivalent to "if (wined3d_buffer_prepare_location(buffer, context, WINED3D_LOCATION_BUFFER))".

Oct. 21, 2016

Re: [PATCH v3] winebus.sys: Handle device reports for hidraw devices

by Aric Stewart

On 10/21/16 7:21 AM, Sebastian Lackner wrote: > On 21.10.2016 05:39, Aric Stewart wrote: >>>> >>>> Another idea we could consider (at least for Linux) would be to >>>> use wine_server_fd_to_handle(), and then NtReadFile(). This would >>>> allow to get rid of the thread, and we could use an APC to run >>>> IoCompleteRequest(). Have you actually looked into that? And >>>> which mechanism is required on other platforms, do they also >>>> require a separate thread? >>>> >> >> I have not explored wine_server_fd_to_handle as I was not really >> aware of it, Mac and IOHID uses a callback, so a run loop thread is >> needed, but no read of any sort. If you thinking doing that wine >> magic is correct then I would be up for exploring it, but it seems >> like the sort of thing that Alexandre would frown on. >> >> -aric > > Well, using async NtReadFile would have the advantage that we can > start listening for reports at any time, without spawning a thread. > Nevertheless, if other platforms do not offer some similar mechanism, > it probably does not make any sense to implement it like that. I'm > also fine with a thread based approach. > > I would still suggest to do further investigations regarding report > loss though. If there is not sufficient evidence that this happens > on Windows aswell, I would still prefer a more reliable Wine > implementation. MSDN suggests that in order to avoid dropped HID > reports, applications can try to increase HidD_SetNumInputBuffers - > but that will obviously not help if we already drop them in the > lower level drivers. The idea with a simple flags only works if we > can be sure that the thread runs fast enough to ensure records > are never dropped. Otherwise a more complicated approach would be > required. > Yes HidD_SetNumInputBuffers only affects the size of the ring buffer at the upper level HID device. I my preferences is to go forward with the flag and wait to see if this is really an issue or not and then revisit it if we really see an issue. -aric > Regards, > Sebastian >

Oct. 21, 2016

Re: [PATCH v3] winebus.sys: Handle device reports for hidraw devices

by Sebastian Lackner

On 21.10.2016 05:39, Aric Stewart wrote: >>> >>> Another idea we could consider (at least for Linux) would be to >>> use wine_server_fd_to_handle(), and then NtReadFile(). This would >>> allow to get rid of the thread, and we could use an APC to run >>> IoCompleteRequest(). Have you actually looked into that? And >>> which mechanism is required on other platforms, do they also >>> require a separate thread? >>> > > I have not explored wine_server_fd_to_handle as I was not really > aware of it, Mac and IOHID uses a callback, so a run loop thread is > needed, but no read of any sort. If you thinking doing that wine > magic is correct then I would be up for exploring it, but it seems > like the sort of thing that Alexandre would frown on. > > -aric Well, using async NtReadFile would have the advantage that we can start listening for reports at any time, without spawning a thread. Nevertheless, if other platforms do not offer some similar mechanism, it probably does not make any sense to implement it like that. I'm also fine with a thread based approach. I would still suggest to do further investigations regarding report loss though. If there is not sufficient evidence that this happens on Windows aswell, I would still prefer a more reliable Wine implementation. MSDN suggests that in order to avoid dropped HID reports, applications can try to increase HidD_SetNumInputBuffers - but that will obviously not help if we already drop them in the lower level drivers. The idea with a simple flags only works if we can be sure that the thread runs fast enough to ensure records are never dropped. Otherwise a more complicated approach would be required. Regards, Sebastian

Oct. 21, 2016

Re: [PATCH] hidclass.sys: Use IoSetCompletionRoutine

by Aric Stewart

On 10/18/16 5:40 PM, Sebastian Lackner wrote: > On 18.10.2016 17:07, Aric Stewart wrote: >> Signed-off-by: Aric Stewart <aric(a)codeweavers.com> >> --- >> dlls/hidclass.sys/device.c | 20 +++++++------------- >> dlls/hidclass.sys/main.c | 13 +++++-------- >> dlls/hidclass.sys/pnp.c | 10 +++++----- >> 3 files changed, 17 insertions(+), 26 deletions(-) > > Thanks! While you are just at it, could you please also check the two > occurances of "if (irp->IoStatus.u.Status == STATUS_PENDING)"? They > look wrong to me, the correct way would be to check the return value > of IoCallDriver. Also, it looks like call_minidriver is leaking "buffer". Actually that was intentional at the time. There was code in ntoskrnl that was freeing that buffer already so I was getting a double free issue. Now you have been doing a lot of work there so maybe that has changed. I have not gone back and revisited it. -aric > > Signed-off-by: Sebastian Lackner <sebastian(a)fds-team.de> >

Oct. 20, 2016

Re: [PATCH v3] winebus.sys: Handle device reports for hidraw devices

by Aric Stewart

On 10/19/16 10:21 PM, Sebastian Lackner wrote: > On 19.10.2016 21:32, Aric Stewart wrote: >> Hi, >> >> It is important to remember that there is a big difference between >> the user level communications with the HID device and the minidriver >> level communications. The top of the stack, the HID device, has a >> ring buffer and such. >> > [...] >> >> Depends on the call that is made. IOCTL_HID_GET_INPUT_REPORT should >> always return immediately, if there is no report then it returns 0. >> If there is a report it returns the last report from the device. >> >> IOCTL_HID_READ_REPORT is expected to pend until a report is >> available. In the grand scheme of things IOCTL_HID_READ_REPORT is >> expected to block until a report is available because the general use >> is that hidclass will be in a loop where it does a >> IOCTL_HID_READ_REPORT call and when the call succeeds it places that >> report into the ring buffer and then immediately calls again to get >> the next report. >> >> IOCTL_HID_GET_INPUT_REPORT is generated by the corresponding >> IOCTL_HID_GET_INPUT_REPORT in the class driver and >> HidD_GetInputReport. >> >> IOCTL_HID_READ_REPORT is like doing a FileRead. > > I'm aware of those differences. The part I'm more interested in is how > IOCTL_HID_READ_REPORT is implemented in Windows drivers. ;) > Even if Windows does not explicitly mention any kind of buffering, I > believe that data loss is much more unlikely than with the proposed Wine > implementation. > > Usually hardware events trigger an interrupt service routine, which can > immediately complete the request. The code responsible for queuing a new > IOCTL_HID_READ_REPORT ioctl also runs in kernel mode, maybe even with a > higher priority to avoid any interruptions by user mode processes. > In addition, devices might also do their own buffering, to ensure that > the transmission works reliable. > >> >> So they need to behave quite a bit differently. >> >>>> https://www.julianloehr.de/wp-content/uploads/2014/07/Paper_HIDWiimote.pdf >>> >>>> >> """In case the Wiimote state changes when there is no read request >>>> currently buffered, a flag is used to signal whether a newly >>>> received read request shall be processed and completed >>>> immediately.""" >>>> >> That would not be hard, just a flag that showed if the last reports >> has been read or not. In all practical terms the minidriver only has >> 1 consumer, the hidclass class driver devices, so in many ways the >> whole IRP queue is overkill as there should only ever be 1 client. >> It would not be hard to add that. >> >>>> In Wine we have to cache the last report anyway, so we could >>>> implement something similar very easy. Do you think it makes >>>> sense? The implementation of IOCTL_HID_GET_INPUT_REPORT and >>>> IOCTL_HID_READ_REPORT would also not be much more difficult, as >>>> shown in the following pseudocode: >>>> >>>> --- snip --- case IOCTL_HID_GET_INPUT_REPORT: case >>>> IOCTL_HID_READ_REPORT: EnterCriticalSection(report_cs); ... start >>>> processing if not done yet ... if (ioctl == IOCTL_HID_READ_REPORT >>>> && (!last_report || last_report_seen)) { ... async handling ... >>>> break; } ... synchronous handlng ... last_report_seen = TRUE; >>>> LeaveCriticalSection(report_cs); break; --- snip --- >> I will have to explore this when I have more time to code things but >> the basic idea looks pretty good though IOCTL_HID_GET_INPUT_REPORT >> uses HID_XFER_PACKETs and IOCTL_HID_READ_REPORT does not so that >> complicates things a fair bit more. > > You are right, it would be a bit more complicated. Probably we could > move some of the code to a helper function, but I guess we will have > to try it out. ;) > > Another idea we could consider (at least for Linux) would be to use > wine_server_fd_to_handle(), and then NtReadFile(). This would allow > to get rid of the thread, and we could use an APC to run IoCompleteRequest(). > Have you actually looked into that? And which mechanism is required > on other platforms, do they also require a separate thread? > I have not explored wine_server_fd_to_handle as I was not really aware of it, Mac and IOHID uses a callback, so a run loop thread is needed, but no read of any sort. If you thinking doing that wine magic is correct then I would be up for exploring it, but it seems like the sort of thing that Alexandre would frown on. -aric >> >> -aric >> >

Oct. 20, 2016

Re: [PATCH] ws2_32: Allow user to enable IP dual stack (v6).

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=26243 Your paranoid android. === w2008s64 (64 bit sock) === sock: unhandled exception c0000005 at 000007FEFF1220C0

Oct. 20, 2016

Re: [PATCH 01/10 v2] server: Introduced iosb struct for server-side IO_STATUS_BLOCK representation and use it in irp_call.

by Sebastian Lackner

On 19.10.2016 19:05, Jacek Caban wrote: > v2: Don't store async queues in a list. > > Signed-off-by: Jacek Caban <jacek(a)codeweavers.com> > --- > server/async.c | 34 +++++++++++++++++++++++++++++++++ > server/device.c | 58 > +++++++++++++++++++++++++-------------------------------- > server/file.h | 14 ++++++++++++++ > 3 files changed, 73 insertions(+), 33 deletions(-) > I'm not sure if Alexandre has already agreed to this approach, but from my point of view it would make sense to do some measurements. How fast is the proposed named pipe approach compared to alternatives (unpatched Wine, or the Linux-only implementation included in Wine Staging)? In general I like the idea, but I fear that adding significant optimizations afterwards will be very difficult. Regards, Sebastian

Oct. 20, 2016

Re: [PATCH v3] winebus.sys: Handle device reports for hidraw devices

by Sebastian Lackner

On 19.10.2016 21:32, Aric Stewart wrote: > Hi, > > It is important to remember that there is a big difference between > the user level communications with the HID device and the minidriver > level communications. The top of the stack, the HID device, has a > ring buffer and such. > [...] > > Depends on the call that is made. IOCTL_HID_GET_INPUT_REPORT should > always return immediately, if there is no report then it returns 0. > If there is a report it returns the last report from the device. > > IOCTL_HID_READ_REPORT is expected to pend until a report is > available. In the grand scheme of things IOCTL_HID_READ_REPORT is > expected to block until a report is available because the general use > is that hidclass will be in a loop where it does a > IOCTL_HID_READ_REPORT call and when the call succeeds it places that > report into the ring buffer and then immediately calls again to get > the next report. > > IOCTL_HID_GET_INPUT_REPORT is generated by the corresponding > IOCTL_HID_GET_INPUT_REPORT in the class driver and > HidD_GetInputReport. > > IOCTL_HID_READ_REPORT is like doing a FileRead. I'm aware of those differences. The part I'm more interested in is how IOCTL_HID_READ_REPORT is implemented in Windows drivers. ;) Even if Windows does not explicitly mention any kind of buffering, I believe that data loss is much more unlikely than with the proposed Wine implementation. Usually hardware events trigger an interrupt service routine, which can immediately complete the request. The code responsible for queuing a new IOCTL_HID_READ_REPORT ioctl also runs in kernel mode, maybe even with a higher priority to avoid any interruptions by user mode processes. In addition, devices might also do their own buffering, to ensure that the transmission works reliable. > > So they need to behave quite a bit differently. > >>> https://www.julianloehr.de/wp-content/uploads/2014/07/Paper_HIDWiimote.pdf >> >>> > """In case the Wiimote state changes when there is no read request >>> currently buffered, a flag is used to signal whether a newly >>> received read request shall be processed and completed >>> immediately.""" >>> > That would not be hard, just a flag that showed if the last reports > has been read or not. In all practical terms the minidriver only has > 1 consumer, the hidclass class driver devices, so in many ways the > whole IRP queue is overkill as there should only ever be 1 client. > It would not be hard to add that. > >>> In Wine we have to cache the last report anyway, so we could >>> implement something similar very easy. Do you think it makes >>> sense? The implementation of IOCTL_HID_GET_INPUT_REPORT and >>> IOCTL_HID_READ_REPORT would also not be much more difficult, as >>> shown in the following pseudocode: >>> >>> --- snip --- case IOCTL_HID_GET_INPUT_REPORT: case >>> IOCTL_HID_READ_REPORT: EnterCriticalSection(report_cs); ... start >>> processing if not done yet ... if (ioctl == IOCTL_HID_READ_REPORT >>> && (!last_report || last_report_seen)) { ... async handling ... >>> break; } ... synchronous handlng ... last_report_seen = TRUE; >>> LeaveCriticalSection(report_cs); break; --- snip --- > I will have to explore this when I have more time to code things but > the basic idea looks pretty good though IOCTL_HID_GET_INPUT_REPORT > uses HID_XFER_PACKETs and IOCTL_HID_READ_REPORT does not so that > complicates things a fair bit more. You are right, it would be a bit more complicated. Probably we could move some of the code to a helper function, but I guess we will have to try it out. ;) Another idea we could consider (at least for Linux) would be to use wine_server_fd_to_handle(), and then NtReadFile(). This would allow to get rid of the thread, and we could use an APC to run IoCompleteRequest(). Have you actually looked into that? And which mechanism is required on other platforms, do they also require a separate thread? > > -aric >

Oct. 19, 2016

Re: [PATCH v3] winebus.sys: Handle device reports for hidraw devices

by Aric Stewart

Hi, It is important to remember that there is a big difference between the user level communications with the HID device and the minidriver level communications. The top of the stack, the HID device, has a ring buffer and such. On 10/19/16 12:32 PM, Sebastian Lackner wrote: > On 19.10.2016 10:50, Aric Stewart wrote: >> >> I dont think that is a big problem. Not that it wont happen but that >> it is ok to have happen. The 2 ways to read from the >> minidriver/device layer is to either get the last report from the >> device or current device state (IOCTL_HID_GET_INPUT_REPORT) or to >> wait for the next report from the device(IOCTL_HID_READ_REPORT). >> >> There is not specification of a need for a ring buffer or anything >> like that in the MSDN docs that I can see so it seem like keeping it >> simple and direct is the best. > > Even if missing a report is not a problem, doesn't it cause an arbitrary > long delay when we have to wait for the next report? I have not been able > to find much information about buffering in HID drivers, but at least > some drivers seem to implement that. For example: > Depends on the call that is made. IOCTL_HID_GET_INPUT_REPORT should always return immediately, if there is no report then it returns 0. If there is a report it returns the last report from the device. IOCTL_HID_READ_REPORT is expected to pend until a report is available. In the grand scheme of things IOCTL_HID_READ_REPORT is expected to block until a report is available because the general use is that hidclass will be in a loop where it does a IOCTL_HID_READ_REPORT call and when the call succeeds it places that report into the ring buffer and then immediately calls again to get the next report. IOCTL_HID_GET_INPUT_REPORT is generated by the corresponding IOCTL_HID_GET_INPUT_REPORT in the class driver and HidD_GetInputReport. IOCTL_HID_READ_REPORT is like doing a FileRead. So they need to behave quite a bit differently. > https://www.julianloehr.de/wp-content/uploads/2014/07/Paper_HIDWiimote.pdf > """In case the Wiimote state changes when there is no read request > currently buffered, a flag is used to signal whether a newly received > read request shall be processed and completed immediately.""" > That would not be hard, just a flag that showed if the last reports has been read or not. In all practical terms the minidriver only has 1 consumer, the hidclass class driver devices, so in many ways the whole IRP queue is overkill as there should only ever be 1 client. It would not be hard to add that. > In Wine we have to cache the last report anyway, so we could implement > something similar very easy. Do you think it makes sense? The > implementation of IOCTL_HID_GET_INPUT_REPORT and IOCTL_HID_READ_REPORT > would also not be much more difficult, as shown in the following > pseudocode: > > --- snip --- > case IOCTL_HID_GET_INPUT_REPORT: > case IOCTL_HID_READ_REPORT: > EnterCriticalSection(report_cs); > ... start processing if not done yet ... > if (ioctl == IOCTL_HID_READ_REPORT && (!last_report || last_report_seen)) > { > ... async handling ... > break; > } > ... synchronous handlng ... > last_report_seen = TRUE; > LeaveCriticalSection(report_cs); > break; > --- snip --- I will have to explore this when I have more time to code things but the basic idea looks pretty good though IOCTL_HID_GET_INPUT_REPORT uses HID_XFER_PACKETs and IOCTL_HID_READ_REPORT does not so that complicates things a fair bit more. -aric

Oct. 19, 2016

Re: [PATCH] d2d1: Initial implementation of bitmap render target

by Nikolay Sivov

On 10/19/2016 01:13 PM, Henri Verbeet wrote: > On 14 October 2016 at 18:12, Nikolay Sivov <nsivov(a)codeweavers.com> wrote: >> + if (!pixel_size && size) >> + { >> + texture_desc.Width = ceilf((size->width * parent_target->desc.dpiX) / 96.0f); >> + texture_desc.Height = ceilf((size->height * parent_target->desc.dpiY) / 96.0f); >> + dxgi_rt_desc.dpiX = (texture_desc.Width * 96.0f) / size->width; >> + dxgi_rt_desc.dpiY = (texture_desc.Height * 96.0f) / size->height; >> + } >> + else if (pixel_size && !size) >> + { >> + texture_desc.Width = pixel_size->width; >> + texture_desc.Height = pixel_size->height; >> + dxgi_rt_desc.dpiX = parent_target->desc.dpiX; >> + dxgi_rt_desc.dpiY = parent_target->desc.dpiY; >> + } >> + else if (pixel_size && size) >> + { >> + texture_desc.Width = pixel_size->width; >> + texture_desc.Height = pixel_size->height; >> + dxgi_rt_desc.dpiX = (pixel_size->width * 96.0f) / size->width; >> + dxgi_rt_desc.dpiY = (pixel_size->height * 96.0f) / size->height; >> + } >> + else >> + { >> + texture_desc.Width = parent_target->pixel_size.width; >> + texture_desc.Height = parent_target->pixel_size.height; >> + dxgi_rt_desc.dpiX = parent_target->desc.dpiX; >> + dxgi_rt_desc.dpiY = parent_target->desc.dpiY; >> + } > It may just be taste, but I think this is a bit harder to follow than > it needs to be. What do you think about the following? > > Sure, just sent v2. Initially I did it in the same orderit's documented.

Oct. 19, 2016

How to make winedbg listen to same port every time?

by A B

If i run : winedbg --gdb --no-start test.exe it say : target remote localhost:55304 But if I run it second time , it say a different port , for example target remote localhost:111111 is there a command line argument for winedbg so it to listen to same port every time , and to do not change the port ,so I can specity to witch port it to listen , or I have to recompile winedbg ? Thank you!

Oct. 19, 2016

Re: [PATCH 6/6] wined3d: Rename buffer_invalidate_bo_range() to buffer_mark_dirty_area().

by Henri Verbeet

On 19 October 2016 at 13:52, Józef Kucia <joseph.kucia(a)gmail.com> wrote: > Yes, it's mainly for consistency with buffer_clear_dirty_areas(), but > additionally: > * having two functions with very similar names is confusing > (wined3d_buffer_invalidate_range() and buffer_invalidate_bo_range()), Somewhat. It would be good to make that less confusing somehow, but I don't think using different terminology makes it better. One is a more specific helper for the other, and I imagine ultimately wined3d_buffer_invalidate_range() would be the only caller of buffer_invalidate_bo_range(). I.e., similar to how e.g. surface_load_location() and surface_load_texture() are related. > * dirty areas are also used for flushing modified areas when BO is > being unmapped (see wined3d_buffer_unmap()). Yes, but I don't think that conflicts with the current name?

Oct. 19, 2016

Re: [PATCH 6/6] wined3d: Rename buffer_invalidate_bo_range() to buffer_mark_dirty_area().

by Józef Kucia

On Wed, Oct 19, 2016 at 1:32 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote: > On 16 October 2016 at 21:46, Józef Kucia <jkucia(a)codeweavers.com> wrote: >> -static void buffer_invalidate_bo_range(struct wined3d_buffer *buffer, unsigned int offset, unsigned int size) >> +static void buffer_mark_dirty_area(struct wined3d_buffer *buffer, unsigned int offset, unsigned int size) > I assume this is for consistency with buffer_clear_dirty_areas() and > the like, but in the larger context we're using validate/invalidate > for terminology. Yes, it's mainly for consistency with buffer_clear_dirty_areas(), but additionally: * having two functions with very similar names is confusing (wined3d_buffer_invalidate_range() and buffer_invalidate_bo_range()), * dirty areas are also used for flushing modified areas when BO is being unmapped (see wined3d_buffer_unmap()).

Oct. 19, 2016

Re: [PATCH 6/6] wined3d: Rename buffer_invalidate_bo_range() to buffer_mark_dirty_area().

by Henri Verbeet

On 16 October 2016 at 21:46, Józef Kucia <jkucia(a)codeweavers.com> wrote: > -static void buffer_invalidate_bo_range(struct wined3d_buffer *buffer, unsigned int offset, unsigned int size) > +static void buffer_mark_dirty_area(struct wined3d_buffer *buffer, unsigned int offset, unsigned int size) I assume this is for consistency with buffer_clear_dirty_areas() and the like, but in the larger context we're using validate/invalidate for terminology.

Oct. 19, 2016

Re: [PATCH 3/6] wined3d: Rename buffer_internal_preload() to wined3d_buffer_load().

by Henri Verbeet

On 16 October 2016 at 21:46, Józef Kucia <jkucia(a)codeweavers.com> wrote: > -void buffer_internal_preload(struct wined3d_buffer *buffer, struct wined3d_context *context, > +void wined3d_buffer_load(struct wined3d_buffer *buffer, struct wined3d_context *context, > const struct wined3d_state *state) This makes the naming consistent with wined3d_texture_load(), so that's good. Still, I don't think that one necessarily covers what the function does very well either, so if anyone has good suggestions...

Oct. 19, 2016

Re: [PATCH 2/6] wined3d: Clean up buffer_internal_preload().

by Henri Verbeet

On 16 October 2016 at 21:46, Józef Kucia <jkucia(a)codeweavers.com> wrote: > -static inline void fixup_transformed_pos(float *p) > +static inline unsigned int fixup_transformed_pos(float *p) > { > /* rhw conversion like in position_float4(). */ > if (p[3] != 1.0f && p[3] != 0.0f) > @@ -452,6 +454,8 @@ static inline void fixup_transformed_pos(float *p) > p[2] *= w; > p[3] = w; > } > + > + return 4 * sizeof(*p); > } Since you're cleaning things up, note that this should probably use struct wined3d_vec4. > - for (i = start / buffer->stride; i < min((end / buffer->stride) + 1, vertices); ++i) > + for (i = start / buffer->stride; i < min((end / buffer->stride) + 1, vertex_count); ++i) > { > - for (j = 0; j < buffer->stride; ++j) > + for (j = 0; j < buffer->stride;) > { > switch (buffer->conversion_map[j]) > { In general this conversion code shouldn't be used a lot, but I could imagine it being better to loop over the vertices in the fixup functions instead of in the outer loop.

Oct. 19, 2016

Re: [PATCH v3] winebus.sys: Handle device reports for hidraw devices

by Sebastian Lackner

On 19.10.2016 10:50, Aric Stewart wrote: > > I dont think that is a big problem. Not that it wont happen but that > it is ok to have happen. The 2 ways to read from the > minidriver/device layer is to either get the last report from the > device or current device state (IOCTL_HID_GET_INPUT_REPORT) or to > wait for the next report from the device(IOCTL_HID_READ_REPORT). > > There is not specification of a need for a ring buffer or anything > like that in the MSDN docs that I can see so it seem like keeping it > simple and direct is the best. Even if missing a report is not a problem, doesn't it cause an arbitrary long delay when we have to wait for the next report? I have not been able to find much information about buffering in HID drivers, but at least some drivers seem to implement that. For example: https://www.julianloehr.de/wp-content/uploads/2014/07/Paper_HIDWiimote.pdf """In case the Wiimote state changes when there is no read request currently buffered, a flag is used to signal whether a newly received read request shall be processed and completed immediately.""" In Wine we have to cache the last report anyway, so we could implement something similar very easy. Do you think it makes sense? The implementation of IOCTL_HID_GET_INPUT_REPORT and IOCTL_HID_READ_REPORT would also not be much more difficult, as shown in the following pseudocode: --- snip --- case IOCTL_HID_GET_INPUT_REPORT: case IOCTL_HID_READ_REPORT: EnterCriticalSection(report_cs); ... start processing if not done yet ... if (ioctl == IOCTL_HID_READ_REPORT && (!last_report || last_report_seen)) { ... async handling ... break; } ... synchronous handlng ... last_report_seen = TRUE; LeaveCriticalSection(report_cs); break; --- snip --- Regards, Sebastian

Oct. 19, 2016

Re: [PATCH] d2d1: Initial implementation of bitmap render target

by Henri Verbeet

On 14 October 2016 at 18:12, Nikolay Sivov <nsivov(a)codeweavers.com> wrote: > + if (!pixel_size && size) > + { > + texture_desc.Width = ceilf((size->width * parent_target->desc.dpiX) / 96.0f); > + texture_desc.Height = ceilf((size->height * parent_target->desc.dpiY) / 96.0f); > + dxgi_rt_desc.dpiX = (texture_desc.Width * 96.0f) / size->width; > + dxgi_rt_desc.dpiY = (texture_desc.Height * 96.0f) / size->height; > + } > + else if (pixel_size && !size) > + { > + texture_desc.Width = pixel_size->width; > + texture_desc.Height = pixel_size->height; > + dxgi_rt_desc.dpiX = parent_target->desc.dpiX; > + dxgi_rt_desc.dpiY = parent_target->desc.dpiY; > + } > + else if (pixel_size && size) > + { > + texture_desc.Width = pixel_size->width; > + texture_desc.Height = pixel_size->height; > + dxgi_rt_desc.dpiX = (pixel_size->width * 96.0f) / size->width; > + dxgi_rt_desc.dpiY = (pixel_size->height * 96.0f) / size->height; > + } > + else > + { > + texture_desc.Width = parent_target->pixel_size.width; > + texture_desc.Height = parent_target->pixel_size.height; > + dxgi_rt_desc.dpiX = parent_target->desc.dpiX; > + dxgi_rt_desc.dpiY = parent_target->desc.dpiY; > + } It may just be taste, but I think this is a bit harder to follow than it needs to be. What do you think about the following? if (pixel_size) { texture_desc.Width = pixel_size->width; texture_desc.Height = pixel_size->height; } else if (size) { texture_desc.Width = ceilf((size->width * parent_target->desc.dpiX) / 96.0f); texture_desc.Height = ceilf((size->height * parent_target->desc.dpiY) / 96.0f); } else { texture_desc.Width = parent_target->pixel_size.width; texture_desc.Height = parent_target->pixel_size.height; } if (size) { dxgi_rt_desc.dpiX = (texture_desc.Width * 96.0f) / size->width; dxgi_rt_desc.dpiY = (texture_desc.Height * 96.0f) / size->height; } else { dxgi_rt_desc.dpiX = parent_target->desc.dpiX; dxgi_rt_desc.dpiY = parent_target->desc.dpiY; }

Oct. 19, 2016

Re: [PATCH v3] winebus.sys: Handle device reports for hidraw devices

by Aric Stewart

On 10/18/16 7:12 PM, Sebastian Lackner wrote: > On 18.10.2016 18:58, Sebastian Lackner wrote: >> Not sure if it is >> a problem, but due to the fact that the device has been opened a long time before starting to >> read from it, couldn't there be a large amount of reports in the kernel buffers? Do you think >> it makes sense to skip such outdated reports? > > Actually, isn't there also a problem that reports are skipped in certain situations? Handling > of hidclass looks correct to me, but there is a risk that process_hid_report() will drop the > report if there is no pending IRP in the list. This happens for a short time inbetween of the > IOCTL_HID_READ_REPORT ioctls. > > Regards, > Sebastian > I dont think that is a big problem. Not that it wont happen but that it is ok to have happen. The 2 ways to read from the minidriver/device layer is to either get the last report from the device or current device state (IOCTL_HID_GET_INPUT_REPORT) or to wait for the next report from the device(IOCTL_HID_READ_REPORT). There is not specification of a need for a ring buffer or anything like that in the MSDN docs that I can see so it seem like keeping it simple and direct is the best. As for your other review points, I will not get a real block of time to work on it until maybe Monday. But I will give it a try then. Of course if you have more comments before then they would be welcome. -aric

Oct. 19, 2016

Re: [PATCH 1/2] taskschd: Added ITriggerCollection::Create implementation.

by Dmitry Timoshkov

Jacek Caban <jacek(a)codeweavers.com> wrote: > + if(IsEqualGUID(&IID_IUnknown, riid) || IsEqualGUID(&IID_IDispatch, riid) || IsEqualGUID(&IID_ITrigger, riid) > + || IsEqualGUID(&IID_IDailyTrigger, riid)) { > + *ppv = &This->IDailyTrigger_iface; > + }else { Please try to follow the style of the existing code for placing braces, add space between if and (, } and else, and split the above line of comparisons. > @@ -144,8 +373,29 @@ static HRESULT WINAPI TriggerCollection_get__NewEnum(ITriggerCollection *iface, > static HRESULT WINAPI TriggerCollection_Create(ITriggerCollection *iface, TASK_TRIGGER_TYPE2 type, ITrigger **trigger) > { > trigger_collection *This = impl_from_ITriggerCollection(iface); > - FIXME("(%p)->(%d %p)\n", This, type, trigger); > - return E_NOTIMPL; > + > + TRACE("(%p)->(%d %p)\n", This, type, trigger); > + > + switch(type) { > + case TASK_TRIGGER_DAILY: { > + DailyTrigger *daily_trigger; > + > + daily_trigger = heap_alloc(sizeof(*daily_trigger)); > + if(!daily_trigger) > + return E_OUTOFMEMORY; > + > + daily_trigger->IDailyTrigger_iface.lpVtbl = &DailyTrigger_vtbl; > + daily_trigger->ref = 1; > + > + *trigger = (ITrigger*)&daily_trigger->IDailyTrigger_iface; > + break; > + } > + default: > + FIXME("Unimplemented type %d\n", type); > + return E_NOTIMPL; > + } Please add a global constructor for the triggger, and use it here. -- Dmitry.

Oct. 18, 2016

Re: [PATCH v2] kernel32: Improved GetUserPreferredUILanguages stub.

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=26213 Your paranoid android. === w1064 (32 bit locale) === locale.c:2650: Test failed: LCMapStringEx should fail with bad locale name locale.c:2651: Test failed: unexpected error code -559038737 locale.c:2742: Test failed: Expected lcid == 0, got 00001000, error -559038737 locale.c:4478: Test failed: IsValidLocaleName should have failed locale.c:4480: Test failed: IsValidLocaleName should have failed locale.c:4863: Test failed: For id 7, expected ret == 4, got 1, error 87 locale.c:4865: Test failed: For id 7, Expected IVC, got '' === w1064 (64 bit locale) === locale.c:2650: Test failed: LCMapStringEx should fail with bad locale name locale.c:2651: Test failed: unexpected error code -559038737 locale.c:2742: Test failed: Expected lcid == 0, got 00001000, error -559038737 locale.c:4478: Test failed: IsValidLocaleName should have failed locale.c:4480: Test failed: IsValidLocaleName should have failed locale.c:4863: Test failed: For id 7, expected ret == 4, got 1, error 87 locale.c:4865: Test failed: For id 7, Expected IVC, got ''

Oct. 18, 2016

Re: kernel32: Improved GetUserPreferredUILanguages stub.

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=26212 Your paranoid android. === build (build) === Patch failed to apply

Oct. 18, 2016

Re: [PATCH v3] winebus.sys: Handle device reports for hidraw devices

by Sebastian Lackner

On 18.10.2016 18:58, Sebastian Lackner wrote: > Not sure if it is > a problem, but due to the fact that the device has been opened a long time before starting to > read from it, couldn't there be a large amount of reports in the kernel buffers? Do you think > it makes sense to skip such outdated reports? Actually, isn't there also a problem that reports are skipped in certain situations? Handling of hidclass looks correct to me, but there is a risk that process_hid_report() will drop the report if there is no pending IRP in the list. This happens for a short time inbetween of the IOCTL_HID_READ_REPORT ioctls. Regards, Sebastian

Oct. 18, 2016

Re: [PATCH v3] winebus.sys: Handle device reports for hidraw devices

by Sebastian Lackner

On 17.10.2016 17:14, Aric Stewart wrote: > v2: Suggestions from Sebastian Lackner > v3: Further suggestions from Sebastian Lackner > Signed-off-by: Aric Stewart <aric(a)codeweavers.com> > --- > dlls/winebus.sys/bus.h | 3 + > dlls/winebus.sys/bus_udev.c | 73 ++++++++++++++++++++++++ > dlls/winebus.sys/main.c | 133 +++++++++++++++++++++++++++++++++++++++++++- > 3 files changed, 208 insertions(+), 1 deletion(-) > > Now that all the handling for last_report is in main.c, I would suggest to split it out into a separate patch for easier reviewing. The first patch could contain IOCTL_HID_READ_REPORT and corresponding platform code only. Caching of the last record can then be implemented in the second patch. > > v3-0003-winebus.sys-Handle-device-reports-for-hidraw-device.txt > > > diff --git a/dlls/winebus.sys/bus.h b/dlls/winebus.sys/bus.h > index 7ce54bf..339449d 100644 > --- a/dlls/winebus.sys/bus.h > +++ b/dlls/winebus.sys/bus.h > @@ -25,6 +25,8 @@ typedef struct > int (*compare_platform_device)(DEVICE_OBJECT *device, void *platform_dev); > NTSTATUS (*get_reportdescriptor)(DEVICE_OBJECT *device, BYTE *buffer, DWORD length, DWORD *out_length); > NTSTATUS (*get_string)(DEVICE_OBJECT *device, DWORD index, WCHAR *buffer, DWORD length); > + NTSTATUS (*begin_report_processing)(DEVICE_OBJECT *device); > + void (*end_report_processing)(DEVICE_OBJECT *device); > } platform_vtbl; > > void *get_platform_private(DEVICE_OBJECT *device) DECLSPEC_HIDDEN; > @@ -37,3 +39,4 @@ DEVICE_OBJECT *bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW, > DEVICE_OBJECT *bus_find_hid_device(const platform_vtbl *vtbl, void *platform_dev) DECLSPEC_HIDDEN; > void bus_remove_hid_device(DEVICE_OBJECT *device) DECLSPEC_HIDDEN; > NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) DECLSPEC_HIDDEN; > +void process_hid_report(DEVICE_OBJECT *device, BYTE *report, DWORD length) DECLSPEC_HIDDEN; > diff --git a/dlls/winebus.sys/bus_udev.c b/dlls/winebus.sys/bus_udev.c > index 243aabb..517a7d3 100644 > --- a/dlls/winebus.sys/bus_udev.c > +++ b/dlls/winebus.sys/bus_udev.c > @@ -61,6 +61,8 @@ WINE_DEFAULT_DEBUG_CHANNEL(plugplay); > > #ifdef HAVE_UDEV > > +WINE_DECLARE_DEBUG_CHANNEL(hid_report); > + > static struct udev *udev_context = NULL; > static DRIVER_OBJECT *udev_driver_obj = NULL; > > @@ -73,6 +75,11 @@ struct platform_private > { > struct udev_device *udev_device; > int device_fd; > + > + BYTE *report_buffer; > + int buffer_length; > + HANDLE report_thread; > + int control_pipe[2]; > }; > > static inline struct platform_private *impl_from_DEVICE_OBJECT(DEVICE_OBJECT *device) > @@ -222,11 +229,77 @@ static NTSTATUS hidraw_get_string(DEVICE_OBJECT *device, DWORD index, WCHAR *buf > return STATUS_SUCCESS; > } > > +static DWORD CALLBACK device_report_thread(void *args) > +{ > + DEVICE_OBJECT *device = (DEVICE_OBJECT*)args; > + struct platform_private *private = impl_from_DEVICE_OBJECT(device); > + struct pollfd plfds[2]; > + > + plfds[0].fd = private->device_fd; > + plfds[0].events = POLLIN; > + plfds[0].revents = 0; > + plfds[1].fd = private->control_pipe[0]; > + plfds[1].events = POLLIN; > + plfds[1].revents = 0; > + > + while (1) > + { > + int size; > + if (poll(plfds, 2, -1) <= 0) continue; > + if (plfds[1].revents || !private->report_buffer || private->buffer_length == 0) > + break; > + size = read(plfds[0].fd, private->report_buffer, private->buffer_length); > + if (size == -1) > + TRACE_(hid_report)("Read failed. Likely an unplugged device\n"); > + else if (size == 0) > + TRACE_(hid_report)("Failed to read report\n"); > + else > + process_hid_report(device, private->report_buffer, size); > + } > + return 0; > +} > + > +static NTSTATUS begin_report_processing(DEVICE_OBJECT *device) > +{ > + struct platform_private *private = impl_from_DEVICE_OBJECT(device); > + private->buffer_length = 1024; > + private->report_buffer = HeapAlloc(GetProcessHeap(), 0, private->buffer_length); My understanding is that report_buffer is only used from the device_report_thread. Do we really have to store it in the platform private data then? For such a small amount of data, you could probably even allocate it directly on the stack in device_report_thread. > + > + pipe(private->control_pipe); Please also check for success here. > + private->report_thread = CreateThread(NULL, 0, device_report_thread, device, 0, NULL); > + if (!private->report_thread) > + { > + ERR("Unable to create device report thread\n"); > + close(private->control_pipe[0]); > + close(private->control_pipe[1]); > + return STATUS_UNSUCCESSFUL; > + } > + else > + return STATUS_SUCCESS; > +} > + > +static void end_report_processing(DEVICE_OBJECT *device) > +{ > + struct platform_private *private = impl_from_DEVICE_OBJECT(device); > + if (private->report_thread) > + { > + write(private->control_pipe[1], "q", 1); > + WaitForSingleObject(private->report_thread, INFINITE); > + close(private->control_pipe[0]); > + close(private->control_pipe[1]); report_thread should also be closed when no longer needed. > + } > + private->buffer_length = 0; > + HeapFree(GetProcessHeap(), 0, private->report_buffer); > + private->report_buffer = NULL; > +} > + > static const platform_vtbl hidraw_vtbl = > { > compare_platform_device, > hidraw_get_reportdescriptor, > hidraw_get_string, > + begin_report_processing, > + end_report_processing, > }; > > static void try_add_device(struct udev_device *dev) > diff --git a/dlls/winebus.sys/main.c b/dlls/winebus.sys/main.c > index 86adbf5..0bd2c76 100644 > --- a/dlls/winebus.sys/main.c > +++ b/dlls/winebus.sys/main.c > @@ -40,6 +40,7 @@ > #include "bus.h" > > WINE_DEFAULT_DEBUG_CHANNEL(plugplay); > +WINE_DECLARE_DEBUG_CHANNEL(hid_report); > > struct pnp_device > { > @@ -58,6 +59,14 @@ struct device_extension > const WCHAR *busid; /* Expected to be a static constant */ > > const platform_vtbl *vtbl; > + > + BOOL processing_started; > + BYTE *last_report; > + DWORD last_report_size; > + DWORD buffer_size; > + LIST_ENTRY irp_queue; > + CRITICAL_SECTION report_cs; > + > BYTE platform_private[1]; > }; > > @@ -214,6 +223,10 @@ DEVICE_OBJECT *bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW, > ext->busid = busidW; > ext->vtbl = vtbl; > It might be better to explicitly initialize all fields here. I don't know if you can rely on the fact that Wines current implementation nulls the driver extension. > + InitializeListHead(&ext->irp_queue); > + InitializeCriticalSection(&ext->report_cs); > + ext->report_cs.DebugInfo->Spare[0] = (DWORD_PTR)(__FILE__ ": report_cs"); > + > /* add to list of pnp devices */ > pnp_dev->device = device; > list_add_tail(&pnp_devset, &pnp_dev->entry); > @@ -271,6 +284,8 @@ void bus_remove_hid_device(DEVICE_OBJECT *device) > { > struct device_extension *ext = (struct device_extension *)device->DeviceExtension; > struct pnp_device *pnp_device = ext->pnp_device; > + LIST_ENTRY *entry; > + IRP *irp; > > TRACE("(%p)\n", device); > > @@ -279,7 +294,28 @@ void bus_remove_hid_device(DEVICE_OBJECT *device) > LeaveCriticalSection(&device_list_cs); > > IoInvalidateDeviceRelations(device, RemovalRelations); > + > + if (ext->processing_started) > + ext->vtbl->end_report_processing(device); At this point the device_fd was already closed (too early). Do we really need the callback, or could we also do the platform specific cleanup in try_remove_device()? > + > + /* Cancel pending IRPs */ > + EnterCriticalSection(&ext->report_cs); > + entry = RemoveHeadList(&ext->irp_queue); > + while(entry != &ext->irp_queue) You could write this loop a bit easier like: while ((entry = RemoveHeadList(&ext->irp_queue)) != &ext->irp_queue) { ... } The same also applies to the code below. > + { > + irp = CONTAINING_RECORD(entry, IRP, Tail.Overlay.ListEntry); > + irp->IoStatus.u.Status = STATUS_CANCELLED; > + irp->IoStatus.Information = 0; > + IoCompleteRequest(irp, IO_NO_INCREMENT); > + entry = RemoveHeadList(&ext->irp_queue); > + } > + LeaveCriticalSection(&ext->report_cs); > + > + ext->report_cs.DebugInfo->Spare[0] = 0; > + DeleteCriticalSection(&ext->report_cs); > + > HeapFree(GetProcessHeap(), 0, ext->serial); > + HeapFree(GetProcessHeap(), 0, ext->last_report); > IoDeleteDevice(device); > > /* pnp_device must be released after the device is gone */ > @@ -432,6 +468,58 @@ NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) > irp->IoStatus.Information = (strlenW((WCHAR *)irp->UserBuffer) + 1) * sizeof(WCHAR); > break; > } > + case IOCTL_HID_GET_INPUT_REPORT: > + { > + HID_XFER_PACKET *packet = (HID_XFER_PACKET*)(irp->UserBuffer); > + TRACE_(hid_report)("IOCTL_HID_GET_INPUT_REPORT\n"); > + EnterCriticalSection(&ext->report_cs); > + if (!ext->processing_started) > + { > + irp->IoStatus.u.Status = status = ext->vtbl->begin_report_processing(device); > + if (status == STATUS_SUCCESS) > + ext->processing_started = TRUE; > + else > + { > + LeaveCriticalSection(&ext->report_cs); > + break; > + } > + } > + > + if (packet->reportBufferLen < ext->last_report_size) > + { > + irp->IoStatus.u.Status = status = STATUS_BUFFER_TOO_SMALL; > + LeaveCriticalSection(&ext->report_cs); > + break; > + } > + > + if (ext->last_report_size) > + memcpy(packet->reportBuffer, ext->last_report, ext->last_report_size); > + packet->reportBufferLen = ext->last_report_size; > + irp->IoStatus.Information = ext->last_report_size; > + irp->IoStatus.u.Status = status = STATUS_SUCCESS; > + LeaveCriticalSection(&ext->report_cs); > + break; > + } > + case IOCTL_HID_READ_REPORT: > + { > + TRACE_(hid_report)("IOCTL_HID_READ_REPORT\n"); > + EnterCriticalSection(&ext->report_cs); > + if (!ext->processing_started) > + { > + if (ext->vtbl->begin_report_processing(device) == STATUS_SUCCESS) Did you want to assign the result to "status" here? Also, it is a bit unfortunate that we have tracking of report processing in both the common code and also in the platform specific code. If you modify begin_report_processing to return STATUS_SUCCESS if it already has been started, you could simplify this code to something like: --- snip --- status = ext->vtbl->begin_report_processing(device); if (status != STATUS_SUCCESS) { ... } --- snip --- It also ensures that the status in both components never gets out of sync. Not sure if it is a problem, but due to the fact that the device has been opened a long time before starting to read from it, couldn't there be a large amount of reports in the kernel buffers? Do you think it makes sense to skip such outdated reports? > + ext->processing_started = TRUE; > + else > + { > + irp->IoStatus.u.Status = status; > + LeaveCriticalSection(&ext->report_cs); > + break; > + } > + } > + InsertTailList(&ext->irp_queue, &irp->Tail.Overlay.ListEntry); > + status = STATUS_PENDING; > + LeaveCriticalSection(&ext->report_cs); > + break; > + } > default: > { > ULONG code = irpsp->Parameters.DeviceIoControl.IoControlCode; > @@ -441,11 +529,54 @@ NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) > } > } > > - IoCompleteRequest(irp, IO_NO_INCREMENT); > + if (status != STATUS_PENDING) > + IoCompleteRequest(irp, IO_NO_INCREMENT); > > return status; > } > > +void process_hid_report(DEVICE_OBJECT *device, BYTE *report, DWORD length) > +{ > + struct device_extension *ext = (struct device_extension*)device->DeviceExtension; > + IRP *irp; > + LIST_ENTRY *entry; > + > + EnterCriticalSection(&ext->report_cs); > + if (length > ext->buffer_size) > + { > + HeapFree(GetProcessHeap(), 0, ext->last_report); > + ext->last_report = HeapAlloc(GetProcessHeap(), 0, length); It would be better to handle failures here. > + ext->buffer_size = length; > + } > + > + memcpy(ext->last_report, report, length); > + ext->last_report_size = length; > + > + entry = RemoveHeadList(&ext->irp_queue); > + while(entry != &ext->irp_queue) > + { > + IO_STACK_LOCATION *irpsp; > + TRACE_(hid_report)("Processing Request\n"); > + irp = CONTAINING_RECORD(entry, IRP, Tail.Overlay.ListEntry); > + irpsp = IoGetCurrentIrpStackLocation(irp); > + > + if (irpsp->Parameters.DeviceIoControl.OutputBufferLength < length) > + { > + irp->IoStatus.Information = 0; > + irp->IoStatus.u.Status = STATUS_BUFFER_TOO_SMALL; > + } > + else > + { > + memcpy(irp->UserBuffer, report, length); > + irp->IoStatus.Information = length; > + irp->IoStatus.u.Status = STATUS_SUCCESS; > + } > + IoCompleteRequest(irp, IO_NO_INCREMENT); > + entry = RemoveHeadList(&ext->irp_queue); > + } > + LeaveCriticalSection(&ext->report_cs); > +} > + > NTSTATUS WINAPI DriverEntry( DRIVER_OBJECT *driver, UNICODE_STRING *path ) > { > static const WCHAR udevW[] = {'\\','D','r','i','v','e','r','\\','U','D','E','V',0}; > > >

Oct. 18, 2016

Re: [PATCH v2 2/2] hid/tests: Add HID device enumeration test

by Sebastian Lackner

On 17.10.2016 15:58, Aric Stewart wrote: > v2: Suggestions from Sebastian Lackner > Signed-off-by: Aric Stewart <aric(a)codeweavers.com> > --- > configure | 1 + > configure.ac | 1 + > dlls/hid/tests/Makefile.in | 5 +++ > dlls/hid/tests/device.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 89 insertions(+) > create mode 100644 dlls/hid/tests/Makefile.in > create mode 100644 dlls/hid/tests/device.c > > > > v2-0002-hid-tests-Add-HID-device-enumeration-test.txt > > > diff --git a/configure b/configure > index c21bda3..b397b38 100755 > --- a/configure > +++ b/configure > @@ -17940,6 +17940,7 @@ wine_fn_config_dll gpkcsp enable_gpkcsp > wine_fn_config_dll hal enable_hal > wine_fn_config_dll hhctrl.ocx enable_hhctrl_ocx clean,implib htmlhelp > wine_fn_config_dll hid enable_hid implib > +wine_fn_config_test dlls/hid/tests hid_test > wine_fn_config_dll hidclass.sys enable_hidclass_sys implib hidclass > wine_fn_config_dll hlink enable_hlink clean,implib > wine_fn_config_test dlls/hlink/tests hlink_test > diff --git a/configure.ac b/configure.ac > index c624c9e..40060b7 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -3027,6 +3027,7 @@ WINE_CONFIG_DLL(gpkcsp) > WINE_CONFIG_DLL(hal) > WINE_CONFIG_DLL(hhctrl.ocx,,[clean,implib],[htmlhelp]) > WINE_CONFIG_DLL(hid,,[implib]) > +WINE_CONFIG_TEST(dlls/hid/tests) > WINE_CONFIG_DLL(hidclass.sys,,[implib],[hidclass]) > WINE_CONFIG_DLL(hlink,,[clean,implib]) > WINE_CONFIG_TEST(dlls/hlink/tests) > diff --git a/dlls/hid/tests/Makefile.in b/dlls/hid/tests/Makefile.in > new file mode 100644 > index 0000000..6bf3773 > --- /dev/null > +++ b/dlls/hid/tests/Makefile.in > @@ -0,0 +1,5 @@ > +TESTDLL = hid.dll > +IMPORTS = hid setupapi > + > +C_SRCS = \ > + device.c > diff --git a/dlls/hid/tests/device.c b/dlls/hid/tests/device.c > new file mode 100644 > index 0000000..47ad1a3 > --- /dev/null > +++ b/dlls/hid/tests/device.c > @@ -0,0 +1,82 @@ > +/* > + * Copyright (c) 2016 Aric Stewart > + * > + * This library is free software; you can redistribute it and/or > + * modify it under the terms of the GNU Lesser General Public > + * License as published by the Free Software Foundation; either > + * version 2.1 of the License, or (at your option) any later version. > + * > + * This library is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * Lesser General Public License for more details. > + * > + * You should have received a copy of the GNU Lesser General Public > + * License along with this library; if not, write to the Free Software > + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA > + */ > + > +#include "ntstatus.h" > +#define WIN32_NO_STATUS > +#include "windows.h" > +#include "setupapi.h" > +#include "ddk/hidsdi.h" > + > +#include "wine/test.h" > + > +static void enumerate_devices(void) I didn't point this out earlier, but usually test functions start with "test_". An exception would be if you plan to use this as a helper function to run other tests for all enumerated devices. Then you could either move most of the code to START_TEST, for example: --- snip --- START_TEST(device) { ... enumeration code ... test_device_info(file); ... enumeration code ... } --- snip --- or rename it to something like "run_for_each_device(callback)" and use a callback-based approach: --- snip --- START_TEST(device) { run_for_each_device(test_device_info); } --- snip --- > +{ > + GUID hid_guid; > + WCHAR device_name[128]; > + HDEVINFO info_set; > + DWORD index = 0; > + SP_DEVICE_INTERFACE_DATA interface_data; > + DWORD detail_size = MAX_PATH * sizeof(WCHAR); > + SP_DEVICE_INTERFACE_DETAIL_DATA_W *data; > + > + HidD_GetHidGuid(&hid_guid); > + > + ZeroMemory(&interface_data, sizeof(interface_data)); > + interface_data.cbSize = sizeof(interface_data); > + > + data = HeapAlloc(GetProcessHeap(), 0, sizeof(*data) + detail_size); > + data->cbSize = sizeof(*data); > + > + info_set = SetupDiGetClassDevsW(&hid_guid, NULL, NULL, DIGCF_DEVICEINTERFACE); > + while (SetupDiEnumDeviceInterfaces(info_set, NULL, &hid_guid, index, &interface_data)) > + { > + index ++; > + > + if (SetupDiGetDeviceInterfaceDetailW(info_set, &interface_data, data, sizeof(*data) + detail_size, NULL, NULL)) > + { > + NTSTATUS status; > + BOOL rc; > + PHIDP_PREPARSED_DATA ppd; > + HIDP_CAPS Caps; > + HANDLE file = CreateFileW(data->DevicePath, GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, 0, 0); > + if (file == INVALID_HANDLE_VALUE) > + { > + trace("Failed to access device %s, likely not plugged in or access is denied.\n", wine_dbgstr_w(data->DevicePath)); > + continue; > + } > + rc = HidD_GetPreparsedData(file, &ppd); > + ok(rc, "Failed to get preparsed data(0x%x)\n", GetLastError()); > + status = HidP_GetCaps(ppd, &Caps); > + ok(status == HIDP_STATUS_SUCCESS, "Failed to get Caps(0x%x)\n", status); > + rc = HidD_GetProductString(file, device_name, sizeof(device_name)); > + ok(rc, "Failed to get product string(0x%x)\n", GetLastError()); > + trace("Found device %s (%s, %02x, %02x)\n", wine_dbgstr_w(device_name), wine_dbgstr_w(data->DevicePath), Caps.UsagePage, Caps.Usage); > + rc = HidD_FreePreparsedData(ppd); > + ok(rc, "Failed to free preparsed data(0x%x)\n", GetLastError()); > + CloseHandle(file); > + } > + } > + HeapFree(GetProcessHeap(), 0, data); > + if (info_set) > + SetupDiDestroyDeviceInfoList(info_set); According to MSDN SetupDiGetClassDevs returns INVALID_HANDLE_VALUE on failure, not NULL. If this check is not necessary you can also remove it. > +} > + > +START_TEST(device) > +{ > + enumerate_devices(); > +} > > >

Oct. 18, 2016

Re: [PATCH] ws2_32: Allow user to enable IP dual stack (v5).

by Dmitry Timoshkov

Roman Pišl <rpisl(a)seznam.cz> wrote: > Ok, sorry, I wanted to give him a credit as my patch is only a fine-tuning of > tests. Then it's probably better to add From: at the beginng of your patch with the original author's name, and add your sign-off. > Is it really possible that get_sock_fd() can fail after a successfull > create_socket request? It's better to be on the safe side. > What should be done if it fails, just skip setting the > option? Probably. Have a look at other places that use this combo. > Are these the only objections and should I send v6? I have no objections other than the ones mentioned above. -- Dmitry.

Oct. 18, 2016

Re: [PATCH] ws2_32: Allow user to enable IP dual stack (v5).

by Roman Pišl

Ok, sorry, I wanted to give him a credit as my patch is only a fine-tuning of tests. Is it really possible that get_sock_fd() can fail after a successfull create_socket request? What should be done if it fails, just skip setting the option? Are these the only objections and should I send v6? Thanks. Roman On Tuesday 18 of October 2016 19:30:53 Dmitry Timoshkov wrote: > Roman Pišl <rpisl(a)seznam.cz> wrote: > > IP dual stack (v4+v6) should be disabled by default, but previous code > > was setting IPV6_V6ONLY in bind() which prevented user to override it. > > This patch moves setting IPV6_V6ONLY to socket creation time. > > > > V5 - fixes failing test on Windows > > > > Based on > > https://www.winehq.org/pipermail/wine-patches/2016-July/151919.html > > > > Signed-off-by: Matthieu Nottale <matthieu.nottale(a)infinit.io> > > Signed-off-by: Roman Pišl <rpisl(a)seznam.cz> > > Probably you shouldn't add someone's else sign-off without the author's > agreement, or mention that you've got an explicit agreement from the patch > author otherwise. > > > +#ifdef IPV6_V6ONLY > > + if (unixaf == AF_INET6) > > + { > > + int enable = 1; > > + int fd = get_sock_fd(ret, 0, NULL); > > + setsockopt(fd, IPPROTO_IPV6, IPV6_V6ONLY, &enable, > > sizeof(enable)); + } > > +#endif > > It's better to verify that get_sock_fd() succeded before using its result, > and call release_sock_fd() at the end.

Oct. 18, 2016

Re: [PATCH] ws2_32: Allow user to enable IP dual stack (v5).

by Dmitry Timoshkov

Roman Pišl <rpisl(a)seznam.cz> wrote: > IP dual stack (v4+v6) should be disabled by default, but previous code > was setting IPV6_V6ONLY in bind() which prevented user to override it. > This patch moves setting IPV6_V6ONLY to socket creation time. > > V5 - fixes failing test on Windows > > Based on https://www.winehq.org/pipermail/wine-patches/2016-July/151919.html > > Signed-off-by: Matthieu Nottale <matthieu.nottale(a)infinit.io> > Signed-off-by: Roman Pišl <rpisl(a)seznam.cz> Probably you shouldn't add someone's else sign-off without the author's agreement, or mention that you've got an explicit agreement from the patch author otherwise. > +#ifdef IPV6_V6ONLY > + if (unixaf == AF_INET6) > + { > + int enable = 1; > + int fd = get_sock_fd(ret, 0, NULL); > + setsockopt(fd, IPPROTO_IPV6, IPV6_V6ONLY, &enable, sizeof(enable)); > + } > +#endif It's better to verify that get_sock_fd() succeded before using its result, and call release_sock_fd() at the end. -- Dmitry.

Oct. 18, 2016

Re: [PATCH] ws2_32: Allow user to enable IP dual stack (v4).

by Bruno Jesus

Hi, thanks for the retry. I'm not at home and I'm replying from mobile so I can't help much, from the test results it is clear that MS broke the API starting on Vista, the.tests need to be.enhaced so that they pass on both pré vista and post xp. In the sockets tests I have made that many times due to other API breaks, look for the word behavior around tests line 1760. So you can split and make tests pass on all systems. On Monday, October 17, 2016, Roman Pišl <rpisl(a)seznam.cz> wrote: > IP dual stack (v4+v6) should be disabled by default, but previous code > was setting IPV6_V6ONLY in bind() which prevented user to override it. > This patch moves setting IPV6_V6ONLY to socket creation time. > > Based on https://www.winehq.org/pipermail/wine-patches/2016- > July/151919.html > > Signed-off-by: Matthieu Nottale <matthieu.nottale(a)infinit.io > <javascript:;>> > Signed-off-by: Roman Pišl <rpisl(a)seznam.cz <javascript:;>> > --- > dlls/ws2_32/socket.c | 23 +++++++++------------- > dlls/ws2_32/tests/sock.c | 50 ++++++++++++++++++++++++++++++ > ++++++++++++++++-- > 2 files changed, 57 insertions(+), 16 deletions(-) > > diff --git a/dlls/ws2_32/socket.c b/dlls/ws2_32/socket.c > index e8c5130..d14e9d6 100644 > --- a/dlls/ws2_32/socket.c > +++ b/dlls/ws2_32/socket.c > @@ -3240,20 +3240,6 @@ int WINAPI WS_bind(SOCKET s, const struct > WS_sockaddr* name, int namelen) > } > else > { > -#ifdef IPV6_V6ONLY > - const struct sockaddr_in6 *in6 = (const struct > sockaddr_in6*) &uaddr; > - if (name->sa_family == WS_AF_INET6 && > - !memcmp(&in6->sin6_addr, &in6addr_any, sizeof(struct > in6_addr))) > - { > - int enable = 1; > - if (setsockopt(fd, IPPROTO_IPV6, IPV6_V6ONLY, > &enable, sizeof(enable)) == -1) > - { > - release_sock_fd( s, fd ); > - SetLastError(WSAEAFNOSUPPORT); > - return SOCKET_ERROR; > - } > - } > -#endif > if (name->sa_family == WS_AF_INET) > { > struct sockaddr_in *in4 = (struct sockaddr_in*) > &uaddr; > @@ -7233,6 +7219,15 @@ SOCKET WINAPI WSASocketW(int af, int type, int > protocol, > TRACE("\tcreated %04lx\n", ret ); > if (ipxptype > 0) > set_ipx_packettype(ret, ipxptype); > + > +#ifdef IPV6_V6ONLY > + if (unixaf == AF_INET6) > + { > + int enable = 1; > + int fd = get_sock_fd(ret, 0, NULL); > + setsockopt(fd, IPPROTO_IPV6, IPV6_V6ONLY, &enable, > sizeof(enable)); > + } > +#endif > return ret; > } > > diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c > index a144bd3..a676255 100644 > --- a/dlls/ws2_32/tests/sock.c > +++ b/dlls/ws2_32/tests/sock.c > @@ -1732,7 +1732,7 @@ static void test_so_reuseaddr(void) > DWORD err; > > saddr.sin_family = AF_INET; > - saddr.sin_port = htons(9375); > + saddr.sin_port = htons(SERVERPORT+1); > saddr.sin_addr.s_addr = inet_addr("127.0.0.1"); > > s1=socket(AF_INET, SOCK_STREAM, 0); > @@ -6110,6 +6110,8 @@ static void test_ipv6only(void) > struct sockaddr_in sin4; > struct sockaddr_in6 sin6; > int ret; > + int enabled; > + int len; > > memset(&sin4, 0, sizeof(sin4)); > sin4.sin_family = AF_INET; > @@ -6142,6 +6144,50 @@ static void test_ipv6only(void) > ok(!ret, "Could not bind IPv4 address (LastError: %d; %d expected if > IPv6 binds to IPv4 as well).\n", > WSAGetLastError(), WSAEADDRINUSE); > > + closesocket(v4); > + closesocket(v6); > + v4 = INVALID_SOCKET; > + v6 = INVALID_SOCKET; > + > + /* Test again, this time disabling V6ONLY. */ > + sin4.sin_port = htons(SERVERPORT+2); > + sin6.sin6_port = htons(SERVERPORT+2); > + > + v6 = socket(AF_INET6, SOCK_STREAM, IPPROTO_TCP); > + ok(v6 != INVALID_SOCKET, "Could not create IPv6 socket (LastError: > %d; %d expected if IPv6 not available).\n", > + WSAGetLastError(), WSAEAFNOSUPPORT); > + > + enabled = 2; > + len = sizeof(enabled); > + ret = getsockopt(v6, IPPROTO_IPV6, IPV6_V6ONLY, (char*)&enabled, > &len); > + if (ret) { > + skip("Could not check IPV6_V6ONLY (option not supported?) > ((LastError: %d).\n", WSAGetLastError()); > + goto end; > + } > + ok(enabled == 1, "IPV6_V6ONLY is not enabled by default.\n"); > + > + enabled = 0; > + ret = setsockopt(v6, IPPROTO_IPV6, IPV6_V6ONLY, (char*)&enabled, len); > + ok(!ret, "Failed to disable IPV6_V6ONLY (LastError: %d).\n", > WSAGetLastError()); > + > + ret = bind(v6, (struct sockaddr*)&sin6, sizeof(sin6)); > + ok(!ret, "Could not bind IPv6 address (LastError: %d).\n", > WSAGetLastError()); > + > + enabled = 2; > + len = sizeof(enabled); > + getsockopt(v6, IPPROTO_IPV6, IPV6_V6ONLY, (char*)&enabled, &len); > + ok(enabled == 0, "IPV6_V6ONLY is enabled after bind.\n"); > + > + v4 = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP); > + ok(v4 != INVALID_SOCKET, "Could not create IPv4 socket (LastError: > %d).\n", > + WSAGetLastError()); > + > + WSASetLastError(0xdeadbeef); > + bind(v4, (struct sockaddr*)&sin4, sizeof(sin4)); > + > + ok(WSAGetLastError() == WSAEADDRINUSE, "Failed to disable IPV6_V6ONLY > (LastError: %d; %d expected if IPv6 binds to IPv4 as well).\n", > + WSAGetLastError(), WSAEADDRINUSE); > + > end: > if (v4 != INVALID_SOCKET) > closesocket(v4); > @@ -8024,7 +8070,7 @@ static void test_TransmitFile(void) > /* Setup a properly connected socket for transfers */ > memset(&bindAddress, 0, sizeof(bindAddress)); > bindAddress.sin_family = AF_INET; > - bindAddress.sin_port = htons(9375); > + bindAddress.sin_port = htons(SERVERPORT+1); > bindAddress.sin_addr.s_addr = inet_addr("127.0.0.1"); > iret = bind(server, (struct sockaddr*)&bindAddress, > sizeof(bindAddress)); > if (iret != 0) > -- > 2.7.3 > > > >

Oct. 17, 2016

Re: [PATCH] ws2_32: Allow user to enable IP dual stack (v4).

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=26191 Your paranoid android. === wvistau64 (32 bit sock) === sock.c:6188: Test failed: Failed to disable IPV6_V6ONLY (LastError: -559038737; 10048 expected if IPv6 binds to IPv4 as well). === w2008s64 (32 bit sock) === sock.c:6188: Test failed: Failed to disable IPV6_V6ONLY (LastError: -559038737; 10048 expected if IPv6 binds to IPv4 as well). === w7u (32 bit sock) === sock.c:6188: Test failed: Failed to disable IPV6_V6ONLY (LastError: 0; 10048 expected if IPv6 binds to IPv4 as well). === w7pro64 (32 bit sock) === sock.c:6188: Test failed: Failed to disable IPV6_V6ONLY (LastError: 0; 10048 expected if IPv6 binds to IPv4 as well). sock.c:7810: Test failed: bytesReturned isn't supposed to be 1 sock.c:7834: Test failed: GetOverlappedResult failed, error 0 === w8 (32 bit sock) === sock.c:6188: Test failed: Failed to disable IPV6_V6ONLY (LastError: 0; 10048 expected if IPv6 binds to IPv4 as well). === w864 (32 bit sock) === sock.c:6188: Test failed: Failed to disable IPV6_V6ONLY (LastError: 0; 10048 expected if IPv6 binds to IPv4 as well). === w1064 (32 bit sock) === sock.c:6188: Test failed: Failed to disable IPV6_V6ONLY (LastError: 0; 10048 expected if IPv6 binds to IPv4 as well). === wvistau64 (64 bit sock) === sock.c:6188: Test failed: Failed to disable IPV6_V6ONLY (LastError: -559038737; 10048 expected if IPv6 binds to IPv4 as well). === w2008s64 (64 bit sock) === sock.c:6188: Test failed: Failed to disable IPV6_V6ONLY (LastError: -559038737; 10048 expected if IPv6 binds to IPv4 as well). === w7pro64 (64 bit sock) === sock.c:6188: Test failed: Failed to disable IPV6_V6ONLY (LastError: 0; 10048 expected if IPv6 binds to IPv4 as well). sock.c:7810: Test failed: bytesReturned isn't supposed to be 1 sock.c:7856: Test failed: Waiting for timeout failed with 0 + errno 997 === w864 (64 bit sock) === sock.c:6188: Test failed: Failed to disable IPV6_V6ONLY (LastError: 0; 10048 expected if IPv6 binds to IPv4 as well). === w1064 (64 bit sock) === sock.c:6188: Test failed: Failed to disable IPV6_V6ONLY (LastError: 0; 10048 expected if IPv6 binds to IPv4 as well).

Oct. 17, 2016

Re: [PATCH v2] winebus.sys: Handle device reports for hidraw devices

by Sebastian Lackner

On 17.10.2016 15:29, Aric Stewart wrote: > v2: Suggestions from Sebastian Lackner > Signed-off-by: Aric Stewart <aric(a)codeweavers.com> > --- > dlls/winebus.sys/bus.h | 3 ++ > dlls/winebus.sys/bus_udev.c | 73 ++++++++++++++++++++++++++ > dlls/winebus.sys/main.c | 123 +++++++++++++++++++++++++++++++++++++++++++- > 3 files changed, 198 insertions(+), 1 deletion(-) > > > > v2-0001-winebus.sys-Handle-device-reports-for-hidraw-device.txt > > > diff --git a/dlls/winebus.sys/bus.h b/dlls/winebus.sys/bus.h > index 7ce54bf..bcb1163 100644 > --- a/dlls/winebus.sys/bus.h > +++ b/dlls/winebus.sys/bus.h > @@ -25,6 +25,8 @@ typedef struct > int (*compare_platform_device)(DEVICE_OBJECT *device, void *platform_dev); > NTSTATUS (*get_reportdescriptor)(DEVICE_OBJECT *device, BYTE *buffer, DWORD length, DWORD *out_length); > NTSTATUS (*get_string)(DEVICE_OBJECT *device, DWORD index, WCHAR *buffer, DWORD length); > + NTSTATUS (*begin_report_processing)(DEVICE_OBJECT *device, BYTE **report_buffer, DWORD *buffer_length); > + void (*end_report_processing)(DEVICE_OBJECT *device, BYTE *report_buffer); > } platform_vtbl; > > void *get_platform_private(DEVICE_OBJECT *device) DECLSPEC_HIDDEN; > @@ -37,3 +39,4 @@ DEVICE_OBJECT *bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW, > DEVICE_OBJECT *bus_find_hid_device(const platform_vtbl *vtbl, void *platform_dev) DECLSPEC_HIDDEN; > void bus_remove_hid_device(DEVICE_OBJECT *device) DECLSPEC_HIDDEN; > NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) DECLSPEC_HIDDEN; > +void process_hid_report(DEVICE_OBJECT *device, const WCHAR *busidW, BYTE *report, DWORD length) DECLSPEC_HIDDEN; > diff --git a/dlls/winebus.sys/bus_udev.c b/dlls/winebus.sys/bus_udev.c > index 243aabb..37c8412 100644 > --- a/dlls/winebus.sys/bus_udev.c > +++ b/dlls/winebus.sys/bus_udev.c > @@ -61,6 +61,8 @@ WINE_DEFAULT_DEBUG_CHANNEL(plugplay); > > #ifdef HAVE_UDEV > > +WINE_DECLARE_DEBUG_CHANNEL(hid_report); > + > static struct udev *udev_context = NULL; > static DRIVER_OBJECT *udev_driver_obj = NULL; > > @@ -73,6 +75,11 @@ struct platform_private > { > struct udev_device *udev_device; > int device_fd; > + > + BYTE *report_buffer; > + int buffer_length; > + HANDLE report_thread; > + int control_pipe[2]; > }; > > static inline struct platform_private *impl_from_DEVICE_OBJECT(DEVICE_OBJECT *device) > @@ -222,11 +229,77 @@ static NTSTATUS hidraw_get_string(DEVICE_OBJECT *device, DWORD index, WCHAR *buf > return STATUS_SUCCESS; > } > > +static DWORD CALLBACK device_report_thread(void *args) > +{ > + DEVICE_OBJECT *device = (DEVICE_OBJECT*)args; > + struct platform_private *private = impl_from_DEVICE_OBJECT(device); > + struct pollfd plfds[2]; > + > + plfds[0].fd = private->device_fd; > + plfds[0].events = POLLIN; > + plfds[0].revents = 0; > + plfds[1].fd = private->control_pipe[0]; > + plfds[1].events = POLLIN; > + plfds[1].revents = 0; > + > + while (1) > + { > + int size; > + if (poll(plfds, 2, -1) <= 0) continue; > + if (plfds[1].revents || !private->report_buffer || private->buffer_length == 0) > + break; > + size = read(plfds[0].fd, private->report_buffer, private->buffer_length); This could still cause problems when another thread copies the last_report while it is updated. I don't know how you are planning to implement it for other platforms, but wouldn't it be better to use a separate buffer in this thread, and let process_hid_report() do the copying? > + if (size == -1) > + TRACE_(hid_report)("Read failed. Likely an unplugged device\n"); > + else if (size == 0) > + TRACE_(hid_report)("Failed to read report\n"); > + else > + process_hid_report(device, hidraw_busidW, private->report_buffer, size); > + } > + return 0; > +} > + > +static NTSTATUS begin_report_processing(DEVICE_OBJECT *device, BYTE **buffer, DWORD *length) > +{ > + struct platform_private *private = impl_from_DEVICE_OBJECT(device); > + *length = private->buffer_length = 1024; > + *buffer = private->report_buffer = HeapAlloc(GetProcessHeap(), 0, *length); > + > + pipe(private->control_pipe); > + private->report_thread = CreateThread(NULL, 0, device_report_thread, device, 0, NULL); > + if (!private->report_thread) > + { > + ERR("Unable to create device report thread\n"); > + close(private->control_pipe[0]); > + close(private->control_pipe[1]); > + return STATUS_UNSUCCESSFUL; > + } > + else > + return STATUS_SUCCESS; > +} > + > +static void end_report_processing(DEVICE_OBJECT *device, BYTE *buffer) > +{ > + struct platform_private *private = impl_from_DEVICE_OBJECT(device); > + if (private->report_thread) > + { > + write(private->control_pipe[1], "q", 1); > + WaitForSingleObject(private->report_thread, INFINITE); > + close(private->control_pipe[0]); > + close(private->control_pipe[1]); > + } > + private->buffer_length = 0; > + private->report_buffer = NULL; > + HeapFree(GetProcessHeap(), 0, buffer); > +} > + > static const platform_vtbl hidraw_vtbl = > { > compare_platform_device, > hidraw_get_reportdescriptor, > hidraw_get_string, > + begin_report_processing, > + end_report_processing, > }; > > static void try_add_device(struct udev_device *dev) > diff --git a/dlls/winebus.sys/main.c b/dlls/winebus.sys/main.c > index 86adbf5..49dcc97 100644 > --- a/dlls/winebus.sys/main.c > +++ b/dlls/winebus.sys/main.c > @@ -40,6 +40,7 @@ > #include "bus.h" > > WINE_DEFAULT_DEBUG_CHANNEL(plugplay); > +WINE_DECLARE_DEBUG_CHANNEL(hid_report); > > struct pnp_device > { > @@ -58,6 +59,14 @@ struct device_extension > const WCHAR *busid; /* Expected to be a static constant */ > > const platform_vtbl *vtbl; > + > + BOOL processing_started; > + BYTE *last_report; > + DWORD last_report_size; > + DWORD buffer_size; > + LIST_ENTRY irp_queue; > + CRITICAL_SECTION report_cs; > + > BYTE platform_private[1]; > }; > > @@ -214,6 +223,9 @@ DEVICE_OBJECT *bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW, > ext->busid = busidW; > ext->vtbl = vtbl; > > + InitializeListHead(&ext->irp_queue); > + InitializeCriticalSection(&ext->report_cs); You might want to give this CS a name, like done in other parts of wine. For example: --- snip --- InitializeCriticalSection(&src->lock); src->lock.DebugInfo->Spare[0] = (DWORD_PTR)(__FILE__ ": XA2SourceImpl.lock"); --- snip --- > + > /* add to list of pnp devices */ > pnp_dev->device = device; > list_add_tail(&pnp_devset, &pnp_dev->entry); > @@ -271,6 +283,8 @@ void bus_remove_hid_device(DEVICE_OBJECT *device) > { > struct device_extension *ext = (struct device_extension *)device->DeviceExtension; > struct pnp_device *pnp_device = ext->pnp_device; > + LIST_ENTRY *entry; > + IRP *irp; > > TRACE("(%p)\n", device); > > @@ -279,6 +293,23 @@ void bus_remove_hid_device(DEVICE_OBJECT *device) > LeaveCriticalSection(&device_list_cs); > > IoInvalidateDeviceRelations(device, RemovalRelations); > + > + if (ext->processing_started) > + ext->vtbl->end_report_processing(device, ext->last_report); > + > + /* Cancel pending IRPs */ > + EnterCriticalSection(&ext->report_cs); > + entry = RemoveHeadList(&ext->irp_queue); > + while(entry != &ext->irp_queue) > + { > + irp = CONTAINING_RECORD(entry, IRP, Tail.Overlay.ListEntry); > + irp->IoStatus.u.Status = STATUS_CANCELLED; > + irp->IoStatus.Information = 0; > + IoCompleteRequest(irp, IO_NO_INCREMENT); > + entry = RemoveHeadList(&ext->irp_queue); > + } > + LeaveCriticalSection(&ext->report_cs); > + You will have to delete the CS here. Also make sure to reset Spare[0] if it was previously set to a debug string. For example: --- snip --- src->lock.DebugInfo->Spare[0] = 0; DeleteCriticalSection(&src->lock); --- snip --- > HeapFree(GetProcessHeap(), 0, ext->serial); > IoDeleteDevice(device); > > @@ -432,6 +463,58 @@ NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) > irp->IoStatus.Information = (strlenW((WCHAR *)irp->UserBuffer) + 1) * sizeof(WCHAR); > break; > } > + case IOCTL_HID_GET_INPUT_REPORT: > + { > + HID_XFER_PACKET *packet = (HID_XFER_PACKET*)(irp->UserBuffer); > + TRACE_(hid_report)("IOCTL_HID_GET_INPUT_REPORT\n"); > + EnterCriticalSection(&ext->report_cs); > + if (!ext->processing_started) > + { > + irp->IoStatus.u.Status = status = ext->vtbl->begin_report_processing(device, &ext->last_report, &ext->buffer_size); > + if (status == STATUS_SUCCESS) > + ext->processing_started = TRUE; > + else > + { > + LeaveCriticalSection(&ext->report_cs); > + break; > + } > + } > + > + if (packet->reportBufferLen < ext->last_report_size) > + { > + irp->IoStatus.u.Status = status = STATUS_BUFFER_TOO_SMALL; > + LeaveCriticalSection(&ext->report_cs); > + break; > + } > + > + ZeroMemory(packet->reportBuffer, packet->reportBufferLen); Do we need this line? Usually callers do not expect valid data after the length provided by irp->IoStatus.Information. Is it different in this case? > + memcpy(packet->reportBuffer, ext->last_report, ext->last_report_size); > + packet->reportBufferLen = ext->last_report_size; > + irp->IoStatus.Information = ext->last_report_size; > + irp->IoStatus.u.Status = status = STATUS_SUCCESS; > + LeaveCriticalSection(&ext->report_cs); > + break; > + } > + case IOCTL_HID_READ_REPORT: > + { > + TRACE_(hid_report)("IOCTL_HID_READ_REPORT\n"); > + EnterCriticalSection(&ext->report_cs); > + if (!ext->processing_started) > + { > + if (ext->vtbl->begin_report_processing(device, &ext->last_report, &ext->buffer_size) == STATUS_SUCCESS) > + ext->processing_started = TRUE; > + else > + { > + irp->IoStatus.u.Status = status; > + LeaveCriticalSection(&ext->report_cs); > + break; > + } > + } > + InsertTailList(&ext->irp_queue, &irp->Tail.Overlay.ListEntry); > + status = STATUS_PENDING; > + LeaveCriticalSection(&ext->report_cs); > + break; > + } > default: > { > ULONG code = irpsp->Parameters.DeviceIoControl.IoControlCode; > @@ -441,11 +524,49 @@ NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) > } > } > > - IoCompleteRequest(irp, IO_NO_INCREMENT); > + if (status != STATUS_PENDING) > + IoCompleteRequest(irp, IO_NO_INCREMENT); > > return status; > } > > +void process_hid_report(DEVICE_OBJECT *device, const WCHAR *busidW, BYTE *report, DWORD length) > +{ > + struct device_extension *ext = (struct device_extension*)device->DeviceExtension; > + IRP *irp; > + LIST_ENTRY *entry; > + > + EnterCriticalSection(&ext->report_cs); > + if (ext->last_report && report != ext->last_report && ext->buffer_size <= length) Shouldn't it be "length <= ext->buffer_size" ? > + { > + memcpy(ext->last_report, report, length); > + ext->last_report_size = length; > + } > + entry = RemoveHeadList(&ext->irp_queue); > + while(entry != &ext->irp_queue) > + { > + IO_STACK_LOCATION *irpsp; > + TRACE_(hid_report)("Processing Request\n"); > + irp = CONTAINING_RECORD(entry, IRP, Tail.Overlay.ListEntry); > + irpsp = IoGetCurrentIrpStackLocation(irp); > + > + if (irpsp->Parameters.DeviceIoControl.OutputBufferLength < length) > + { > + irp->IoStatus.Information = 0; > + irp->IoStatus.u.Status = STATUS_BUFFER_TOO_SMALL; > + } > + else > + { > + memcpy(irp->UserBuffer, report, length); > + irp->IoStatus.Information = length; > + irp->IoStatus.u.Status = STATUS_SUCCESS; > + } > + IoCompleteRequest(irp, IO_NO_INCREMENT); > + entry = RemoveHeadList(&ext->irp_queue); > + } > + LeaveCriticalSection(&ext->report_cs); > +} > + > NTSTATUS WINAPI DriverEntry( DRIVER_OBJECT *driver, UNICODE_STRING *path ) > { > static const WCHAR udevW[] = {'\\','D','r','i','v','e','r','\\','U','D','E','V',0}; > > >

Oct. 17, 2016

Re: [PATCH] hid/tests: Add HID device enumeration test

by Sebastian Lackner

On 14.10.2016 11:36, Aric Stewart wrote: > Signed-off-by: Aric Stewart <aric(a)codeweavers.com> > --- > configure | 1 + > configure.ac | 1 + > dlls/hid/tests/Makefile.in | 5 +++ > dlls/hid/tests/device.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 87 insertions(+) > create mode 100644 dlls/hid/tests/Makefile.in > create mode 100644 dlls/hid/tests/device.c > > > > 0001-hid-tests-Add-HID-device-enumeration-test.txt > > > diff --git a/configure b/configure > index 99793b0..7bb6fef 100755 > --- a/configure > +++ b/configure > @@ -17936,6 +17936,7 @@ wine_fn_config_dll gpkcsp enable_gpkcsp > wine_fn_config_dll hal enable_hal > wine_fn_config_dll hhctrl.ocx enable_hhctrl_ocx clean,implib htmlhelp > wine_fn_config_dll hid enable_hid implib > +wine_fn_config_test dlls/hid/tests hid_test > wine_fn_config_dll hidclass.sys enable_hidclass_sys implib hidclass > wine_fn_config_dll hlink enable_hlink clean,implib > wine_fn_config_test dlls/hlink/tests hlink_test > diff --git a/configure.ac b/configure.ac > index ba8fd0f..9cab382 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -3025,6 +3025,7 @@ WINE_CONFIG_DLL(gpkcsp) > WINE_CONFIG_DLL(hal) > WINE_CONFIG_DLL(hhctrl.ocx,,[clean,implib],[htmlhelp]) > WINE_CONFIG_DLL(hid,,[implib]) > +WINE_CONFIG_TEST(dlls/hid/tests) > WINE_CONFIG_DLL(hidclass.sys,,[implib],[hidclass]) > WINE_CONFIG_DLL(hlink,,[clean,implib]) > WINE_CONFIG_TEST(dlls/hlink/tests) > diff --git a/dlls/hid/tests/Makefile.in b/dlls/hid/tests/Makefile.in > new file mode 100644 > index 0000000..d4a69f0 > --- /dev/null > +++ b/dlls/hid/tests/Makefile.in > @@ -0,0 +1,5 @@ > +TESTDLL = hid.dll > +IMPORTS = hid setupapi > + > +C_SRCS = \ > + device.c \ The "\" at the end seems wrong. Please note that you can also use tools/make_makefiles to update/fix this file automatically for you. > diff --git a/dlls/hid/tests/device.c b/dlls/hid/tests/device.c > new file mode 100644 > index 0000000..f3260ec > --- /dev/null > +++ b/dlls/hid/tests/device.c > @@ -0,0 +1,80 @@ > +/* > + * Copyright (c) 2016 Aric Stewart > + * > + * This library is free software; you can redistribute it and/or > + * modify it under the terms of the GNU Lesser General Public > + * License as published by the Free Software Foundation; either > + * version 2.1 of the License, or (at your option) any later version. > + * > + * This library is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * Lesser General Public License for more details. > + * > + * You should have received a copy of the GNU Lesser General Public > + * License along with this library; if not, write to the Free Software > + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA > + */ > + > +#include "ntstatus.h" > +#define WIN32_NO_STATUS > +#include "windows.h" > +#include "setupapi.h" > + > +#ifndef WINE_NTSTATUS_DECLARED > +#define WINE_NTSTATUS_DECLARED > +typedef LONG NTSTATUS; > +#endif Such hacks should be avoided in tests. I believe you are trying to workaround a bug in the Wine hidsdi.h header file. Wine declares NTSTATUS after the includes: --- snip --- #include <hidusage.h> #include <ddk/hidpi.h> #ifndef WINE_NTSTATUS_DECLARED #define WINE_NTSTATUS_DECLARED typedef LONG NTSTATUS; #endif --- snip --- On Windows its declared before: --- snip --- typedef _Return_type_success_(return >= 0) LONG NTSTATUS; #include "hidusage.h" #include "hidpi.h" --- snip --- > + > +#include "ddk/hidsdi.h" > + > +#include "wine/test.h" > + > +static void enumerate_devices(void) > +{ > + GUID hid_guid; > + WCHAR device_name[128]; > + HDEVINFO info_set; > + DWORD index = 0; > + SP_DEVICE_INTERFACE_DATA interface_data; > + DWORD detail_size = MAX_PATH * sizeof(WCHAR); > + SP_DEVICE_INTERFACE_DETAIL_DATA_W *data; > + > + HidD_GetHidGuid(&hid_guid); > + > + ZeroMemory(&interface_data, sizeof(interface_data)); > + interface_data.cbSize = sizeof(interface_data); > + > + data = HeapAlloc(GetProcessHeap(), 0, sizeof(*data) + detail_size); > + data->cbSize = sizeof(*data); > + > + info_set = SetupDiGetClassDevsW(&hid_guid, NULL, NULL, DIGCF_DEVICEINTERFACE); Shouldn't this be released afterwards? > + while (SetupDiEnumDeviceInterfaces(info_set, NULL, &hid_guid, index, &interface_data)) > + { > + index ++; > + > + if (SetupDiGetDeviceInterfaceDetailW(info_set, &interface_data, data, detail_size, NULL, NULL)) Shouldn't the DeviceInterfaceDetailDataSize parameter also include the sizeof(*data) ? > + { > + PHIDP_PREPARSED_DATA ppd; > + HIDP_CAPS Caps; > + HANDLE file = CreateFileW(data->DevicePath, GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, 0, 0); > + if (file == INVALID_HANDLE_VALUE) > + { > + trace("Failed to access device %s, likely not plugged in or access is denied.\n", wine_dbgstr_w(data->DevicePath)); > + continue; > + } > + ok(HidD_GetPreparsedData(file, &ppd), "Failed to preparsed data\n"); Is there a "get" missing in the debug string? > + ok(HidP_GetCaps(ppd, &Caps) == HIDP_STATUS_SUCCESS, "Failed to get Caps\n"); > + ok(HidD_GetProductString(file, device_name, sizeof(device_name)), "Failed to get product string\n"); > + trace("Found device %s (%s, %02x, %02x)\n", wine_dbgstr_w(device_name), wine_dbgstr_w(data->DevicePath), Caps.UsagePage, Caps.Usage); > + ok(HidD_FreePreparsedData(ppd), "Failed to free preparsed data\n"); Its a matter of taste, but usually its better to assign those values to a variable. This also allows to print the error code / GetLastError() in case of a failure. > + CloseHandle(file); > + } > + } > + HeapFree(GetProcessHeap(), 0, data); > +} > + > +START_TEST(device) > +{ > + enumerate_devices(); > +} > > >

Oct. 17, 2016

Re: [PATCH] winebus.sys: Handle device reports for hidraw devices

by Sebastian Lackner

On 17.10.2016 06:41, Aric Stewart wrote: > Signed-off-by: Aric Stewart <aric(a)codeweavers.com> > --- > dlls/winebus.sys/bus.h | 3 ++ > dlls/winebus.sys/bus_udev.c | 63 +++++++++++++++++++++++++++++ > dlls/winebus.sys/main.c | 97 ++++++++++++++++++++++++++++++++++++++++++++- > 3 files changed, 162 insertions(+), 1 deletion(-) > > > > 0001-winebus.sys-Handle-device-reports-for-hidraw-devices.txt > > > diff --git a/dlls/winebus.sys/bus.h b/dlls/winebus.sys/bus.h > index 7ce54bf..bcb1163 100644 > --- a/dlls/winebus.sys/bus.h > +++ b/dlls/winebus.sys/bus.h > @@ -25,6 +25,8 @@ typedef struct > int (*compare_platform_device)(DEVICE_OBJECT *device, void *platform_dev); > NTSTATUS (*get_reportdescriptor)(DEVICE_OBJECT *device, BYTE *buffer, DWORD length, DWORD *out_length); > NTSTATUS (*get_string)(DEVICE_OBJECT *device, DWORD index, WCHAR *buffer, DWORD length); > + NTSTATUS (*begin_report_processing)(DEVICE_OBJECT *device, BYTE **report_buffer, DWORD *buffer_length); > + void (*end_report_processing)(DEVICE_OBJECT *device, BYTE *report_buffer); > } platform_vtbl; > > void *get_platform_private(DEVICE_OBJECT *device) DECLSPEC_HIDDEN; > @@ -37,3 +39,4 @@ DEVICE_OBJECT *bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW, > DEVICE_OBJECT *bus_find_hid_device(const platform_vtbl *vtbl, void *platform_dev) DECLSPEC_HIDDEN; > void bus_remove_hid_device(DEVICE_OBJECT *device) DECLSPEC_HIDDEN; > NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) DECLSPEC_HIDDEN; > +void process_hid_report(DEVICE_OBJECT *device, const WCHAR *busidW, BYTE *report, DWORD length) DECLSPEC_HIDDEN; > diff --git a/dlls/winebus.sys/bus_udev.c b/dlls/winebus.sys/bus_udev.c > index 243aabb..5307224 100644 > --- a/dlls/winebus.sys/bus_udev.c > +++ b/dlls/winebus.sys/bus_udev.c > @@ -61,6 +61,8 @@ WINE_DEFAULT_DEBUG_CHANNEL(plugplay); > > #ifdef HAVE_UDEV > > +WINE_DECLARE_DEBUG_CHANNEL(hid_report); > + > static struct udev *udev_context = NULL; > static DRIVER_OBJECT *udev_driver_obj = NULL; > > @@ -73,6 +75,10 @@ struct platform_private > { > struct udev_device *udev_device; > int device_fd; > + > + BYTE *report_buffer; > + int buffer_length; > + HANDLE report_thread; > }; > > static inline struct platform_private *impl_from_DEVICE_OBJECT(DEVICE_OBJECT *device) > @@ -222,11 +228,68 @@ static NTSTATUS hidraw_get_string(DEVICE_OBJECT *device, DWORD index, WCHAR *buf > return STATUS_SUCCESS; > } > > +static DWORD CALLBACK device_report_thread(void *args) > +{ > + DEVICE_OBJECT *device = (DEVICE_OBJECT*)args; > + struct platform_private *private = impl_from_DEVICE_OBJECT(device); > + struct pollfd plfds[1]; > + > + plfds[0].fd = private->device_fd; > + plfds[0].events = POLLIN; > + > + while (1) > + { > + int size; > + if (poll(plfds, 1, -1) <= 0) continue; > + if (!private->report_buffer || private->buffer_length == 0) > + break; > + size = read(plfds[0].fd, private->report_buffer, private->buffer_length); > + if (size == -1) > + TRACE_(hid_report)("Read failed. Likely an unplugged device\n"); > + else if (size == 0) > + TRACE_(hid_report)("Failed to read report\n"); > + else > + process_hid_report(device, hidraw_busidW, private->report_buffer, size); > + } > + return 0; > +} > + > +static NTSTATUS begin_report_processing(DEVICE_OBJECT *device, BYTE **buffer, DWORD *length) > +{ > + struct platform_private *private = impl_from_DEVICE_OBJECT(device); > + *length = private->buffer_length = 1024; > + *buffer = private->report_buffer = HeapAlloc(GetProcessHeap(), 0, *length); > + > + private->report_thread = CreateThread(NULL, 0, device_report_thread, device, 0, NULL); > + if (!private->report_thread) > + { > + ERR("Unable to create device report thread\n"); > + return STATUS_UNSUCCESSFUL; > + } > + else > + return STATUS_SUCCESS; > +} > + > +static void end_report_processing(DEVICE_OBJECT *device, BYTE *buffer) > +{ > + struct platform_private *private = impl_from_DEVICE_OBJECT(device); > + if (private->report_thread) > + { > + TerminateThread(private->report_thread, 0); TerminateThread is not really the best way to clean up this worker thread. Depending on when exactly it is called, it can corrupt internal data and crash the whole process. In this case, it would definitely be better to have some synchronization mechanism (for example event pipe) to allow a clean shutdown. > + WaitForSingleObject(private->report_thread, INFINITE); > + } > + private->buffer_length = 0; > + private->report_buffer = NULL; > + HeapFree(GetProcessHeap(), 0, buffer); > +} > + > static const platform_vtbl hidraw_vtbl = > { > compare_platform_device, > hidraw_get_reportdescriptor, > hidraw_get_string, > + begin_report_processing, > + end_report_processing, > }; > > static void try_add_device(struct udev_device *dev) > diff --git a/dlls/winebus.sys/main.c b/dlls/winebus.sys/main.c > index 86adbf5..f12cdf7 100644 > --- a/dlls/winebus.sys/main.c > +++ b/dlls/winebus.sys/main.c > @@ -40,6 +40,7 @@ > #include "bus.h" > > WINE_DEFAULT_DEBUG_CHANNEL(plugplay); > +WINE_DECLARE_DEBUG_CHANNEL(hid_report); > > struct pnp_device > { > @@ -58,6 +59,13 @@ struct device_extension > const WCHAR *busid; /* Expected to be a static constant */ > > const platform_vtbl *vtbl; > + > + BOOL processing_started; > + BYTE *last_report; > + DWORD last_report_size; > + DWORD buffer_size; > + LIST_ENTRY irp_queue; > + > BYTE platform_private[1]; > }; > > @@ -214,6 +222,8 @@ DEVICE_OBJECT *bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW, > ext->busid = busidW; > ext->vtbl = vtbl; > > + InitializeListHead(&ext->irp_queue); > + > /* add to list of pnp devices */ > pnp_dev->device = device; > list_add_tail(&pnp_devset, &pnp_dev->entry); > @@ -271,6 +281,8 @@ void bus_remove_hid_device(DEVICE_OBJECT *device) > { > struct device_extension *ext = (struct device_extension *)device->DeviceExtension; > struct pnp_device *pnp_device = ext->pnp_device; > + LIST_ENTRY *entry; > + IRP *irp; > > TRACE("(%p)\n", device); > > @@ -279,6 +291,21 @@ void bus_remove_hid_device(DEVICE_OBJECT *device) > LeaveCriticalSection(&device_list_cs); > > IoInvalidateDeviceRelations(device, RemovalRelations); > + > + /* Cancel pending IRPs */ > + entry = RemoveHeadList(&ext->irp_queue); > + while(entry != &ext->irp_queue) > + { > + irp = CONTAINING_RECORD(entry, IRP, Tail.Overlay.ListEntry); > + irp->IoStatus.u.Status = STATUS_CANCELLED; > + irp->IoStatus.Information = 0; > + IoCompleteRequest(irp, IO_NO_INCREMENT); > + entry = RemoveHeadList(&ext->irp_queue); > + } > + > + if (ext->processing_started) > + ext->vtbl->end_report_processing(device, ext->last_report); > + > HeapFree(GetProcessHeap(), 0, ext->serial); > IoDeleteDevice(device); > > @@ -432,6 +459,49 @@ NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) > irp->IoStatus.Information = (strlenW((WCHAR *)irp->UserBuffer) + 1) * sizeof(WCHAR); > break; > } > + case IOCTL_HID_GET_INPUT_REPORT: > + { > + HID_XFER_PACKET *packet = (HID_XFER_PACKET*)(irp->UserBuffer); > + TRACE_(hid_report)("IOCTL_HID_GET_INPUT_REPORT\n"); > + if (!ext->processing_started) > + { > + irp->IoStatus.u.Status = status = ext->vtbl->begin_report_processing(device, &ext->last_report, &ext->buffer_size); > + if (status == STATUS_SUCCESS) > + ext->processing_started = TRUE; > + else > + break; > + } We might need synchronization here to ensure that only one worker thread is started (if there are multiple IOCTL_HID_GET_INPUT_REPORT requests at the same time). > + > + if (packet->reportBufferLen < ext->last_report_size) > + { > + irp->IoStatus.u.Status = status = STATUS_BUFFER_TOO_SMALL; > + break; > + } > + > + ZeroMemory(packet->reportBuffer, packet->reportBufferLen); > + memcpy(packet->reportBuffer, ext->last_report, ext->last_report_size); The worker thread always uses the same buffer. How do you ensure that the "last_report" you are copying here is a complete record, and not corrupted (for example partially old data)? > + packet->reportBufferLen = ext->last_report_size; > + irp->IoStatus.Information = ext->last_report_size; > + irp->IoStatus.u.Status = status = STATUS_SUCCESS; > + break; > + } > + case IOCTL_HID_READ_REPORT: > + { > + TRACE_(hid_report)("IOCTL_HID_READ_REPORT\n"); > + if (!ext->processing_started) > + { > + if (ext->vtbl->begin_report_processing(device, &ext->last_report, &ext->buffer_size) == STATUS_SUCCESS) > + ext->processing_started = TRUE; > + else > + { > + irp->IoStatus.u.Status = status; > + break; > + } > + } > + InsertTailList(&ext->irp_queue, &irp->Tail.Overlay.ListEntry); The irp_queue will require locking when it is accessed from multiple threads. > + status = STATUS_PENDING; Thats fine for now, but I'm planning to send some additional patches soon to fix STATUS_PENDING handling. Drivers on Windows have to call IoMarkIrpPending() when they return STATUS_PENDING. Also see: https://msdn.microsoft.com/en-us/library/windows/hardware/ff549422(v=vs.85)… """Otherwise, the I/O manager attempts to complete the IRP as soon as the dispatch routine returns control.""" and """If a driver queues incoming IRPs, it should call IoMarkIrpPending before it queues each IRP. Otherwise, an IRP could be dequeued, completed by another driver routine, and freed by the system before the call to IoMarkIrpPending occurs, thereby causing a crash. """ > + break; > + } > default: > { > ULONG code = irpsp->Parameters.DeviceIoControl.IoControlCode; > @@ -441,11 +511,36 @@ NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) > } > } > > - IoCompleteRequest(irp, IO_NO_INCREMENT); > + if (status != STATUS_PENDING) > + IoCompleteRequest(irp, IO_NO_INCREMENT); > > return status; > } > > +void process_hid_report(DEVICE_OBJECT *device, const WCHAR *busidW, BYTE *report, DWORD length) > +{ > + struct device_extension *ext = (struct device_extension*)device->DeviceExtension; > + IRP *irp; > + LIST_ENTRY *entry; > + > + if (ext->last_report && report != ext->last_report && ext->buffer_size <= length) > + { > + memcpy(ext->last_report, report, length); > + ext->last_report_size = length; > + } > + entry = RemoveHeadList(&ext->irp_queue); > + while(entry != &ext->irp_queue) > + { > + TRACE_(hid_report)("Processing Request\n"); > + irp = CONTAINING_RECORD(entry, IRP, Tail.Overlay.ListEntry); > + memcpy(irp->UserBuffer, report, length); Shouldn't there also be some kind of length validation? > + irp->IoStatus.Information = length; > + irp->IoStatus.u.Status = STATUS_SUCCESS; > + IoCompleteRequest(irp, IO_NO_INCREMENT); > + entry = RemoveHeadList(&ext->irp_queue); > + } > +} > + > NTSTATUS WINAPI DriverEntry( DRIVER_OBJECT *driver, UNICODE_STRING *path ) > { > static const WCHAR udevW[] = {'\\','D','r','i','v','e','r','\\','U','D','E','V',0}; > > >

Oct. 17, 2016

Re: Imort system certificates in crypt32@Wine

by Jacek Caban

Hi Donat, On 10/16/16 9:34 PM, Donat Enikeev wrote: > Hi Jacek, > >> I agree that hardcoded paths are not nice, but if we don't have >> better generic solution, we have to live with that > > So, could expressing certificates into registry once and beforehand > during wine-prefix creation - be that one "better generic solution"? I > think I could spend some time on it, if that considered a right way to go This solution would change nothing in that regards - you still need hardcoded list of paths to do the import. You would just use it slightly differently. Also, I did not say to do that for wine prefix creation. User may intentionally remove a certificate in host system after wineserver is created and we should reflect that in Wine (not to mention moving wine prefix to another machine). That's why I suggested using REG_OPTION_VIOLATILE - so that registry keys would be gone when wineserver terminates (aka. Windows shutdown) and we'd recreate them the next time root store is created. And yes, I would consider such solution better. >> How about instead of current CRYPT_RootOpenStore call, we'd create >> registry entries expressing system certificates using >> REG_OPTION_VIOLATILE once for Wine session? > > That will work as well, but would require another custom wine-specific > piece of code for the certificates import using that flag for registry > keys OR passing special flag to the existing Store management functions. I'm not sure why you need more that we already have. We already have special handling for root store in CRYPT_SysRegOpenStoreW. You could just change it to call new import_system_certs function and continue like for other stores instead of calling CRYPT_RootOpenStore and returning. > In general, if sticking to the current way of implementation is the > most reasonable option in the context, just proper handling of flags > and priorities for collections is required (following Remarks at > https://msdn.microsoft.com/en-us/library/windows/desktop/aa376022(v=vs.85).… > ), so the collection will forward added certificates to the > appropriate store: registry one with RW access, not the memory based > created by CRYPT_RootOpenStore with RO access I would need deeper look at the code to comment that. However, note that with that we'd use registry stores that are already used for non-root cases. That said, I wouldn't be surprised if you find that existing code would needs fixes. Thanks, Jacek

Oct. 16, 2016

Re: Imort system certificates in crypt32@Wine

by Donat Enikeev

Hi Jacek, > I agree that hardcoded paths are not nice, but if we don't have > better generic solution, we have to live with that So, could expressing certificates into registry once and beforehand during wine-prefix creation - be that one "better generic solution"? I think I could spend some time on it, if that considered a right way to go > How about instead of current CRYPT_RootOpenStore call, we'd create > registry entries expressing system certificates using > REG_OPTION_VIOLATILE once for Wine session? That will work as well, but would require another custom wine-specific piece of code for the certificates import using that flag for registry keys OR passing special flag to the existing Store management functions. In general, if sticking to the current way of implementation is the most reasonable option in the context, just proper handling of flags and priorities for collections is required (following Remarks at https://msdn.microsoft.com/en-us/library/windows/desktop/aa376022(v=vs.85).… ), so the collection will forward added certificates to the appropriate store: registry one with RW access, not the memory based created by CRYPT_RootOpenStore with RO access Considering these, what do you think? Best, Donnie On Вс, окт 16, 2016 at 10:07 , Jacek Caban <jacek(a)codeweavers.com> wrote: > Hi Donat, > > > On 10/16/16 7:50 PM, Donat Enikeev wrote: >> Hi Guys, >> Watched video recently from WineConf 2015, and resulting directions >> around opening community looks really promising, so I am performing >> 3rd attempt to be useful here :) >> The bug https://bugs.winehq.org/show_bug.cgi?id=30187 : Cisco IP >> Communicator failing to setup due to 'certmgr.exe' tool fails to >> install certificate to the system-wide trusted certificate store. >> Wine uses registry-based certificates stores by default, but treats >> HKLM\Root store as a special case. Whenever app opens such store, >> Crypt32 goes through hard-coded paths in rootstore.c: >> static const char * const CRYPT_knownLocations[] = { >> "/etc/ssl/certs/ca-certificates.crt", "/etc/ssl/certs", ... }; >> And adds all found certificates to the special store. In the context >> of bug, this special-case store doesn't support adding certificates, >> and thus Cisco IP fails to install. >> So a backward compatible patch (attached) that just fixes this bug >> looks straightforward: make a stores collection, add there HKLM\Root >> certificates registry store at first and then that system store with >> certificates from the environment, and return the collection. It >> will allow applications do whatever they used to with certificates, >> keeping all the linux certificates available for verification of any >> kind. >> The problem with this approach is that current wine crypt32 doesn't >> actually save certificates that were added to a collection of stores >> (the test for this attached), while Win does. Although it could be >> fixed with a different patch in one function, >> but I would like to hear your thoughts first around following >> questions: >> 1. Does this backward-compatible patch-set make sense at all and >> worth proceeding? Probably you have some ongoing activities >> 2. Do you still think that wine should import system certificates >> during HKLM\Root request at all, not just shipping with those from >> typical windows installation? >> 3. Have you considered different approach of utilizing system >> certificates in Wine? For example, import all system certificates to >> the *registry* during wine-prefix initialization process, and work >> with them from there in a way windows does (even native crypt32 will >> benefit from this approach). That will allow to unify and simplify >> crypt32 and remove all that arguable hard-coded paths in the code, >> and bring more familiar environment to the windows application and, >> at the same time, isolation. > > First of all, I think we want to keep current solution of using host > system's cert store and configuration as much as possible. I agree > that hardcoded paths are not nice, but if we don't have better > generic solution, we have to live with that (although it's possible > to improve it in some cases, like it's done for Mac). > > That said, I think that expressing system certificates in Wine > registry would be the right solution. How about instead of current > CRYPT_RootOpenStore call, we'd create registry entries expressing > system certificates using REG_OPTION_VIOLATILE once for Wine session? > Then we could use regular registry store for root store. > > Thanks, > Jacek * > *

Oct. 16, 2016

Re: Imort system certificates in crypt32@Wine

by Jacek Caban

Hi Donat, On 10/16/16 7:50 PM, Donat Enikeev wrote: > Hi Guys, > Watched video recently from WineConf 2015, and resulting directions > around opening community looks really promising, so I am performing > 3rd attempt to be useful here :) > The bug https://bugs.winehq.org/show_bug.cgi?id=30187 : Cisco IP > Communicator failing to setup due to 'certmgr.exe' tool fails to > install certificate to the system-wide trusted certificate store. > Wine uses registry-based certificates stores by default, but treats > HKLM\Root store as a special case. Whenever app opens such store, > Crypt32 goes through hard-coded paths in rootstore.c: > static const char * const CRYPT_knownLocations[] = > { "/etc/ssl/certs/ca-certificates.crt", "/etc/ssl/certs", ... }; > And adds all found certificates to the special store. In the context > of bug, this special-case store doesn't support adding certificates, > and thus Cisco IP fails to install. > So a backward compatible patch (attached) that just fixes this bug > looks straightforward: make a stores collection, add there HKLM\Root > certificates registry store at first and then that system store with > certificates from the environment, and return the collection. It will > allow applications do whatever they used to with certificates, keeping > all the linux certificates available for verification of any kind. > The problem with this approach is that current wine crypt32 doesn't > actually save certificates that were added to a collection of stores > (the test for this attached), while Win does. Although it could be > fixed with a different patch in one function, > but I would like to hear your thoughts first around following questions: > 1. Does this backward-compatible patch-set make sense at all and worth > proceeding? Probably you have some ongoing activities > 2. Do you still think that wine should import system certificates > during HKLM\Root request at all, not just shipping with those from > typical windows installation? > 3. Have you considered different approach of utilizing system > certificates in Wine? For example, import all system certificates to > the *registry* during wine-prefix initialization process, and work > with them from there in a way windows does (even native crypt32 will > benefit from this approach). That will allow to unify and simplify > crypt32 and remove all that arguable hard-coded paths in the code, and > bring more familiar environment to the windows application and, at the > same time, isolation. First of all, I think we want to keep current solution of using host system's cert store and configuration as much as possible. I agree that hardcoded paths are not nice, but if we don't have better generic solution, we have to live with that (although it's possible to improve it in some cases, like it's done for Mac). That said, I think that expressing system certificates in Wine registry would be the right solution. How about instead of current CRYPT_RootOpenStore call, we'd create registry entries expressing system certificates using REG_OPTION_VIOLATILE once for Wine session? Then we could use regular registry store for root store. Thanks, Jacek * *

Oct. 16, 2016

Imort system certificates in crypt32@Wine

by Donat Enikeev

[Resending in plain/text, apologies for html] Hi Guys, Watched video recently from WineConf 2015, and results around opening community looks really promising, so I am performing 3rd attempt to be useful here :) The bug https://bugs.winehq.org/show_bug.cgi?id=30187 : Cisco IP Communicator failing to setup due to 'certmgr.exe' tool fails to install certificate to the system-wide trusted certificate store. Wine uses registry-based certificates stores by default, but treats HKLM\Root store as a special case: whenever app opens such store, Crypt32 goes through hard-coded paths in rootstore.c: static const char * const CRYPT_knownLocations[] = { "/etc/ssl/certs/ca-certificates.crt", "/etc/ssl/certs", ... }; And adds all found certificates to the special store. In the context of bug, this special-case store doesn't support adding certificates, and thus Cisco IP fails to install. So a backward compatible patch (attached) that just fixes this bug looks straightforward: make a stores collection, add there HKLM\Root certificates registry store first and then, add that system store with certificates from the environment. And return the resulting collection as a store to work with. It will allow applications do whatever they used to with certificates, keeping all the linux certificates available for verification of any kind. The problem with this approach is that current wine crypt32 doesn't actually save certificates that were added to a collection of stores (the test for this attached), while Win does. Although it could be fixed with a different patch in one function, but I would like to hear your thoughts first around following questions: 1. Does this backward-compatible patch-set make sense at all and worth proceeding? Probably you have some ongoing activities 2. Do you still think that wine should import system certificates during HKLM\Root request at all, not just shipping with those from typical windows installation? 3. Have you considered different approach of utilizing system certificates in Wine? For example, import all system certificates to the *registry* during wine-prefix initialization process, and work with them from there in a way windows does (even native crypt32 will benefit from this approach). That will allow to unify and simplify crypt32 and remove all that arguable hard-coded paths in the code, and bring more familiar environment to the windows application and, at the same time, isolation. Please share your thoughts, Donnie

Oct. 16, 2016

Re: mscoree: Add CorIsLatestSvc function stub.

by Sebastian Lackner

On 16.10.2016 17:06, Vincent Povirk wrote: >>> Have you found a real app that calls this? As far as I know, it's only >>> called by native .NET components that shouldn't be used with builtin >>> mscoree. >> >> At least two apps calls this function after .NET 4.5.1 (offline) have >> been installed. > > If .NET is installed then builtin mscoree should not be used. > > How exactly have you installed .NET 4.5.1? It sounds like you are missing some overrides. In case you did a manual installation, please note that mscoree has to be set to "native", not "native,builtin". In the second case builtin can still get loaded when apps look for mscoree at an unusual location, or if the app has the wrong architecture. Regards, Sebastian

Oct. 16, 2016