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

[Rob McKenna]

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?

     -Rob

On 23/05/12 19:58, Alan Bateman wrote:
> On 23/05/2012 18:44, Rob McKenna wrote:
>> Hi folks,
>>
>> David Holmes suggested a rewrite of the original bug (7168110) so 
>> I've put together the following:
>>
>> http://cr.openjdk.java.net/~robm/7171184/webrev.01/
>>
>> It will narrow down the cause of the error and return a more specific 
>> message when there is a problem attaching jstack to a process created 
>> in another session.
>>
>> Let me know what you think,
> I wasn't aware of ProcessIdToSessionId but seems a good idea.
>
> I assume you should check the return value from ProcessIdToSessionId 
> before looking at the session id or last error.
>
> Minor nit but you are missing a space in "if(" (line 479)
>
> One idea to avoid duplicating the cleanup (VirtualFreeEx) code is:
>
> if (GetLastError() == ERROR_NOT_ENOUGH_MEMORY) {
>     if (( ProcessIdToSessionId( GetCurrentProcessId(), &cSid) != 0) {
> BOOL result = ProcessIdToSessionId(GetProcessId(hProcess), &pSid);
>         if ((result != 0 && (pSid != cSid)) || GetLastError() == 
> ERROR_ACCESS_DENIED)) {
> JNU_ThrowIOException(env, "useful message");
>             SetLastError(0);
>         }
>     }
> }
> if (GetLastError() != 0)
> JNU_ThrowIOExceptionWithLastError(env, "CreateRemoteThread failed");
>
> Just a suggestion so that you have one return path. The one downside 
> (and you've got this issue already) is that if ProcessIdToSessionId 
> fails with something other access denied then the error for the 
> exception wont' be right.
>
> -Alan.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20120523/b70db0ca/attachment.html