October 2012 - Wine-devel - list.winehq.org

Re: vbscript: Added support for more builtin constants
by Marvin 30 Oct '12

30 Oct '12

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=22595 Your paranoid android. === WNT4WSSP6 (32 bit) === No test summary line found === W2KPROSP4 (32 bit) === No test summary line found === WXPPROSP3 (32 bit) === No test summary line found === W2K3R2SESP2 (32 bit) === No test summary line found === WVISTAADM (32 bit) === No test summary line found === W2K8SE (32 bit) === No test summary line found === W7PRO (32 bit) === No test summary line found === W7PROX64 (32 bit) === No test summary line found === TEST64_W7SP1 (32 bit) === No test summary line found === W7PROX64 (64 bit) === No test summary line found === TEST64_W7SP1 (64 bit) === No test summary line found

1 0

Re: [PATCH] wininet: TLS fallback mechanism (take2)
by Alexandre Julliard 29 Oct '12

29 Oct '12

Hiroshi Miura <miurahr(a)linux.com> writes: > +#ifdef SONAME_LIBSSL > + /* can't connect if we are already connected */ > + if (connection->ssl_s) > + { > + ERR("already connected\n"); > + return ERROR_INTERNET_CANNOT_CONNECT; > + } > + > + /* connect with given TLS options */ > + res = netcon_secure_connect_setup(connection, get_tls_option()); > + if (res == ERROR_SUCCESS) > + return res; You missed some #ifdefs, this won't compile. -- Alexandre Julliard julliard(a)winehq.org

1 0

Re: crypt32: Avoid double free in CRYPT_LoadSIP on error path (coverity))
by Nikolay Sivov 29 Oct '12

29 Oct '12

On 10/29/2012 11:33, Frédéric Delanoy wrote: > FreeLibrary(temp) can be called twice on error. > > CID 714004 > --- > > error: > FreeLibrary(lib); > - FreeLibrary(temp); > + if (temp != NULL) FreeLibrary(temp); > SetLastError(TRUST_E_SUBJECT_FORM_UNKNOWN); > return FALSE; > } This is useless, FreeLibrary() checks for NULL.

2 1

Re: [PATCH] kernel32: Implement CompareStringOrdinal.
by Nikolay Sivov 29 Oct '12

29 Oct '12

On 10/29/2012 00:53, Christian Costa wrote: > --- > dlls/kernel32/kernel32.spec | 1 + > dlls/kernel32/locale.c | 8 ++++++++ > 2 files changed, 9 insertions(+) > > diff --git a/dlls/kernel32/kernel32.spec b/dlls/kernel32/kernel32.spec > index b7efa0f..0bd1adc 100644 > --- a/dlls/kernel32/kernel32.spec > +++ b/dlls/kernel32/kernel32.spec > @@ -201,6 +201,7 @@ > @ stdcall CompareStringA(long long str long str long) > @ stdcall CompareStringW(long long wstr long wstr long) > @ stdcall CompareStringEx(wstr long wstr long wstr long ptr ptr long) > +@ stdcall CompareStringOrdinal(wstr long wstr long long) > @ stdcall ConnectNamedPipe(long ptr) > @ stub ConsoleMenuControl > @ stub ConsoleSubst > diff --git a/dlls/kernel32/locale.c b/dlls/kernel32/locale.c > index c41442c..83de815 100644 > --- a/dlls/kernel32/locale.c > +++ b/dlls/kernel32/locale.c > @@ -3047,6 +3047,14 @@ INT WINAPI CompareStringA(LCID lcid, DWORD flags, > return ret; > } > > +/****************************************************************************** > + * CompareStringOrdinal (KERNEL32.@) > + */ > +INT WINAPI CompareStringOrdinal(LPCWSTR str1, INT len1, LPCWSTR str2, INT len2, BOOL ignore_case) > +{ > + return CompareStringEx(NULL, ignore_case ? NORM_IGNORECASE : 0, str1, len1, str2, len2, NULL, NULL, 0); > +} > + > /************************************************************************* > * lstrcmp (KERNEL32.@) > * lstrcmpA (KERNEL32.@) This is not a way to do it, it doesn't simply work as that. See http://msdn.microsoft.com/en-us/library/windows/desktop/dd318144%28v=vs.85%… And it need tests of course.

2 1

Re: [PATCH 1/2] msvcp90: set eof state in istream<>::peek if got eof
by Alexandre Julliard 29 Oct '12

29 Oct '12

Daniel Lehman <dlehman(a)esri.com> writes: > +static void test_istream_peek(void) > +{ > + unsigned short testus, nextus; > + basic_stringstream_wchar wss; > + basic_stringstream_char ss; > + basic_string_wchar wstr; > + basic_string_char str; > + IOSB_iostate state; > + int i, next, peek; > + wchar_t wide[64]; > + > + struct _test_istream_peek { > + const char *str; > + int peek; > + int next; > + IOSB_iostate state; > + } tests[] = { > + { "", EOF, EOF, IOSTATE_eofbit }, > + { "ABCDEF", 'A', 'A', IOSTATE_goodbit }, > + }; > + > + for(i=0; i<sizeof(tests)/sizeof(tests[0]); i++) { > + /* char version */ > + call_func2(p_basic_string_char_ctor_cstr, &str, tests[i].str); > + call_func4(p_basic_stringstream_char_ctor_str, &ss, &str, OPENMODE_out|OPENMODE_in, TRUE); > + > + peek = (int)call_func1(p_basic_istream_char_peek, &ss.base.base1); > + state = (IOSB_iostate)call_func1(p_ios_base_rdstate, &ss.basic_ios.base); > + next = (int)call_func1(p_basic_istream_char_get, &ss.base.base1); 'peek' is not tested anywhere. -- Alexandre Julliard julliard(a)winehq.org

1 0

Re: gdiplus: fix rounding error (?)
by Vincent Povirk 29 Oct '12

29 Oct '12

That was the worst possible defence of this patch, but I think it should be included. What it's actually doing is removing the duplicated logic between the earlier bit of GdipDrawString, where out-of-range sizes are set to 1 << 23, and this if statement where we only apply a clipping rectangle if the sizes are out of range. We could also have added a check for e.g. scaled_rect.Width >= INT_MAX here, but I think it's better that we don't duplicate the logic.

1 0

Re: dplayx: Reorder some code to avoid memory leaks (coverity)
by Rico Schüller 29 Oct '12

29 Oct '12

On 28.10.2012 16:13, André Hentschel wrote: > - lpGData->lpRemoteData = lpNewData; > + lpGData->lpRemoteData = HeapAlloc( GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof( dwDataSize ) ); > + CopyMemory( lpGData->lpRemoteData, lpData, dwDataSize ); > lpGData->dwRemoteDataSize = dwDataSize; > - lpPData->lpRemoteData = lpNewData; > + lpPData->lpRemoteData = HeapAlloc( GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof( dwDataSize ) ); > + CopyMemory( lpPData->lpRemoteData, lpData, dwDataSize ); > lpPData->dwRemoteDataSize = dwDataSize; Is the HEAP_ZERO_MEMORY really needed? You may kill that too while you change those lines. Cheers Rico

2 1

Re: [3/5] d3dx9: Introduce a function for copying pixels.
by Christian Costa 29 Oct '12

29 Oct '12

> > pixel_format = get_format_info(src_desc.Format); > - if (pixel_format->type != FORMAT_ARGB) > - { > - FIXME("Unsupported pixel format %#x\n", src_desc.Format); > - return E_NOTIMPL; > - } > + if (pixel_format->type == FORMAT_UNKNOWN) return E_NOTIMPL; > > Doesn't this still require a FIXME or at least a WARN?

2 1

Re: vbscript: Added support for negative constants
by Marvin 29 Oct '12

29 Oct '12

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=22550 Your paranoid android. === WNT4WSSP6 (32 bit) === No test summary line found === W2KPROSP4 (32 bit) === No test summary line found === WXPPROSP3 (32 bit) === No test summary line found === W2K3R2SESP2 (32 bit) === No test summary line found === WVISTAADM (32 bit) === No test summary line found === W2K8SE (32 bit) === No test summary line found === W7PRO (32 bit) === No test summary line found === W7PROX64 (32 bit) === No test summary line found === TEST64_W7SP1 (32 bit) === No test summary line found === W7PROX64 (64 bit) === No test summary line found === TEST64_W7SP1 (64 bit) === No test summary line found

1 0

Re: [PATCH] Potential reference count races
by Nikolay Sivov 29 Oct '12

29 Oct '12

On 10/28/2012 17:44, Max TenEyck Woodbury wrote: > On 10/28/2012 02:40 AM, Nikolay Sivov wrote: >> On 10/28/2012 04:59, max(a)mtew.isa-geek.net wrote: >>> From: Max TenEyck Woodbury <max(a)mtew.isa-geek.net> >>> >>> I have been looking at the Microsoft COM and related documentations >>> and noticed that they emphatically recommend using the Interlocked... >>> functions when manipulating reference counts. I managed to set up a >>> search that showed where many of the reference count updates occur and >>> was somewhat surprised at how often this advice was not followed. >> It doesn't mean it always has to be followed. > True in a limited sense, but there is a good reason behind their > recommendation. Unless there is a good reason not to do so, this > particular piece of advice should be followed. COM objects in wine follow this recommendation in general, even object itself is not thread safe. This doesn't mean however that you need this every time you have some kind of refcount of any sort. > >>> While I have not converted every reference count update to use the >>> Interlocked... functions, this set of patches fixes a fair number of >>> them. >>> >>> These are not associated with any particular bug report; they are simply >>> a general precaution against operations that are known to be associated >>> with race conditions. >> This precaution doesn't work in general. It's not enough to atomically >> update refcount to make an implementation thread safe. Also not everything >> is supposed to be thread safe in a first place. >> > First, explain what does not have to be thread safe. Anything really, COM objects in particular if you were talking about them. > I believe that > application may try to use multiple threads anywhere, so everything > that can be made thread safe, should be. No. > Using interlocks on the > reference count updates is a necessary step for thread safety. You > are correct that it is not sufficient, but it is necessary. Again, it depends. > >> Changes like this: >> >>> - for (i=0;i<howmuch;i++) >>> + for (i=0;i<howmuch;++i) >>> TRACE("notify at %d to %p\n", >>> notify[i].dwOffset,notify[i].hEventNotify); >> are not helpful at all. > The post increment and decrement operation are specified as saving > the original value for use in the evaluation of the expression they > are part of and modifying the underlying stored value. In expressions > like this, that saved value is then discarded. The optimization phase > of the compilation usually removes both the save and discard operations. Sure, but I don't think it's enough to justify such changes all over the place, in existing code. > > Murphy's law suggests that this might cause problems at some point when > least desired. Please don't do this. > > Specifying the unnecessary use of a temporary store is a bad habit to > have. You should tell the compiler exactly what needs to be done, not > throw in extraneous operations. > > So, the use of a post operator where a prefix operator is sufficient, > while almost certainly harmless, is still technically a mistake. > > At the minimum, these changes provide examples of the correct way these > statements should be coded. So, I have to disagree, although very > mildly, that changes like that are not helpful. This kind of change > does not deserve a separate patch, but should be perfectly acceptable as > an adjunct to other patches.

4 4