[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR: JDK-8145099 Better error message when SA can't attach to a process
From: Staffan Larsen <staffan.larsen () oracle ! com>
Date: 2015-12-11 11:32:30
Message-ID: 9A3AE520-3A48-4FD2-9E87-3B55A0CFFA04 () oracle ! com
[Download RAW message or body]
> On 11 dec. 2015, at 11:54, Dmitry Samersoff <dmitry.samersoff@oracle.com> wrote:
>
> 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.
if ph->pid == thr->lwp_id the if-statement is skipped and we continue in the \
while-loop.
>
> -Dmitry
>
> On 2015-12-11 11:12, Staffan Larsen wrote:
> >
> > > On 10 dec. 2015, at 23:20, Dmitry Samersoff
> > > <dmitry.samersoff@oracle.com <mailto:dmitry.samersoff@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.stuefe@gmail.com
> > > > > <mailto:thomas.stuefe@gmail.com>
> > > > > <mailto:thomas.stuefe@gmail.com>> wrote:
> > > > >
> > > > > Hi Staffan,
> > > > >
> > > > > this looks fine now, thanks!
> > > > >
> > > > > ..Thomas
> > > > >
> > > > > On Thu, Dec 10, 2015 at 3:42 PM, Staffan Larsen
> > > > > <staffan.larsen@oracle.com
> > > > > <mailto:staffan.larsen@oracle.com> <mailto:staffan.larsen@oracle.com>>
> > > > > wrote:
> > > > >
> > > > > Hi Thomas,
> > > > >
> > > > > > On 10 dec. 2015, at 15:27, Thomas Stüfe <thomas.stuefe@gmail.com
> > > > > > <mailto:thomas.stuefe@gmail.com>
> > > > > > <mailto:thomas.stuefe@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@oracle.com
> > > > > > <mailto:staffan.larsen@oracle.com> <mailto:staffan.larsen@oracle.com>>
> > > > > > wrote:
> > > > > >
> > > > > > Hi Thommas,
> > > > > >
> > > > > > > On 10 dec. 2015, at 14:49, Thomas Stüfe
> > > > > > > <thomas.stuefe@gmail.com
> > > > > > > <mailto:thomas.stuefe@gmail.com> <mailto:thomas.stuefe@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@oracle.com <mailto:staffan.larsen@oracle.com>
> > > > > > > <mailto:staffan.larsen@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.
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic