[PATCH 2/3] server: Add console process list ioctl.

[Roman Pišl]

Hi Jacek,
thank you for the review. I've submitted a second version of the 
patches, hopefully with your suggestions addressed.

Roman

Dne 08. 02. 22 v 22:32 Jacek Caban napsal(a):
> Hi Roman,
> 
> On 2/4/22 19:03, Roman Pišl wrote:
>> Signed-off-by: Roman Pišl <rpisl at seznam.cz>
>> ---
>>   include/wine/condrv.h |  1 +
>>   server/console.c      | 55 +++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 56 insertions(+)
>>
>> diff --git a/include/wine/condrv.h b/include/wine/condrv.h
>> index 4d2332a1ee9..13fb2c0b2c8 100644
>> --- a/include/wine/condrv.h
>> +++ b/include/wine/condrv.h
>> @@ -43,6 +43,7 @@
>>   #define IOCTL_CONDRV_BEEP                  
>> CTL_CODE(FILE_DEVICE_CONSOLE, 20, METHOD_BUFFERED, FILE_ANY_ACCESS)
>>   #define IOCTL_CONDRV_FLUSH                 
>> CTL_CODE(FILE_DEVICE_CONSOLE, 21, METHOD_BUFFERED, FILE_ANY_ACCESS)
>>   #define IOCTL_CONDRV_GET_WINDOW            
>> CTL_CODE(FILE_DEVICE_CONSOLE, 22, METHOD_BUFFERED, FILE_ANY_ACCESS)
>> +#define IOCTL_CONDRV_GET_PROCESS_LIST      
>> CTL_CODE(FILE_DEVICE_CONSOLE, 23, METHOD_BUFFERED, FILE_ANY_ACCESS)
>>   /* console output ioctls */
>>   #define IOCTL_CONDRV_WRITE_CONSOLE         
>> CTL_CODE(FILE_DEVICE_CONSOLE, 30, METHOD_BUFFERED, FILE_WRITE_ACCESS)
>> diff --git a/server/console.c b/server/console.c
>> index 5407fba1411..2df8fed5dfb 100644
>> --- a/server/console.c
>> +++ b/server/console.c
>> @@ -707,6 +707,27 @@ static void propagate_console_signal( struct 
>> console *console,
>>       enum_processes(propagate_console_signal_cb, &csi);
>>   }
>> +struct console_process_list
>> +{
>> +    DWORD size;
>> +    DWORD count;
>> +    DWORD *processes;
>> +    struct console *console;
>> +};
> 
> 
> Please avoid using DWORD for internal server fields (size and count).
> 
> 
>> +
>> +static int console_process_list_cb(struct process *process, void *user)
>> +{
>> +    struct console_process_list* cpl = (struct 
>> console_process_list*)user;
> 
> 
> The cast is not needed.
> 
> 
>> +
>> +    if (process->console == cpl->console)
>> +    {
>> +        if (cpl->count < cpl->size) cpl->processes[cpl->count] = 
>> process->id;
>> +        cpl->count++;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>   /* dumb dump */
>>   static void console_dump( struct object *obj, int verbose )
>>   {
>> @@ -963,6 +984,40 @@ static void console_ioctl( struct fd *fd, 
>> ioctl_code_t code, struct async *async
>>               return;
>>           }
>> +    case IOCTL_CONDRV_GET_PROCESS_LIST:
>> +        {
>> +            struct console_process_list cpl;
>> +            cpl.count = 0;
>> +            cpl.size = get_reply_max_size() / sizeof(DWORD);
>> +
>> +            if (cpl.size < 1)
>> +            {
>> +                set_error( STATUS_INVALID_PARAMETER );
>> +                return;
>> +            }
>> +            if (!console)
>> +            {
>> +                set_error( STATUS_INVALID_HANDLE );
>> +                return;
>> +            }
> 
> 
> There is no need to test for NULL console here.
> 
> 
>> +
>> +            cpl.console = console;
>> +            cpl.processes = (DWORD *) set_reply_data_size( 
>> get_reply_max_size() );
>> +            enum_processes( console_process_list_cb, &cpl );
>> +
>> +            if (cpl.count > cpl.size)
>> +            {
>> +                current->reply_size = sizeof(DWORD);
>> +                *cpl.processes = cpl.count;
>> +                set_error( STATUS_BUFFER_TOO_SMALL );
>> +                return;
>> +            }
>> +
>> +            current->reply_size = cpl.count * sizeof(DWORD);
> 
> 
> over-allocating result and direct manipulation of reply_size does not 
> really look nice. The usual practice would be to test size first and 
> allocate appropriate result when we know its size. In this case, it's 
> probably easiest if you do another enum_processes() call first, just to 
> calculate result size (another solution would be to keep track of 
> attached processes in console object itself).
> 
> 
> Thanks,
> 
> Jacek
>