RFR: JDK-8211343: nsk_jvmti_parseoptions should handle multiple suboptions

[serguei.spitsyn at oracle.com]

On 12/21/18 12:55 PM, gary.adams at oracle.com wrote:
> On 12/21/18 3:33 PM, serguei.spitsyn at oracle.com wrote:
>> Hi Gary,
>>
>> Looks good in general.
>> Thank you for taking care about this!
>>
>> A couple of comments.
>>
>> It seems, with your fixes the macro NSK_JVMTI_OPTION_VAL_SEP is not 
>> needed anymore.
>> Also, I wonder if it makes sense to continue using:
>> 238 value = index(name, NSK_JVMTI_OPTION_VAL_SEP);
> This is one of the places where I feel having the symbolic constant
> actually makes the code harder to read.
>
> The constant is not used anywhere else.
>

Kind of agree with you.

> I'm testing now with strchr because of strings.h issues on other 
> platforms.
>
>>
>>
>> Also, a cleanup is needed to remove the lines 241 and 244 from the 
>> fragment:
>> 240 if (value == NULL) {
>> 241 /* debug: printf ("%s\n", name); */
>> 242 } else {
>> 243 *value++ = '\0';
>> 244 /* debug: printf ("%s=%s\n", name, value); */
>>   245         }
>> Then it makes sense to refactor it:
>> if (value != NULL) {
>> *value++ = '\0';
>>            }
>
> Yes, that is the plan.
>
> Until all the testing is done, it helps to have some quick debugging 
> available.
>

Okay.

Thanks,
Serguei

> ...
>
>> Thanks,
>> Serguei
>>
>>
>> On 12/21/18 11:31, Chris Plummer wrote:
>>> Hi Gary,
>>>
>>> This is much better. Just one question and a couple of minor comments:
>>>
>>> The original code had the following:
>>>
>>>  225     context.options.string = (char*)malloc(len + 2);
>>> ...
>>>  231     strncpy(context.options.string, options, len);
>>>  232     context.options.string[len] = '\0';
>>>  233     context.options.string[len+1] = '\0';
>>>
>>> Do you know why it was placing two NULLs at the end (and the first 
>>> was unnecessary since strncpy already did that)?
>>>
>>>  243             *value++ = '\0';
>>>
>>> This should be rewritten in a manner that make it clearer what it 
>>> does. At first glance you have to ask yourself whether it is setting 
>>> *value or *(value++). Then hopefully you eventually clue in that 
>>> they are both the same since "value++" evaluates to "value". I think 
>>> the following would be better:
>>>
>>>               *value = '\0';
>>>               value++;
>>>
>>>  235     /* Create a temporay copy of the options string to be 
>>> tokenized. */
>>>
>>> "temporay" typo.
>>>
>>> thanks,
>>>
>>> Chris
>>>
>>> On 12/21/18 10:52 AM, Gary Adams wrote:
>>>> Here is a first pass at a replacement parser for jvmti test options.
>>>>
>>>>   Webrev: http://cr.openjdk.java.net/~gadams/8211343/webrev.00/
>>>>   Issue: https://bugs.openjdk.java.net/browse/JDK-8211343
>>>>
>>>> Testing is in progress. Need to check out more platforms.
>>>> On local linux testing one jvmti test failed SetNativeMethodPrefix001
>>>> which passed an empty options string.
>>>> ...
>>>
>>>
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20181221/5227bdfc/attachment.html>