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

Re: include: Add an errno number

by Alexandre Julliard

André Hentschel <nerv(a)dawncrow.de> writes: > @@ -23,6 +23,7 @@ > > # define EPERM 1 > # define ENOENT 2 > +# define ENOFILE ENOENT I don't see that in the MS headers. -- Alexandre Julliard julliard(a)winehq.org

Feb. 23, 2011

Re: [PATCH 1/2] user32: make DIB_GetBitmapInfo checks stricter

by Alexandre Julliard

Wolfram Sang <wolfram(a)the-dreams.de> writes: > @@ -988,6 +988,9 @@ static void test_LoadImage(void) > bmpimage[0x18]++; /* biWidth > 65535 */ > test_LoadImageFile("BMP (too wide)", bmpimage, sizeof(bmpimage), "bmp", 0); > bmpimage[0x18]--; > + bmpimage[0x11]++; /* biSize > 2^24 */ > + test_LoadImageFile("BMP (broken biSize)", bmpimage, sizeof(bmpimage), "bmp", 0); > + bmpimage[0x11]--; That's not a very convincing test. You should test more interesting sizes like intermediate values, off-by-one, etc. -- Alexandre Julliard julliard(a)winehq.org

Feb. 23, 2011

Re: avicap32: Use define instead of hardcoded value

by André Hentschel

sry, please ignore that one. -- Best Regards, André Hentschel

Feb. 23, 2011

Re: comctl32: Add missing break (Coverity)

by Michael Stefaniuc

Nikolay Sivov wrote: > On 2/23/2011 19:56, Michael Stefaniuc wrote: >> Nikolay Sivov wrote: >>> On 2/23/2011 19:28, Amine Khaldi wrote: >>>> CIDs 1581 and 1583. >>>> @@ -1852,6 +1852,7 @@ COMBOEX_EditWndProc (HWND hwnd, UINT uMsg, >>>> WPARAM wParam, LPARAM lParam, >>>> >>>> case VK_UP: >>>> step = -1; >>>> + break; >>>> case VK_DOWN: >>>> /* by default, step is 1 */ >>>> oldItem = SendMessageW (infoPtr->hwndSelf, CB_GETCURSEL, >>>> 0, 0); >>> This is wrong. >>> >>>> @@ -2297,6 +2297,7 @@ static LRESULT >>>> TOOLBAR_Cust_AvailDragListNotification(const CUSTDLG_INFO *custIn >>>> TOOLBAR_Cust_AddButton(custInfo, hwnd, nIndexFrom, >>>> nIndexTo); >>>> } >>>> } >>>> + break; >>>> case DL_CANCELDRAG: >>>> /* Clear drag arrow */ >>>> DrawInsert(hwnd, hwndList, -1); >>> Why? Looks to me it's fine to clear on dropped case too. Coverity is a >>> bit paranoid about missed breaks. >> A missing break is not trivial to detect; especially if something is >> done in a specific case. That's why Wine has a ton of "/* fall through >> */" annotations. IMHO they should be added here too . > Well, there's no third option basing on code - report all or report > none, cause you can't figure out automatically is it ok to fall or not. > A comment you mean is kind of suppression sign for checker?. Speaking I don't really care about checkers but about people reading the code. Adding code to silence a checker is bad taste; adding a comment to improve the readability of the code is good. > about this particular case, I have a better comboex fix in mind, and for > toolbar I think it's fine to add a comment. Cool, thanks. bye michael

Feb. 23, 2011

Re: comctl32: Add missing break (Coverity)

by Marcus Meissner

On Wed, Feb 23, 2011 at 08:24:46PM +0300, Nikolay Sivov wrote: > >>> } > >>>+ break; > >>> case DL_CANCELDRAG: > >>> /* Clear drag arrow */ > >>> DrawInsert(hwnd, hwndList, -1); > >>Why? Looks to me it's fine to clear on dropped case too. Coverity is a > >>bit paranoid about missed breaks. > >A missing break is not trivial to detect; especially if something is > >done in a specific case. That's why Wine has a ton of "/* fall through > >*/" annotations. IMHO they should be added here too . > Well, there's no third option basing on code - report all or report > none, cause you can't figure out automatically is it ok to fall or > not. A comment you mean is kind of suppression sign for checker?. > Speaking about this particular case, I have a better comboex fix in > mind, and for toolbar I think it's fine to add a comment. lint used /* FALLTHROUGH */ as a tag. Not sure if Coverity uses it, but we could use somesuch for tagging such places. Ciao, Marcus

Feb. 23, 2011

Re: comctl32: Add missing break (Coverity)

by Nikolay Sivov

On 2/23/2011 19:56, Michael Stefaniuc wrote: > Nikolay Sivov wrote: >> On 2/23/2011 19:28, Amine Khaldi wrote: >>> CIDs 1581 and 1583. >>> @@ -1852,6 +1852,7 @@ COMBOEX_EditWndProc (HWND hwnd, UINT uMsg, >>> WPARAM wParam, LPARAM lParam, >>> >>> case VK_UP: >>> step = -1; >>> + break; >>> case VK_DOWN: >>> /* by default, step is 1 */ >>> oldItem = SendMessageW (infoPtr->hwndSelf, CB_GETCURSEL, 0, 0); >> This is wrong. >> >>> @@ -2297,6 +2297,7 @@ static LRESULT >>> TOOLBAR_Cust_AvailDragListNotification(const CUSTDLG_INFO *custIn >>> TOOLBAR_Cust_AddButton(custInfo, hwnd, nIndexFrom, >>> nIndexTo); >>> } >>> } >>> + break; >>> case DL_CANCELDRAG: >>> /* Clear drag arrow */ >>> DrawInsert(hwnd, hwndList, -1); >> Why? Looks to me it's fine to clear on dropped case too. Coverity is a >> bit paranoid about missed breaks. > A missing break is not trivial to detect; especially if something is > done in a specific case. That's why Wine has a ton of "/* fall through > */" annotations. IMHO they should be added here too . Well, there's no third option basing on code - report all or report none, cause you can't figure out automatically is it ok to fall or not. A comment you mean is kind of suppression sign for checker?. Speaking about this particular case, I have a better comboex fix in mind, and for toolbar I think it's fine to add a comment. > bye > michael >

Feb. 23, 2011

Re: comctl32: Add missing break (Coverity)

by Michael Stefaniuc

Nikolay Sivov wrote: > On 2/23/2011 19:28, Amine Khaldi wrote: >> CIDs 1581 and 1583. >> @@ -1852,6 +1852,7 @@ COMBOEX_EditWndProc (HWND hwnd, UINT uMsg, >> WPARAM wParam, LPARAM lParam, >> >> case VK_UP: >> step = -1; >> + break; >> case VK_DOWN: >> /* by default, step is 1 */ >> oldItem = SendMessageW (infoPtr->hwndSelf, CB_GETCURSEL, 0, 0); > This is wrong. > >> @@ -2297,6 +2297,7 @@ static LRESULT >> TOOLBAR_Cust_AvailDragListNotification(const CUSTDLG_INFO *custIn >> TOOLBAR_Cust_AddButton(custInfo, hwnd, nIndexFrom, >> nIndexTo); >> } >> } >> + break; >> case DL_CANCELDRAG: >> /* Clear drag arrow */ >> DrawInsert(hwnd, hwndList, -1); > Why? Looks to me it's fine to clear on dropped case too. Coverity is a > bit paranoid about missed breaks. A missing break is not trivial to detect; especially if something is done in a specific case. That's why Wine has a ton of "/* fall through */" annotations. IMHO they should be added here too . bye michael

Feb. 23, 2011

Re: d3dcompiler_43: Add missing break (Coverity)

by Matteo Bruni

2011/2/23 Amine Khaldi <amine.khaldi(a)reactos.org>: > CID 1585. > > Regards, > Amine. > This doesn't seem to be the correct fix. With that break you aren't going to return any string and that will cause problems thereafter. You should replace the "case 2:" with "default:" instead (like BWRITERSPR_RASTOUT some rows above). BTW, thanks for the patches!

Feb. 23, 2011

Re: gdi32: first look for a printer driver name in the registry (try 3)

by Dmitry Timoshkov

Vitaly Perov <vitperov(a)etersoft.ru> wrote: > + if (!hKey) RegCloseKey(hKey); This is a typo. -- Dmitry.

Feb. 23, 2011

Re: [PATCH] user32: remove side effect code out of asserts() (Coverity)

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at http://testbot.winehq.org/JobDetails.pl?Key=9432 Your paranoid android. === WINEBUILD (build) === Make failed

Feb. 23, 2011

Re: comctl32: Add missing break (Coverity)

by Nikolay Sivov

On 2/23/2011 19:28, Amine Khaldi wrote: > CIDs 1581 and 1583. > @@ -1852,6 +1852,7 @@ COMBOEX_EditWndProc (HWND hwnd, UINT uMsg, > WPARAM wParam, LPARAM lParam, > > case VK_UP: > step = -1; > + break; > case VK_DOWN: > /* by default, step is 1 */ > oldItem = SendMessageW (infoPtr->hwndSelf, CB_GETCURSEL, 0, 0); This is wrong. > @@ -2297,6 +2297,7 @@ static LRESULT > TOOLBAR_Cust_AvailDragListNotification(const CUSTDLG_INFO *custIn > TOOLBAR_Cust_AddButton(custInfo, hwnd, nIndexFrom, nIndexTo); > } > } > + break; > case DL_CANCELDRAG: > /* Clear drag arrow */ > DrawInsert(hwnd, hwndList, -1); Why? Looks to me it's fine to clear on dropped case too. Coverity is a bit paranoid about missed breaks. Also don't include completely unrelated changes to one patch. > > Regards, > Amine. > > >

Feb. 23, 2011

Re: comctl32: Add missing break (Coverity)

by Marcus Meissner

On Wed, Feb 23, 2011 at 05:28:28PM +0100, Amine Khaldi wrote: > CIDs 1581 and 1583. > > Regards, > Amine. the comboex one seems to be intended to fall through to the next case. Ciao, Marcus

Feb. 23, 2011

Re: [PATCH] cryptui: fixed bad sizeof() (Coverity)

by Marcus Meissner

On Wed, Feb 23, 2011 at 07:29:41AM -0700, Vitaliy Margolen wrote: > On 02/22/2011 09:30 PM, Marcus Meissner wrote: > >Hi, > > > >CID 1181. copying a pointer to a struct into a struct is likely bad, > >fixed. > > > >- memcpy(&infoW,&info, sizeof(info)); > >+ memcpy(&infoW, info, sizeof(*info)); > Should probably use direct struct copying instead: > infoW = *info; Wont work, except if we cast the type.... :/ infoW is the _W struct, info a ptr to _A struct (both are the same size however). Ciao, Marcus

Feb. 23, 2011

Re: [PATCH] cryptui: fixed bad sizeof() (Coverity)

by Vitaliy Margolen

On 02/22/2011 09:30 PM, Marcus Meissner wrote: > Hi, > > CID 1181. copying a pointer to a struct into a struct is likely bad, > fixed. > > - memcpy(&infoW,&info, sizeof(info)); > + memcpy(&infoW, info, sizeof(*info)); Should probably use direct struct copying instead: infoW = *info; Vitaliy.

Feb. 23, 2011

Re: [2/5] msxml3: Remove appendChild() forward (try2)

by Jacek Caban

On 2/23/11 1:27 PM, Nikolay Sivov wrote: > On 2/23/2011 15:25, Jacek Caban wrote: >> On 2/23/11 1:21 PM, Nikolay Sivov wrote: >>> - TRACE("(%p)->(%p %p)\n", This, newChild, outNewChild); >>> - >>> - hr = IXMLDOMNode_get_nodeType(newChild,&type); >>> + hr = IXMLDOMNode_get_nodeType(child,&type); >>> if(FAILED(hr) || type == NODE_ATTRIBUTE) { >>> - if(outNewChild) *outNewChild = NULL; >>> + if (outChild) *outChild = NULL; >>> return E_FAIL; >>> } >> >> This check for an attribute node shouldn't be needed here. Attribute >> node may just return failure instead of forwarding the call. Also I'd >> expect the same from other nodes that can't have children, like text >> node. > It's a check for a child, not for a This node. Oh, right, sorry for too quick response. Jacek

Feb. 23, 2011

Re: [2/5] msxml3: Remove appendChild() forward (try2)

by Nikolay Sivov

On 2/23/2011 15:25, Jacek Caban wrote: > On 2/23/11 1:21 PM, Nikolay Sivov wrote: >> - TRACE("(%p)->(%p %p)\n", This, newChild, outNewChild); >> - >> - hr = IXMLDOMNode_get_nodeType(newChild,&type); >> + hr = IXMLDOMNode_get_nodeType(child,&type); >> if(FAILED(hr) || type == NODE_ATTRIBUTE) { >> - if(outNewChild) *outNewChild = NULL; >> + if (outChild) *outChild = NULL; >> return E_FAIL; >> } > > This check for an attribute node shouldn't be needed here. Attribute > node may just return failure instead of forwarding the call. Also I'd > expect the same from other nodes that can't have children, like text > node. It's a check for a child, not for a This node. > > Jacek

Feb. 23, 2011

Re: [2/5] msxml3: Remove appendChild() forward (try2)

by Jacek Caban

On 2/23/11 1:21 PM, Nikolay Sivov wrote: > - TRACE("(%p)->(%p %p)\n", This, newChild, outNewChild); > - > - hr = IXMLDOMNode_get_nodeType(newChild,&type); > + hr = IXMLDOMNode_get_nodeType(child,&type); > if(FAILED(hr) || type == NODE_ATTRIBUTE) { > - if(outNewChild) *outNewChild = NULL; > + if (outChild) *outChild = NULL; > return E_FAIL; > } This check for an attribute node shouldn't be needed here. Attribute node may just return failure instead of forwarding the call. Also I'd expect the same from other nodes that can't have children, like text node. Jacek

Feb. 23, 2011

Re: [1/5] msxml3: Remove forward to removeChild() method for nodes

by Nikolay Sivov

On 2/23/2011 03:33, Nikolay Sivov wrote: > Remove forward to removeChild() method for nodes > > Ignore that please, I'll resend all 5 patches soon.

Feb. 23, 2011

Coverity Scans Revived

by Amine Khaldi

Hello folks, Recently we (myself and Urias from Haiku) have been working on getting the coverity scans up and running again. I'm happy to announce that we succeeded ! :) I submitted a scan and the results are up online. I'll submit further scans when necessary, like it's already done with clang analyzer scans. Have fun ! Regards, Amine.

Feb. 23, 2011

Re: kernel32/tests: remove win9x hacks (try 2)

by Damjan Jovanovic

On Wed, Feb 23, 2011 at 11:38 AM, Austin English <austinenglish(a)gmail.com> wrote: > -- > -Austin > > > > So test.winehq.org doesn't test Win9x any more, but why are we throwing away perfectly good Win9x tests that took years to get in?

Feb. 23, 2011

Re: kernel32/tests: remove win9x hacks

by Austin English

On Wed, Feb 23, 2011 at 01:32, Paul Vriens <paul.vriens.wine(a)gmail.com> wrote: > On 02/23/2011 10:15 AM, Austin English wrote: >> >> SetLastError(0xdeadbeef); >> ret = GetFullPathNameW(NULL, 0, NULL, NULL); > > I think you can get rid of the above two as well as they are merely used to > trigger the ERROR_CALL_NOT_IMPLEMENTED. > >> - is_win9x = !ret&& GetLastError() == ERROR_CALL_NOT_IMPLEMENTED; >> - >> - if (is_win9x) >> - win_skip("Skipping some tests that cause GetFullPathNameA to >> crash on Win9x\n"); Good catch, will resend, thanks. -- -Austin

Feb. 23, 2011

Re: kernel32/tests: remove win9x hacks

by Paul Vriens

On 02/23/2011 10:15 AM, Austin English wrote: > SetLastError(0xdeadbeef); > ret = GetFullPathNameW(NULL, 0, NULL, NULL); I think you can get rid of the above two as well as they are merely used to trigger the ERROR_CALL_NOT_IMPLEMENTED. > - is_win9x = !ret&& GetLastError() == ERROR_CALL_NOT_IMPLEMENTED; > - > - if (is_win9x) > - win_skip("Skipping some tests that cause GetFullPathNameA to crash on Win9x\n"); -- Cheers, Paul.

Feb. 23, 2011

New winetricks-alpha 20110222-alpha: new verbs d3dx11_43, dragonage2_demo, regedit, shell, taskmgr, winecfg

by Dan Kegel

Right, it's only been a couple days, but here's a new winetricks already. This release adds dragonage2_demo (released earlier today -- hey, we're starting to keep up!) as well as the handy verbs d3dx11_43, regedit, shell, taskmgr, and winecfg, and a few bugfixes. Download url: http://winetricks.org/winetricks-alpha svn repository / bug tracker: http://winetricks.org Changes since 20110219b-alpha: Dan Kegel: Added Status column to gui, containing 'cached' if game installer is cached. bioshock_demo: add mwo=force for bug 6971 cnc3_demo: fixed install check d3dx11_43: new verb dragonage2_demo: new verb Display install size and time for (some) games. Fix 'winetricks-alpha list'. Thanks to doh123 for pointing out the problem. Pre-check checkboxes for things that are already installed. (winezeug bug 128) regedit: new verb shell: new verb (handy for debugging) taskmgr: new verb tmnationsforever: mention portaudio workaround for choppiness winecfg: new verb Changes since 20110219-alpha: hko: removed, since they no longer offer the torrent download. utorrent: updated to 2.2.1beta. hegemony_demo: update sha1sum gecko is no longer a real verb, so remove media type. Fixes one reported failure in test suite. dc2ba_demo: updated sha1sum d3dxof: fix location of downloaded file msls31: fix typo that broke downloading with clean cache. Fixes ie6. Also mention where files are being downloaded to in console.

Feb. 23, 2011

Re: Apologies to the List

by Tom Wickline

On Wed, Feb 23, 2011 at 10:52 AM, James McKenzie <jjmckenzie51(a)earthlink.net > wrote: > On 2/22/11 4:37 AM, Alexandre Julliard wrote: > >> James McKenzie<jjmckenzie51(a)earthlink.net> writes: >> >> First, my tirade was not intended to be as such. I wanted to pull the >>> patch because it was incorrect and I did not want anyone else working >>> on it while I silently trimmed it and made it better. >>> Second, I realize this has affected my 'Jeremy White' score. I hope >>> that AJ understands why the code was pulled and that this happens >>> frequently with a project this large. >>> >> No, it doesn't. Requesting your code to be pulled is a serious matter, >> and a major break of the trust that is necessary for us all to work >> together. You can't do something like this and expect it not to have >> consequences. >> >> It was already unlikely that you would get any of your patches in, based >> on their technical merit, but now even if you managed to make your code >> acceptable, I wouldn't put it in, because I can't trust you not to make >> me pull it out again next time you get mad at me. >> >> And I agree with this decision. You are technically "the keeper of the > code" and I have on more than one occasion transgressed this unwritten rule. > > However, I will still post patches for comments in Wine Development and > subsequently release them to the LGPL/Wine in Wine-Patches so that others > can work with them. I feel that is only fair for what I have done. Is that > acceptable? > > This seems to be the method used by others, however I have been known to be > massively incorrect in the past, and I could be wrong now. > > If posting them to bug reports is the preferred method I will that instead. > > James McKenzie > > > Hello James, This is just my opinion OK, So don't take this as the opinion of Wine/WineHQ or anyone else. In the past I had a very serious drinking problem and I pissed off allot of people, and most of them were my friends. So one day I decided to stop drinking and to try and repair old friendships, saying sorry will only get you, or me, or anyone of us just so far... The only way to get back trust and friendship is to earn it a little each day by your actions. And not to overreact to a past overreaction. :) So my suggestion is to take it a day at a time and try and mend past mistakes by future actions. I wish you the best! Tom Wickline -- Wine is not a conclusion but a process...

Feb. 22, 2011

Re: gdi32: first look for a printer driver name in the registry (try 2)

by Vitaliy Margolen

On 02/22/2011 09:28 AM, Vitaly Perov wrote: > Fixed possible hKey leak since last send > > + static const WCHAR user_printers_reg_key[] = { 'S','o','f','t','w','a','r','e','\\', The actual key doesn't look like have anything to do with printers. Please use variable name that more corresponds to the actual content of the unicode string. > + if (!res && !RegQueryValueExW(hKey, device, NULL, NULL, (LPBYTE) driver, &size)) > + { > + TRACE("Get value from registry\n"); > + } Message is not really helpful. And just hides empty if() case. It's better to reverse logic to check for error and combine with the win.ini check. Something like: if ((RegOpenKeyExW(HKEY_CURRENT_USER, user_printers_reg_key, 0, KEY_READ, &hKey) || RegQueryValueExW(hKey, device, NULL, NULL, (LPBYTE) driver, &size) && !GetProfileStringW(devicesW, device, empty_strW, driver, size)) > WARN("Unable to find %s in [devices] section of win.ini\n", debugstr_w(device)); This should also mention registry. > + if (!res) RegCloseKey(hKey); Instead of using res, initialize hKey to NULL and close it if it's not NULL. It's more foolproof. Vitaliy

Feb. 22, 2011