April 2016 - Wine-Devel - list.winehq.org

Re: [PATCH 1/2] ieframe: Simplify IPersist_Load, make error handling more straightforward (Coverity)

by Jacek Caban

On 04/11/16 18:00, Nikolay Sivov wrote: > On 11.04.2016 18:30, Jacek Caban wrote: >> Hi Nikolay, >> >> On 04/08/16 21:56, Nikolay Sivov wrote: >>> + /* Now we're going to read in the iconfile and iconindex. >>> + If we don't find them, that's not a failure case -- it's possible >>> + that they just aren't in there. */ >>> + hr = IPropertySetStorage_Open(This->property_set_storage, &FMTID_Intshcut, >>> + STGM_READWRITE | STGM_SHARE_EXCLUSIVE, &pPropStg); >> It's an already existing bug, but we should handle Open failure here. >> > I think I kept it intentionally, to keep original logic intact in > cleanup patch. That makes sense, adding 3/3 is fine then. > By handling it you mean making Load() fail if Open() fails? Save() fails in such case, so I'd say we should fail here. Thanks, Jacek

April 11, 2016

Re: [PATCH 1/2] ieframe: Simplify IPersist_Load, make error handling more straightforward (Coverity)

by Nikolay Sivov

On 11.04.2016 18:30, Jacek Caban wrote: > Hi Nikolay, > > On 04/08/16 21:56, Nikolay Sivov wrote: >> + /* Now we're going to read in the iconfile and iconindex. >> + If we don't find them, that's not a failure case -- it's possible >> + that they just aren't in there. */ >> + hr = IPropertySetStorage_Open(This->property_set_storage, &FMTID_Intshcut, >> + STGM_READWRITE | STGM_SHARE_EXCLUSIVE, &pPropStg); > > It's an already existing bug, but we should handle Open failure here. > I think I kept it intentionally, to keep original logic intact in cleanup patch. By handling it you mean making Load() fail if Open() fails? Do you want me to make this change in the same patch? I can make it 3/3 too. > Thanks, > Jacek > >

April 11, 2016

Re: [PATCH 2/2] ieframe: Convert to integer without additional string buffer allocation

by Nikolay Sivov

On 11.04.2016 18:31, Jacek Caban wrote: > On 04/08/16 21:56, Nikolay Sivov wrote: >> Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> >> --- >> dlls/ieframe/intshcut.c | 5 ++--- >> 1 file changed, 2 insertions(+), 3 deletions(-) >> >> diff --git a/dlls/ieframe/intshcut.c b/dlls/ieframe/intshcut.c >> index 385b059..79458ac 100644 >> --- a/dlls/ieframe/intshcut.c >> +++ b/dlls/ieframe/intshcut.c >> @@ -511,12 +511,11 @@ static HRESULT WINAPI PersistFile_Load(IPersistFile *pFile, LPCOLESTR pszFileNam >> >> if (get_profile_string(str_header, str_iconindex, pszFileName, &iconindexstring) == S_OK) >> { >> + WCHAR *end; >> int iconindex; >> PROPSPEC ps; >> PROPVARIANT pv; >> - char *iconindexastring = co_strdupWtoA(iconindexstring); >> - sscanf(iconindexastring, "%d", &iconindex); >> - CoTaskMemFree(iconindexastring); >> + iconindex = strtolW(iconindexstring, &end, 10); > > If you don't need end value, you could pass NULL instead. Makes sense, thanks. > > Thanks, > Jacek > >

April 11, 2016

Re: [PATCH 2/2] ieframe: Convert to integer without additional string buffer allocation

by Jacek Caban

On 04/08/16 21:56, Nikolay Sivov wrote: > Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> > --- > dlls/ieframe/intshcut.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/dlls/ieframe/intshcut.c b/dlls/ieframe/intshcut.c > index 385b059..79458ac 100644 > --- a/dlls/ieframe/intshcut.c > +++ b/dlls/ieframe/intshcut.c > @@ -511,12 +511,11 @@ static HRESULT WINAPI PersistFile_Load(IPersistFile *pFile, LPCOLESTR pszFileNam > > if (get_profile_string(str_header, str_iconindex, pszFileName, &iconindexstring) == S_OK) > { > + WCHAR *end; > int iconindex; > PROPSPEC ps; > PROPVARIANT pv; > - char *iconindexastring = co_strdupWtoA(iconindexstring); > - sscanf(iconindexastring, "%d", &iconindex); > - CoTaskMemFree(iconindexastring); > + iconindex = strtolW(iconindexstring, &end, 10); If you don't need end value, you could pass NULL instead. Thanks, Jacek

April 11, 2016

Re: [PATCH 1/2] ieframe: Simplify IPersist_Load, make error handling more straightforward (Coverity)

by Jacek Caban

Hi Nikolay, On 04/08/16 21:56, Nikolay Sivov wrote: > + /* Now we're going to read in the iconfile and iconindex. > + If we don't find them, that's not a failure case -- it's possible > + that they just aren't in there. */ > + hr = IPropertySetStorage_Open(This->property_set_storage, &FMTID_Intshcut, > + STGM_READWRITE | STGM_SHARE_EXCLUSIVE, &pPropStg); It's an already existing bug, but we should handle Open failure here. Thanks, Jacek

April 11, 2016

Re: [PATCH 3/5] user32: Don't enable owner in DIALOG_DoDialogBox.

by Jacek Caban

On 04/11/16 14:24, Dmitry Timoshkov wrote: > Jacek Caban <jacek(a)codeweavers.com> wrote: > >>> Actually the fact that you can't reproduce the described above behaviour >>> with in your own tests tells quite a bit (yes, EndDialog() is not supposed >>> to enable a previously disabled owner under some conditions). >> Yes, that's the case if you reparent the dialog. My tests show that and >> implementation behaves as expected. We don't need DF_OWNERENABLED for that. > If you mean the trick with GWLP_HWNDPARENT then I'm not sure it's the best > or really correct way of solving the problem. But yes, it looks like a good > workaround. I'm going to ignore your comment about not being correct or being a workaround because they are not backed by any technical argument. All that's left is that it works as expected so your previous concern is already addressed by the patch. Thanks. Jacek

April 11, 2016

EVE using Wine on Mac

by Snorri Sturluson

About a month ago CCP announced support for running EVE Online on Mac using Wine (https://forums.eveonline.com/default.aspx?g=posts&m=6380823#post6380823) I am the primary developer on this project, and wanted to express my gratitude to the Wine community for making this possible. This experiment is going well - there have been some issues with getting Wine to run on all the different OS X versions and different hardware configurations out there, but that has largely been resolved (I think) with proper configuration of the build machine producing the Wine build. My plan is to proactively hunt down and fix any issues our Mac players run into, whether the fix entails digging into Wine source or our own codebase. Ideally everything we do in EVE should just work under Wine, but sometimes a workaround in our code may be easier than hunting down why Wine behaves different from Windows, but being able to debug both sides makes finding workarounds easier. Any fixes I come up with I will contribute to Wine, of course - it remains to be seen if they end up being very EVE specific, as EVE will of course be my main focus. My plan is also to document my experience along the way at http://ccpsnorlax.blogspot.is/ - if you find this interesting, please leave comments there. I hope that my initiative with this project meets with your approval, and look forward to working with the Wine codebase and you, the Wine developers in the future. Snorri Sturluson, Software Architect, CCP Games @CCP_Snorlax - http://ccpsnorlax.blogspot.is/

April 11, 2016

Re: [PATCH 3/5] user32: Don't enable owner in DIALOG_DoDialogBox.

by Dmitry Timoshkov

Jacek Caban <jacek(a)codeweavers.com> wrote: > > Actually the fact that you can't reproduce the described above behaviour > > with in your own tests tells quite a bit (yes, EndDialog() is not supposed > > to enable a previously disabled owner under some conditions). > > Yes, that's the case if you reparent the dialog. My tests show that and > implementation behaves as expected. We don't need DF_OWNERENABLED for that. If you mean the trick with GWLP_HWNDPARENT then I'm not sure it's the best or really correct way of solving the problem. But yes, it looks like a good workaround. -- Dmitry.

April 11, 2016

Re: [PATCH 3/5] user32: Don't enable owner in DIALOG_DoDialogBox.

by Jacek Caban

On 04/11/16 13:21, Dmitry Timoshkov wrote: > > Actually the fact that you can't reproduce the described above behaviour > with in your own tests tells quite a bit (yes, EndDialog() is not supposed > to enable a previously disabled owner under some conditions). Yes, that's the case if you reparent the dialog. My tests show that and implementation behaves as expected. We don't need DF_OWNERENABLED for that. > you decided to dismiss the cooperation request from Sebastian, I didn't dismiss it, I replied to his technical objections. I think I explained things enough given that he didn't have more comments. Jacek

April 11, 2016

Re: [PATCH 3/5] user32: Don't enable owner in DIALOG_DoDialogBox.

by Dmitry Timoshkov

Jacek Caban <jacek(a)codeweavers.com> wrote: > >> On 04/06/16 15:24, Jacek Caban wrote: > >>> Could you share those tests (WIP is fine) or run my patches against them? > >> Ping. Did you have a chance to run those tests? > > No. But since you already know that removing DF_OWNERENABLED handling > > is wrong it shouldn't be too hard to add appropriate tests. > > No, as far as I know removing is right and I added tests that reach > EnableWindow() in every possible code path. I know only that you said > it's wrong with no prove to back that. I guess that you misinterpreted > results of your own tests and the fact that you claim you didn't bother > to run my patches against those tests tells pretty much all about > quality and intentions of your reviews. Actually the fact that you can't reproduce the described above behaviour with in your own tests tells quite a bit (yes, EndDialog() is not supposed to enable a previously disabled owner under some conditions). You really shouldn't be surprised that much by the lack of help from me to reproduce this really simple thing: you decided to dismiss the cooperation request from Sebastian, and were pretty unpleasant during the whole conversation. -- Dmitry.

April 11, 2016