Nikolay Sivov bunglehead@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.