Forgot to CC wine-devel..
On Fri, Apr 21, 2017 at 12:47 AM, Aaryaman Vasishta < jem456.vasishta@gmail.com> wrote:
Hi,
On Thu, Apr 20, 2017 at 6:51 AM, Akarsha Sehwag akarsha15010@iiitd.ac.in wrote:
Hi Could someone take out a little time to review the code once? Bug I intend to solve: https://bugs.winehq.org/show_bug.cgi?id=39640
I am new to wine. So, I would appreciate suggestions regarding my coding style if that's not-so-perfect according to wine standards. Or any other problems with the code I have written. I apologize for submitting an untested code earlier. I hope now it works fine.
No worries! your efforts in contributing is always appreciated :)
/*-------------------------------------------CODE-----------
---------------------------------------*/
Try using diffs/patches instead of manually copying them in this format. Diffs are easier to apply and test with using git. That's how Stefan was able to detect compilation errors in the patches you sent over on wine-patches. I suggest sending another email with the changes in a diif format if possible.
*//DOUBT: *Would sizeof(list->pid) be better or sizeof(int)? //Since in the former case, since list->pid is actually a pointer to DWORD, so //sizeof(list->pid) would return 8 bytes and not 4 bytes.
}
Probably sizeof(DWORD) would be fine as well. In general, try following what the surrounding, previously committed code is doing. FWIW, pointers don't necessarily have to be 8 bytes in size. The size of a pointer depends on whether your application is compiled as 32 or 64 bit. It's possible to e.g. compile a program as 32 bit on a 64 bit machine. For wine in 64 bit: https://wiki.winehq.org/FAQ#Is_there_a_64_bit_Wine.3F
Cheers, Aaryaman
On 20 April 2017 at 17:49, Aaryaman Vasishta jem456.vasishta@gmail.com wrote:
//DOUBT: Would sizeof(list->pid) be better or sizeof(int)? //Since in the former case, since list->pid is actually a pointer to DWORD, so //sizeof(list->pid) would return 8 bytes and not 4 bytes.
}
Probably sizeof(DWORD) would be fine as well. In general, try following what the surrounding, previously committed code is doing.
sizeof(list->pid) is indeed wrong, since that's the size of the "pid" pointer. sizeof(DWORD) would work, but ideally this would use "sizeof(*list->pid)". Agreed that diffs are the preferred format for review.
Hi I hope this is more readable and easier to review.
/*==========================CODE====================================*/ diff --git a/programs/taskmgr/endproc.c b/programs/taskmgr/endproc.c index 89c2d7b..b1a9242 100644 --- a/programs/taskmgr/endproc.c +++ b/programs/taskmgr/endproc.c @@ -31,18 +31,26 @@ #include "wine/unicode.h" #include "taskmgr.h" #include "perfdata.h" +#include "tlhelp32.h" +
static WCHAR wszWarnMsg[511]; static WCHAR wszWarnTitle[255]; static WCHAR wszUnable2Terminate[255];
+typedef struct process_list { + DWORD *pid; /*dynamic array to store the process IDs*/ + SIZE_T count; /*index to maintain the last entry of the array;*/ + SIZE_T size; /*the current size of the pid array*/ +} process_list; + static void load_message_strings(void) { LoadStringW(hInst, IDS_TERMINATE_MESSAGE, wszWarnMsg, sizeof(wszWarnMsg)/sizeof(WCHAR)); LoadStringW(hInst, IDS_TERMINATE_UNABLE2TERMINATE, wszUnable2Terminate, sizeof(wszUnable2Terminate)/sizeof(WCHAR)); LoadStringW(hInst, IDS_WARNING_TITLE, wszWarnTitle, sizeof(wszWarnTitle)/sizeof(WCHAR)); } - +
void ProcessPage_OnEndProcess(void) { LVITEMW lvitem; @@ -93,6 +101,57 @@ void ProcessPage_OnEndProcess(void) CloseHandle(hProcess); }
+ +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, list->size * sizeof(*(list->pid))); + list->count = 0; +} + +static void increase_list_size(process_list *list) { + list->size *= 2; + list->pid = HeapReAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, list->pid, list->size * sizeof(*(list->pid))); +} + +static void process_list_append(process_list *list, DWORD id) { + if(list->count == list->size) + increase_list_size(list); + list->pid[list->count] = id; + list->count += 1; +} + +static void free_process_list(process_list *list) { + HeapFree(GetProcessHeap(), 0, list->pid); +} + + +static void enum_process_children(HANDLE snapshot, process_list *list, DWORD pid) { + PROCESSENTRY32 entry; + + SIZE_T start, end, i; + + start = list->count; + + entry.dwSize = sizeof(entry); + + 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]); + } +} + + void ProcessPage_OnEndProcessTree(void) { LVITEMW lvitem; @@ -100,6 +159,9 @@ void ProcessPage_OnEndProcessTree(void) DWORD dwProcessId; HANDLE hProcess; WCHAR wstrErrorText[256]; + process_list list; + SIZE_T i; + HANDLE snapshot;
load_message_strings();
@@ -125,20 +187,40 @@ void ProcessPage_OnEndProcessTree(void) if (MessageBoxW(hMainWnd, wszWarnMsg, wszWarnTitle, MB_YESNO|MB_ICONWARNING) != IDYES) return;
- hProcess = OpenProcess(PROCESS_TERMINATE, FALSE, dwProcessId);
- if (!hProcess) - { - GetLastErrorText(wstrErrorText, sizeof(wstrErrorText)/sizeof(WCHAR)); - MessageBoxW(hMainWnd, wstrErrorText,wszUnable2Terminate, MB_OK|MB_ICONSTOP); + init_process_list(&list); + + if(list.pid == NULL) return; - }
- if (!TerminateProcess(hProcess, 0)) - { - GetLastErrorText(wstrErrorText, sizeof(wstrErrorText)/sizeof(WCHAR)); - MessageBoxW(hMainWnd, wstrErrorText,wszUnable2Terminate, MB_OK|MB_ICONSTOP); + process_list_append(&list, dwProcessId); + + snapshot = CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS, 0);//TH32SNAPPROCESS + + if(!snapshot) + return; + + enum_process_children(snapshot, &list, dwProcessId); + + CloseHandle(snapshot); + + for(i = 0; i < list.count; ++i) { + hProcess = OpenProcess(PROCESS_TERMINATE, FALSE, list.pid[i]);
+ + if (!hProcess) + { + GetLastErrorText(wstrErrorText, sizeof(wstrErrorText)/sizeof(WCHAR)); + MessageBoxW(hMainWnd, wstrErrorText,wszUnable2Terminate, MB_OK|MB_ICONSTOP); + break; + } + + if (!TerminateProcess(hProcess, 0)) + { + GetLastErrorText(wstrErrorText, sizeof(wstrErrorText)/sizeof(WCHAR)); + MessageBoxW(hMainWnd, wstrErrorText,wszUnable2Terminate, MB_OK|MB_ICONSTOP); + } + CloseHandle(hProcess); }
- CloseHandle(hProcess); -} + free_process_list(&list); +} \ No newline at end of file
Regards -- *Akarsha Sehwag* 2015010 Rang Coordinator Adventure Club Coordinator Sports Council | Ink member CSE Undergrad, IIIT Delhi
On Thu, Apr 20, 2017 at 9:19 PM, Aaryaman Vasishta < jem456.vasishta@gmail.com> wrote:
Forgot to CC wine-devel..
On Fri, Apr 21, 2017 at 12:47 AM, Aaryaman Vasishta < jem456.vasishta@gmail.com> wrote:
Hi,
On Thu, Apr 20, 2017 at 6:51 AM, Akarsha Sehwag <akarsha15010@iiitd.ac.in
wrote:
Hi Could someone take out a little time to review the code once? Bug I intend to solve: https://bugs.winehq.org/show_bug.cgi?id=39640
I am new to wine. So, I would appreciate suggestions regarding my coding style if that's not-so-perfect according to wine standards. Or any other problems with the code I have written. I apologize for submitting an untested code earlier. I hope now it works fine.
No worries! your efforts in contributing is always appreciated :)
/*-------------------------------------------CODE-----------
---------------------------------------*/
Try using diffs/patches instead of manually copying them in this format. Diffs are easier to apply and test with using git. That's how Stefan was able to detect compilation errors in the patches you sent over on wine-patches. I suggest sending another email with the changes in a diif format if possible.
*//DOUBT: *Would sizeof(list->pid) be better or sizeof(int)? //Since in the former case, since list->pid is actually a pointer to DWORD, so //sizeof(list->pid) would return 8 bytes and not 4 bytes.
}
Probably sizeof(DWORD) would be fine as well. In general, try following what the surrounding, previously committed code is doing. FWIW, pointers don't necessarily have to be 8 bytes in size. The size of a pointer depends on whether your application is compiled as 32 or 64 bit. It's possible to e.g. compile a program as 32 bit on a 64 bit machine. For wine in 64 bit: https://wiki.winehq.org/FAQ#Is_there_a_64_bit_Wine.3F
Cheers, Aaryaman
On 20 April 2017 at 20:10, Akarsha Sehwag akarsha15010@iiitd.ac.in wrote:
Hi I hope this is more readable and easier to review.
It looks like your mail client wrapped the patch. When using webmail in particular it's probably better to just attach the patch, although using git send-email would be best.
I think the patch is largely ok, but do have a few comments.
+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, list->size *
sizeof(*(list->pid)));
- list->count = 0;
+}
HEAP_ZERO_MEMORY is unnecessary, since "pid" entries are always initialised before being used in process_list_append().
+static void increase_list_size(process_list *list) {
- list->size *= 2;
- list->pid = HeapReAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, list->pid,
list->size * sizeof(*(list->pid))); +}
Although unlikely, HeapReAlloc() can fail and return NULL. It would be better to handle that possibility. Since increase_list_size() and process_list_append() are both small functions, I think it's worth considering merging increase_list_size() into process_list_append(), since that would probably simplify handling HeapReAlloc() failures. The comment about HEAP_ZERO_MEMORY in init_process_list() applies here as well.
@@ -125,20 +187,40 @@ void ProcessPage_OnEndProcessTree(void) if (MessageBoxW(hMainWnd, wszWarnMsg, wszWarnTitle, MB_YESNO|MB_ICONWARNING) != IDYES) return;
hProcess = OpenProcess(PROCESS_TERMINATE, FALSE, dwProcessId);
if (!hProcess)
{
GetLastErrorText(wstrErrorText,sizeof(wstrErrorText)/sizeof(WCHAR));
MessageBoxW(hMainWnd, wstrErrorText,wszUnable2Terminate,MB_OK|MB_ICONSTOP);
- init_process_list(&list);
- if(list.pid == NULL) return;
Although not necessarily wrong, I think checking "list.pid" here is a little awkward, since the possibility of it being NULL is an implementation detail of init_process_list(). I think it would be better to either make init_process_list() return an error and handle that here, or just inline init_process_list() and free_process_list() into their callers.
- snapshot = CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS,
0);//TH32SNAPPROCESS
- if(!snapshot)
return;
CreateToolhelp32Snapshot() returns INVALID_HANDLE_VALUE instead of NULL on failure. Returning here would leak "list.pid". Also, there seems to be a leftover comment.
- for(i = 0; i < list.count; ++i) {
hProcess = OpenProcess(PROCESS_TERMINATE, FALSE, list.pid[i]);if (!hProcess){GetLastErrorText(wstrErrorText,sizeof(wstrErrorText)/sizeof(WCHAR));
MessageBoxW(hMainWnd, wstrErrorText,wszUnable2Terminate,MB_OK|MB_ICONSTOP);
break;}if (!TerminateProcess(hProcess, 0)){GetLastErrorText(wstrErrorText,sizeof(wstrErrorText)/sizeof(WCHAR));
MessageBoxW(hMainWnd, wstrErrorText,wszUnable2Terminate,MB_OK|MB_ICONSTOP);
}CloseHandle(hProcess);
It may be worth considering introducing a function to terminate a process here. I.e.:
if (!kill_process(list.pid[i])) { GetLastErrorText(error_text, sizeof(error_text) / sizeof(*error_text)); MessageBoxW(hMainWnd, error_text, wszUnable2Terminate, MB_OK | MB_ICONSTOP); break; }
Such a function could be used by ProcessPage_OnEndProcess() as well.
Henri
Although unlikely, HeapReAlloc() can fail and return NULL. It would be better to handle that possibility. Since increase_list_size() and process_list_append() are both small functions, I think it's worth considering merging increase_list_size() into process_list_append(), since that would probably simplify handling HeapReAlloc() failures. The comment about HEAP_ZERO_MEMORY in init_process_list() applies here as well.
I didn't understand the function of the flags in HeapAlloc() and HeapReAlloc(). I read the function RtlAllocateHeap() but I am unable to figure out which flag to use out of HEAP_GENERATE_EXCEPTIONS, HEAP_NO_SERIALIZE and HEAP_ZERO_MEMORY.
All other changes done.* Diff file attached. *
On Thu, Apr 27, 2017 at 9:11 AM, Akarsha Sehwag akarsha15010@iiitd.ac.in wrote:
...
I didn't understand the function of the flags in HeapAlloc() and HeapReAlloc(). I read the function RtlAllocateHeap() but I am unable to figure out which flag to use out of HEAP_GENERATE_EXCEPTIONS, HEAP_NO_SERIALIZE and HEAP_ZERO_MEMORY.
When no flags are required you can always use 0.
diff --git a/programs/taskmgr/endproc.c b/programs/taskmgr/endproc.c index 89c2d7b..c890f06 100644 --- a/programs/taskmgr/endproc.c +++ b/programs/taskmgr/endproc.c @@ -31,17 +31,42 @@ #include "wine/unicode.h" #include "taskmgr.h" #include "perfdata.h" +#include "tlhelp32.h"
Please avoid double empty line.
static WCHAR wszWarnMsg[511]; static WCHAR wszWarnTitle[255]; static WCHAR wszUnable2Terminate[255];
+typedef struct process_list {
- DWORD *pid; /*dynamic array to store the process IDs*/
- SIZE_T count; /*index to maintain the last entry of the array;*/
- SIZE_T size; /*the current size of the pid array*/
+} process_list;
Please use a space between /* */ and the comments, but anyway the struct above is pretty self explanatory so there is no need to comment each field.
static void load_message_strings(void) { LoadStringW(hInst, IDS_TERMINATE_MESSAGE, wszWarnMsg, sizeof(wszWarnMsg)/sizeof(WCHAR)); LoadStringW(hInst, IDS_TERMINATE_UNABLE2TERMINATE, wszUnable2Terminate, sizeof(wszUnable2Terminate)/sizeof(WCHAR)); LoadStringW(hInst, IDS_WARNING_TITLE, wszWarnTitle, sizeof(wszWarnTitle)/sizeof(WCHAR)); }
+static void kill_process(HANDLE hProcess, WCHAR *wstrErrorText) +{
- if (!hProcess)
- {
GetLastErrorText(wstrErrorText, sizeof(wstrErrorText)/sizeof(WCHAR));MessageBoxW(hMainWnd, wstrErrorText,wszUnable2Terminate, MB_OK|MB_ICONSTOP);return;- }
- if (!TerminateProcess(hProcess, 0))
- {
GetLastErrorText(wstrErrorText, sizeof(wstrErrorText)/sizeof(WCHAR));MessageBoxW(hMainWnd, wstrErrorText,wszUnable2Terminate, MB_OK|MB_ICONSTOP);- }
+}
I known this was copy pasted from original code but please fix the missing space for ",wszUnable2Terminate".
void ProcessPage_OnEndProcess(void) { @@ -77,22 +102,71 @@ void ProcessPage_OnEndProcess(void)
hProcess = OpenProcess(PROCESS_TERMINATE, FALSE, dwProcessId);
- if (!hProcess)
- {
GetLastErrorText(wstrErrorText, sizeof(wstrErrorText)/sizeof(WCHAR));MessageBoxW(hMainWnd, wstrErrorText,wszUnable2Terminate, MB_OK|MB_ICONSTOP);return;
- kill_process(hProcess, wstrErrorText);
- CloseHandle(hProcess);
+}
Double empty line.
+static INT init_process_list(process_list *list) {
- list->size = 4; /*initialise size with 4. Will increase if necessary.*/
- list->pid = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, list->size * sizeof(*(list->pid)));
- list->count = 0;
- if(list->pid == NULL)
return 0;- return 1;
+}
Just change the HEAP_ZERO_MEMORY for 0. There is no need for extra parenthesis at "sizeof(*(list->pid))", so "sizeof(*list->pid)". Make the function BOOL and "return list->pid != NULL" instead of double return 0/1.
+static INT process_list_append(process_list *list, DWORD id) {
- if(list->count == list->size){
list->size *= 2;list->pid = HeapReAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, list->pid, list->size * sizeof(*(list->pid)));if(!list->pid)return 0;
The rest of the code uses { in the next line below the if, please do the same here. And add the missing space from "if(" to "if (". You need an intermediate variable to store the results of HeapReAlloc otherwise you will lose the original pointer in case of error.
- list->pid[list->count] = id;
- list->count += 1;
- return 1;
+}
Make the function BOOL. Minor style nitpick: use list->count++. You can also merge both lines: list->pid[list->count++] = id;
- if (!TerminateProcess(hProcess, 0))
+static void free_process_list(process_list *list) {
- HeapFree(GetProcessHeap(), 0, list->pid);
+}
Double empty line.
+static void enum_process_children(HANDLE snapshot, process_list *list, DWORD pid) {
- PROCESSENTRY32 entry;
- SIZE_T start, end, i;
"{" in the next line. Remove the empty line between variable declarations.
- start = list->count;
- entry.dwSize = sizeof(entry);
- if(!Process32First(snapshot, &entry))
return;- do
- {
if(entry.th32ParentProcessID == pid){if(!process_list_append(list, entry.th32ProcessID))return;}- } while (Process32Next(snapshot, &entry));
- end = list->count;
- for(i = start; i < end; ++i) {
GetLastErrorText(wstrErrorText, sizeof(wstrErrorText)/sizeof(WCHAR));MessageBoxW(hMainWnd, wstrErrorText,wszUnable2Terminate, MB_OK|MB_ICONSTOP);
enum_process_children(snapshot, list, list->pid[i]);
- CloseHandle(hProcess);
}
Double empty line.
void ProcessPage_OnEndProcessTree(void) { LVITEMW lvitem; @@ -100,6 +174,9 @@ void ProcessPage_OnEndProcessTree(void) DWORD dwProcessId; HANDLE hProcess; WCHAR wstrErrorText[256];
process_list list;
SIZE_T i;
HANDLE snapshot;
load_message_strings();
@@ -125,20 +202,32 @@ void ProcessPage_OnEndProcessTree(void) if (MessageBoxW(hMainWnd, wszWarnMsg, wszWarnTitle, MB_YESNO|MB_ICONWARNING) != IDYES) return;
hProcess = OpenProcess(PROCESS_TERMINATE, FALSE, dwProcessId);
if (!hProcess)
- if(!init_process_list(&list))
return;- if(!process_list_append(&list, dwProcessId))
return;
You are not freeing the list in case of error here.
- snapshot = CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS, 0);
- if(snapshot == INVALID_HANDLE_VALUE) {
GetLastErrorText(wstrErrorText, sizeof(wstrErrorText)/sizeof(WCHAR));MessageBoxW(hMainWnd, wstrErrorText,wszUnable2Terminate, MB_OK|MB_ICONSTOP);
free_process_list(&list); return;
- if (!TerminateProcess(hProcess, 0))
- {
GetLastErrorText(wstrErrorText, sizeof(wstrErrorText)/sizeof(WCHAR));MessageBoxW(hMainWnd, wstrErrorText,wszUnable2Terminate, MB_OK|MB_ICONSTOP);
- enum_process_children(snapshot, &list, dwProcessId);
- CloseHandle(snapshot);
- for(i = 0; i < list.count; ++i) {
hProcess = OpenProcess(PROCESS_TERMINATE, FALSE, list.pid[i]);kill_process(hProcess, wstrErrorText);CloseHandle(hProcess);
"{" in the next line, no need for empty lines between functions.
- CloseHandle(hProcess);
- free_process_list(&list);
}
Watch out for trailing space issues:
/home/bruno/Downloads/diff.txt:9: trailing whitespace. #include "tlhelp32.h" /home/bruno/Downloads/diff.txt:28: trailing whitespace.
/home/bruno/Downloads/diff.txt:43: trailing whitespace. } /home/bruno/Downloads/diff.txt:57: trailing whitespace.
/home/bruno/Downloads/diff.txt:100: trailing whitespace. entry.dwSize = sizeof(entry); warning: squelched 5 whitespace errors warning: 10 lines add whitespace errors.
Best wishes, Bruno
-
Aaryaman Vasishta -
Akarsha Sehwag -
Bruno Jesus -
Henri Verbeet