Codereview request for 7171184: Make the failure message in 7168110 more specific

[Rob McKenna]

Apologies, I mixed up the docs with what I was seeing in testing. You're 
right, the sid might not always be 0. I've been speaking with Alan and 
we agree that its not worth pursuing given the direction this is headed.

     -Rob

On 24/05/12 00:58, David Holmes wrote:
> On 24/05/2012 7:02 AM, Alan Bateman wrote:
>> On 23/05/2012 20:37, Rob McKenna wrote:
>>> Hi Alan,
>>>
>>> Sorry, you've picked me up on that "if (" space a number of times.
>>> Thanks for pointing it out again.
>>>
>>> What should I be checking w.r.t. the ProcessIdToSessionId? It returns
>>> a 0 on failure so once the session id's are unequal or we've
>>> encountered an ERROR_ACCESS_DENIED we should really be good to go 
>>> right?
>>>
>>> Looking at that reminds me that I had got an && instead of an || for
>>> the session id / error check. Its a good point that if the
>>> ProcessIdToSessionId call fails for a reason outside ACCESS_DENIED, we
>>> won't know about it. I'm wondering if I should revert that now?
>> What you have will work but we shouldn't be comparing pSid to cSid
>> unless both calls to ProcessIdToSessionId succeed.
>
> Yeah I raised this previously with Rob and the response was that on 
> failure the sessionId will be set to zero but I do not see that in the 
> docs for ProcessIdToSessionId. The other assumptions was that 
> requesting the session Id for the current process could never fail - I 
> don't know how valid that is (these APIs don't document any actual 
> errors). If those two assumptions hold then the current code is 
> correct, otherwise ... something like this:
>
> if (( ProcessIdToSessionId( GetCurrentProcessId(), &cSid) != 0) {
>     BOOL result = ProcessIdToSessionId(GetProcessId(hProcess), &pSid);
>     if (result != 0) { // have valid sessionIds
>         if  (pSid != cSid)
>             JNU_ThrowIOException(env, "useful message");
>         // else same session so original memory error stands
>     } else if (GetLastError() == ERROR_ACCESS_DENIED) {
>         // implies that the sessions must be different
>         JNU_ThrowIOException(env, "useful message");
>      }
>      // else some other error getting other session id
> }
> // else some error getting current session Id
>
> But this needs to ensure the original error is not lost, and it would 
> be nice not to have two throws statements :(
>
> I get the feeling what seemed like a simple suggestion is getting out 
> of control. Feel free to drop this.
>
> David
> -----
>
>
>> -Alan.