RFR: JDK-8145099 Better error message when SA can't attach to a process
Dmitry Samersoff
dmitry.samersoff at oracle.com
Fri Dec 11 10:54:22 UTC 2015
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.samersoff at oracle.com <mailto:dmitry.samersoff at 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 at gmail.com
>>>> <mailto: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> <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>
>>>>> <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> <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> <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>
>>>>>> <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.
>
--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.
More information about the serviceability-dev
mailing list