RFR: JDK-8211343: nsk_jvmti_parseoptions should handle multiple suboptions

[gary.adams at oracle.com]

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.

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.

...

> 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/958d3f97/attachment-0001.html>