Re: [PATCH 1/2] endproc.c: Implement ProcessPage_OnEndProcessTree

[Stefan Dösinger]

Hi Akarsha,

Thank you for your patches! My apologies for the very late review of
your code. I am currently catching up on my mail after moving :-\ .

Your patch has numerous compile errors. Please make sure that the code
compiles without errors or warnings before sending it.

Am 2017-03-30 um 19:57 schrieb Akarsha Sehwag:
> +typedef struct process_list {
> +    int         *pid;       /*dynamic array to store the process IDs*/
> MODULEENTRY32.th32ProcessID is a DWORD, please use it here as well.
> +    SIZE_T      count;      /*index to maintain the last entry of the array;*/
> +    SIZE_T      size;       /*the current size of the pid array*/
> +} process_list;
> Is it necessary to store the child processes in a separate list? I'd
> expect that CreateToolhelp32Snapshot creates a snapshot that is not
> affected by changes to the process tree after it returns. If this is the
> case you can kill processes as you iterate over the snapshot with
> Process32Next. This would simplify the code a lot

If you keep the list please use a wine list from include/wine/list.h.
This will also give you more flexibility in the order of how you kill
the processes, see below.
> +static void init_process_list(process_list *list) {
> +    list->size = 4;            /*initialise size with 4. Will increase if necessary.*/
> +    list->pid = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, size * sizeof(int));
> Minor style hint: You can use sizeof(*list->pid) instead of sizeof(int).
> That way the code doesn't need changes if the type of pid changes.
> +    if(!Process32First(snapshot, &entry))
> +        return;
> +
> +    do 
> +    {
> +        if(entry.th32ParentProcessID == pid)
> +            process_list_append(list, entry.th32ProcessID);
> +    } while (Process32Next(snapshot, &entry));
> +
> +    end = list->count;
> +
> +    for(i = start; i < end; ++i)
> +    {
> +        enum_process_children(snapshot, list, list->pid[i]);
> +    }
> I am not sure if this matters, but this will kill parent processes
> before their children. It may or may not be the correct thing and may be
> an interesting thing to find out with a test (unfortunately writing
> automated tests for taskmgr is not easy)

I think the same kind of recursion should work if you operate on the
snapshot directly without the helper list.
> +    for(i = 0; i <= list->last; ++i) {
> +        hProcess = OpenProcess(PROCESS_TERMINATE, FALSE, list->pid[i]);
>
> --- patch 2 ---
> -    for(i = 0; i <= list->last; ++i) {
> +    for(i = 0; i < list->count; ++i) {
> This should be merged into patch 1. It is important that the code always
> builds to prevent problems in git bisect (e.g. assume I am looking for a
> regression in the d3d code, but one of the revisions I want to test
> doesn't compile taskmgr)

With best regards,
Stefan

[PATCH 1/2] endproc.c: Implement ProcessPage_OnEndProcessTree

[Stefan Dösinger]

Am 2017-04-11 um 12:26 schrieb Stefan Dösinger:
> Is it necessary to store the child processes in a separate list? I'd
> expect that CreateToolhelp32Snapshot creates a snapshot that is not
> affected by changes to the process tree after it returns. If this is the
> case you can kill processes as you iterate over the snapshot with
> Process32Next. This would simplify the code a lot
> I've been made aware that Process32First/Next doesn't work with
> recursion as it has to store the current iteration position in the
> handle. So my idea here doesn't work :-\ .
> If you keep the list please use a wine list from include/wine/list.h.
> This will also give you more flexibility in the order of how you kill
> the processes, see below.
> And this idea would mean wrapping a DWORD into a struct list structure,
> which is a bit wasteful. So while I do not like custom-written list /
> array structures using the dynamic array here is probably the lesser evil.