Re: [1/2] shlwapi: Fixed the todo blocks in the SHCreateStreamOnFile tests. [try 2]

[Alexandre Julliard]

"Reece Dunn" <msclrhd(a)googlemail.com> writes:
> @@ -120,12 +119,12 @@ static HRESULT WINAPI IStream_fnRead(IStream *iface, void* pv, ULONG cb, ULONG*
>    TRACE("(%p,%p,0x%08x,%p)\n", This, pv, cb, pcbRead);
>
> if (!pv)
> -    return STG_E_INVALIDPOINTER;
> +    return S_OK;

It's very unlikely that this is the correct behavior in all cases.
> @@ -459,7 +458,9 @@ HRESULT WINAPI SHCreateStreamOnFileEx(LPCWSTR lpszPath, DWORD dwMode,
>    switch(STGM_CREATE_MODE(dwMode))
>    {
>    case STGM_FAILIFTHERE:
> -    dwCreate = OPEN_EXISTING;
> +    if (bCreate && PathFileExistsW(lpszPath))
> +      return HRESULT_FROM_WIN32(ERROR_FILE_EXISTS);
> +    dwCreate = bCreate ? CREATE_ALWAYS : OPEN_EXISTING;

That case should be handled by CreateFile, you shouldn't check for the
file existence explicitly.

-- 
Alexandre Julliard
julliard(a)winehq.org

[1/2] shlwapi: Fixed the todo blocks in the SHCreateStreamOnFile tests. [try 2]

[Reece Dunn]

On 14/04/2008, Alexandre Julliard <julliard(a)winehq.org> wrote:
> "Reece Dunn" <msclrhd(a)googlemail.com> writes:
> > @@ -120,12 +119,12 @@ static HRESULT WINAPI IStream_fnRead(IStream *iface, void* pv, ULONG cb, ULONG*
> >    TRACE("(%p,%p,0x%08x,%p)\n", This, pv, cb, pcbRead);
> >
> > if (!pv)
> > -    return STG_E_INVALIDPOINTER;
> > +    return S_OK;
>
> It's very unlikely that this is the correct behavior in all cases.

That is what the tests appear to be implying. Unless they are
returning S_OK if there is no content to be read, but that wouldn't
make any sense, unless the parameter is passed directly to ReadFile
without any checking and that is how ReadFile behaves.

In that case, I'll leave this as a todo_wine block for the time being
until there is a more complete set of tests for the read method.
> > @@ -459,7 +458,9 @@ HRESULT WINAPI SHCreateStreamOnFileEx(LPCWSTR lpszPath, DWORD dwMode,
> >    switch(STGM_CREATE_MODE(dwMode))
> >    {
> >    case STGM_FAILIFTHERE:
> > -    dwCreate = OPEN_EXISTING;
> > +    if (bCreate && PathFileExistsW(lpszPath))
> > +      return HRESULT_FROM_WIN32(ERROR_FILE_EXISTS);
> > +    dwCreate = bCreate ? CREATE_ALWAYS : OPEN_EXISTING;
>
> That case should be handled by CreateFile, you shouldn't check for the
>  file existence explicitly.

The MSDN documentation for CreateFile
(http://msdn2.microsoft.com/en-us/library/aa363858.aspx) suggests that
CREATE_NEW has the desired behaviour. However, the Wine implementation
is not handling this correctly. Therefore, I'll improve the CreateFile
tests to test the CREATE_NEW behaviour.

I will also resubmit this patch with those tests marked as todo_wine.

- Reece

[1/2] shlwapi: Fixed the todo blocks in the SHCreateStreamOnFile tests. [try 2]

[Reece Dunn]

On 15/04/2008, Reece Dunn <msclrhd(a)googlemail.com> wrote:
> On 14/04/2008, Alexandre Julliard <julliard(a)winehq.org> wrote:
> > "Reece Dunn" <msclrhd(a)googlemail.com> writes:
> > > @@ -459,7 +458,9 @@ HRESULT WINAPI SHCreateStreamOnFileEx(LPCWSTR lpszPath, DWORD dwMode,
> > >    switch(STGM_CREATE_MODE(dwMode))
> > >    {
> > >    case STGM_FAILIFTHERE:
> > > -    dwCreate = OPEN_EXISTING;
> > > +    if (bCreate && PathFileExistsW(lpszPath))
> > > +      return HRESULT_FROM_WIN32(ERROR_FILE_EXISTS);
> > > +    dwCreate = bCreate ? CREATE_ALWAYS : OPEN_EXISTING;
> >
> > That case should be handled by CreateFile, you shouldn't check for the
> >  file existence explicitly.
>
> The MSDN documentation for CreateFile
>  (http://msdn2.microsoft.com/en-us/library/aa363858.aspx) suggests that
>  CREATE_NEW has the desired behaviour. However, the Wine implementation
>  is not handling this correctly. Therefore, I'll improve the CreateFile
>  tests to test the CREATE_NEW behaviour.

There were tests for the CREATE_NEW case in kernel32/tests/file.c,
verifying this behaviour. I tracked the problem down to a strange way
of converting a GetLastError status to a HRESULT. Fixing that got the
desired behaviour (see my new patch).

- Reece