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
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.
-
Stefan Dösinger