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

Re: [PATCH 1/5] d3dx9_36: Implement D3DXFileCreate. (try 3)

by Rico Schüller

On 24.10.2012 18:18, Dmitry Timoshkov wrote: > Rico Schüller <kgbricola(a)web.de> wrote: > >>>>>> + if (!object) >>>>>> + { >>>>>> + ERR("Out of memory\n"); >>>>>> + return E_OUTOFMEMORY; >>>>>> + } >>>>> >>>>> The ERR() is useless here, just return E_OUTOFMEMORY in such situations. >>>>> >>>>> >>>>> >>>> It's done this way in many places in wine. >>> >>> Then that other places need to be fixed as well. Printing an ERR() on memory >>> allocation errors is not helpful. >>> >> In which cases do you use ERR? I thought it is used, when there are >> system errors. And I think ERR is fine for out of memory, because the >> system is not able to hand out the needed memory. Just do a git grep. >> There are ~400 usages for ERR and out of memory... What's your >> suggestion how to do that with the "Out of memory"? > > A much more illustrative thing that happens after an out of memory > condition is a crash followed by a backtrace (if the caller does not > handle it gracefully), printing an ERR() is completely useless in that > case, and provides no useful information. > Well the app may initialize some resources (some of them fail with out of memory) and run half an hour. If it accesses the invalid pointer it crashes. Do you see the problem from the backtrace? The real problem happened half an hour ago and the err message doesn't hurt anyone, it just shows the problem as early as possible, which I think is better than to relay on a eventually crashing app at some point. The err doesn't spam the console, since it only happens when there is a problem. Cheers Rico

Oct. 24, 2012

Re: [PATCH 1/5] d3dx9_36: Implement D3DXFileCreate. (try 3)

by Christian Costa

Le 24/10/2012 19:39, Nikolay Sivov a écrit : > On 10/24/2012 19:02, Rico Schüller wrote: >> On 24.10.2012 16:33, Dmitry Timoshkov wrote: >>> Christian Costa <titan.costa(a)gmail.com> wrote: >>> >>>>>> +static HRESULT WINAPI ID3DXFileImpl_QueryInterface(ID3DXFile >>>>>> *iface, >>>>> REFIID riid, void **ret_iface) >>>>>> +{ >>>>>> + TRACE("(%p)->(%s, %p)\n", iface, debugstr_guid(riid), >>>>>> ret_iface); >>>>>> + >>>>>> + if (IsEqualGUID(riid, &IID_IUnknown) || >>>>>> + IsEqualGUID(riid, &IID_ID3DXFile)) >>>>>> + { >>>>>> + iface->lpVtbl->AddRef(iface); >>>>> >>>>> Isn't there an appropriate xxx_AddRef() macro? >>>>> >>>> >>>> No. >>> >>> Is there a reason why? >>> >> Yes, they are not in the headers which are supplied by the SDK (at >> least not in the one I checked). I guess you have to ask ms why they >> didn't add it. So I think we shouldn't add that in our headers. > Actually I don't see why we can't add an .idl for it and let widl > produce a header. It still will be accessible directly through vtbl > pointer if someone feels like build with our headers. Header we're > talking about d3dx9xof.h is small enough to be a first converted to > idl imho, instead of adding more code that uses in SDK way. As an > example we have dwrite.idl now that SDK does not provide. > > > > I'm not against this if this is encouraged but I would left it for another patch.

Oct. 24, 2012

Re: [PATCH 1/5] d3dx9_36: Implement D3DXFileCreate. (try 3)

by Christian Costa

Le 24/10/2012 17:53, Dmitry Timoshkov a écrit : > Christian Costa <titan.costa(a)gmail.com> wrote: > >> Well, that would have been better to spot this *much* earlier. >> >> More generally about coding style or rules, that would be *much* better to >> write a wiki page about this and point to it >> along with code style comments. I don't mind following rules or adopt "nice >> to do" but I cannot guess the difference >> between formal rules, informal ones and personal tastes. It would be better >> for everyone and focus the review >> on technical stuff. And this is not the first time I talk about such a page. > Handling out of memory errors, avoiding useless spam to the console, avoiding > blanket memset() calls are not the coding style things and they are not specific > to Wine or any other project, this is basic things a programmers needs to follow > in general IMHO. > I said "more generally". It doesn't apply particularly to this but usual comments we have on the list : indentation, tabs, LPxxx, Microsoft variable naming, COM, debugstr_a, sizeof, ... I proposed to do such a page but people need to agree on the principle and reviewers to use it to point to it along with comments. This way reviewers can more concentrate on technical stuff. This also help people cleanup the code when there are sending patch. If I agree with out of memory error, regarding the memset it is particular to COM and the incremental nature of Wine. In addition not all people are good programmers nor usual Wine developper? Note that for some goto is evil and should never used. If we have some tips or rules based on Wine code practical experience that would help patch submission.

Oct. 24, 2012

Re: [PATCH 1/5] d3dx9_36: Implement D3DXFileCreate. (try 3)

by Max TenEyck Woodbury

On 10/24/2012 12:02 PM, Rico Schüller wrote: > On 24.10.2012 16:33, Dmitry Timoshkov wrote: >> Christian Costa <titan.costa(a)gmail.com> wrote: >> >>>>> + if (!object) >>>>> + { >>>>> + ERR("Out of memory\n"); >>>>> + return E_OUTOFMEMORY; >>>>> + } >>>> >>>> The ERR() is useless here, just return E_OUTOFMEMORY in such >>>> situations. >>>> >>>> >>>> >>> It's done this way in many places in wine. >> >> Then that other places need to be fixed as well. Printing an ERR() on >> memory >> allocation errors is not helpful. >> > In which cases do you use ERR? I thought it is used, when there are > system errors. And I think ERR is fine for out of memory, because the > system is not able to hand out the needed memory. Just do a git grep. > There are ~400 usages for ERR and out of memory... What's your > suggestion how to do that with the "Out of memory"? > My understanding is that this is similar (although less severe) than writing log messages when out of disk space. The report may not be generated because the resources needed to produce the report are not available. The absence of the report may be taken to mean that the problem is on some other code path. Deliberately not reporting 'out of whatever' errors or, better yet, commenting that reporting such errors are not being generated may make diagnosing such problems easier.

Oct. 24, 2012

Re: [PATCH 1/5] d3dx9_36: Implement D3DXFileCreate. (try 3)

by Dmitry Timoshkov

Rico Schüller <kgbricola(a)web.de> wrote: > >>>> + if (!object) > >>>> + { > >>>> + ERR("Out of memory\n"); > >>>> + return E_OUTOFMEMORY; > >>>> + } > >>> > >>> The ERR() is useless here, just return E_OUTOFMEMORY in such situations. > >>> > >>> > >>> > >> It's done this way in many places in wine. > > > > Then that other places need to be fixed as well. Printing an ERR() on memory > > allocation errors is not helpful. > > > In which cases do you use ERR? I thought it is used, when there are > system errors. And I think ERR is fine for out of memory, because the > system is not able to hand out the needed memory. Just do a git grep. > There are ~400 usages for ERR and out of memory... What's your > suggestion how to do that with the "Out of memory"? A much more illustrative thing that happens after an out of memory condition is a crash followed by a backtrace (if the caller does not handle it gracefully), printing an ERR() is completely useless in that case, and provides no useful information. -- Dmitry.

Oct. 24, 2012

windowscodecs questions.

by Max TenEyck Woodbury

Background: I am trying to improve the performance/reliability of GuildWars2 under wine. In particular, PNG handling seems very slow, so I am looking at windowscodecs and I seem to be misunderstanding some things. I have read the PNG spec. First: The CRC check. The spec says that a 'chunk' contains a four byte size of data field, a four byte 'chunk identifier', the data and a CRC32 of the data. I found some code that reads a generic chunk. I see where the size and type are read, where a correctly sized record is allocated and where the data is read into the record. There is a 'FIXME' that asks about checking the CRC, but no code to actually read or check the CRC. I added code to read and check the CRC to my working copy of the git tree. That threw off the block synchronization, so I think there is code someplace else that either checks or skips the CRC, but I have not found it after a few hours searching for it. An indication on where to look and other advice would be very helpful at this time. Second: At least two of the methods GuildWars2 wants to use are stubbed. To implement those methods, the frame section of the WIC object should contain an array (or something with similar properties) of pointers to chunks. From what I have read about WIC, other formats could use a similar array. Some could even use an array of pointers to frames. Before I go messing with that level of change, I think I should listen to other peoples opinions of the subject. Third: On a very broad level, the whole OLE implementation seems to be very messed up. In particular, the usual practice for doing inheritance in C does not seem to be being used. That practice is to have the elements common to the base and extending object be placed at the beginning of the implementing data structures. While the member names need not be the same in all instances, the function, type and order of the common elements must be the same for economical implementation. This has been done for the basic object; all start with a pointer to the 'vtable'. What I think should follow is the critical section lock structure and, for 'IUnknown', the reference count. What I see is that these common elements are placed more or less at random in each extended object. Is there any reason, other than inertia, that at least these two fields should not be moved to the beginning of implementing objects. This can be done one object at a time and would allow the changed objects to use a common implementation of 'AddRef' at least, and another common routine or macro that handles the critical part of 'Release'.

Oct. 24, 2012

Re: [PATCH 1/5] d3dx9_36: Implement D3DXFileCreate. (try 3)

by Rico Schüller

On 24.10.2012 16:33, Dmitry Timoshkov wrote: > Christian Costa <titan.costa(a)gmail.com> wrote: > >>>> +static HRESULT WINAPI ID3DXFileImpl_QueryInterface(ID3DXFile *iface, >>> REFIID riid, void **ret_iface) >>>> +{ >>>> + TRACE("(%p)->(%s, %p)\n", iface, debugstr_guid(riid), ret_iface); >>>> + >>>> + if (IsEqualGUID(riid, &IID_IUnknown) || >>>> + IsEqualGUID(riid, &IID_ID3DXFile)) >>>> + { >>>> + iface->lpVtbl->AddRef(iface); >>> >>> Isn't there an appropriate xxx_AddRef() macro? >>> >> >> No. > > Is there a reason why? > Yes, they are not in the headers which are supplied by the SDK (at least not in the one I checked). I guess you have to ask ms why they didn't add it. So I think we shouldn't add that in our headers. >>>> + if (!object) >>>> + { >>>> + ERR("Out of memory\n"); >>>> + return E_OUTOFMEMORY; >>>> + } >>> >>> The ERR() is useless here, just return E_OUTOFMEMORY in such situations. >>> >>> >>> >> It's done this way in many places in wine. > > Then that other places need to be fixed as well. Printing an ERR() on memory > allocation errors is not helpful. > In which cases do you use ERR? I thought it is used, when there are system errors. And I think ERR is fine for out of memory, because the system is not able to hand out the needed memory. Just do a git grep. There are ~400 usages for ERR and out of memory... What's your suggestion how to do that with the "Out of memory"? Cheers Rico

Oct. 24, 2012

Re: [PATCH 1/5] d3dx9_36: Implement D3DXFileCreate. (try 3)

by Dmitry Timoshkov

Christian Costa <titan.costa(a)gmail.com> wrote: > Well, that would have been better to spot this *much* earlier. > > More generally about coding style or rules, that would be *much* better to > write a wiki page about this and point to it > along with code style comments. I don't mind following rules or adopt "nice > to do" but I cannot guess the difference > between formal rules, informal ones and personal tastes. It would be better > for everyone and focus the review > on technical stuff. And this is not the first time I talk about such a page. Handling out of memory errors, avoiding useless spam to the console, avoiding blanket memset() calls are not the coding style things and they are not specific to Wine or any other project, this is basic things a programmers needs to follow in general IMHO. -- Dmitry.

Oct. 24, 2012

Re: [PATCH 1/5] d3dx9_36: Implement D3DXFileCreate. (try 3)

by Christian Costa

2012/10/24 Dmitry Timoshkov <dmitry(a)baikal.ru> > Christian Costa <titan.costa(a)gmail.com> wrote: > > > > > +static HRESULT WINAPI ID3DXFileImpl_QueryInterface(ID3DXFile *iface, > > > REFIID riid, void **ret_iface) > > > > +{ > > > > + TRACE("(%p)->(%s, %p)\n", iface, debugstr_guid(riid), > ret_iface); > > > > + > > > > + if (IsEqualGUID(riid, &IID_IUnknown) || > > > > + IsEqualGUID(riid, &IID_ID3DXFile)) > > > > + { > > > > + iface->lpVtbl->AddRef(iface); > > > > > > Isn't there an appropriate xxx_AddRef() macro? > > > > > > > No. > > Is there a reason why? > It's not in headers as for effect interfaces. > > > > > + *ret_iface = iface; > > > > + return S_OK; > > > > + } > > > > + > > > > + ERR("(%p)->(%s, %p), not found\n", iface, debugstr_guid(riid), > > > ret_iface); > > > > > > FIXME seems more appropriate here. > > > > > > > No. All interfaces are implemented here. > > Then ERR() is even more inappropriate here. > I'll turn it to a WARN then. > > > > +HRESULT WINAPI D3DXFileCreate(ID3DXFile **dxfile) > > > > +{ > > > > + ID3DXFileImpl *object; > > > > + > > > > + TRACE("(%p)\n", dxfile); > > > > + > > > > + object = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, > > > sizeof(*object)); > > > > > > What's the reason for HEAP_ZERO_MEMORY here? > > > > > > > It's common to do it this way. Only non zero value are set on creation. > > Usually several members are set in methods. > > There is no memebers left at 0 here. > I do it this way for all COM objects because members can be added as the implementation advance. In this particular case, I don't expect any new member so I will remove the flag if it's important. > > > > + if (!object) > > > > + { > > > > + ERR("Out of memory\n"); > > > > + return E_OUTOFMEMORY; > > > > + } > > > > > > The ERR() is useless here, just return E_OUTOFMEMORY in such > situations. > > > > > > > > > > > It's done this way in many places in wine. > > Then that other places need to be fixed as well. Printing an ERR() on > memory > allocation errors is not helpful. > > Well, that would have been better to spot this *much* earlier. More generally about coding style or rules, that would be *much* better to write a wiki page about this and point to it along with code style comments. I don't mind following rules or adopt "nice to do" but I cannot guess the difference between formal rules, informal ones and personal tastes. It would be better for everyone and focus the review on technical stuff. And this is not the first time I talk about such a page.

Oct. 24, 2012

[PATCH(0/1)] wininet: TLS fallback mechanism

by Hiroshi Miura(＠osmf)

Hi, Evernote windows client on Wine/OpenSSL 1.0.1/Ubuntu12.04/12.10 cannot connect its server. It is caused by server is only support TLS1.0/SSL3.0, but client ask TLS1.1/1.2 and then server returns that session is fails. A wininet in Microsoft Windows7 implementation has a behavior to re-try by TLS1.0 after TLS1.2 fails and got FIN from server. This patch enabled fall back mechanism to SSLv3/TLSv1. It is also a response for Alexandre's suggestion. >> How do you think whether we should implement similar way or not? >> If we adopt similar fall back mechanism, compatibility may become well, >> but is it good behavior? > > If it's possible to implement some sort of automatic fall back, that > would certainly be preferable. > > -- > Alexandre Julliard Hiroshi Hiroshi Miura (1): wininet: TLS fallback mechanism dlls/wininet/netconnection.c | 163 ++++++++++++++++++++++++++++++++---------- 1 file changed, 124 insertions(+), 39 deletions(-) -- 1.7.9.5

Oct. 24, 2012