RFR 8067447: Factor out the shared implementation of the VM flags manipulation code

[Jaroslav Bachorik]

On 8.1.2015 03:45, David Holmes wrote:
> On 8/01/2015 1:59 AM, Jaroslav Bachorik wrote:
>> On 7.1.2015 02:31, David Holmes wrote:
>>> On 17/12/2014 8:19 PM, Jaroslav Bachorik wrote:
>>>> Please, review the following change.
>>>>
>>>> Issue : https://bugs.openjdk.java.net/browse/JDK-8067447
>>>> Webrev: http://cr.openjdk.java.net/~jbachorik/8067447/webrev.00
>>>>
>>>> This patch is a precursor for implementing
>>>> https://bugs.openjdk.java.net/browse/JDK-8054890 which itself is a part
>>>> of JEP-228 (https://bugs.openjdk.java.net/browse/JDK-8043764).
>>>>
>>>> Here, the code related to manipulating JVM flags is extracted to a
>>>> separate ManagedFlags class and the codebease is adjusted to use this
>>>> class.
>>>
>>> Not clear to me what this is addressing exactly - do we really need
>>> platform specific variants of "set flag" ??
>>
>> It has just been moved from the corresponding attachListener_<os>.cpp
>> files. And it is 'pd_set_flag' what, I suppose, means "platform
>> dependent"?
>
> Yes it does and it mostly made sense inside the already pd
> attachListener implementations, but it isn't obvious to me that it makes
> sense in the ManagedFlag context. It is the choice about whether the
> flag can be changed that is "pd" not the actual setting and those
> choices are inherent to the attachListener mechanism they are not

Why would the ability to set Solaris specific flags via DTrace be 
specific to the attachListener mechanism? Also, AFAICS here it is the 
mechanism of setting the flag which is "pd" and not the choice 
(DTrace::* vs CommandLineFlags::*)

> inherent to ManagedFlags - so this refactoring seems wrong to me. What
> exactly is ManagedFlag supposed to represent?

A shared functionality between attachListener.cpp, management.cpp and 
the new diagnostic commands to be introduced later (as mentioned in the 
original synopsis of this RFR). It did seem preferable over just copying 
the implementation over to a few more places.

>
>>>
>>> All the new code seems incorrect:
>>>
>>> jint ManagedFlags::pd_set_flag(const char* flag_name, const char*
>>> flag_value, Flag::Flags origin, outputStream* out) {
>>>     out->print_cr("flag '%s' cannot be changed", op->arg(0));
>>>     return JNI_ERR;
>>>   };
>>>
>>> op->arg(0) comes from the original code where op was an
>>> AttachOperation*. Here is should be using flag_name.
>>
>> Correct. Slipped through and then replicated :/
>
> And obviously never compiled. RFRs should be for tested code.

Yes, one should run always "make clean" first, just in case. I should 
remember this well to prevent further embarrassment.

-JB-

>
> David
> -----
>
>> Updated webrev: http://cr.openjdk.java.net/~jbachorik/8067447/webrev.01
>>
>> -JB-
>>
>>>
>>> David
>>>
>>>
>>>> Thanks,
>>>>
>>>> -JB-
>>