On 4/24/2014 00:10, Vincent Povirk wrote:
> +static HRESULT StorageImpl_GrabLocks(StorageImpl *This, DWORD openFlags)
> +{
> + HRESULT hr;
> + ULARGE_INTEGER offset;
> + ULARGE_INTEGER cb;
> + DWORD share_mode = STGM_SHARE_MODE(openFlags);
> +
> + /* Wrap all other locking inside a single lock so we can check ranges safely */
> + offset.QuadPart = 0x7fffff92;
> + cb.QuadPart = 1;
> + hr = StorageImpl_LockRegionSync(This, offset, cb, LOCK_ONLYONCE);
> +
> + /* If the ILockBytes doesn't support locking that's ok. */
> + if (FAILED(hr)) return S_OK;
> +
> + hr = S_OK;
> +
> + /* First check for any conflicting locks. */
> + if (SUCCEEDED(hr) && (openFlags & STGM_PRIORITY) == STGM_PRIORITY)
> + hr = StorageImpl_CheckLockRange(This, 0x80, 0x80, STG_E_LOCKVIOLATION);
> +
> + if (SUCCEEDED(hr) && (STGM_ACCESS_MODE(openFlags) != STGM_WRITE))
> + hr = StorageImpl_CheckLockRange(This, 0xbb, 0xce, STG_E_SHAREVIOLATION);
> +
> + if (SUCCEEDED(hr) && (STGM_ACCESS_MODE(openFlags) != STGM_READ))
> + hr = StorageImpl_CheckLockRange(This, 0xcf, 0xe2, STG_E_SHAREVIOLATION);
> +
> + if (SUCCEEDED(hr) && (share_mode == STGM_SHARE_DENY_READ || share_mode == STGM_SHARE_EXCLUSIVE))
> + hr = StorageImpl_CheckLockRange(This, 0x93, 0xa6, STG_E_LOCKVIOLATION);
> +
> + if (SUCCEEDED(hr) && (share_mode == STGM_SHARE_DENY_WRITE || share_mode == STGM_SHARE_EXCLUSIVE))
> + hr = StorageImpl_CheckLockRange(This, 0xa7, 0xba, STG_E_LOCKVIOLATION);
> +
> + /* Then grab our locks. */
> + if (SUCCEEDED(hr) && (openFlags & STGM_PRIORITY) == STGM_PRIORITY)
> + {
> + hr = StorageImpl_LockOne(This, 0x58, 0x7f);
> + if (SUCCEEDED(hr))
> + hr = StorageImpl_LockOne(This, 0x81, 0x91);
> + }
> +
> + if (SUCCEEDED(hr) && (STGM_ACCESS_MODE(openFlags) != STGM_WRITE))
> + hr = StorageImpl_LockOne(This, 0x93, 0xa6);
> +
> + if (SUCCEEDED(hr) && (STGM_ACCESS_MODE(openFlags) != STGM_READ))
> + hr = StorageImpl_LockOne(This, 0xa7, 0xba);
> +
> + if (SUCCEEDED(hr) && (share_mode == STGM_SHARE_DENY_READ || share_mode == STGM_SHARE_EXCLUSIVE))
> + hr = StorageImpl_LockOne(This, 0xbb, 0xce);
> +
> + if (SUCCEEDED(hr) && (share_mode == STGM_SHARE_DENY_WRITE || share_mode == STGM_SHARE_EXCLUSIVE))
> + hr = StorageImpl_LockOne(This, 0xcf, 0xe2);
> +
> + offset.QuadPart = 0x7fffff92;
> + cb.QuadPart = 1;
> + ILockBytes_UnlockRegion(This->lockBytes, offset, cb, LOCK_ONLYONCE);
> +
> + return hr;
Maybe it'd be better to add some symbols for all these magic numbers?
They are at least used twice in this functions, didn't look at others.
Also it looks like it's possible to collapse it a bit, for example
STGM_SHARE_EXCLUSIVE could check whole 0x93-0xba range with
a single code, if I'm reading this right.