Re: [PATCH v9 4/4] ntdll: Keep pagemap file open after first use of NtQueryVirtualMemory(MemoryWorkingSetExInformation)

[Zebediah Figura]

On 9/17/22 12:13, Witold Baryluk wrote:
> From: Witold Baryluk
>
> dlls/ntdll/unix/virtual.c | 19 +++++++++++--------
>   1 file changed, 11 insertions(+), 8 deletions(-)

This looks good, just a few nitpicks occur to me...
> diff --git a/dlls/ntdll/unix/virtual.c b/dlls/ntdll/unix/virtual.c
> index 58afc87375c..6dc2abf4666 100644
> --- a/dlls/ntdll/unix/virtual.c
> +++ b/dlls/ntdll/unix/virtual.c
> @@ -199,6 +199,10 @@ static BYTE **pages_vprot;
>   static BYTE *pages_vprot;
>   #endif
>
> +#if !defined(HAVE_LIBPROCSTAT)
> +static int pagemap_fd = -1;
> +#endif
> -

This doesn't actually need to be moved to global scope.
> static struct file_view *view_block_start, *view_block_end, *next_free_view;
>   static const size_t view_block_size = 0x100000;
>   static void *preload_reserve_start;
> @@ -4203,7 +4207,6 @@ static NTSTATUS get_working_set_ex( HANDLE process, LPCVOID addr,
>                                       MEMORY_WORKING_SET_EX_INFORMATION *info,
>                                       SIZE_T len, SIZE_T *res_len )
>   {
> - int pagemap_fd;
>  MEMORY_WORKING_SET_EX_INFORMATION *p;
>  sigset_t sigset;
>
> @@ -4266,14 +4269,16 @@ static NTSTATUS get_working_set_ex( HANDLE process, LPCVOID addr,
>               procstat_close( pstat );
>       }
>   #else
> - pagemap_fd = open( "/proc/self/pagemap", O_RDONLY, 0 );
> - if (pagemap_fd == -1)
> - server_enter_uninterrupted_section( &virtual_mutex, &sigset );
>  {
> -
>    static int once;
> -
>    if (!once++) WARN( "unable to open /proc/self/pagemap\n" );
> -
>    static int once = 0;

Static variables are implicitly initialized to zero. That said, you 
could get rid of "once" by using some other negative value as a sentinel 
for pagemap_fd (e.g. -2).
> -
>    if (once == 0 && pagemap_fd == -1) {

Braces go on the new line, as in surrounding code.
> -
>        once = 1;
> -
>        pagemap_fd = open( "/proc/self/pagemap", O_RDONLY, 0 );
> -
>        if (pagemap_fd == -1) WARN( "unable to open /proc/self/pagemap\n" );
> -
>    }
> }
>
> - server_enter_uninterrupted_section( &virtual_mutex, &sigset );
>  for (p = info; (UINT_PTR)(p + 1) <= (UINT_PTR)info + len; p++)
>  {
>      BYTE vprot;
>
> @@ -4304,8 +4309,6 @@ static NTSTATUS get_working_set_ex( HANDLE process, LPCVOID addr,
>       server_leave_uninterrupted_section( &virtual_mutex, &sigset );
>   #endif
> - if (pagemap_fd != -1)
> -
>    close( pagemap_fd );
> if (res_len)
>     *res_len = (UINT_PTR)p - (UINT_PTR)info;
> return STATUS_SUCCESS;