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

Re: [PATCH 1/4] mpr: Implement local name automatic redirection for disks.

by Pierre Schweitzer

I also modified 2nd patch of the patch, because it was suffering the same issue and wasn't applying any longer. Both have been resent. Le 28/07/2016 à 10:12, Alexandre Julliard a écrit : > Pierre Schweitzer <pierre(a)reactos.org> writes: > >> Hi, >> >> I wanted to avoid yet another recopy (in the ANSI case) especially since >> input is well-known: user necessarily provided NULL ptr to reach that point. >> But, if you prefer, I can do another copy. But in the ANSI case, it will >> mean that NETRESOURCE structure has been copied twice (rather expensive). > > It's not expensive, it's a small structure, and you don't need to > duplicate the pointers. > -- Pierre Schweitzer <pierre at reactos.org> System & Network Administrator Senior Kernel Developer ReactOS Deutschland e.V.

July 28, 2016

Re: [PATCH 1/4] mpr: Implement local name automatic redirection for disks.

by Alexandre Julliard

Pierre Schweitzer <pierre(a)reactos.org> writes: > Hi, > > I wanted to avoid yet another recopy (in the ANSI case) especially since > input is well-known: user necessarily provided NULL ptr to reach that point. > But, if you prefer, I can do another copy. But in the ANSI case, it will > mean that NETRESOURCE structure has been copied twice (rather expensive). It's not expensive, it's a small structure, and you don't need to duplicate the pointers. -- Alexandre Julliard julliard(a)winehq.org

July 28, 2016

Re: [PATCH 3/4] mpr: Implement WNetCancelConnection2W().

by Nikolay Sivov

On 27.07.2016 16:16, Pierre Schweitzer wrote: > > DWORD WINAPI WNetCancelConnection2W( LPCWSTR lpName, DWORD dwFlags, BOOL fForce ) > { > - FIXME( "(%s, %08X, %d), stub\n", debugstr_w(lpName), dwFlags, fForce ); I think it's better to keep this as TRACE, also maybe change lpName -> name, dwFlags -> flags, fForce -> force. Flags are unused, so maybe some fixme message for them is appropriate. > + DWORD ret = WN_NO_NETWORK; > + DWORD index; > > - return WN_SUCCESS; > + if (providerTable != NULL) > + { > + for (index = 0; index < providerTable->numProviders; index++) > + { > + if(providerTable->table[index].getCaps(WNNC_CONNECTION) & > + WNNC_CON_GETCONNECTIONS) Shouldn't this be WNNC_CON_CANCELCONNECTION? > + { > + if (providerTable->table[index].cancelConnection) > + ret = providerTable->table[index].cancelConnection((LPWSTR)lpName, fForce); > + else > + ret = WN_NO_NETWORK; > + if (ret == WN_SUCCESS || ret == WN_OPEN_FILES) > + break; > + } According to docs, WN_OPEN_FILES happens when force == FALSE, does it make sense to handle it only in this case? Of course we have no easy way to test any of that. > + } > + } > + return ret; > }

July 28, 2016

Re: [PATCH 1/4] mpr: Implement local name automatic redirection for disks.

by Pierre Schweitzer

Hi, I wanted to avoid yet another recopy (in the ANSI case) especially since input is well-known: user necessarily provided NULL ptr to reach that point. But, if you prefer, I can do another copy. But in the ANSI case, it will mean that NETRESOURCE structure has been copied twice (rather expensive). Cheers, Le 28/07/2016 à 09:50, Alexandre Julliard a écrit : > Pierre Schweitzer <pierre(a)reactos.org> writes: > >> +done: >> + if (redirect) >> + ctxt->resource->lpLocalName = NULL; > > It looks quite suspicious to modify that input parameter and restore it > at the end. You should probably use a copy instead. > -- Pierre Schweitzer <pierre at reactos.org> System & Network Administrator Senior Kernel Developer ReactOS Deutschland e.V.

July 28, 2016

Re: [PATCH 1/4] mpr: Implement local name automatic redirection for disks.

by Alexandre Julliard

Pierre Schweitzer <pierre(a)reactos.org> writes: > +done: > + if (redirect) > + ctxt->resource->lpLocalName = NULL; It looks quite suspicious to modify that input parameter and restore it at the end. You should probably use a copy instead. -- Alexandre Julliard julliard(a)winehq.org

July 28, 2016

Re: [PATCH 2/2] msvcp140: add sprintf

by Nikolay Sivov

> +#if _MSVCP_VER >= 140 > +int __cdecl sprintf(char *buf, const char *fmt, ...) > +{ > + int ret; > + __ms_va_list valist; > + __ms_va_start(valist, fmt); > + ret = __stdio_common_vsprintf(2, buf, -1, fmt, NULL, valist); > + __ms_va_end(valist); > + return ret; > +} > +#endif Are we missing patch 1/2 that adds msvcp140? Also won't it work the same if you forward it to MSVCRT_sprintf? If _common_ functions should actually be used, options should use existing private defines.

July 28, 2016

Re: [PATCH 3/3] ntdll: For Mac 64-bit, poke NtCurrentTeb()->ThreadLocalStoragePointer to the corresponding offset from %gs.

by Alexandre Julliard

Sebastian Lackner <sebastian(a)fds-team.de> writes: > It looks like RtlRunOnceExecuteOnce is not safe to use (keyed_event not initialized yet). > You could use pthread (like used before in init_teb_key), but probably moving to > a function is sufficient to improve code readability. By avoiding the heavily indented > code it would already look a bit less hacky. ;) Simplifying the error handling would probably also help. For instance, pthread_setspecific() is not going to fail in any meaningful way. -- Alexandre Julliard julliard(a)winehq.org

July 28, 2016

Re: [PATCH 2/6] d3drm: Implement IDirect3DRM*::CreateViewport. (v2 resend)

by Stefan Dösinger

On 2016-07-27 20:27, Aaryaman Vasishta wrote: > > > On Thu, Jul 28, 2016 at 12:21 AM, Stefan Dösinger > <stefandoesinger(a)gmail.com <mailto:stefandoesinger(a)gmail.com>> wrote: > > > + if (!device || !camera) > > + return D3DRMERR_BADOBJECT; > > + if (!viewport) > > + return D3DRMERR_BADVALUE; > These checks are redundant, Init() does that as well. > > Agreed for version 3 of CreateViewport, but this this apply for version > 1/2 as well? Since I'm dereferencing these interfaces there I thought > the checks were justified. No, in 1 and 2 you need them as you say. Otherwise you'll crash when you try to call QI.

July 27, 2016

Re: [PATCH 1/6] d3drm: Implement IDirect3DRMViewport*::Init. (v7)

by Aaryaman Vasishta

On Thu, Jul 28, 2016 at 12:53 AM, Henri Verbeet <hverbeet(a)gmail.com> wrote: > On 27 July 2016 at 21:09, Aaryaman Vasishta <jem456.vasishta(a)gmail.com> > wrote: > > On Thu, Jul 28, 2016 at 12:13 AM, Stefan Dösinger > >> > + if (material) > >> > + IDirect3DMaterial_Release(material); > >> This will destroy the material, viewport::SetBackground doesn't addref > it. > > > > Right, henri suggested the same. I'll move this outside the cleanup > label. > I did say the sequence was odd, but it does work because "material" is > set to NULL on successful viewport initialisation. On the other hand, > I just noticed you'll also leak the material if > IDirect3DViewport_SetBackground() fails. > Right, sorry to not mention about the sequence. I also noticed the leak while fixing the same. Cheers, Aaryaman

July 27, 2016

Re: [PATCH 2/6] d3drm: Implement IDirect3DRM*::CreateViewport. (v2 resend)

by Aaryaman Vasishta

On Thu, Jul 28, 2016 at 12:21 AM, Stefan Dösinger <stefandoesinger(a)gmail.com > wrote: > > + if (!device || !camera) > > + return D3DRMERR_BADOBJECT; > > + if (!viewport) > > + return D3DRMERR_BADVALUE; > These checks are redundant, Init() does that as well. > Agreed for version 3 of CreateViewport, but this this apply for version 1/2 as well? Since I'm dereferencing these interfaces there I thought the checks were justified. > > > + ok(ref4 == frame_ref, "Expected ref4 == frame_ref, got frame_ref = > %u, ref4 = %u.\n", frame_ref, ref4); > > + > > + device_ref = get_refcount((IUnknown *)device1); > > + frame_ref = get_refcount((IUnknown *)frame); > The get_refcount here seems redundant because you've just shown that it is > back to where it used to be. > Right, these are definitely redundant. Thanks for the review! Cheers, Aaryaman

July 27, 2016