RFR(S):8251374:jmap -dump should not accept invalid options(Internet mail)
David Holmes
david.holmes at oracle.com
Mon Aug 17 01:17:59 UTC 2020
Hi Dan,
On 15/08/2020 11:57 pm, Daniel D. Daugherty wrote:
> First, Paul sent his request on the wrong email thread and I didn't
> notice until just now that we should have been using this one:
Yes this has all been very confusing.
> Subject: RFR(L): 8215624: add parallel heap inspection support for
> jmap histo(G1)(Internet mail)
>
> Fortunately, the correction was applied to the proper changeset.
>
> The process that Stefan K. proposed and that I executed was this:
Thanks for explaining. Now I can see the correct sequence of changes
pertaining to 8215624.
David
-----
> $ hg export 5036ca733469 > 8215624.patch
> <edit 8215624.patch and replace author phh with author lzang>
>
> # Switch the repo to the parent changeset of 5036ca733469:
> $ hg update 7b7be8c2b336
>
> # Import the fixed patch as a sibling changeset:
> $ hg import 8215624.patch
>
> # Make sure it looks correct:
> $ hg log -r tip
> changeset: 60553:b1afb7c82d59
> parent: 60550:7b7be8c2b336
> user: lzang
> date: Thu Aug 13 11:31:37 2020 -0700
> summary: 8215624: Add parallel heap iteration for jmap –histo
>
> # Switch the repo back to the main line:
> $ hg update
>
> # Merge the two branches:
> $ hg merge
>
> # Commit the "Merge":
> $ hg commit
> <with "Merge" as the comment>
>
> # Push the repair:
> $ hg push
>
>
> The result of this repair is almost identical to the same patch being
> backported to JDK-(N-1) after being pushed first to JDK-N. When JDK-(N-1)
> is sync'ed into JDK-N, we'll have two (almost identical) changesets in
> JDK-N for the same bug ID.
>
> In the usual backport scenario, the only difference is the changeset ID.
> In this case, the only differences are the changeset ID and the author.
>
> So no [BACKOUT] was involved at all and the "Merge" changeset is exactly
> that: a merge.
>
> Paul added a link to the corrected changeset two days ago. I added a
> comment with a handcrafted "notification" comment this morning.
>
> Dan
>
>
>
>
> On 8/15/20 1:07 AM, David Holmes wrote:
>> So am I right in thinking that what you did was forge a Merge
>> changeset that actually did a backout, and then recreated the
>> changeset correctly and pushed under the same bug number?
>>
>> I don't think that is a process we want to endorse. A "Merge"
>> changeset should be exactly that.
>>
>> Also the bug needs updating with a link to the second changeset.
>>
>> David
>> -----
>>
>> On 14/08/2020 5:52 am, Daniel D. Daugherty wrote:
>>> s/like a change/like a charm/
>>>
>>> Typing too fast today...
>>>
>>> Dan
>>>
>>>
>>> On 8/13/20 3:51 PM, Daniel D. Daugherty wrote:
>>>> Stefan K's idea worked like a change. A corrected changeset has
>>>> been created, merged and pushed.
>>>>
>>>> Dan
>>>>
>>>>
>>>> On 8/13/20 3:34 PM, Daniel D. Daugherty wrote:
>>>>> Paul,
>>>>>
>>>>> Hold up on trying to fix this.
>>>>>
>>>>> I'm discussing another idea with Stefan K.
>>>>>
>>>>> Dan
>>>>>
>>>>>
>>>>> On 8/13/20 3:01 PM, Daniel D. Daugherty wrote:
>>>>>> That's something that's very hard to do. It would involve black
>>>>>> listing
>>>>>> the existing changeset and repushing a new changeset. Black listing a
>>>>>> changeset is very, very rarely done and in the past Ops has
>>>>>> declined to
>>>>>> do that for something like an authorship error.
>>>>>>
>>>>>> Two options:
>>>>>>
>>>>>> 1) Manually remember that this changeset should be credited to Lin
>>>>>> as author.
>>>>>> 2a) [BACKOUT] the changeset using a new bug ID.
>>>>>> 2b) [REDO] the changeset with corrected author information with a
>>>>>> new bug ID.
>>>>>>
>>>>>> Dan
>>>>>>
>>>>>> On 8/13/20 2:36 PM, Hohensee, Paul wrote:
>>>>>>> I mistakenly committed and pushed Lin's patch with myself as
>>>>>>> author. Would someone with repo access please change the author
>>>>>>> to 'lzang'? Or tell me how to do it myself?
>>>>>>>
>>>>>>> https://hg.openjdk.java.net/jdk/jdk/rev/5036ca733469
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Paul
>>>>>>>
>>>>>>> On 8/13/20, 9:48 AM, "serviceability-dev on behalf of Hohensee,
>>>>>>> Paul" <serviceability-dev-retn at openjdk.java.net on behalf of
>>>>>>> hohensee at amazon.com> wrote:
>>>>>>>
>>>>>>> Will do.
>>>>>>>
>>>>>>> On 8/13/20, 7:08 AM, "linzang(臧琳)" <linzang at tencent.com>
>>>>>>> wrote:
>>>>>>>
>>>>>>> Thanks Paul！
>>>>>>> May I ask your help to push it?
>>>>>>>
>>>>>>> BRs,
>>>>>>> Lin
>>>>>>>
>>>>>>> > On Aug 13, 2020, at 10:06 PM, Hohensee, Paul
>>>>>>> <hohensee at amazon.com> wrote:
>>>>>>> >
>>>>>>> > +1, except that the indentation for the final 'else'
>>>>>>> clause needs to be 4 spaces instead of 3. :)
>>>>>>> >
>>>>>>> > Thanks,
>>>>>>> > Paul
>>>>>>> >
>>>>>>> > On 8/12/20, 6:21 PM, "serguei.spitsyn at oracle.com"
>>>>>>> <serguei.spitsyn at oracle.com> wrote:
>>>>>>> >
>>>>>>> > Hi Lin.
>>>>>>> >
>>>>>>> > Thank you for the update.
>>>>>>> > It looks good.
>>>>>>> >
>>>>>>> > Thanks,
>>>>>>> > Serguei
>>>>>>> >
>>>>>>> >
>>>>>>> >> On 8/12/20 17:08, linzang(臧琳) wrote:
>>>>>>> >> Hi Paul and Serguei,
>>>>>>> >> Thanks for your comments, here is the updated
>>>>>>> patch: http://cr.openjdk.java.net/~lzang/8251374/webrev02/
>>>>>>> >>
>>>>>>> >> BRs,
>>>>>>> >> Lin
>>>>>>> >>
>>>>>>> >> On 2020/8/13, 12:55 AM, "serguei.spitsyn at oracle.com"
>>>>>>> <serguei.spitsyn at oracle.com> wrote:
>>>>>>> >>
>>>>>>> >> Hi Lin,
>>>>>>> >>
>>>>>>> >> It looks good.
>>>>>>> >> Just one comment.
>>>>>>> >>
>>>>>>> >> + System.err.println("Fail: invalid option:
>>>>>>> '" + subopt +"'");
>>>>>>> >> + System.exit(1);
>>>>>>> >>
>>>>>>> >> Exit needs to be replaced wit usage for consistency.
>>>>>>> >>
>>>>>>> >> Thanks,
>>>>>>> >> Serguei
>>>>>>> >>
>>>>>>> >>
>>>>>>> >>> On 8/10/20 19:57, linzang(臧琳) wrote:
>>>>>>> >>> Here is the webrev:
>>>>>>> http://cr.openjdk.java.net/~lzang/8251374/webrev01/
>>>>>>> >>>
>>>>>>> >>> BRs,
>>>>>>> >>> Lin
>>>>>>> >>>
>>>>>>> >>>> On 2020/8/11, 10:52 AM, "linzang(臧琳)"
>>>>>>> <linzang at tencent.com> wrote:
>>>>>>> >>>
>>>>>>> >>> Hi All,
>>>>>>> >>> May I ask your help to review this tiny
>>>>>>> patch? It fix an issue that jmap -dump could wrongly accept
>>>>>>> invalid optioins.
>>>>>>> >>> Bugs:
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8251374
>>>>>>> >>> Patch: (Can not connect to webrev ftp
>>>>>>> currently, will try it later, following are all code changes)
>>>>>>> >>>
>>>>>>> >>> ################################
>>>>>>> >>> ---
>>>>>>> old/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java
>>>>>>> 2020-08-11 10:42:32.044567791 +0800
>>>>>>> >>> +++
>>>>>>> new/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java
>>>>>>> 2020-08-11 10:42:31.876568681 +0800
>>>>>>> >>> @@ -207,6 +207,11 @@
>>>>>>> >>> liveopt = "-live";
>>>>>>> >>> } else if
>>>>>>> (subopt.startsWith("file=")) {
>>>>>>> >>> filename = parseFileName(subopt);
>>>>>>> >>> + } else if
>>>>>>> (subopt.equals("format=b")) {
>>>>>>> >>> + // ignore format (not needed at
>>>>>>> this time)
>>>>>>> >>> + } else {
>>>>>>> >>> + System.err.println("Fail: invalid option: '" +
>>>>>>> subopt +"'");
>>>>>>> >>> + System.exit(1);
>>>>>>> >>> }
>>>>>>> >>> }
>>>>>>> >>> ################################
>>>>>>> >>>
>>>>>>> >>> Thanks,
>>>>>>> >>> Lin
>>>>>>> >>>
>>>>>>> >>>
>>>>>>> >>
>>>>>>> >>
>>>>>>> >>
>>>>>>> >
>>>>>>> >
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>
More information about the serviceability-dev
mailing list