Re: kernel32: Fix for WritePrivateProfileString Sharing Problem [1/2](ATTEMPT 2)

[Dmitry Timoshkov]

"Erich Hoover"  wrote:
> - oldError = GetLastError();
> - /* Read and Write sharing are necessary if a flush is performed on an open file */
> - hFile = CreateFileW(CurProfile->filename, GENERIC_WRITE, FILE_SHARE_READ | FILE_SHARE_WRITE, NULL, CREATE_ALWAYS,
>
> FILE_ATTRIBUTE_NORMAL, NULL);
>      if (hFile == INVALID_HANDLE_VALUE)
>      {
>          WARN("could not save profile file %s (error was %d)\n", debugstr_w(CurProfile->filename), GetLastError());
>          return FALSE;
>      }
> - /* The operation has succeed, do not over-write the error code */
> - SetLastError(oldError);

It's been said many times that if you need to save/restore last error value
you are doing something wrong.
> - /* Get* routines set S_OK on success, Write* routines return last error */
> - SetLastError(S_OK);

There is no point in setting or checking last error value on success in vast
majority of cases, do you have an app that depends on this? Besides, S_OK is
an OLE error code and can not be used in kernel.

kernel32: Fix for WritePrivateProfileString Sharing Problem [1/2](ATTEMPT 2)

[Erich Hoover]

On Wed, Apr 9, 2008 at 10:02 PM, Dmitry Timoshkov 
wrote:
> "Erich Hoover"  wrote:
> - oldError = GetLastError();
> > - /* Read and Write sharing are necessary if a flush is performed on
> >
> > an open file */
> > - hFile = CreateFileW(CurProfile->filename, GENERIC_WRITE,
> >
> > FILE_SHARE_READ | FILE_SHARE_WRITE, NULL, CREATE_ALWAYS,
> > FILE_ATTRIBUTE_NORMAL, NULL);
> >     if (hFile == INVALID_HANDLE_VALUE)
> >     {
> >         WARN("could not save profile file %s (error was %d)\n",
> > debugstr_w(CurProfile->filename), GetLastError());
> >         return FALSE;
> >     }
> > - /* The operation has succeed, do not over-write the error code */
> > - SetLastError(oldError);
>
> It's been said many times that if you need to save/restore last error
> value
> you are doing something wrong.

Well, I could call NtCreateFile directly so that the error code never gets
set in the first place...
> - /* Get* routines set S_OK on success, Write* routines return last
> > error */
> > - SetLastError(S_OK);
>
> There is no point in setting or checking last error value on success in
> vast
> majority of cases, do you have an app that depends on this? Besides, S_OK
> is
> an OLE error code and can not be used in kernel.

This was done to satisfy a problem pointed out in the previous attempt, that
patch broke a test in test_profile_sections where it checks that
GetPrivateProfileSectionA()'s GetLastError() is S_OK.  I figured that the
code should be consistent with the (existing) test, if you think it's more
appropriate then both could be changed to ERROR_SUCCESS.
> --
> Dmitry.