Re: [ros-dev] [ros-diffs] [akhaldi] 64994: [ADVAPI32] * Update ImpersonateNamedPipeClient(). CORE-8540
It was wrong to address this to Amine, because it's not his code (neither old, nor new). Well, my question still stands of course, but it's up to the original author/maintainter of that code part to decide what to do with it :-) I'm not keen on that change, but if Eric likes it and it makes future work easier - pretty cool then. Regards, Aleksey On 26.10.2014 23:07, Aleksey Bragin wrote: Sometimes, If it ain't broke, don't fix it is a good thing. Or in other case, specify what does this sync actually fix. To me (and I'm known fan of Wine code, just look into Arwinss) it plainly looks like Winisation of our own, good code (the style, function behavior, whatever else matched ReactOS coding style, development practices, etc). Amine - please fix this any way you prefer. Thanks! Regards, Aleksey Bragin On 26.10.2014 0:10, Ged Murphy wrote: Eeww, these are a bit ugly :( On 25/10/2014 19:30, akha...@svn.reactos.org akha...@svn.reactos.org wrote: Author: akhaldi Date: Sat Oct 25 18:30:05 2014 New Revision: 64994 URL: http://svn.reactos.org/svn/reactos?rev=64994view=rev Log: [ADVAPI32] * Update ImpersonateNamedPipeClient(). CORE-8540 Modified: trunk/reactos/dll/win32/advapi32/wine/security.c Modified: trunk/reactos/dll/win32/advapi32/wine/security.c URL: http://svn.reactos.org/svn/reactos/trunk/reactos/dll/win32/advapi32/wine/s ecurity.c?rev=64994r1=64993r2=64994view=diff == --- trunk/reactos/dll/win32/advapi32/wine/security.c [iso-8859-1] (original) +++ trunk/reactos/dll/win32/advapi32/wine/security.c [iso-8859-1] Sat Oct 25 18:30:05 2014 @@ -954,37 +954,14 @@ return TRUE; } -/** - * ImpersonateNamedPipeClientEXPORTED - * - * @implemented - */ -BOOL -WINAPI -ImpersonateNamedPipeClient(HANDLE hNamedPipe) -{ -IO_STATUS_BLOCK StatusBlock; -NTSTATUS Status; - -TRACE(ImpersonateNamedPipeClient() called\n); - -Status = NtFsControlFile(hNamedPipe, - NULL, - NULL, - NULL, - StatusBlock, - FSCTL_PIPE_IMPERSONATE, - NULL, - 0, - NULL, - 0); -if (!NT_SUCCESS(Status)) -{ -SetLastError(RtlNtStatusToDosError(Status)); -return FALSE; -} - -return TRUE; +BOOL WINAPI ImpersonateNamedPipeClient( HANDLE hNamedPipe ) +{ +IO_STATUS_BLOCK io_block; + +TRACE((%p)\n, hNamedPipe); + +return set_ntstatus( NtFsControlFile(hNamedPipe, NULL, NULL, NULL, + io_block, FSCTL_PIPE_IMPERSONATE, NULL, 0, NULL, 0) ); } /* ___ Ros-dev mailing list Ros-dev@reactos.org http://www.reactos.org/mailman/listinfo/ros-dev
Re: [ros-dev] [ros-diffs] [akhaldi] 64994: [ADVAPI32] * Update ImpersonateNamedPipeClient(). CORE-8540
To be fair, it doesn't: static __inline BOOL set_ntstatus( NTSTATUS status ) { if (!NT_SUCCESS(status)) SetLastError( RtlNtStatusToDosError( status )); return NT_SUCCESS(status); } So essentially it's just refactoring. Regards, Aleksey On 26.10.2014 0:25, Pierre Schweitzer wrote: And it changes behavior of the functions. I'd rather trust a !NT_SUCESS(Status)) than a status. This means these functions with Wine code would fail on an informational status (whereas they didn't before). Is it intentional? Do we have tests to assess such behavior? It's a bit a shame to replace well written ReactOS to Wine code with broken style :-(. On 25/10/2014 22:10, Ged Murphy wrote: Eeww, these are a bit ugly :( On 25/10/2014 19:30, akha...@svn.reactos.org akha...@svn.reactos.org wrote: Author: akhaldi Date: Sat Oct 25 18:30:05 2014 New Revision: 64994 URL: http://svn.reactos.org/svn/reactos?rev=64994view=rev Log: [ADVAPI32] * Update ImpersonateNamedPipeClient(). CORE-8540 Modified: trunk/reactos/dll/win32/advapi32/wine/security.c Modified: trunk/reactos/dll/win32/advapi32/wine/security.c URL: http://svn.reactos.org/svn/reactos/trunk/reactos/dll/win32/advapi32/wine/s ecurity.c?rev=64994r1=64993r2=64994view=diff == --- trunk/reactos/dll/win32/advapi32/wine/security.c[iso-8859-1] (original) +++ trunk/reactos/dll/win32/advapi32/wine/security.c[iso-8859-1] Sat Oct 25 18:30:05 2014 @@ -954,37 +954,14 @@ return TRUE; } -/** - * ImpersonateNamedPipeClient EXPORTED - * - * @implemented - */ -BOOL -WINAPI -ImpersonateNamedPipeClient(HANDLE hNamedPipe) -{ -IO_STATUS_BLOCK StatusBlock; -NTSTATUS Status; - -TRACE(ImpersonateNamedPipeClient() called\n); - -Status = NtFsControlFile(hNamedPipe, - NULL, - NULL, - NULL, - StatusBlock, - FSCTL_PIPE_IMPERSONATE, - NULL, - 0, - NULL, - 0); -if (!NT_SUCCESS(Status)) -{ -SetLastError(RtlNtStatusToDosError(Status)); -return FALSE; -} - -return TRUE; +BOOL WINAPI ImpersonateNamedPipeClient( HANDLE hNamedPipe ) +{ +IO_STATUS_BLOCK io_block; + +TRACE((%p)\n, hNamedPipe); + +return set_ntstatus( NtFsControlFile(hNamedPipe, NULL, NULL, NULL, + io_block, FSCTL_PIPE_IMPERSONATE, NULL, 0, NULL, 0) ); } /* ___ Ros-dev mailing list Ros-dev@reactos.org http://www.reactos.org/mailman/listinfo/ros-dev
Re: [ros-dev] [ros-diffs] [akhaldi] 64994: [ADVAPI32] * Update ImpersonateNamedPipeClient(). CORE-8540
Sometimes, If it ain't broke, don't fix it is a good thing. Or in other case, specify what does this sync actually fix. To me (and I'm known fan of Wine code, just look into Arwinss) it plainly looks like Winisation of our own, good code (the style, function behavior, whatever else matched ReactOS coding style, development practices, etc). Amine - please fix this any way you prefer. Thanks! Regards, Aleksey Bragin On 26.10.2014 0:10, Ged Murphy wrote: Eeww, these are a bit ugly :( On 25/10/2014 19:30, akha...@svn.reactos.org akha...@svn.reactos.org wrote: Author: akhaldi Date: Sat Oct 25 18:30:05 2014 New Revision: 64994 URL: http://svn.reactos.org/svn/reactos?rev=64994view=rev Log: [ADVAPI32] * Update ImpersonateNamedPipeClient(). CORE-8540 Modified: trunk/reactos/dll/win32/advapi32/wine/security.c Modified: trunk/reactos/dll/win32/advapi32/wine/security.c URL: http://svn.reactos.org/svn/reactos/trunk/reactos/dll/win32/advapi32/wine/s ecurity.c?rev=64994r1=64993r2=64994view=diff == --- trunk/reactos/dll/win32/advapi32/wine/security.c[iso-8859-1] (original) +++ trunk/reactos/dll/win32/advapi32/wine/security.c[iso-8859-1] Sat Oct 25 18:30:05 2014 @@ -954,37 +954,14 @@ return TRUE; } -/** - * ImpersonateNamedPipeClient EXPORTED - * - * @implemented - */ -BOOL -WINAPI -ImpersonateNamedPipeClient(HANDLE hNamedPipe) -{ -IO_STATUS_BLOCK StatusBlock; -NTSTATUS Status; - -TRACE(ImpersonateNamedPipeClient() called\n); - -Status = NtFsControlFile(hNamedPipe, - NULL, - NULL, - NULL, - StatusBlock, - FSCTL_PIPE_IMPERSONATE, - NULL, - 0, - NULL, - 0); -if (!NT_SUCCESS(Status)) -{ -SetLastError(RtlNtStatusToDosError(Status)); -return FALSE; -} - -return TRUE; +BOOL WINAPI ImpersonateNamedPipeClient( HANDLE hNamedPipe ) +{ +IO_STATUS_BLOCK io_block; + +TRACE((%p)\n, hNamedPipe); + +return set_ntstatus( NtFsControlFile(hNamedPipe, NULL, NULL, NULL, + io_block, FSCTL_PIPE_IMPERSONATE, NULL, 0, NULL, 0) ); } /* ___ Ros-dev mailing list Ros-dev@reactos.org http://www.reactos.org/mailman/listinfo/ros-dev
Re: [ros-dev] [ros-diffs] [akhaldi] 64994: [ADVAPI32] * Update ImpersonateNamedPipeClient(). CORE-8540
Eeww, these are a bit ugly :( On 25/10/2014 19:30, akha...@svn.reactos.org akha...@svn.reactos.org wrote: Author: akhaldi Date: Sat Oct 25 18:30:05 2014 New Revision: 64994 URL: http://svn.reactos.org/svn/reactos?rev=64994view=rev Log: [ADVAPI32] * Update ImpersonateNamedPipeClient(). CORE-8540 Modified: trunk/reactos/dll/win32/advapi32/wine/security.c Modified: trunk/reactos/dll/win32/advapi32/wine/security.c URL: http://svn.reactos.org/svn/reactos/trunk/reactos/dll/win32/advapi32/wine/s ecurity.c?rev=64994r1=64993r2=64994view=diff == --- trunk/reactos/dll/win32/advapi32/wine/security.c [iso-8859-1] (original) +++ trunk/reactos/dll/win32/advapi32/wine/security.c [iso-8859-1] Sat Oct 25 18:30:05 2014 @@ -954,37 +954,14 @@ return TRUE; } -/** - * ImpersonateNamedPipeClient EXPORTED - * - * @implemented - */ -BOOL -WINAPI -ImpersonateNamedPipeClient(HANDLE hNamedPipe) -{ -IO_STATUS_BLOCK StatusBlock; -NTSTATUS Status; - -TRACE(ImpersonateNamedPipeClient() called\n); - -Status = NtFsControlFile(hNamedPipe, - NULL, - NULL, - NULL, - StatusBlock, - FSCTL_PIPE_IMPERSONATE, - NULL, - 0, - NULL, - 0); -if (!NT_SUCCESS(Status)) -{ -SetLastError(RtlNtStatusToDosError(Status)); -return FALSE; -} - -return TRUE; +BOOL WINAPI ImpersonateNamedPipeClient( HANDLE hNamedPipe ) +{ +IO_STATUS_BLOCK io_block; + +TRACE((%p)\n, hNamedPipe); + +return set_ntstatus( NtFsControlFile(hNamedPipe, NULL, NULL, NULL, + io_block, FSCTL_PIPE_IMPERSONATE, NULL, 0, NULL, 0) ); } /* ___ Ros-dev mailing list Ros-dev@reactos.org http://www.reactos.org/mailman/listinfo/ros-dev
Re: [ros-dev] [ros-diffs] [akhaldi] 64994: [ADVAPI32] * Update ImpersonateNamedPipeClient(). CORE-8540
And it changes behavior of the functions. I'd rather trust a !NT_SUCESS(Status)) than a status. This means these functions with Wine code would fail on an informational status (whereas they didn't before). Is it intentional? Do we have tests to assess such behavior? It's a bit a shame to replace well written ReactOS to Wine code with broken style :-(. On 25/10/2014 22:10, Ged Murphy wrote: Eeww, these are a bit ugly :( On 25/10/2014 19:30, akha...@svn.reactos.org akha...@svn.reactos.org wrote: Author: akhaldi Date: Sat Oct 25 18:30:05 2014 New Revision: 64994 URL: http://svn.reactos.org/svn/reactos?rev=64994view=rev Log: [ADVAPI32] * Update ImpersonateNamedPipeClient(). CORE-8540 Modified: trunk/reactos/dll/win32/advapi32/wine/security.c Modified: trunk/reactos/dll/win32/advapi32/wine/security.c URL: http://svn.reactos.org/svn/reactos/trunk/reactos/dll/win32/advapi32/wine/s ecurity.c?rev=64994r1=64993r2=64994view=diff == --- trunk/reactos/dll/win32/advapi32/wine/security.c [iso-8859-1] (original) +++ trunk/reactos/dll/win32/advapi32/wine/security.c [iso-8859-1] Sat Oct 25 18:30:05 2014 @@ -954,37 +954,14 @@ return TRUE; } -/** - * ImpersonateNamedPipeClient EXPORTED - * - * @implemented - */ -BOOL -WINAPI -ImpersonateNamedPipeClient(HANDLE hNamedPipe) -{ -IO_STATUS_BLOCK StatusBlock; -NTSTATUS Status; - -TRACE(ImpersonateNamedPipeClient() called\n); - -Status = NtFsControlFile(hNamedPipe, - NULL, - NULL, - NULL, - StatusBlock, - FSCTL_PIPE_IMPERSONATE, - NULL, - 0, - NULL, - 0); -if (!NT_SUCCESS(Status)) -{ -SetLastError(RtlNtStatusToDosError(Status)); -return FALSE; -} - -return TRUE; +BOOL WINAPI ImpersonateNamedPipeClient( HANDLE hNamedPipe ) +{ +IO_STATUS_BLOCK io_block; + +TRACE((%p)\n, hNamedPipe); + +return set_ntstatus( NtFsControlFile(hNamedPipe, NULL, NULL, NULL, + io_block, FSCTL_PIPE_IMPERSONATE, NULL, 0, NULL, 0) ); } /* ___ Ros-dev mailing list Ros-dev@reactos.org http://www.reactos.org/mailman/listinfo/ros-dev -- Pierre Schweitzer pierre at reactos.org System Network Administrator Senior Kernel Developer ReactOS Deutschland e.V. smime.p7s Description: S/MIME Cryptographic Signature ___ Ros-dev mailing list Ros-dev@reactos.org http://www.reactos.org/mailman/listinfo/ros-dev