RFR: JDK-8145099 Better error message when SA can't attach to a process

[Dmitry Samersoff]

Staffan,

1.

strerror_r comes in two versions - returning string (GNU) and returning
int (XSI)

To make developers live more interesting GNU version doesn't guarantee
to fill buf with appropriate string, so you can't just void return value.

Default version tends to be XSI now days and I'm not sure that build
system add -D_GNU_SOURCE on all platforms.

So you have to add something like

#if _POSIX_C_SOURCE >= 200112L || _XOPEN_SOURCE >= 600) && ! _GNU_SOURCE
 use XSI strerror_r
#else
 use GNU strerror_r
#endif

Or just don't care of threads and use plain strerror - it widely used in
hotspot and it doesn't crash on a race.

2.

err_buff remains empty if the attach fails at ll.412 of ps_proc.c,
but see below.

3. (* just an opinion *)

Extra parameters to ptrace_attach/PGrab linux makes in different from
other platforms.

So I would prefer to have it unchanged - you can either mimic Solaris
behavior and add Pgrab_error() function or just move all strerror staff
to LinuxDebuggerLocal_attach0 - errno from ptrace_attach/Pgrab is
available there.

(except calloc case, but if we can't allocate couple of bytes for struct
ps_prochandle we most likely will not see the error anyway).


-Dmitry


On 2015-12-10 18:10, Staffan Larsen wrote:
> Thanks!
> 
>> On 10 dec. 2015, at 15:56, Thomas Stüfe <thomas.stuefe at gmail.com
>> <mailto:thomas.stuefe at gmail.com>> wrote:
>>
>> Hi Staffan,
>>
>> this looks fine now, thanks!
>>
>> ..Thomas
>>
>> On Thu, Dec 10, 2015 at 3:42 PM, Staffan Larsen
>> <staffan.larsen at oracle.com <mailto:staffan.larsen at oracle.com>> wrote:
>>
>>     Hi Thomas, 
>>
>>>     On 10 dec. 2015, at 15:27, Thomas Stüfe <thomas.stuefe at gmail.com
>>>     <mailto:thomas.stuefe at gmail.com>> wrote:
>>>
>>>     Hi Staffan,
>>>
>>>     thanks!
>>>
>>>     It also just occurred to me that strerror is not considered
>>>     threadsafe. Maybe strerror_r() would a better option (depending
>>>     on the version you use you would have to check the return value
>>>     for NULL, though).
>>
>>     Updated here: http://cr.openjdk.java.net/~sla/JDK-8145099/webrev.03/
>>
>>     Thanks,
>>     /Staffan
>>
>>>
>>>     sorry for this piecemeal review.
>>>
>>>     Kind Regards, Thomas
>>>
>>>
>>>
>>>     On Thu, Dec 10, 2015 at 3:22 PM, Staffan Larsen
>>>     <staffan.larsen at oracle.com <mailto:staffan.larsen at oracle.com>> wrote:
>>>
>>>         Hi Thommas,
>>>
>>>>         On 10 dec. 2015, at 14:49, Thomas Stüfe
>>>>         <thomas.stuefe at gmail.com <mailto:thomas.stuefe at gmail.com>>
>>>>         wrote:
>>>>
>>>>         Hi Stefan,
>>>>
>>>>         Disclaimer: not a "R"eviewer.
>>>>
>>>>         http://cr.openjdk.java.net/~sla/JDK-8145099/webrev.01/agent/src/os/linux/LinuxDebuggerLocal.c.udiff.html
>>>>
>>>>         I am not sure why you pass sizeof(err_buf) - 1 instead of
>>>>         sizeof(err_buf) to Pgrab and sizeof(msg) - 1 instead of
>>>>         sizeof(msg) to snprintf? snprintf will always zero terminate
>>>>         in case of truncation, at least on posix platforms.
>>>
>>>         Good point. I was just being overly cautious without thinking.
>>>
>>>         Updated
>>>         webrev: http://cr.openjdk.java.net/~sla/JDK-8145099/webrev.02/
>>>
>>>         Thanks,
>>>         /Staffan
>>>
>>>>
>>>>         Otherwise this looks good.
>>>>
>>>>         Kind Regards, Thomas
>>>>
>>>>
>>>>         On Thu, Dec 10, 2015 at 2:19 PM, Staffan Larsen
>>>>         <staffan.larsen at oracle.com
>>>>         <mailto:staffan.larsen at oracle.com>> wrote:
>>>>
>>>>             Please review this patch to add a better error message
>>>>             to SA when it fails to attach to a process on linux.
>>>>             Currently the error says "Can't attach to the process”.
>>>>             After this change the message will look something like:
>>>>             "Can't attach to the process: ptrace(PTRACE_ATTACH, ..)
>>>>             failed for 28417: Operation not permitted”
>>>>
>>>>             webrev: http://cr.openjdk.java.net/~sla/JDK-8145099/webrev.01/
>>>>             bug: https://bugs.openjdk.java.net/browse/JDK-8145099
>>>>
>>>>             Thanks,
>>>>             /Staffan
>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>
>>
> 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.