Re: ole32: Accept STG_E_UNIMPLEMENTEDFUNCTION when ILockBytes implementation doesn't support locking.
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?
Nikolay Sivov <bunglehead(a)gmail.com> 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. -- Dmitry.
On 04.07.2015 7:40, Dmitry Timoshkov wrote:
Nikolay Sivov <bunglehead(a)gmail.com> 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?
Nikolay Sivov <bunglehead(a)gmail.com> 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. -- Dmitry.
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.
...
Nikolay Sivov <bunglehead(a)gmail.com> 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. -- Dmitry.
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?
Vincent Povirk <madewokherd(a)gmail.com> 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. -- Dmitry.
participants (3)
-
Dmitry Timoshkov -
Nikolay Sivov -
Vincent Povirk