Hi David, thanks for looking at the change. New webrev: http://cr.openjdk.java.net/~goetz/wr16/8170663-corlib_s11y/webrev.04/
> src/java.base/share/native/libjli/java.c > As far as I can see the existing code is working perfectly fine. Ah, thanks for the explanation, now I got it! Removed. > Again I'm not sure what the bug is here. On AIX the output string is > expanded with: > "%s/lib/%s/jli:" I first edited this against jdk9/hs, where the arch is gone since 8066474, http://hg.openjdk.java.net/jdk9/hs/jdk/rev/81508186e5bc but the // was not adapted. Then I moved the change to jdk9/dev because I thought I have to push it there. And yes, in that coding // would be correct. So I have to wait until hs is promoted ... > The jvmpath -> jvm_newpath change wasn't really necessary - a comment on > the strdup would have sufficed IMO. Dmitry asked me to add it. But I think it's ok. Can I consider this reviewed now? I.e. could I push it once 8066474 is promoted and Joe Darcy agreed? Best regards, Goetz. > -----Original Message----- > From: David Holmes [mailto:david.hol...@oracle.com] > Sent: Donnerstag, 8. Dezember 2016 09:14 > To: Lindenmaier, Goetz <goetz.lindenma...@sap.com>; 'Dmitry Samersoff' > <dmitry.samers...@oracle.com>; Java Core Libs <core-libs- > d...@openjdk.java.net>; serviceability-dev (serviceability- > d...@openjdk.java.net) <serviceability-...@openjdk.java.net> > Subject: Re: RFR(M): 8170663: Fix minor issues in corelib and servicabilty > coding. > > Hi Goetz, > > On 8/12/2016 1:26 AM, Lindenmaier, Goetz wrote: > > Hi Dmitry, > > > > yes, new_jvmpath is consistent with the other variables. > > I also merged the ifs in SDE.c. > > > > new webrev: > > http://cr.openjdk.java.net/~goetz/wr16/8170663-corlib_s11y/webrev.03/ > > src/java.base/share/native/libjli/java.c > > As far as I can see the existing code is working perfectly fine. Given a > jvm.cfg with: > > -server KNOWN > -client IGNORE > -myvm KNOWN > -oldvm ALIASED_TO -server > > The usage text is: > > -server to select the "server" VM > -myvm to select the "myvm" VM > -oldvm is a synonym for the "server" VM [deprecated] > The default VM is server, > because you are running on a server-class machine. > > which is exactly what I would expect. Why do you think there is a bug? > > --- > > src/java.base/unix/native/libjli/java_md_solinux.c > > Again I'm not sure what the bug is here. On AIX the output string is > expanded with: > > "%s/lib/%s/jli:" > > so it needs to account for the extra "/lib//jli:" characters - and that > is exactly what the existing code does: > > + JLI_StrLen("/lib//jli:") > > The jvmpath -> jvm_newpath change wasn't really necessary - a comment on > the strdup would have sufficed IMO. > > Thanks, > David > ----- > > > > > > Best regards, > > Goetz. > > > >> -----Original Message----- > >> From: Dmitry Samersoff [mailto:dmitry.samers...@oracle.com] > >> Sent: Wednesday, December 07, 2016 2:43 PM > >> To: Lindenmaier, Goetz <goetz.lindenma...@sap.com>; Java Core Libs > >> <core-libs-dev@openjdk.java.net>; serviceability-dev (serviceability- > >> d...@openjdk.java.net) <serviceability-...@openjdk.java.net> > >> Subject: Re: RFR(M): 8170663: Fix minor issues in corelib and servicabilty > >> coding. > >> > >> Goetz, > >> > >> SDE.c: > >> > >> You might combine if at ll. 260 and 263 to one but it's just matter of > >> test. > >> > >> if (sti == baseStratumIndex || sti < 0) { > >> return; /* Java stratum - return unchanged */ > >> } > >> > >>> I'm not sure what you mean. I tried to fix it, but please > >>> double-check the new webrev. > >> > >> if cnt is <= 0 loop at l.267 > >> > >> for (; cnt-- > 0; ++fromEntry) { > >> > >> is never run and we effectively do > >> > >> *entryCountPtr = 0; > >> > >> at l.283 > >> > >> So if we you suspect that cnt may become negative or 0: > >> (your v.01 changes) > >> > >> 260 if (sti < 0 && cnt > 0) { > >> 261 return; > >> 262 } > >> > >> it's better to check it early. > >> > >> But I'm not sure we have to care about negative/zero cnt here. > >> > >> -Dmitry > >> > >> On 2016-12-07 11:37, Lindenmaier, Goetz wrote: > >>> Hi Dmitry, > >>> > >>> thanks for looking at my change! > >>> Updated webrev: > >>> http://cr.openjdk.java.net/~goetz/wr16/8170663-corlib_s11y/webrev.02 > >>> > >>>> * src/java.base/unix/native/libjli/java_md_solinux.c > >>>> Is this line correct? > >>>> 519 jvmpath = JLI_StringDup(jvmpath); > >>> > >>> It seems pointless. Should I remove it? (The whole file is a horror.) > >>> > >>>> * src/jdk.jdwp.agent/share/native/libjdwp/SDE.c > >>>> It might be better to return immediately if cnt < 0, > >>>> regardless of value of sti. > >>> > >>> I'm not sure what you mean. I tried to fix it, but please > >>> double-check the new webrev. > >>> > >>>> * src/jdk.jdwp.agent/unix/native/libdt_socket/socket_md.c > >>>> It might be better to write > >>>> arg.l_linger = (on) ? (unsigned short)value.i : 0; > >>>> and leave one copy of setsockopt() call > >>> > >>> Yes, looks better. > >>> > >>> Best regards, > >>> Goetz > >>> > >>> > >>>> > >>>> -Dmitry > >>>> > >>>> > >>>> On 2016-12-06 16:12, Lindenmaier, Goetz wrote: > >>>>> Hi, > >>>>> > >>>>> > >>>>> > >>>>> This change fixes some minor issues found in our code scans. > >>>>> > >>>>> I hope this correctly addresses corelib and serviceability issues. > >>>>> > >>>>> > >>>>> > >>>>> Please review: > >>>>> > >>>>> http://cr.openjdk.java.net/~goetz/wr16/8170663- > >> corlib_s11y/webrev.01/ > >>>>> > >>>>> > >>>>> > >>>>> Best regards, > >>>>> > >>>>> Goetz. > >>>>> > >>>>> > >>>>> > >>>>> Changes in detail: > >>>>> > >>>>> > >>>>> > >>>>> e_asin.c > >>>>> > >>>>> Code scan reports missing {}. > >>>>> > >>>>> > >>>>> The code "if (huge+x>one) {" is only there to set the inexact flag of > >>>>> the processor. > >>>>> It's a way to avoid the C compiler to optimize the code away. It is > >>>>> always true, > >>>>> so the parenthesis of the outer else don't matter. > >>>>> > >>>>> Although this basically just adds the {} I would like to submit this to > >>>>> > >>>>> avoid anybody else spends more the 30sec on understanding these > >>>>> > >>>>> if statements. > >>>>> > >>>>> > >>>>> k_standard.c > >>>>> > >>>>> exc.retval is returned below and thus should always be initialized. > >>>>> > >>>>> > >>>>> imageDecompressor.cpp > >>>>> > >>>>> Wrong destructor is used. Reported also by David CARLIER > >>>>> > >>>>> > >>>>> java.c > >>>>> > >>>>> in line 1865 'name' was used, it should be 'alias'. > >>>>> > >>>>> > >>>>> java_md_solinux.c > >>>>> > >>>>> "//" in path is useless. Further down a free is missing. > >>>>> > >>>>> > >>>>> SDE.c > >>>>> > >>>>> Call to stratumTableIndex can return negative value if defaultStratumId > >>>>> == null. > >>>>> > >>>>> > >>>>> socket_md.c > >>>>> > >>>>> arg.l_linger is passed to setsockopt uninitialized. Its use is hidden in > >>>>> the macros. > >>>>> > >>>>> > >>>>> unpack.cpp > >>>>> > >>>>> n.slice should not get negative argument for end, which is passed from > >>>>> dollar1. > >>>>> But dollar1 can get negative where it is set to the result of > >>>>> lastIndexOf(DOLLAR_MIN, DOLLAR_MAX, n, dollar2 - 1). > >>>>> > >>>> > >>>> > >>>> -- > >>>> 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.
