February 2008 - Wine-Devel - list.winehq.org

Re: Code quality (was Re: comctl32: Fix invalid syntax)

by Marcel Partap

hi base, >> If winehq.org is Alexandre's own property, this is something >> fundametally wrong again. Well.. then 'something' must be wrong with the linux kernel aswell.. how comes noone forked? hmmm.. > You may try to learn how things work in the Wine project by reading > the following thread: > http://www.winehq.org/pipermail/wine-devel/2006-September/050915.html Although time consuming, it was rather a good read. While digesting the follow up 'governance ideas' thread I had a little idea that might help, though I doubt the problems discussed at that time are still that relevant. IMHO most of the wine-patches-black-hole problem is complexity of submitted patches, making them hard to review and rather lead to those patches being silently skipped in favor of the infinitly queuing later patches. One simple fix for this would be to recommend a maximum size for diffs on the developer wiki (additionally to the recommendation to focus on a small context per patch) and set up a wine-patches auto-reply for patches beyond that size, warning the submitter that because of sheer size (=complexity), the patch is likely to be skipped because of merge pressure of the following patches. That way, new contributors would get another visible warning to submit patches that are easier to review. Again, I can't estimate how much of an issue this is atm. I see a lot of patches commit or discussed on -devel, so it might not be at all. Anyways.. Another thing: after my important exam in march, I will probably try setting up a drupal site and see how it can be merged/ integrated with the wineforum, mailing lists and bugzilla.. there are some cool modules and regexp power waiting to be explored :) Last note: Jesse's DIB engine stuff, my work on printing aswell as Detlef's progress on sanitizing the spooler will hopefully show some results, too. Jesse I'll discuss that stuff with you next month be sure to be ready for some wine-patches resubmit cycles ;D regards, marcel. -- <div id="signature"> "Obstacles are those frightful things you see when you take your eyes off your goal." -- Henry Ford (1863-1947) Change the world! Vote revolution: http://hfopi.org/vote-future </div>

Feb. 8, 2008

Re: user32: Implement GetWindowModuleFileName with tests

by Dmitry Timoshkov

> + hproc = OpenProcess(PROCESS_QUERY_INFORMATION, 0, pid); After reading MSDN and guessing from the API name shouldn't it simply fetch GWL_HINSTANCE and call GetModuleFileName on it? -- Dmitry.

Feb. 8, 2008

Re: user32: Implement GetWindowModuleFileName with tests

by Reece Dunn

On 08/02/2008, Dmitry Timoshkov <dmitry(a)codeweavers.com> wrote: > "Reece Dunn" <msclrhd(a)googlemail.com> wrote: > >> What's the point of saving/restoring last error value? If WideCharToMultiByte > >> fails you need to return an error in that case, not silently continue. > > > > The usual model is something like this: > > > > SetLastError(0xdeadbeef); > > WinApiCall(); > > ok(GetLastError() == ERROR_SUCCESS, "WinApiCall failed, expected > > ERROR_SUCCESS, got %d", GetLastError()); > > In the tests, which is not the case here. Sorry, yes. You are correct. - Reece

Feb. 8, 2008

Re: user32: Implement GetWindowModuleFileName with tests

by Dmitry Timoshkov

"Reece Dunn" <msclrhd(a)googlemail.com> wrote: >> > + retval = GetWindowModuleFileNameW(hwnd, filenameW, cchFileNameMax); >> > + if (retval) >> > + { >> > + DWORD lasterror = GetLastError(); >> > + WideCharToMultiByte(CP_ACP, 0, filenameW, -1, lpszFileName, cchFileNameMax, NULL, NULL); >> > + SetLastError(lasterror); >> >> What's the point of saving/restoring last error value? If WideCharToMultiByte >> fails you need to return an error in that case, not silently continue. > > The usual model is something like this: > > SetLastError(0xdeadbeef); > WinApiCall(); > ok(GetLastError() == ERROR_SUCCESS, "WinApiCall failed, expected > ERROR_SUCCESS, got %d", GetLastError()); In the tests, which is not the case here. -- Dmitry.

Feb. 8, 2008

Re: user32: Implement GetWindowModuleFileName with tests

by Reece Dunn

On 08/02/2008, Dmitry Timoshkov <dmitry(a)codeweavers.com> wrote: > "Maarten Lankhorst" <m.b.lankhorst(a)gmail.com> wrote: > > > + retval = GetWindowModuleFileNameW(hwnd, filenameW, cchFileNameMax); > > + if (retval) > > + { > > + DWORD lasterror = GetLastError(); > > + WideCharToMultiByte(CP_ACP, 0, filenameW, -1, lpszFileName, cchFileNameMax, NULL, NULL); > > + SetLastError(lasterror); > > What's the point of saving/restoring last error value? If WideCharToMultiByte > fails you need to return an error in that case, not silently continue. The usual model is something like this: SetLastError(0xdeadbeef); WinApiCall(); ok(GetLastError() == ERROR_SUCCESS, "WinApiCall failed, expected ERROR_SUCCESS, got %d", GetLastError()); - Reece

Feb. 8, 2008

Re: user32: Implement GetWindowModuleFileName with tests

by Dmitry Timoshkov

"Maarten Lankhorst" <m.b.lankhorst(a)gmail.com> wrote: > UINT WINAPI GetWindowModuleFileNameA( HWND hwnd, LPSTR lpszFileName, UINT cchFileNameMax) > { > - FIXME("GetWindowModuleFileNameA(hwnd %p, lpszFileName %p, cchFileNameMax %u) stub!\n", > - hwnd, lpszFileName, cchFileNameMax); > - return 0; > + LPWSTR filenameW = NULL; > + UINT retval; > + > + if (cchFileNameMax) > + { > + filenameW = HeapAlloc(GetProcessHeap(), 0, cchFileNameMax); > + if (!filenameW) > + { > + SetLastError(ERROR_NOT_ENOUGH_MEMORY); > + return 0; > + } > + } Probably GetWindowModuleFileNameA should simply allocate a buffer of size MAX_PATH on the stack and fail with ERROR_FILENAME_EXCED_RANGE if unicode counterpart returns a string longer than MAX_PATH. kernel32 APIs behave that way. > + retval = GetWindowModuleFileNameW(hwnd, filenameW, cchFileNameMax); > + if (retval) > + { > + DWORD lasterror = GetLastError(); > + WideCharToMultiByte(CP_ACP, 0, filenameW, -1, lpszFileName, cchFileNameMax, NULL, NULL); > + SetLastError(lasterror); What's the point of saving/restoring last error value? If WideCharToMultiByte fails you need to return an error in that case, not silently continue. Also GetWindowModuleFileNameA also should be fixed to return correct value, and not assume that the buffer is '\0' terminated. > UINT WINAPI GetWindowModuleFileNameW( HWND hwnd, LPWSTR lpszFileName, UINT cchFileNameMax) > { > - FIXME("GetWindowModuleFileNameW(hwnd %p, lpszFileName %p, cchFileNameMax %u) stub!\n", > - hwnd, lpszFileName, cchFileNameMax); > - return 0; > + PROCESS_BASIC_INFORMATION pbi; > + DWORD pid, retval; > + HANDLE hproc; > + DWORD lasterror = GetLastError(); > + UNICODE_STRING *pathname; > + TRACE("GetWindowModuleFileNameW(hwnd %p, lpszFileName %p, cchFileNameMax %u)\n", hwnd, lpszFileName, > cchFileNameMax); TRACE/WARN/ERR/etc already print the caller's name, there is no need to duplicate it. > + if (!GetWindowThreadProcessId(hwnd, &pid)) > + { > + SetLastError(ERROR_INVALID_WINDOW_HANDLE); > + return 0; > + } GetWindowThreadProcessId already sets the error to the correct value, there is no need to guess why it failed. > + if (!cchFileNameMax) > + { /* Fail silently without setting filename */ > + SetLastError(lasterror); > + return 0; > + } Saving/restoring last error value doesn't look correct at all. > + hproc = OpenProcess(PROCESS_QUERY_INFORMATION, 0, pid); > + if (!hproc) > + { > + FIXME("Is this correct?"); > + SetLastError(ERROR_INVALID_WINDOW_HANDLE); > + return 0; > + } There is no need to guess why OpenProcess failed. > + retval = NtQueryInformationProcess(hproc, ProcessBasicInformation, &pbi, sizeof(pbi), NULL); > + pathname = &pbi.PebBaseAddress->ProcessParameters->ImagePathName; > + if (pathname->Length / sizeof(WCHAR) < cchFileNameMax) > + { > + memcpy(lpszFileName, pathname->Buffer, pathname->Length); > + lpszFileName[pathname->Length / sizeof(WCHAR)] = 0; > + retval = pathname->Length / sizeof(WCHAR) + 1; > + } > + else > + { > + memcpy(lpszFileName, pathname->Buffer, cchFileNameMax * sizeof(WCHAR)); > + retval = cchFileNameMax; > + } Both branches above should be merged, that will simplify the code. Just '\0' terminate the result if there is enough space in the provided buffer. > + CloseHandle(hproc); > + SetLastError(lasterror); > + FIXME("--> %u %s\n", retval, debugstr_w(lpszFileName)); Left-over from debugging? -- Dmitry.

Feb. 8, 2008

Re: kernel32: Implement ReplaceFileW

by Michael Stefaniuc

luis.busquets(a)ilidium.com wrote: > I have read this patch sent in several times and have realised that every > time it contains a large number of "goto"'s. > > I certainly do not like "goto" in structured code. > My question to the forum is if it is allowed to use "goto" in wine or if > you prefer not to use it. Wine takes the right (TM) approach to goto: the non ideological one: goto is fine for handling a lot and complex cleanups needed on error paths. You always need to balance the advantages and disadvantages of goto: does the goto make the code more readable and clear? If yes use the goto if not then don't. See also Donald Knuth paper "Structured Programming with Goto Statements" http://pplab.snu.ac.kr/courses/adv_pl05/papers/p261-knuth.pdf > Even when this will make the code larger but it may give a little nore > readability, No, it doesn't increase readability it decreases readability a lot. Especially as you have to replace "goto error:" with deeply nested if/else code and that is not readable at all. Using goto right is an art; you need to look at the code and say that it looks good. bye michael -- Michael Stefaniuc Tel.: +49-711-96437-199 Consulting Communications Engineer Fax.: +49-711-96437-111 -------------------------------------------------------------------- Reg. Adresse: Red Hat GmbH, Hauptstätter Strasse 58, 70178 Stuttgart Handelsregister: Amtsgericht Stuttgart HRB 153243 Geschäftsführer: Brendan Lane, Charlie Peters, Michael Cunningham, Werner Knoblich

Feb. 8, 2008

Re: Code quality (was Re: comctl32: Fix invalid syntax)

by Francois Gouget

On Fri, 8 Feb 2008, Bang Jun-young wrote: [...] > In fact, this is a well known mistake many newbie Win32 developers > make (and fix in minutes). It shouldn't have been in the tree in the > first place if he actually have read the patch. News flash!!! You too are allowed to review patches posted to wine-patches and to point out their flaws. Some people do so regularly and thus help Alexandre. If bad patches are accepted it's partly your fault. (and mine too cause I don't review patches enough) > There are a lot of easily catchable bugs in the tree, for example, > potential security holes like buffer overrun, meaningless comparison > of unsigned < 0 (or > 0), misuse of BOOL vs. HRESULT, misuse of > functions such as strcasecmp(), use of non-portable syntax, etc. You are welcome to review the Wine code and submit patches too. Again some others do so on a regular basis (me included this time :-). But if you don't want Wine to get better, then sure, just bitch about it. > That's the main reason why Wine keeps crashing every time I give it a > try with my Windows apps. <sarcasm> Riiiight. It must be the *main* cause of crashes. All the stub functions and unimplemented undocumented features must really be secondary causes of crashes in comparison. </sarcasm> -- Francois Gouget <fgouget(a)free.fr> http://fgouget.free.fr/ 1 + e ^ ( i * pi ) = 0

Feb. 8, 2008

Re: Patch tracking system - again...

by Kai Blin

On Friday 08 February 2008 08:51:15 Steven Edwards wrote: I'll skip the technical discussion and go right to the beef. > Is there even still the desire to have a graphical patch tracking system? Personally I tend to use feature branches in git that I rebase on the top of Alexandre's tree. Simply starting gitk will show me which of my patches have been accepted and which of them are still on top of the tree. Of course that means I need to track which of those I sent. Using more branches, this is even more clear. Assume you have two git branches, 'submitted' and 'my-cool-feature' You develop in 'my-cool-feature'. Once a patch is ready to be committed, you git-cherry-pick it to the 'submitted' branch, git-format-patch | git-imap-send or whatever you do to send it to wine-patches. Now you can keep track of your submitted patches easily. A web interface can be nice, so if anyone feels like hacking this up I'm definitely not going to stop them. All I want to say is that I think it's not needed for personal use. Cheers, Kai -- Kai Blin WorldForge developer http://www.worldforge.org/ Wine developer http://wiki.winehq.org/KaiBlin Samba team member http://www.samba.org/samba/team/ -- Will code for cotton.

Feb. 8, 2008

kernel32: Implement ReplaceFileW

by luis.busquets＠ilidium.com

I have read this patch sent in several times and have realised that every time it contains a large number of "goto"'s. I certainly do not like "goto" in structured code. My question to the forum is if it is allowed to use "goto" in wine or if you prefer not to use it. Even when this will make the code larger but it may give a little nore readability,

Feb. 8, 2008