Re: ole32: Accept STG_E_UNIMPLEMENTEDFUNCTION when ILockBytes implementation doesn't support locking.

[Nikolay Sivov]

On 03.07.2015 10:27, Dmitry Timoshkov wrote:
> Some custom implementations return it instead of STG_E_INVALIDFUNCTION.
>
> dlls/ole32/storage32.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/dlls/ole32/storage32.c b/dlls/ole32/storage32.c
> index 220e17e..aa53758 100644
> --- a/dlls/ole32/storage32.c
> +++ b/dlls/ole32/storage32.c
> @@ -5015,7 +5015,7 @@ static HRESULT StorageImpl_GrabLocks(StorageImpl *This, DWORD openFlags)
>       hr = StorageImpl_LockRegionSync(This, offset, cb, LOCK_ONLYONCE);
>   /* If the ILockBytes doesn't support locking that's ok. */
> - if (hr == STG_E_INVALIDFUNCTION) return S_OK;
> -
>
> if (hr == STG_E_INVALIDFUNCTION || hr == STG_E_UNIMPLEMENTEDFUNCTION) return S_OK;
>  else if (FAILED(hr)) return hr;
>
> hr = S_OK;

It looks like StorageImpl_LockRegionSync should be fixed instead to 
filter some error codes. Could you add a test with external ILockBytes 
implementation that does that?

ole32: Accept STG_E_UNIMPLEMENTEDFUNCTION when ILockBytes implementation doesn't support locking.

[Dmitry Timoshkov]

Nikolay Sivov  wrote:
> >   /* If the ILockBytes doesn't support locking that's ok. */
> > - if (hr == STG_E_INVALIDFUNCTION) return S_OK;
> > -
> >
> > if (hr == STG_E_INVALIDFUNCTION || hr == STG_E_UNIMPLEMENTEDFUNCTION) return S_OK;
> >  else if (FAILED(hr)) return hr;
> >
> > hr = S_OK;
>
> It looks like StorageImpl_LockRegionSync should be fixed instead to 
> filter some error codes. Could you add a test with external ILockBytes 
> implementation that does that?

Thers is no any comment or a test case about accepting ILockByte failures
neither in the original commit 65887802c502c4eeeb3fc905990e3e2f4548a482,
nor in the next one 1645f7b9e30e01bc92a8bfd1febe76d4856e046e which limits
accepted error codes to STG_E_INVALIDFUNCTION.

Besides, my patch fixes a regression caused by IStorage locking rewrite,
and should be pretty obvious without a test case. With this regression
fixed an application that I have here can open its database files again
as it was before.

If you really insist on a test case, please ask Vincent, he is the original
authour of this implementation.

ole32: Accept STG_E_UNIMPLEMENTEDFUNCTION when ILockBytes implementation doesn't support locking.

[Nikolay Sivov]

On 04.07.2015 7:40, Dmitry Timoshkov wrote:
> Nikolay Sivov  wrote:
> > >    /* If the ILockBytes doesn't support locking that's ok. */
> > > - if (hr == STG_E_INVALIDFUNCTION) return S_OK;
> > > -
> > >
> > > if (hr == STG_E_INVALIDFUNCTION || hr == STG_E_UNIMPLEMENTEDFUNCTION) return S_OK;
> > >   else if (FAILED(hr)) return hr;
> > >
> > > hr = S_OK;
> >
> > It looks like StorageImpl_LockRegionSync should be fixed instead to
> > filter some error codes. Could you add a test with external ILockBytes
> > implementation that does that?
>
> Thers is no any comment or a test case about accepting ILockByte failures
> neither in the original commit 65887802c502c4eeeb3fc905990e3e2f4548a482,
> nor in the next one 1645f7b9e30e01bc92a8bfd1febe76d4856e046e which limits
> accepted error codes to STG_E_INVALIDFUNCTION.

Well it doesn't matter, we don't have tests for many things, it doesn't 
mean we shouldn't ever think about having one.
> Besides, my patch fixes a regression caused by IStorage locking rewrite,
> and should be pretty obvious without a test case. With this regression
> fixed an application that I have here can open its database files again
> as it was before.
>
> If you really insist on a test case, please ask Vincent, he is the original
> authour of this implementation.

He's not the one sending this, so why should he be writing tests for 
your changes?

ole32: Accept STG_E_UNIMPLEMENTEDFUNCTION when ILockBytes implementation doesn't support locking.

[Dmitry Timoshkov]

Nikolay Sivov  wrote:
> > > >    /* If the ILockBytes doesn't support locking that's ok. */
> > > > - if (hr == STG_E_INVALIDFUNCTION) return S_OK;
> > > > -
> > > >
> > > > if (hr == STG_E_INVALIDFUNCTION || hr == STG_E_UNIMPLEMENTEDFUNCTION) return S_OK;
> > > >   else if (FAILED(hr)) return hr;
> > > >
> > > > hr = S_OK;
> > >
> > > It looks like StorageImpl_LockRegionSync should be fixed instead to
> > > filter some error codes. Could you add a test with external ILockBytes
> > > implementation that does that?
> >
> > Thers is no any comment or a test case about accepting ILockByte failures
> > neither in the original commit 65887802c502c4eeeb3fc905990e3e2f4548a482,
> > nor in the next one 1645f7b9e30e01bc92a8bfd1febe76d4856e046e which limits
> > accepted error codes to STG_E_INVALIDFUNCTION.
>
> Well it doesn't matter, we don't have tests for many things, it doesn't 
> mean we shouldn't ever think about having one.

Thinking is good I guess.
> > Besides, my patch fixes a regression caused by IStorage locking rewrite,
> > and should be pretty obvious without a test case. With this regression
> > fixed an application that I have here can open its database files again
> > as it was before.
> >
> > If you really insist on a test case, please ask Vincent, he is the original
> > authour of this implementation.
>
> He's not the one sending this, so why should he be writing tests for 
> your changes?

My patch just adds another obvious case of a possible ILockBytes failure.
If the tests were not needed for an already existing case then my patch
changes nothing in that regard. Fixing a regression with a pretty obvious
fix shouldn't deserve so much of salt from your side Nikolay.

ole32: Accept STG_E_UNIMPLEMENTEDFUNCTION when ILockBytes implementation doesn't support locking.

[Nikolay Sivov]

On 04.07.2015 11:46, Dmitry Timoshkov wrote:
> > > Besides, my patch fixes a regression caused by IStorage locking rewrite,
> > > and should be pretty obvious without a test case. With this regression
> > > fixed an application that I have here can open its database files again
> > > as it was before.
> > >
> > > If you really insist on a test case, please ask Vincent, he is the original
> > > authour of this implementation.
> >
> > He's not the one sending this, so why should he be writing tests for
> > your changes?
>
> My patch just adds another obvious case of a possible ILockBytes failure.
> If the tests were not needed for an already existing case then my patch
> changes nothing in that regard.

Asking for a test especially if you found a regression is not uncommon. 
Your change brings inconsistency in error handling as I mentioned 
already, and it's not obvious at all what errors should be ignored. I 
really don't understand why are you so resistant sometimes.
> Fixing a regression with a pretty obvious
> fix shouldn't deserve so much of salt from your side Nikolay.

...

ole32: Accept STG_E_UNIMPLEMENTEDFUNCTION when ILockBytes implementation doesn't support locking.

[Dmitry Timoshkov]

Nikolay Sivov  wrote:
> > > > Besides, my patch fixes a regression caused by IStorage locking rewrite,
> > > > and should be pretty obvious without a test case. With this regression
> > > > fixed an application that I have here can open its database files again
> > > > as it was before.
> > > >
> > > > If you really insist on a test case, please ask Vincent, he is the original
> > > > authour of this implementation.
> > >
> > > He's not the one sending this, so why should he be writing tests for
> > > your changes?
> >
> > My patch just adds another obvious case of a possible ILockBytes failure.
> > If the tests were not needed for an already existing case then my patch
> > changes nothing in that regard.
>
> Asking for a test especially if you found a regression is not uncommon. 
> Your change brings inconsistency in error handling as I mentioned 
> already, and it's not obvious at all what errors should be ignored.

What inconsistency do you mean?
> I really don't understand why are you so resistant sometimes.

I just really don't undrestand why you are so selective sometimes.
Certainly I can understand your desire to have a test for something,
what I don't get is why your desire didn't appear earlier for the same
piece of code but sent by somebody else. Besides my patch is so obvious
and trivial, that requesting to write completely new set of tests is not
justified at all IMHO.

I'll probably can have stab at it when I have a bit of free time, but this
should be a separate and not related patch set.
> > Fixing a regression with a pretty obvious
> > fix shouldn't deserve so much of salt from your side Nikolay.

There is not much to add.

ole32: Accept STG_E_UNIMPLEMENTEDFUNCTION when ILockBytes implementation doesn't support locking.

[Vincent Povirk]

This looks OK to me, and if it fixes a regression I don't think we
need a test case (though it never hurts).

I don't think StorageImpl_LockRegionSync should be filtering these
codes (the caller needs to know the difference between a failure and
unimplemented function), though we should probably also fix the call
inside StorageImpl_LockTransaction, the check in UnlockTransaction,
and the check in StorageImpl_LockRegionSync. And it seems we also
check for E_NOTIMPL when we call LockTransaction..

Maybe an HR_IS_UNIMPLEMENTED macro would be warranted?

ole32: Accept STG_E_UNIMPLEMENTEDFUNCTION when ILockBytes implementation doesn't support locking.

[Dmitry Timoshkov]

Vincent Povirk  wrote:
> This looks OK to me, and if it fixes a regression I don't think we
> need a test case (though it never hurts).

Thanks Vincent.
> I don't think StorageImpl_LockRegionSync should be filtering these
> codes (the caller needs to know the difference between a failure and
> unimplemented function), though we should probably also fix the call
> inside StorageImpl_LockTransaction, the check in UnlockTransaction,
> and the check in StorageImpl_LockRegionSync. And it seems we also
> check for E_NOTIMPL when we call LockTransaction..
>
> Maybe an HR_IS_UNIMPLEMENTED macro would be warranted?

Not a macro, but probably an inline helper? Otherwise seems reasonable
thing to do to me.