> On 10 dec. 2015, at 23:20, Dmitry Samersoff <dmitry.samers...@oracle.com> > wrote: > > 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
We are getting the _GNU_SOURCE version when building. If that changes in the makefiles somewhere so that we get the XSI version, then this code should not compile - it should issue a warning when assigning an int to char*. > > 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. It should get filled in in the ptrace_attach() call. > > 3. (* just an opinion *) > > Extra parameters to ptrace_attach/PGrab linux makes in different from > other platforms. This is all platform specific code - and all the platforms are different. Of course, it would be good if all platforms reported good error message always - and we can continue working on that. Thanks, /Staffan > > 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> >>> <mailto: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> >>> <mailto: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> >>>> <mailto: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/ >>> <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> >>>> <mailto: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> >>>>> <mailto: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 >>>>> >>>>> <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/ >>>> <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> >>>>> <mailto: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.
