[PATCH resend] ntdll: Implement JobObjectBasicProcessIdList for NtQueryInformationJobObject.

[Gabriel Ivăncescu]

On 13/09/2021 11:37, Huw Davies wrote:
> On Thu, Aug 12, 2021 at 07:02:37PM +0300, Gabriel Ivăncescu wrote:
>> diff --git a/dlls/ntdll/unix/sync.c b/dlls/ntdll/unix/sync.c
>> index a13e53a..0594a5e 100644
>> --- a/dlls/ntdll/unix/sync.c
>> +++ b/dlls/ntdll/unix/sync.c
>> @@ -791,11 +791,36 @@ NTSTATUS WINAPI NtQueryInformationJobObject( HANDLE handle, JOBOBJECTINFOCLASS c
>>       case JobObjectBasicProcessIdList:
>>       {
>>           JOBOBJECT_BASIC_PROCESS_ID_LIST *process = info;
>> +        DWORD count, i;
>>   
>>           if (len < sizeof(*process)) return STATUS_INFO_LENGTH_MISMATCH;
>> -        memset( process, 0, sizeof(*process) );
>> -        if (ret_len) *ret_len = sizeof(*process);
>> -        return STATUS_SUCCESS;
>> +
>> +        count  = len - FIELD_OFFSET(JOBOBJECT_BASIC_PROCESS_ID_LIST, ProcessIdList);
>> +        count /= sizeof(process->ProcessIdList[0]);
>> +
>> +        SERVER_START_REQ( query_job_pids )
>> +        {
>> +            req->job = wine_server_user_handle(handle);
>> +            wine_server_set_reply(req, process->ProcessIdList, count * sizeof(process_id_t));
>> +            if ((ret = wine_server_call(req)) != STATUS_SUCCESS)
>> +                return ret;
> 
> Returning out of a SERVER_*_REQ block looks odd.
> 
>> +            process->NumberOfAssignedProcesses = reply->count;
>> +            process->NumberOfProcessIdsInList = min(count, reply->count);
>> +        }
>> +        SERVER_END_REQ;
>> +
>> +        if (sizeof(process_id_t) < sizeof(process->ProcessIdList[0]))
>> +        {
>> +            /* start from the end to not overwrite */
>> +            for (i = process->NumberOfProcessIdsInList; i--;)
>> +            {
>> +                ULONG_PTR id = ((process_id_t*)(&process->ProcessIdList[0]))[i];
> 
>                     ULONG_PTR id = ((process_id_t *)(process->ProcessIdList))[i];
> looks simpler to understand to me.
> 
>> +                memcpy(&process->ProcessIdList[i], &id, sizeof(id));
> 
>                     process->ProcessIdList[i] = id;
> should work just fine.
> 
>> +            }
>> +        }
>> +
>> +        if (ret_len) *ret_len = (char*)(&process->ProcessIdList[process->NumberOfProcessIdsInList]) - (char*)process;
>> +        return count < process->NumberOfAssignedProcesses ? STATUS_MORE_ENTRIES : STATUS_SUCCESS;
>>       }
>>       case JobObjectExtendedLimitInformation:
>>       {
>> diff --git a/server/process.c b/server/process.c
>> index 0870de5..48e632a 100644
>> --- a/server/process.c
>> +++ b/server/process.c
>> @@ -286,6 +286,24 @@ static int process_in_job( struct job *job, struct process *process )
>>       return process->job == job;
>>   }
>>   
>> +static data_size_t query_job_pids( struct job *job, data_size_t count, data_size_t maxcount, process_id_t *pids )
>> +{
>> +    struct process *process;
>> +    struct job *j;
>> +
>> +    LIST_FOR_EACH_ENTRY( j, &job->child_job_list, struct job, parent_job_entry )
>> +        count = query_job_pids( j, count, maxcount, pids );
>> +
>> +    LIST_FOR_EACH_ENTRY( process, &job->process_list, struct process, job_entry )
>> +    {
>> +        if (process->end_time) continue;
>> +        if (count < maxcount) pids[count] = process->id;
>> +        count++;
>> +    }
>> +
>> +    return count;
>> +}
>> +
>>   static void add_job_process( struct job *job, struct process *process )
>>   {
>>       struct job *j, *common_parent;
>> @@ -1741,6 +1759,22 @@ DECL_HANDLER(process_in_job)
>>       release_object( process );
>>   }
>>   
>> +/* get a list of the pids associated with the job */
>> +DECL_HANDLER(query_job_pids)
>> +{
>> +    struct job *job = get_job_obj( current->process, req->job, JOB_OBJECT_QUERY );
>> +    process_id_t *pids;
>> +    data_size_t len;
>> +
>> +    if (!job) return;
>> +
>> +    reply->count = query_job_pids( job, 0, 0, NULL );
> 
> Doesn't this just give you job->num_processes?
> 
>> +
>> +    len = min( get_reply_max_size(), reply->count * sizeof(*pids) );
>> +    if (len && ((pids = set_reply_data_size( len ))))
>> +        query_job_pids( job, 0, len / sizeof(*pids), pids );
>> +}
>> +
>>   /* retrieve information about a job */
>>   DECL_HANDLER(get_job_info)
>>   {
>> diff --git a/server/protocol.def b/server/protocol.def
>> index 133d6ad..5d97cb5 100644
>> --- a/server/protocol.def
>> +++ b/server/protocol.def
>> @@ -3671,6 +3671,15 @@ struct handle_info
>>   @END
>>   
>>   
>> +/* Query a list of pids associated with the job */
>> + at REQ(query_job_pids)
>> +    obj_handle_t job;             /* handle to the job */
>> + at REPLY
>> +    data_size_t count;            /* number of processes associated with the job */
>> +    VARARG(pids,uints);           /* list of pids */
>> + at END
>> +
> 
> Rather than introduce a new request, couldn't you extend the
> get_job_info request?
> 
> Huw.
> 

Hi Huw,

Thanks for the review. Indeed, I'm not very familiar with server code, I 
was worried for nothing that num_processes didn't include only the 
active processes, but you are absolutely right, I see it now in 
get_job_info—and extending it makes the code even simpler.

I'll send a new simpler version addressing all points.

Thanks,
Gabriel