Review request for JMC-6548: Duplicated Flags rule somewhat naive

[Henrik Dafgård]

LGTM!


Cheers,
Henrik Dafgård


On Thu, 8 Aug 2019 at 17:13, Florian David <florian.david at datadoghq.com>
wrote:

> Here is the new version of the patch.
>
> JIRA: https://bugs.openjdk.java.net/browse/JMC-6548
> Webrev: http://cr.openjdk.java.net/~fdavid/JMC-6548/webrev.1/
>
> Cheers,
> Florian
>
>
> On Tue, Aug 6, 2019 at 6:55 PM Florian David <florian.david at datadoghq.com>
> wrote:
>
>>
>>
>> On Tue, Aug 6, 2019 at 5:53 PM Henrik Dafgård <hdafgard at gmail.com> wrote:
>>
>>> Hi Florian,
>>>
>>> The split needs to have that limit there due to the experimental
>>> arguments with options, i.e. -XX:FlightRecorderOptions=stackdepth=128. With
>>> this change we would get inconsistent results there instead as the argument
>>> would be seen as FlightRecorderOptions=stackdepth=128 instead of
>>> FlightRecorderOptions.
>>>
>>> Oh right, I did not think about these ones.
>>
>>
>>> Also, if an argument matches one in the VERBATIM array it should only be
>>> compared using the full argument and we won't even run that split on that
>>> argument. If we want to compare multiple -javaagent commands that share
>>> jarpaths but might have different options that could be contradictory I
>>> think we need to have a more detailed solution, i.e. have a check for
>>> "-javaagent" as a prefix instead of adding it to the VERBATIM array and
>>> handling those arguments specially.
>>>
>>>
>> After an offline discussion with Henrick, we think it's better to display
>> a message in case there is duplicate agents ("duplicate" here meaning
>> agents with the same jar) even if given options are different. This is
>> something the current patch does not catch, I'll update it accordingly.
>>
>>
>>>
>>> Cheers,
>>> Henrik Dafgård
>>>
>>>
>>> On Tue, 6 Aug 2019 at 14:41, Florian David <florian.david at datadoghq.com>
>>> wrote:
>>>
>>>> Hi,
>>>>
>>>> Please find attached a webrev to solve JMC-6548.
>>>>
>>>> I have also corrected a small bug on line 123 (String[] split =
>>>> fullArgument.split("[=:]",
>>>> 2);). In case fullArgument contains more than one ':' (for instance
>>>> "-javaagent:C:/mypath"), split[1] does not contain the exact option
>>>> argument (in this case only "C"), which could give inconsistent results.
>>>>
>>>> JIRA: https://bugs.openjdk.java.net/browse/JMC-6548
>>>> Webrev: http://cr.openjdk.java.net/~fdavid/JMC-6548/webrev.0/
>>>>
>>>> Thanks,
>>>> Florian
>>>>
>>>