Re: [ros-dev] [ros-diffs] [akhaldi] 64994: [ADVAPI32] * Update ImpersonateNamedPipeClient(). CORE-8540

[Ged Murphy]

Eeww, these are a bit ugly :(


On 25/10/2014 19:30, "akhaldi(a)svn.reactos.org" <akhaldi(a)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=64994&view=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=64994&r1=64993&r2=64994&view=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) );
>  }
>  
>  /*
>
>

[Pierre Schweitzer]

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, "akhaldi(a)svn.reactos.org" <akhaldi(a)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=64994&view=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=64994&r1=64993&r2=64994&view=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(a)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.

[Aleksey Bragin]

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, "akhaldi(a)svn.reactos.org"
> <akhaldi(a)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=64994&view=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=64994&r1=64993&r2=64994&view=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) );
> >> }
> >>
> >> /*
> >>
> >>
> >

[Aleksey Bragin]

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, "akhaldi(a)svn.reactos.org" <akhaldi(a)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=64994&view=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=64994&r1=64993&r2=64994&view=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) );
> >  }
> >
> >  /*
> >
> >  
>

[Aleksey Bragin]

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, "akhaldi(a)svn.reactos.org" <akhaldi(a)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=64994&view=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=64994&r1=64993&r2=64994&view=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) );
> > >  }
> > >
> > >  /*
> > >
> > >  
> >  
>