David, > Thanks, this looks okay.
Thank you for review! > Only minor concern is whether we have to > apply casts to the results of geteuid() and st.st_uid when used with > %d format specifier? I didn't see any complains neither from jprt nor building locally. uid_t is 4 byte type for both 32bit/64bit so I don't think we need to cast. -Dmitry On 2016-08-15 07:23, David Holmes wrote: > On 12/08/2016 7:04 PM, Dmitry Samersoff wrote: >> David, >> >> Updated webrev is: >> >> http://cr.openjdk.java.net/~dsamersoff/JDK-8157236/webrev.04/ > > Thanks, this looks okay. Only minor concern is whether we have to > apply casts to the results of geteuid() and st.st_uid when used with > %d format specifier? > >> Windows is absolutely different story that requires significant >> efforts to reproduce error conditions and test changes. Also it has >> nothing to do with ARMv7. >> >> So I would prefer to address windows issues separately either as a >> part of JDK-8159799 or as a separate CR. > > Ok. > > Thanks, David > >> -Dmitry >> >> On 2016-08-12 03:24, David Holmes wrote: >>> Hi Dmitry, >>> >>> On 12/08/2016 2:55 AM, Dmitry Samersoff wrote: >>>> David, >>>> >>>> Please see updated webrev. >>>> >>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8157236/webrev.03/ >>>> >>>> I didn't touch windows version because it quite different from >>>> *NIX one. >>> >>> Do we ever see failures on Windows? Is so we should add >>> diagnostics there too even if they are different to *NIX. >>> >>> I would still like to see what file it is working with. We need >>> some logging in here: >>> >>> bool AttachListener::is_init_trigger() { if (init_at_startup() || >>> is_initialized()) { return false; // initialized at >>> startup or already initialized } char fn[PATH_MAX+1]; sprintf(fn, >>> ".attach_pid%d", os::current_process_id()); int ret; struct >>> stat64 st; RESTARTABLE(::stat64(fn, &st), ret); if (ret == -1) { >>> + log ("failed to find attach file: %s, trying alternate", >>> fn) snprintf(fn, sizeof(fn), "%s/.attach_pid%d", >>> os::get_temp_directory(), os::current_process_id()); >>> RESTARTABLE(::stat64(fn, &st), ret); + if (ret == -1) { + >>> log("failed to find attach file: %s", fn); + } } >>> >>> All failure paths need to show us what it was that failed. >>> >>> typos: trigerred -> triggered >>> >>> Thanks, David >>> >>>> -Dmitry >>>> >>>> On 2016-08-08 02:40, David Holmes wrote: >>>>> Hi Dmitry, >>>>> >>>>> On 5/08/2016 7:25 PM, Dmitry Samersoff wrote: >>>>>> Everybody, >>>>>> >>>>>> Please review the fix: >>>>>> >>>>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8157236/webrev.02/ >>>>>> >>>>>> >>>>>> Problem: >>>>>> Tests fail intermittently because it can't attach to child >>>>>> process, these attach failures is hard to debug because >>>>>> attach framework doesn't provide enough diagnostic >>>>>> information. >>>>>> >>>>>> Solution: >>>>>> >>>>>> a) Increase attach timeout b) Slightly change attach loop >>>>>> to save a bit of CPU power. c) Add some logging to attach >>>>>> listener. >>>>>> >>>>>> It's just a first step in this direction. Complete cleanup >>>>>> of attach code (remove LinuxThreads support and convert all >>>>>> printing to UL) is not a goal of this fix - I'll file a >>>>>> separate CR for it. >>>>> >>>>> I still think you need more logging now to aid in debugging >>>>> these cases. In particular we want to be able to verify that >>>>> the path of the attach file is what we expect in all cases ie >>>>> whether we find the .attach_pid file in cwd or whether we >>>>> are looking in temp directory, and whether we ultimately >>>>> succeed or fail. >>>>> >>>>> Plus whatever you do now should be done consistently for all >>>>> platforms. >>>>> >>>>> Thanks, David >>>>> >>>>>> -Dmitry >>>>>> >>>> >>>> >> >> -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
