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

Re: wininet: Constify a formal parameter of CommitURLCacheEntry{A|W}() (Resend)

by James Hawkins

On 2/26/07, Andrew Talbot <Andrew.Talbot(a)talbotville.com> wrote: > James Hawkins wrote: > > > On 2/26/07, Andrew Talbot <Andrew.Talbot(a)talbotville.com> wrote: > >> What is wrong with this patch, please? > >> > > > > This isn't how it's defined in the SDK headers. > > > > Am I right in thinking that for non-embedded-CE versions of Windows the > lpHeaderInfo variable of CommitURLCacheEntry() is now a LPCTSTR? In which > case, I presume that the comment about the ascii version requiring a LPBYTE > is now wrong: the "A" version should now have a LPCSTR and the "W" version, > a LPCWSTR. > #ifndef USE_FIXED_COMMIT_URL_CACHE_ENTRY // Temporary state of affairs until we reconcile our apis. // Why are we doing this? HeaderInfo _should_ be string data. // However, one group is passing binary data instead. For the // unicode api, we've decided to disallow this, but this // brings up an inconsistency between the u and a apis, which // is undesirable. // For Beta 1, we'll go with this behaviour, but in future releases // we want to make these apis consistent. BOOLAPI CommitUrlCacheEntryA( __in LPCSTR lpszUrlName, __in_opt LPCSTR lpszLocalFileName, __in FILETIME ExpireTime, __in FILETIME LastModifiedTime, __in DWORD CacheEntryType, __in_ecount_opt(dwHeaderSize) LPBYTE lpHeaderInfo, __in_opt DWORD dwHeaderSize, __reserved LPCSTR lpszFileExtension, __in_opt LPCSTR lpszOriginalUrl ); BOOLAPI CommitUrlCacheEntryW( __in LPCWSTR lpszUrlName, __in_opt LPCWSTR lpszLocalFileName, __in FILETIME ExpireTime, __in FILETIME LastModifiedTime, __in DWORD CacheEntryType, __in_ecount_opt(dwHeaders) LPWSTR lpszHeaderInfo, __in DWORD dwHeaders, __reserved LPCWSTR lpszFileExtension, __in_opt LPCWSTR lpszOriginalUrl ); #ifdef UNICODE #define CommitUrlCacheEntry CommitUrlCacheEntryW #else #define CommitUrlCacheEntry CommitUrlCacheEntryA #endif #else CommitUrlCacheEntryA( __in LPCSTR lpszUrlName, __in_opt LPCSTR lpszLocalFileName, __in FILETIME ExpireTime, __in FILETIME LastModifiedTime, __in DWORD CacheEntryType, __in_ecount_opt(dwHeaderSize) LPCSTR lpHeaderInfo, __in DWORD dwHeaderSize, __reserved LPCSTR lpszFileExtension, __in_opt LPCSTR lpszOriginalUrl ); CommitUrlCacheEntryW( __in LPCWSTR lpszUrlName, __in_opt LPCWSTR lpszLocalFileName, __in FILETIME ExpireTime, __in FILETIME LastModifiedTime, __in DWORD CacheEntryType, __in_ecount_opt(dwHeaderSize) LPCWSTR lpHeaderInfo, __in DWORD dwHeaderSize, __reserved LPCWSTR lpszFileExtension, __in_opt LPCWSTR lpszOriginalUrl ); #ifdef UNICODE #define CommitUrlCacheEntry CommitUrlCacheEntryW #else #define CommitUrlCacheEntry CommitUrlCacheEntryA #endif // !UNICODE #endif -- James Hawkins

Feb. 26, 2007

Re: wininet: Constify a formal parameter of CommitURLCacheEntry{A|W}() (Resend)

by Andrew Talbot

Andrew Talbot wrote: > ... but the corresponding parameter of > CommitURLCacheEntry() [...] ought to be a LPTSTR.... Or rather, a LPCTSTR. So, is there any scope to constify, here? -- A.

Feb. 26, 2007

Re: wininet: Constify a formal parameter of CommitURLCacheEntry{A|W}() (Resend)

by Andrew Talbot

The problem seems to be that the INTERNET_CACHE_ENTRY_INFO structure has a member lpHeaderInfo that is a LPBYTE, but the corresponding parameter of CommitURLCacheEntry() - sorry, I truncated its name in the original posting - ought to be a LPTSTR. It is not yet clear to me what is required. -- Andy.

Feb. 26, 2007

Re: wininet: Constify a formal parameter of CommitURLCacheEntry{A|W}() (Resend)

by Andrew Talbot

James Hawkins wrote: > On 2/26/07, Andrew Talbot <Andrew.Talbot(a)talbotville.com> wrote: >> What is wrong with this patch, please? >> > > This isn't how it's defined in the SDK headers. > Am I right in thinking that for non-embedded-CE versions of Windows the lpHeaderInfo variable of CommitURLCacheEntry() is now a LPCTSTR? In which case, I presume that the comment about the ascii version requiring a LPBYTE is now wrong: the "A" version should now have a LPCSTR and the "W" version, a LPCWSTR. -- Andy.

Feb. 26, 2007

Re: Add localizable strings support for popup menu in selection tree control.

by James Hawkins

On 2/26/07, Крылов Николай <krna(a)rambler.ru> wrote: > diff --git a/dlls/msi/dialog.c b/dlls/msi/dialog.c > index 35a192c..3826326 100644 > --- a/dlls/msi/dialog.c > +++ b/dlls/msi/dialog.c > @@ -1755,17 +1755,41 @@ msi_seltree_popup_menu( HWND hwnd, INT x > HMENU hMenu; > INT r; > > + static const WCHAR szMenuLocal[] = > {'M','e','n','u','L','o','c','a','l',0}; > + static const WCHAR szMenuAllLocal[] = > {'M','e','n','u','A','l','l','L','o','c','a','l',0}; > + static const WCHAR szMenuAdvertise[] = > {'M','e','n','u','A','d','v','e','r','t','i','s','e',0}; > + static const WCHAR szMenuAbsent[] = > {'M','e','n','u','A','b','s','e','n','t',0}; > + /* If msi_dialog_get_uitext returns NULL - use default strings: */ > + static const WCHAR szMenuLocalDef[] = > {'I','n','s','t','a','l','l',' ','f','e','a','t','u','r','e',' ', > 'l','o','c','a','l','l','y',0}; > + static const WCHAR szMenuAllLocalDef[] = > {'I','n','s','t','a','l','l',' ','e','n','t','i','r','e',' > ','f','e','a','t','u','r','e',0}; > + static const WCHAR szMenuAdvertiseDef[] = > {'I','n','s','t','a','l','l',' ','o','n',' ','d','e','m','a','n','d',0}; > + static const WCHAR szMenuAbsentDef[] = {'D','o','n','\'','t',' > ','i','n','s','t','a','l','l',0}; > + > + LPWSTR szLocal,szAllLocal,szAdvertise,szAbsent; > + struct msi_selection_tree_info *info; > + > + info = GetPropW(hwnd, szButtonData); > + > + szLocal= msi_dialog_get_uitext( info->dialog, szMenuLocal ); > + szAllLocal= msi_dialog_get_uitext( info->dialog, szMenuAllLocal ); > + szAdvertise= msi_dialog_get_uitext( info->dialog, szMenuAdvertise > ); > + szAbsent= msi_dialog_get_uitext( info->dialog, szMenuAbsent ); > + > /* create a menu to display */ > hMenu = CreatePopupMenu(); > > - /* FIXME: load strings from resources */ > - AppendMenuA( hMenu, MF_ENABLED, INSTALLSTATE_LOCAL, "Install > feature locally"); > - AppendMenuA( hMenu, MF_GRAYED, 0x1000, "Install entire feature"); > - AppendMenuA( hMenu, MF_ENABLED, INSTALLSTATE_ADVERTISED, "Install > on demand"); > - AppendMenuA( hMenu, MF_ENABLED, INSTALLSTATE_ABSENT, "Don't > install"); > + /* Use strings from resources */ > + AppendMenuW( hMenu, MF_ENABLED, INSTALLSTATE_LOCAL, > (szLocal==NULL)?szMenuLocalDef:szLocal ); > + AppendMenuW( hMenu, MF_GRAYED, 0x1000, > (szAllLocal==NULL)?szMenuAllLocalDef:szAllLocal ); > + AppendMenuW( hMenu, MF_ENABLED, INSTALLSTATE_ADVERTISED, > (szAdvertise==NULL)?szMenuAdvertiseDef:szAdvertise ); > + AppendMenuW( hMenu, MF_ENABLED, INSTALLSTATE_ABSENT, > (szAbsent==NULL)?szMenuAbsentDef:szAbsent ); > r = TrackPopupMenu( hMenu, TPM_LEFTALIGN | TPM_TOPALIGN | > TPM_RETURNCMD, > x, y, 0, hwnd, NULL ); > DestroyMenu( hMenu ); > + if(szLocal) msi_free(szLocal); > + if(szAllLocal) msi_free(szAllLocal); > + if(szAdvertise) msi_free(szAdvertise); > + if(szAbsent) msi_free(szAbsent); > return r; > } > Don't check values for NULL before freeing them. The check is redundant, and we spent a long time removing such checks. -- James Hawkins

Feb. 26, 2007

Re: kernel32: Implement ReplaceFileA/ReplaceFileW

by Felix Nawothnig

Erich Hoover wrote: > I assume you're referring to the file existence check and file delete, > followed by the actual copy and move. I implemented these checks in > this manner in order to provide error codes consistent with the > documentation. I am not incredibly familiar with the Wine internals, > but it looks to me like the functions called should handle a > simultaneous call from another thread (without unnecessary code > duplication). For example, if the "replaced" file goes missing between > the check and the CopyFileW call then CopyFileW will return an error. > If the "replaced" file appears between the DeleteFileW call and the > MoveFileExW call (since the MOVEFILE_REPLACE_EXISTING flag is not set), > then the MoveFileExW call will fail. The documentation's expected error > codes for ReplaceFile seem to be consistent with these potential > outcomes. Reading MSDN that doesn't seem to be the case. The way I see it all those errors documented could be caused by file permisions. MSDN doesn't say anything about race cons. Some notes about the patch: + /* Maker sure the "replacement" file exists before continuing */ + if(!RtlDoesFileExists_U( lpReplacementFileName )) + { + SetLastError(ERROR_INVALID_PARAMETER); + return FALSE; + } This would probably the right place to lock the file... + /* + * If the user wants a backup then that needs to be performed first + * * Do not fail to backup if "replaced" does not exist + */ + if( lpBackupFileName && RtlDoesFileExists_U( lpReplacedFileName ) ) The call to RtlDoesFileExists_U() is redundant and introduces a race con. CopyFileW() will check for existence, check GLE afterwards. + { + /* If an existing backup exists then copy over it */ + */ + if(!CopyFileW( lpReplacedFileName, lpBackupFileName, FALSE )) + { + SetLastError(ERROR_INVALID_PARAMETER); + return FALSE; + } + } + /* + * Now that the backup has been performed (if requested), copy the replacement + * into place (make sure we can delete the original file first) + * * Do not fail if "replaced" does not exist + */ + if(RtlDoesFileExists_U( lpReplacedFileName ) && !DeleteFileW( lpReplacedFileName )) Same here, DeleteFile() will check for existence. + { + SetLastError(ERROR_UNABLE_TO_REMOVE_REPLACED); + return FALSE; + } + if(!MoveFileExW( lpReplacementFileName, lpReplacedFileName, flags )) Couldn't you just get rid of the DeleteFile, check why MoveFileEx() failed and get rid of the call to RtlDoesFileExists_U(lpReplacedFileName)? + { + SetLastError(ERROR_UNABLE_TO_MOVE_REPLACEMENT); + return FALSE; + } + /* All done, file replacement successful */ + return TRUE;

Feb. 26, 2007

Re: [1/12] - [12/12] WineD3D (multithreading)

by Mirek

Stefan Dösinger napsal(a): > Am Montag 26 Februar 2007 19:58 schrieb Mirek: >> Hi, this series of patches broke some textures in TES IV: Oblivion, >> should I create bug in winebugs? I tried some other apps, they looks ok. >> (bug is here after patch 7/12 and 8/12). > Which patch breaks oblivion? Can you describe the kind of breakage? > > *cough* That game seems to be really picky *cough* Patch number 7 and 8 (after patch 7 game crashed, after patch 8 it works normal but with graphical problems). I have Core 2 Duo CPU and SMP enabled, Nvidia GF6800GS with latest stable drivers. Here are some screenshots: http://headline.czela.net/Mirek/wine/TES%20IV:%20Oblivion/regression/ 2007-02-26-224223.jpg is for comparsion. This is realy strange, because after I do some spells (graphical efects) in game it just recover to normal state (everything looks like before).

Feb. 26, 2007

Re: wininet: Constify a formal parameter of CommitURLCache{A|W}() (Resend)

by James Hawkins

On 2/26/07, Andrew Talbot <Andrew.Talbot(a)talbotville.com> wrote: > What is wrong with this patch, please? > This isn't how it's defined in the SDK headers. -- James Hawkins

Feb. 26, 2007

Re: [1/12] - [12/12] WineD3D (multithreading)

by Stefan Dösinger

Am Montag 26 Februar 2007 19:58 schrieb Mirek: > Hi, this series of patches broke some textures in TES IV: Oblivion, > should I create bug in winebugs? I tried some other apps, they looks ok. > (bug is here after patch 7/12 and 8/12). Which patch breaks oblivion? Can you describe the kind of breakage? *cough* That game seems to be really picky *cough*

Feb. 26, 2007

Re: kernel32: Implement ReplaceFileA/ReplaceFileW

by Erich Hoover

I assume you're referring to the file existence check and file delete, followed by the actual copy and move. I implemented these checks in this manner in order to provide error codes consistent with the documentation. I am not incredibly familiar with the Wine internals, but it looks to me like the functions called should handle a simultaneous call from another thread (without unnecessary code duplication). For example, if the "replaced" file goes missing between the check and the CopyFileW call then CopyFileW will return an error. If the "replaced" file appears between the DeleteFileW call and the MoveFileExW call (since the MOVEFILE_REPLACE_EXISTING flag is not set), then the MoveFileExW call will fail. The documentation's expected error codes for ReplaceFile seem to be consistent with these potential outcomes. I apologize if there's something I'm missing here, please let me know. Erich Hoover ehoover(a)mines.edu On 2/26/07, Felix Nawothnig <flexo(a)holycrap.org> wrote: > > Erich Hoover wrote: > > Real Name: > > Erich Hoover > > Description: > > Implements the functions ReplaceFileA and ReplaceFileW in kernel32 > > (Bug #7544). Also provides conformance test code to ensure proper > > functionality. > > Changelog: > > kernel32: Implement ReplaceFileA/ReplaceFileW > > Your patch seems to introduce lots of race cons. It's possible these > exist on Win32 too but can't we avoid them? >

Feb. 26, 2007