Staffan, > 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.
OK. > It should get filled in in the ptrace_attach() call. if ph->pid == thr->lwp_id at ll. 409 the function return NULL but doesn't touch err_buf. -Dmitry On 2015-12-11 11:12, Staffan Larsen wrote: > >> On 10 dec. 2015, at 23:20, Dmitry Samersoff >> <dmitry.samers...@oracle.com <mailto: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>> 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>> >>>> 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>> 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> <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>> >>>>>> 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> >>>>>> <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. > -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
