[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


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20170411/a2227e45/attachment.sig>