Re: [4/4] ole32: Initial storage file locking implementation.

[Nikolay Sivov]

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.

[4/4] ole32: Initial storage file locking implementation.

[Vincent Povirk]

> 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.

Maybe. I think the reason that didn't occur to me is that their layout
is important, and symbols would obscure that. For instance, the
priority ranges are right next to the commit and check locks, so it's
possible to lock that entire range in one command.

Then again, having some symbols you could look up (and correct when we
learn more) is probably better than a block comment of what I think
the layout looks like.
> 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.

Maybe, but we can't collapse the LockOne stuff, and I like having
parallels between the CheckLockRange and LockOne parts.