September 2012 - Wine-Devel

Re: dmusic: Fix a copy and paste error

by Christian Costa

Le 29/09/2012 17:11, Sven Baars a écrit : > This should fix http://bugs.winehq.org/show_bug.cgi?id=31805 > > Good catch !

Sept. 29, 2012

Re: [PATCH] msi: Use next cabinet from the media table instead of failing when there is a mismatch with continuous cabinet.

by Christian Costa

Le 28/09/2012 09:05, Hans Leidekker a écrit : > On Thu, 2012-09-27 at 23:07 +0200, Christian Costa wrote: >>>> if (!(cabinet_file = get_cabinet_filename(mi))) >>> The requirement that the continuous cabinet name matches the next cabinet in the media table >>> might simply be too strict. Perhaps we need to try them one by one and let FDI decide based >>> on the setID and iCabinet fields. Can you please add some tests? >>> >> Does not seem trivial to test this but I will try. > It shouldn't be too hard, we already have some tests for continuous cabinets. > > > Ok. I will take a look.

Sept. 29, 2012

Re: [PATCH] wined3d: move texture reference decrement to end of usage (Coverity)

by Henri Verbeet

On 29 September 2012 11:14, Marcus Meissner <marcus(a)jet.franken.de> wrote: > Hi, > > I do not fully understand all this, but it seem the decref needs to > be behind the last usage of prev. > Sort of. It's probably better to do it like this, but a non-zero bind_count implies a non-zero reference count as well, so in practice there's no danger of using prev after it's free'd.

Sept. 29, 2012

Re: Fwd: CIA going down: KGB wants your commits!

by Stefan Dösinger

Am Donnerstag, 27. September 2012, 16:10:56 schrieb Austin English: > IRC bots running in > our personal servers listen for SOAP messages sent by kgb-clients Shouldn't you call them kgb-agents? :-)

Sept. 29, 2012

Re: comctl32: use GetWindowLong to check for scrollbar (try 2)

by Nikolay Sivov

On 9/28/2012 20:43, Daniel Jelinski wrote: > Corrected from first try: > - uses GetWindowLong instead of GetWindowLongPtr > - does not touch dwStyle Sorry, was busy to reply earlier. I think this fix is ok if it really helps, thanks for working on this.

Sept. 28, 2012

Re: [PATCH] configure: use -Wp, -D_FORTIFY_SOURCE=0 instead of -D_FORTIFY_SOURCE=0

by Anssi Hannula

28.09.2012 18:14, Alexandre Julliard kirjoitti: > Anssi Hannula <anssi.hannula(a)iki.fi> writes: > >> Some Linux distributions (at least Mageia) build packages with >> -Wp,-D_FORTIFY_SOURCE=2 by default. There is a check in configure which >> checks for enabled fortify and adds "-U_FORTIFY_SOURCE >> -D_FORTIFY_SOURCE=0". Unfortunately, -Wp,-Dfoo are applied after -Dfoo, >> so the added flags fail to override the fortify in the CFLAGS. > > I would say that's a distro bug. Defaulting to Fortify is already broken > enough, but making it even harder to disable is stupid. I don't think > we want to get into that kind of arms race. Well, the intention is probably not to make it harder to override. Mageia uses the -Wp,x form simply because Mandriva and Fedora use that as well (according to logs at http://koji.fedoraproject.org/ ), I'm not sure if Fedora had some good reason to not use -D_FORTIFY_SOURCE=2 originally... IMHO those three distros (esp. Fedora) using -Wp,x is enough justification to add the "-Wp," prefixes to the existing fortify hack in wine configure.ac. However, I do understand and accept your position that it is not really wine's job to override all the weird compilation flags the user might be using. I wouldn't have sent the patch at all if the existing fortify hack hadn't already been there in the configure script :) For the record, Fedora already disables fortify for wine build, and Mageia is currently rolling updated wine packages that have fortify disabled. Mandriva seems to still be affected. No idea if there are more distributions using -Wp,-D_FORTIFY_SOURCE=2 for package builds. -- Anssi Hannula

Sept. 28, 2012

Re: [PATCH] configure: use -Wp, -D_FORTIFY_SOURCE=0 instead of -D_FORTIFY_SOURCE=0

by Alexandre Julliard

Anssi Hannula <anssi.hannula(a)iki.fi> writes: > Some Linux distributions (at least Mageia) build packages with > -Wp,-D_FORTIFY_SOURCE=2 by default. There is a check in configure which > checks for enabled fortify and adds "-U_FORTIFY_SOURCE > -D_FORTIFY_SOURCE=0". Unfortunately, -Wp,-Dfoo are applied after -Dfoo, > so the added flags fail to override the fortify in the CFLAGS. I would say that's a distro bug. Defaulting to Fortify is already broken enough, but making it even harder to disable is stupid. I don't think we want to get into that kind of arms race. -- Alexandre Julliard julliard(a)winehq.org

Sept. 28, 2012

Re: gdiplus: Reuse an existing image stream in GdipImageSelectActiveFrame. Take 2.

by Vincent Povirk

Looks good, thanks.

Sept. 28, 2012

Re: [PATCH 2/2] vbscript: Added MonthName implementation

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=21880 Your paranoid android. === WINEBUILD (build) === Patch failed to apply

Sept. 28, 2012

Re: gdi32: Fixed freetype working function

by Alexandre Julliard

Сергей Мосин <svmosin(a)gmail.com> writes: > The function WineEngAddFontMemResourceEx in gdi32/fretype.c contained > strange XOR operation with some random number 0x87654321, instead of > returning the real font handle. Do you have an app that depends on this? What does it do with the handle? -- Alexandre Julliard julliard(a)winehq.org

Sept. 28, 2012

Re: [PATCH] msi: Use next cabinet from the media table instead of failing when there is a mismatch with continuous cabinet.

by Hans Leidekker

On Thu, 2012-09-27 at 23:07 +0200, Christian Costa wrote: > >> if (!(cabinet_file = get_cabinet_filename(mi))) > > The requirement that the continuous cabinet name matches the next cabinet in the media table > > might simply be too strict. Perhaps we need to try them one by one and let FDI decide based > > on the setID and iCabinet fields. Can you please add some tests? > > > Does not seem trivial to test this but I will try. It shouldn't be too hard, we already have some tests for continuous cabinets.

Sept. 28, 2012

Re: CIA going down: KGB wants your commits!

by Charles Davis

On Sep 27, 2012, at 5:10 PM, Austin English wrote: > -- > -Austin > > ---------- Forwarded message ---------- > From: Martín Ferrari <martin.ferrari(a)gmail.com> > Date: Thu, Sep 27, 2012 at 3:52 PM > Subject: CIA going down: KGB wants your commits! > To: debian-project(a)lists.debian.org, Debian Developers > <debian-devel(a)lists.debian.org> > > > Hi there! > > Not to "to knock 'em when they're down", but since it seems that the CIA.vc > service has officially closed recently (cannot find exactly when did that > happen), I think it's worthwhile offering this little project we (dam@, > gregoa@, and me) have since a few years ago, called KGB (ha-ha). > > It's a very simple and dumb client-server architecture: IRC bots running in > our personal servers listen for SOAP messages sent by kgb-clients called > from post-commit hooks in repos out there. The bot just relays and formats > a message, nothing fancier than that. > > So, if you're looking for a simple IRC notification of your commits, this > might be of help. I'd like to put forward another alternative, from the guy who maintained the CIA scripts for git: http://www.catb.org/esr/irker/ (Yeah, that ESR.) After the CIA announcement, he rushed to get it out as a replacement, though he's seen this coming and has been working on this for quite some time. It's even backwards-compatible (to some extent) with CIA: it can take XML-RPC notifications sent by CIA clients. Just making sure you evaluate all your options. :) Oh, and personally, I wouldn't trust anything associated with the KGB. (Just kidding... ;) Chip > > You can either run your own bot (debian package kgb-bot), or use ours. As > for the client, alioth does not have it installed, but you can run it off > /home/groups/kgb. There's a sample configuration there too. In > /home/groups/pkg-perl/meta/pkg-perl-post-receive you can see how to use it > from a post-commit hook. > > If you rather use our bots, we'll need you to provide: a project/repository > name, an IRC channel, and a password (used to avoid spam, not really > secure). > > -- > Martín Ferrari > > > -- > To UNSUBSCRIBE, email to debian-devel-REQUEST(a)lists.debian.org > with a subject of "unsubscribe". Trouble? Contact listmaster(a)lists.debian.org > Archive: http://lists.debian.org/CAL60Pd_7zoPk4w4rta49-jymxzKOU+K7DdqZUiqsP32KZ9bQBA… > >

Sept. 28, 2012

Fwd: CIA going down: KGB wants your commits!

by Austin English

-- -Austin ---------- Forwarded message ---------- From: Martín Ferrari <martin.ferrari(a)gmail.com> Date: Thu, Sep 27, 2012 at 3:52 PM Subject: CIA going down: KGB wants your commits! To: debian-project(a)lists.debian.org, Debian Developers <debian-devel(a)lists.debian.org> Hi there! Not to "to knock 'em when they're down", but since it seems that the CIA.vc service has officially closed recently (cannot find exactly when did that happen), I think it's worthwhile offering this little project we (dam@, gregoa@, and me) have since a few years ago, called KGB (ha-ha). It's a very simple and dumb client-server architecture: IRC bots running in our personal servers listen for SOAP messages sent by kgb-clients called from post-commit hooks in repos out there. The bot just relays and formats a message, nothing fancier than that. So, if you're looking for a simple IRC notification of your commits, this might be of help. You can either run your own bot (debian package kgb-bot), or use ours. As for the client, alioth does not have it installed, but you can run it off /home/groups/kgb. There's a sample configuration there too. In /home/groups/pkg-perl/meta/pkg-perl-post-receive you can see how to use it from a post-commit hook. If you rather use our bots, we'll need you to provide: a project/repository name, an IRC channel, and a password (used to avoid spam, not really secure). -- Martín Ferrari -- To UNSUBSCRIBE, email to debian-devel-REQUEST(a)lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmaster(a)lists.debian.org Archive: http://lists.debian.org/CAL60Pd_7zoPk4w4rta49-jymxzKOU+K7DdqZUiqsP32KZ9bQBA…

Sept. 27, 2012

Re: [PATCH] msi: Use next cabinet from the media table instead of failing when there is a mismatch with continuous cabinet.

by Christian Costa

Le 27/09/2012 11:26, Hans Leidekker a écrit : > On Thu, 2012-09-27 at 09:59 +0200, Christian Costa wrote: > >> diff --git a/dlls/msi/media.c b/dlls/msi/media.c >> index 612624d..8a39fcd 100644 >> --- a/dlls/msi/media.c >> +++ b/dlls/msi/media.c >> @@ -358,8 +358,18 @@ static INT_PTR cabinet_next_cabinet(FDINOTIFICATIONTYPE fdint, >> >> if (strcmpiW( mi->cabinet, cab )) >> { >> - ERR("Continuous cabinet does not match the next cabinet in the Media table\n"); >> - goto done; >> + LPSTR next_cab; >> + >> + WARN("Continuous cabinet %s does not match the next cabinet %s in the media table\n", debugstr_w(cab), debugstr_w(mi->cabinet)); >> + >> + /* Use cabinet name from the media table */ >> + next_cab = strdupWtoA(mi->cabinet); >> + /* Modify path to cabinet file with full filename (psz3 points to a 256 bytes buffer that can be modified contrary to psz1 and psz2) */ >> + strcat(pfdin->psz3, "\\"); >> + strcat(pfdin->psz3, next_cab); >> + /* Path psz3 and cabinet psz1 are concatenated by FDI so just reset psz1 */ >> + *pfdin->psz1 = 0; >> + msi_free(next_cab); >> } >> >> if (!(cabinet_file = get_cabinet_filename(mi))) > The requirement that the continuous cabinet name matches the next cabinet in the media table > might simply be too strict. Perhaps we need to try them one by one and let FDI decide based > on the setID and iCabinet fields. Can you please add some tests? > > > Does not seem trivial to test this but I will try. Thanks Christian

Sept. 27, 2012

Re: is CreateThread(CREATE_SUSPENDED) fully functional?

by André Hentschel

Am 27.09.2012 09:13, schrieb Joerg-Cyril.Hoehle(a)t-systems.com: > Hi, > > Recently I thought I found a use-case for CreateThread(CREATE_SUSPENDED). > The idiom is to get the thread handle before the thread starts a life on its own. > > I looked at the kernel tests, > http://source.winehq.org/source/dlls/kernel32/tests/thread.c#L460 > > However, both invocations with CREATE_SUSPENDED actually test the > pair CreateThread + SuspendThread. The tests do not prove that CREATE_SUSPENDED works! > They only prove that the suspend counter and perhaps SuspendThread work. > > a) Does Wine not support CREATE_SUSPENDED, so that the tests do the best they can? > In such a case, a WARN("ignoring SUSPENDED flag, beware of race conditions\n") would be appropriate. > > or > b) CREATE_SUSPENDED is fully supported on all platforms. > Then the tests need some refinement. Why did you only had a look at the tests? I checked the source in 30 seconds and answered the question for me: CREATE_SUSPENDED is an essential part of the CreateThread Implementation. Anyway, more tests are always welcome. -- Best Regards, André Hentschel

Sept. 27, 2012

Re: is CreateThread(CREATE_SUSPENDED) fully functional?

by Vincent Povirk

I'm pretty sure CREATE_SUSPENDED works. It's such a basic thing that would cause many race conditions if it did not, including in parts of Wine that use it and in the test you just linked. If the new thread were not initially suspended, it would be possible for the thread to return between the initial CreateThread call and the following SuspendThread call. This would cause the first WaitForSingleObject to succeed and the test to fail. I guess you could change the test to include a wait before the first SuspendThread call, but given that there probably aren't any broken implementations that this would specifically catch (and those implementations would already fail sometimes), I don't see the point.

Sept. 27, 2012

Re: [1/3] gdiplus: Move font substitution test into a separate body.

by Vincent Povirk

The series looks good to me. Very cool to have a fix for this.

Sept. 27, 2012

Re: gdiplus: Reuse an existing image stream in GdipImageSelectActiveFrame.

by Vincent Povirk

> @@ -3898,7 +3894,7 @@ GpStatus WINGDIPAPI GdipImageSelectActiveFrame(GpImage *image, GDIPCONST GUID *d > return Ok; > } > > - IStream_Release(stream); > + IStream_Release(image->stream); > return stat; > } I don't think it's necessary to AddRef/Release the stream anymore in GdipImageSelectActiveFrame. Creating the new image and freeing the old one will update the refcount as necessary. And it's a bit confusing since technically the current image->stream is released by free_image_data then overwritten (with the same value, fortunately) by copying new_image to image. Looks good otherwise.

Sept. 27, 2012

'sort' implementation (Bug 27933) - any easy way?

by Ann and Jason Edmeades

Hello, While working on more of the 'for' processing in the command shell, I've come across a stumbling block that wildcard expansion 'ordering' is not guaranteed (findfirst/findnext) meaning I cannot automate tests for some of the key bits - this is frustrating as we really need tests for the things I'm looking at as the code is getting complex. Looking in the cmd testsuite, there's already some commented out tests as well, saying we really need a 'sort' implementation, and that would also resolve the problem I have at the moment. Therefore, I'd like to know if there's any simple way to implement a command line sort program which we can call from the command shell. This has subsequently been raised as bug 27933. My first thought was to avoid reinventing the wheel and to have a windows program called sort which parses its parameters and then runs the Unix 'sort' command with an equivalent command line. I think most of the windows command line parameters have Unix equivalents, and for the majority of cases it would be a case of transposing a windows path to a Unix path, and just running the Unix sort. However, I then started to get concerned about stdin / stdout... Is there any way to take the windows stdin and 'connect it' to the unix program you spawn, and vica-versa for the stdout. Whilst I dont accurately need this for the testsuite, without it the wine implementation would not match the windows one. So I think my specific questions are... 1. Is wrappering an existing (Linux) 'sort' program an option for this, or do we really need a whole unique implementation of sort (which I would have no idea how to write!)? If it is an option, the two key questions I have are: 2. How do I handle stdin / stdout, so sort can work in pipes / redirects 3. Is it ok to use wine_get_unix_file_name to convert files and directories to the unix equivalents when building the unix equivalent command line? (as per winepath) Thanks. Jason

Sept. 27, 2012

Re: d3dx9_36 [patch 6/6]: Implement D3DXSHEvalHemisphereLight

by Rico Schüller

Well I think, if you rework a series, please add something like try xyz or so ... Also try to avoid to add / remove some patches, at least if they actually don't have anything todo with the rest of the patch set (if they could be submitted on their own). On 27.09.2012 10:00, Nozomi Kodama wrote: > + TRACE("%u, %p, %p, %p, %p, %p, %p\n", order, dir, &top, &bottom, rout, gout, bout); Sure, that's an other form of the trace. I think we hadn't one with such a version... other traces in the file look like (the one marked with * is my preferred version): TRACE("out %p, order %u, a %p, b %p\n", out, order, a, b); * TRACE("(%p, %p, %u)\n", pout, pin, n); I don't think it makes much sense to print the address for &top. It doesn't gather some useful information, does it? Instead you may print the actual content for r,g,b if you like. > + if ( gout ) Also here, I prefer "if (gout)" ... I know there are several occurrences for the other style ... I should have mentioned that earlier ... it's not something which is critical... git grep "if (" dlls/d3dx9_36/* | wc -l reveals ~2000 while git grep "if ( " dlls/d3dx9_36/* | wc -l does ~100 > + ok(relative_error(expected, blue_out[j]) < admitted_error, > + "Blue: case %u, order %u: expected[%u] = %f, received %f \n", l, order, j, expected, blue_out[j]); Please indent 8 instead of 2 spaces when a new line is used ... Cheers Rico

Sept. 27, 2012

Re: [PATCH] msi: Use next cabinet from the media table instead of failing when there is a mismatch with continuous cabinet.

by Hans Leidekker

On Thu, 2012-09-27 at 09:59 +0200, Christian Costa wrote: > diff --git a/dlls/msi/media.c b/dlls/msi/media.c > index 612624d..8a39fcd 100644 > --- a/dlls/msi/media.c > +++ b/dlls/msi/media.c > @@ -358,8 +358,18 @@ static INT_PTR cabinet_next_cabinet(FDINOTIFICATIONTYPE fdint, > > if (strcmpiW( mi->cabinet, cab )) > { > - ERR("Continuous cabinet does not match the next cabinet in the Media table\n"); > - goto done; > + LPSTR next_cab; > + > + WARN("Continuous cabinet %s does not match the next cabinet %s in the media table\n", debugstr_w(cab), debugstr_w(mi->cabinet)); > + > + /* Use cabinet name from the media table */ > + next_cab = strdupWtoA(mi->cabinet); > + /* Modify path to cabinet file with full filename (psz3 points to a 256 bytes buffer that can be modified contrary to psz1 and psz2) */ > + strcat(pfdin->psz3, "\\"); > + strcat(pfdin->psz3, next_cab); > + /* Path psz3 and cabinet psz1 are concatenated by FDI so just reset psz1 */ > + *pfdin->psz1 = 0; > + msi_free(next_cab); > } > > if (!(cabinet_file = get_cabinet_filename(mi))) The requirement that the continuous cabinet name matches the next cabinet in the media table might simply be too strict. Perhaps we need to try them one by one and let FDI decide based on the setID and iCabinet fields. Can you please add some tests?

Sept. 27, 2012

Re: dsound: Don't bother shrinking the secondary buffer list.

by Michael Stefaniuc

Hello Jörg, as always it depends. On 09/27/2012 09:37 AM, Joerg-Cyril.Hoehle(a)t-systems.com wrote: >> +#include <assert.h> > > your patch is already committed, yet I believe that the least that dsound needs is new asserts. > > Asserts in dsound have been a PITA in Wine for the last decade. > > It's ok for the kernel to crash in an assert. IMHO it is not ok for an optional thing like sound. > > The assert's backtrace will not help anybody, because the logical error will have happened much earlier. I disagree. It helps GyB to open regression bug reports and it helps me to fix them. > Instead, a plain ERR would have been just as telling. Let's quote > http://www.winehq.org/docs/winedev-guide/debugging#DBG-CLASSES > "ERR: Messages in this class indicate serious errors in Wine, > such as as conditions that should never happen by design." > > I recommend: > - assert(device->buffers[0] == pDSB); > + if (pdevice->buffers[0] != pDSB) > + ERR("broken device buffer %p\n", device->buffers[0]); > HeapFree(GetProcessHeap(), 0, device->buffers); > device->buffers = NULL; > > The ERR is IMHO revealing enough in a user's log. That's too late if it hits the user. I had reviewed the code: That code is an implementation detail completely outside the control of the applications. Barring a memory corruption there is no way for it to fail. The assert is there for two reasons: - To crash during "make test" when the next developer touches that code. - As a comment less likely to be removed by Alexandre ;) > If a sound component detects a bogus state, > I'd very much prefer that it disables sound -- without crashing. In general "it depends" of course. In this particular case the ERR would be useless noise as the assert is only for the last sound buffer that is removed; usually at program exit. Until then the code does what you prefer: silently drops sound buffers without crashing thus deferring the crash to the program exit. A crash on program exit does get reported, more so than an ERR on the command line. bye michael

Sept. 27, 2012

Re: d3dx9_36 [patch 1/6]: Implement D3DXSHMultiply4

by Rico Schüller

On 27.09.2012 09:56, Nozomi Kodama wrote: > +FLOAT* WINAPI D3DXSHMultiply4(FLOAT *out, CONST FLOAT *a, CONST FLOAT *b) FLOAT * WINAPI D3DXSHMultiply4(FLOAT *out, const FLOAT *a, const FLOAT *b) A minor nitpick, anyone against something like? I think that was what Matteo meant in http://www.winehq.org/pipermail/wine-devel/2012-September/096924.html with "Fix the FLOAT*"? > + TRACE("(%p, %p, %p)\n", out, a, b); I would probably prefer: TRACE("out %p, a %p, b %p\n", out, a, b); I know the other Multiply functions and so on use your trace format... > + out[0] = 0.28209479f * a[0] * b[0]; There are still double spaces ... Hint: it's not the only occurrence. Cheers Rico

Sept. 27, 2012

Re: d3dx9_36 [patch 2/6]: Speed-up some tests

by Rico Schüller

On 27.09.2012 09:57, Nozomi Kodama wrote: > - FLOAT a[64], b[64], got; > + FLOAT a[49], b[49], got; > CONST FLOAT expected[] = > { 0.5f, 0.5f, 25.0f, 262.5f, 1428.0f, 5362.0f, 15873.0f, 39812.0f, 88400.0f, }; > > - for (i = 0; i < 64; i++) > + for (i = 0; i < 49; i++) > > - for (i = 0; i < 9; i++) > + for (i = 0; i <= D3DXSH_MAXORDER + 1; i++) Again: You may correct the (array) size for the expected value ... If we run til element 7 instead of 8, I think the last element could be thrown away? Cheers Rico

Sept. 27, 2012

dsound: Don't bother shrinking the secondary buffer list.

by Joerg-Cyril.Hoehle＠t-systems.com

Michael, >+#include <assert.h> your patch is already committed, yet I believe that the least that dsound needs is new asserts. Asserts in dsound have been a PITA in Wine for the last decade. It's ok for the kernel to crash in an assert. IMHO it is not ok for an optional thing like sound. The assert's backtrace will not help anybody, because the logical error will have happened much earlier. Instead, a plain ERR would have been just as telling. Let's quote http://www.winehq.org/docs/winedev-guide/debugging#DBG-CLASSES "ERR: Messages in this class indicate serious errors in Wine, such as as conditions that should never happen by design." I recommend: - assert(device->buffers[0] == pDSB); + if (pdevice->buffers[0] != pDSB) + ERR("broken device buffer %p\n", device->buffers[0]); HeapFree(GetProcessHeap(), 0, device->buffers); device->buffers = NULL; The ERR is IMHO revealing enough in a user's log. If a sound component detects a bogus state, I'd very much prefer that it disables sound -- without crashing. Regards, Jörg Höhle

Sept. 27, 2012