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

Re: [PATCH] rasapi32: Ensure device count matches data returned

by Sebastian Lackner

On 26.02.2017 20:41, Bruno Jesus wrote: > From: Bruno Jesus <bjesus(a)codeweavers.com> > > Based on original patch by Qian Hong. > > Fixes bug https://bugs.winehq.org/show_bug.cgi?id=30378 > > Signed-off-by: Bruno Jesus <bjesus(a)codeweavers.com> > --- > dlls/rasapi32/rasapi.c | 1 + > dlls/rasapi32/tests/rasapi.c | 24 ++++++++++++++++++++++++ > 2 files changed, 25 insertions(+) > > diff --git a/dlls/rasapi32/rasapi.c b/dlls/rasapi32/rasapi.c > index fcd0cb8..f97d4c1 100644 > --- a/dlls/rasapi32/rasapi.c > +++ b/dlls/rasapi32/rasapi.c > @@ -254,6 +254,7 @@ DWORD WINAPI RasEnumDevicesA(LPRASDEVINFOA lpRasDevinfo, LPDWORD lpcb, LPDWORD l > if(lpRasDevinfo && lpRasDevinfo->dwSize != sizeof(RASDEVINFOA)) > return ERROR_INVALID_SIZE; > > + *lpcDevices = 1; > if (!lpRasDevinfo || (*lpcb < sizeof(RASDEVINFOA))) { > *lpcb = sizeof(RASDEVINFOA); > return ERROR_BUFFER_TOO_SMALL; > diff --git a/dlls/rasapi32/tests/rasapi.c b/dlls/rasapi32/tests/rasapi.c > index e0ff25f..e982f6c 100644 > --- a/dlls/rasapi32/tests/rasapi.c > +++ b/dlls/rasapi32/tests/rasapi.c > @@ -77,80 +77,104 @@ static void test_rasenum(void) > > /* test first parameter */ > cb = bufsize; > + cDevices = 0xdead; > result = pRasEnumDevicesA(NULL, &cb, &cDevices); > ok(result == ERROR_BUFFER_TOO_SMALL || > result == ERROR_INVALID_USER_BUFFER, /* win98 */ > "Expected ERROR_BUFFER_TOO_SMALL, got %08d\n", result); > + ok(cDevices > 0 && cDevices != 0xdead, "Unexpected device count %d\n", cDevices); > > rasDevInfo[0].dwSize = 0; > cb = bufsize; > + cDevices = 0xdead; > result = pRasEnumDevicesA(rasDevInfo, &cb, &cDevices); > ok(result == ERROR_INVALID_SIZE || > result == ERROR_INVALID_USER_BUFFER, /* win98 */ > "Expected ERROR_INVALID_SIZE, got %08d\n", result); > + ok(cDevices == 0xdead, "Unexpected device count %d\n", cDevices); > > rasDevInfo[0].dwSize = sizeof(RASDEVINFOA) -1; > cb = bufsize; > + cDevices = 0xdead; > result = pRasEnumDevicesA(rasDevInfo, &cb, &cDevices); > ok(result == ERROR_INVALID_SIZE || > result == ERROR_INVALID_USER_BUFFER, /* win98 */ > "Expected ERROR_INVALID_SIZE, got %08d\n", result); > + ok(cDevices == 0xdead, "Unexpected device count %d\n", cDevices); > > rasDevInfo[0].dwSize = sizeof(RASDEVINFOA) +1; > cb = bufsize; > + cDevices = 0xdead; > result = pRasEnumDevicesA(rasDevInfo, &cb, &cDevices); > ok(result == ERROR_INVALID_SIZE || > result == ERROR_INVALID_USER_BUFFER, /* win98 */ > "Expected ERROR_INVALID_SIZE, got %08d\n", result); > + ok(cDevices == 0xdead, "Unexpected device count %d\n", cDevices); > > /* test second parameter */ > rasDevInfo[0].dwSize = sizeof(RASDEVINFOA); > result = pRasEnumDevicesA(rasDevInfo, NULL, &cDevices); > + cDevices = 0xdead; Whats the point to set this after the function call? And also, how is this better than Qian Hongs original patch? Using %u is more correct for unsigned values, and the empty line after the assignment of *lpcDevices also didn't hurt. > ok(result == ERROR_INVALID_PARAMETER, > "Expected ERROR_INVALID_PARAMETER, got %08d\n", result); > + ok(cDevices == 0xdead, "Unexpected device count %d\n", cDevices); > > rasDevInfo[0].dwSize = sizeof(RASDEVINFOA); > cb = 0; > result = pRasEnumDevicesA(rasDevInfo, &cb, &cDevices); > + cDevices = 0xdead; > ok(result == ERROR_BUFFER_TOO_SMALL || > result == ERROR_INVALID_SIZE, /* vista, 2k8 */ > "Expected ERROR_BUFFER_TOO_SMALL/ERROR_INVALID_SIZE, got %08d\n", result); > + ok(cDevices == 0xdead, "Unexpected device count %d\n", cDevices); > > rasDevInfo[0].dwSize = sizeof(RASDEVINFOA); > cb = bufsize -1; > result = pRasEnumDevicesA(rasDevInfo, &cb, &cDevices); > + cDevices = 0xdead; > ok(result == ERROR_BUFFER_TOO_SMALL, > "Expected ERROR_BUFFER_TOO_SMALL, got %08d\n", result); > + ok(cDevices == 0xdead, "Unexpected device count %d\n", cDevices); > > rasDevInfo[0].dwSize = sizeof(RASDEVINFOA); > cb = bufsize +1; > + cDevices = 0xdead; > result = pRasEnumDevicesA(rasDevInfo, &cb, &cDevices); > ok(result == ERROR_SUCCESS, > "Expected ERROR_SUCCESS, got %08d\n", result); > + ok(cDevices > 0 && cDevices != 0xdead, "Unexpected device count %d\n", cDevices); > > /* test third parameter */ > rasDevInfo[0].dwSize = sizeof(RASDEVINFOA); > cb = bufsize; > + cDevices = 0xdead; > result = pRasEnumDevicesA(rasDevInfo, &cb, NULL); > ok(result == ERROR_INVALID_PARAMETER, > "Expected ERROR_INVALID_PARAMETER, got %08d\n", result); > + ok(cDevices == 0xdead, "Unexpected device count %d\n", cDevices); > > /* test combinations of invalid parameters */ > + cDevices = 0xdead; > result = pRasEnumDevicesA(NULL, NULL, &cDevices); > ok(result == ERROR_INVALID_PARAMETER, > "Expected ERROR_INVALID_PARAMETER, got %08d\n", result); > + ok(cDevices == 0xdead, "Unexpected device count %d\n", cDevices); > > + cDevices = 0xdead; > result = pRasEnumDevicesA(NULL, &cb, NULL); > ok(result == ERROR_INVALID_PARAMETER || > result == ERROR_INVALID_USER_BUFFER, /* win98 */ > "Expected ERROR_INVALID_PARAMETER, got %08d\n", result); > + ok(cDevices == 0xdead, "Unexpected device count %d\n", cDevices); > > cb = 0; > rasDevInfo[0].dwSize = 0; > + cDevices = 0xdead; > result = pRasEnumDevicesA(rasDevInfo, &cb, &cDevices); > ok(result == ERROR_INVALID_SIZE || > broken(result == ERROR_BUFFER_TOO_SMALL), /* win98 */ > "Expected ERROR_INVALID_SIZE, got %08d\n", result); > + ok(cDevices == 0xdead, "Unexpected device count %d\n", cDevices); > > HeapFree(GetProcessHeap(), 0, rasDevInfo); > } > -- 2.9.3 >

Feb. 26, 2017

Re: [PATCH 03/18] comctl32: Added basic implementation for task dialogs and add tests

by Nikolay Sivov

On 26.02.2017 21:39, Fabian Maurer wrote: > Thanks for the explanation, > >>>> You can't free list node like that. >>> >>> What do you mean? I allocated the list and it's contents with HeapAlloc, >>> so I need to free it that way. Or am I missing something? >> >> You need to unlink it first. > Do I really need to unlink it when I'm freeing everything? Calling list_remove > before freeing all the entries doesn't really help us, does it? We don't > access the list afterwards anymore. Yes, you need to unlink. And yes, you do access it, because you keep iterating through the list after you freed first entry. Take a look how it's done, this is used a lot all around. > >>>> Static string for that doesn't look right. >>> >>> Why is that? I've looked into the other code, and strings are often >>> defined as static const WCHAR, that was the recommended method, too. >>> Of course that needs to be changed later, since we (probably) want to take >>> localization into account. >> >> Yes, that's what I mean, it should go to resources. > Yes, I have that in mind for the future, but can I leave it that way for the > first implementation? It's a matter of a single LoadStringW call, once you added a string as a resource. Make sure it matches how user32 defines it - as "OK", this way po files will not require translation updates. > > Regards, > Fabian Maurer > > > >

Feb. 26, 2017

Re: [PATCH 04/18] comctl32: Extend TaskDialog, add simple callback and tests

by Fabian Maurer

> > For that please use msg.h from comctl32/tests. > > I'd love to, but I'm not sure how this would work. > After all, the message lists for the taskdialogs use a different struct with > additional values, like the return value it should give, and a sync value. > I agree that the current way isn't too elegant, how would you go about > that? Thought about it a while longer, but it's not easy. The current code uses > while (expected->message && actual->message) to process the messages. However, the problem is that TDN_CREATED is zero, terminating the loop way to early. That's why I added TDN_NO_MORE_MESSAGES in the first place. If I should use msg.h from comctl32, then I need to rewrite a bunch of its code. How do we want to handle it? Regards, Fabian Maurer

Feb. 26, 2017

Re: [PATCH 03/18] comctl32: Added basic implementation for task dialogs and add tests

by Fabian Maurer

Thanks for the explanation, > >> You can't free list node like that. > > > > What do you mean? I allocated the list and it's contents with HeapAlloc, > > so I need to free it that way. Or am I missing something? > > You need to unlink it first. Do I really need to unlink it when I'm freeing everything? Calling list_remove before freeing all the entries doesn't really help us, does it? We don't access the list afterwards anymore. > >> Static string for that doesn't look right. > > > > Why is that? I've looked into the other code, and strings are often > > defined as static const WCHAR, that was the recommended method, too. > > Of course that needs to be changed later, since we (probably) want to take > > localization into account. > > Yes, that's what I mean, it should go to resources. Yes, I have that in mind for the future, but can I leave it that way for the first implementation? Regards, Fabian Maurer

Feb. 26, 2017

Re: Coverity reports for test suite

by Nikolay Sivov

On 26.02.2017 21:32, Austin English wrote: > On Sun, Feb 26, 2017 at 12:09 PM, Nikolay Sivov <bunglehead(a)gmail.com> wrote: >> On 26.02.2017 20:53, Alex Henrie wrote: >>> Hello, >>> >>> I recently noticed that we don't currently get Coverity reports for >>> files in the test suite. I emailed Amine Khaldi, who does the Coverity >>> reports, and asked if we could start getting reports on the tests. >>> Amine made it sound like this had been discussed before and due to the >>> large volume of reports, there was not interest in getting reports for >>> anything other than the actual Wine code. >>> >>> Is there enough interest now to start looking at the test suite >>> reports? Would it hurt anything to be able to see that information? >>> >>> -Alex >>> >>> >> >> Yes, we discussed this on irc at some point, by we I mean Amine and >> myself, and that's probably what he meant. I don't think it makes sense >> to scan tests yet, I'd expect noise rate to be overwhelming, resulting >> in thousands on reports of little to no value, like missing return value >> checks. We still have hundreds of potentially valuable ones for >> implementation part, that not a lot of people are looking at. >> >> So for the last question, I'm worried that it would hurt in a way that >> reports we potentially care about will get buried. Of course I could be >> wrong, we could simply try once and see what happens. I'm not sure >> actually what happens to reports when next scan does not include failed >> files, you'll probably lose all comments, report status updates etc. >> >> If you have time to clean some of existing ones, that would be greatly >> appreciated. > > FWIW, coverity supports putting issues into different components based > on filename. It would be trivial to set up two different components > for wine, e.g., 'wine' and 'wine-tests', then have wine/*/*/tests/* go > to wine-tests, and wine/* to wine. > > For those not familiar with coverity administration, this would not > require two separate builds, the results are separated when results > are displayed on the server. > If it's possible to make them appear separately, so tests don't affect our main reports, I see no harm in scanning tests too.

Feb. 26, 2017

Re: Coverity reports for test suite

by Austin English

On Sun, Feb 26, 2017 at 12:09 PM, Nikolay Sivov <bunglehead(a)gmail.com> wrote: > On 26.02.2017 20:53, Alex Henrie wrote: >> Hello, >> >> I recently noticed that we don't currently get Coverity reports for >> files in the test suite. I emailed Amine Khaldi, who does the Coverity >> reports, and asked if we could start getting reports on the tests. >> Amine made it sound like this had been discussed before and due to the >> large volume of reports, there was not interest in getting reports for >> anything other than the actual Wine code. >> >> Is there enough interest now to start looking at the test suite >> reports? Would it hurt anything to be able to see that information? >> >> -Alex >> >> > > Yes, we discussed this on irc at some point, by we I mean Amine and > myself, and that's probably what he meant. I don't think it makes sense > to scan tests yet, I'd expect noise rate to be overwhelming, resulting > in thousands on reports of little to no value, like missing return value > checks. We still have hundreds of potentially valuable ones for > implementation part, that not a lot of people are looking at. > > So for the last question, I'm worried that it would hurt in a way that > reports we potentially care about will get buried. Of course I could be > wrong, we could simply try once and see what happens. I'm not sure > actually what happens to reports when next scan does not include failed > files, you'll probably lose all comments, report status updates etc. > > If you have time to clean some of existing ones, that would be greatly > appreciated. FWIW, coverity supports putting issues into different components based on filename. It would be trivial to set up two different components for wine, e.g., 'wine' and 'wine-tests', then have wine/*/*/tests/* go to wine-tests, and wine/* to wine. For those not familiar with coverity administration, this would not require two separate builds, the results are separated when results are displayed on the server. -- -Austin GPG: 14FB D7EA A041 937B

Feb. 26, 2017

Re: Coverity reports for test suite

by Nikolay Sivov

On 26.02.2017 20:53, Alex Henrie wrote: > Hello, > > I recently noticed that we don't currently get Coverity reports for > files in the test suite. I emailed Amine Khaldi, who does the Coverity > reports, and asked if we could start getting reports on the tests. > Amine made it sound like this had been discussed before and due to the > large volume of reports, there was not interest in getting reports for > anything other than the actual Wine code. > > Is there enough interest now to start looking at the test suite > reports? Would it hurt anything to be able to see that information? > > -Alex > > Yes, we discussed this on irc at some point, by we I mean Amine and myself, and that's probably what he meant. I don't think it makes sense to scan tests yet, I'd expect noise rate to be overwhelming, resulting in thousands on reports of little to no value, like missing return value checks. We still have hundreds of potentially valuable ones for implementation part, that not a lot of people are looking at. So for the last question, I'm worried that it would hurt in a way that reports we potentially care about will get buried. Of course I could be wrong, we could simply try once and see what happens. I'm not sure actually what happens to reports when next scan does not include failed files, you'll probably lose all comments, report status updates etc. If you have time to clean some of existing ones, that would be greatly appreciated.

Feb. 26, 2017

Coverity reports for test suite

by Alex Henrie

Hello, I recently noticed that we don't currently get Coverity reports for files in the test suite. I emailed Amine Khaldi, who does the Coverity reports, and asked if we could start getting reports on the tests. Amine made it sound like this had been discussed before and due to the large volume of reports, there was not interest in getting reports for anything other than the actual Wine code. Is there enough interest now to start looking at the test suite reports? Would it hurt anything to be able to see that information? -Alex

Feb. 26, 2017

Re: include: Avoid overflow (warning) in RGBA_MAKE.

by Henri Verbeet

On 26 February 2017 at 15:22, Gerald Pfeifer <gerald(a)pfeifer.com> wrote: > On Tue, 6 Sep 2016, Henri Verbeet wrote: >> On 6 September 2016 at 08:41, Dmitry Timoshkov <dmitry(a)baikal.ru> wrote: >>>> --- a/include/d3dtypes.h >>>> +++ b/include/d3dtypes.h >>>> @@ -53,7 +53,7 @@ typedef LONG D3DFIXED; >>>> #define RGBA_GETRED(rgb) (((rgb) >> 16) & 0xff) >>>> #define RGBA_GETGREEN(rgb) (((rgb) >> 8) & 0xff) >>>> #define RGBA_GETBLUE(rgb) ((rgb) & 0xff) >>>> -#define RGBA_MAKE(r, g, b, a) ((D3DCOLOR) (((a) << 24) | ((r) << 16) | ((g) << 8) | (b))) >>>> +#define RGBA_MAKE(r, g, b, a) ((D3DCOLOR) (((unsigned)(a) << 24) | ((r) << 16) | ((g) << 8) | (b))) >>> >>> This is a public header, probably you need to fix the callers of this >>> macro instead. >> Signed overflow is undefined, so this change is compatible. It's also >> unlikely to be an issue in practice on any architecture we support. I >> might have preferred something similar to D3DCOLOR_ARGB though. >> >> That's not to say the users of the macro don't have room for improvement. > > I noticed this has not been committed (nor rejected apparently). > but after reverting it locally, I do not see the warnings that > triggered this any longer. > > Did one of you commit and alternative fix? > Commit 060ea15ae2c1604d51d3df1460e99c7d6e6211cf should have avoided the issue.

Feb. 26, 2017

Re: [PATCH 06/18] comctl32: TaskDialog - Implement main and content text controls

by Fabian Maurer

> >> Why Post*? Dialog handling in user32 has an ability to do this for you. > > > > Ah yes, I forgot about SendDlgItemMessage there. > > What I mean is that dialog code can set control font for you. How? I'm currently not aware of such a method. You mean DS_SETFONT and a font in the DLGTEMPLATE? But then we'd still have the different font sizes to handle. > Different how? In sizes only? Font name should come from > NONCLIENTMETRICS probably, this needs some manual testing, but it's > unlikely that system settings are not respected for task dialogs on Windows. Yes, it's only the font size that's different. How would you suggest we handle the fonts? Regards, Fabian Maurer

Feb. 26, 2017