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.stu...@gmail.com >> <mailto:thomas.stu...@gmail.com>> wrote: >> >> Hi Staffan, >> >> this looks fine now, thanks! >> >> ..Thomas >> >> On Thu, Dec 10, 2015 at 3:42 PM, Staffan Larsen >> <staffan.lar...@oracle.com <mailto:staffan.lar...@oracle.com>> wrote: >> >> Hi Thomas, >> >>> On 10 dec. 2015, at 15:27, Thomas Stüfe <thomas.stu...@gmail.com >>> <mailto:thomas.stu...@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.lar...@oracle.com <mailto:staffan.lar...@oracle.com>> wrote: >>> >>> Hi Thommas, >>> >>>> On 10 dec. 2015, at 14:49, Thomas Stüfe >>>> <thomas.stu...@gmail.com <mailto:thomas.stu...@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.lar...@oracle.com >>>> <mailto:staffan.lar...@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.