JDK-8211343: nsk_jvmti_parseoptions should handle multiple suboptions
Daniel D. Daugherty
daniel.daugherty at oracle.com
Mon Jan 7 18:51:42 UTC 2019
Please see:
JDK-8216059 nsk_jvmti_parseoptions still has dependency on tilde separator
https://bugs.openjdk.java.net/browse/JDK-8216059
Dan
On 1/7/19 1:32 PM, JC Beyler wrote:
> Hi Gary,
>
> My only comment here is that it seems to that the delimiter list you
> have there is not complete (or backwards compatible). The original
> code uses:
> int isOptSep(char c) {
> return isspace(c) || c == '~';
> }
>
> So perhaps the delimiter list you are providing should be augmented to
> support that?
>
> Thanks,
> Jc
>
> On Fri, Dec 21, 2018 at 5:31 AM Gary Adams <gary.adams at oracle.com
> <mailto:gary.adams at oracle.com>> wrote:
>
> My intention is to establish a more robust argument parsing
> and then see how much code and comments can be replaced.
>
> Since strsep is BSD based and not available on windows,
> an alternative is to use strpbrk which is on windows and
> posix.
>
> #include <stdio.h>
> #include <string.h>
>
> char* token(char **s, char *delim) {
> char *p;
> char *start = *s;
>
> if (s == NULL || *s == NULL) {
> return NULL;
> }
>
> p = strpbrk(*s, delim);
> if (p != NULL) {
> /* Advance to next token. */
> *p = '\0';
> *s = p + 1;
> } else {
> /* End of tokens. */
> *s = NULL;
> }
>
> return start;
> }
>
>
> int main(int argc, char **argv){
> char *str = strdup("waittime=5,verbose foo bar= = =rab baz=11");
> char *name;
> char *value;
>
> while ((name = token(&str, " ,")) != NULL) {
> value = index(name, '=');
>
> if (value == NULL) {
> printf ("%s\n", name);
> } else {
> *value++ = '\0';
> printf ("%s=%s\n", name, value);
> }
> }
> }
>
> ...
>
> ./main
> waittime=5
> verbose
> foo
> bar=
> =
> =rab
> baz=11
>
>
>
>
>
> On 12/20/18, 12:10 PM, Chris Plummer wrote:
>> Wow, the existing comments for this function take a lot of work
>> to translate. You basically need to understand the code in order
>> to understand the comment. Kind of backwards. Below I've included
>> all the existing code and comments, with my translation of the
>> comments and also additional annotations.
>>
>> But before getting into that, I think the proposed fix is just
>> working around all the bugs elsewhere in the function by making
>> opt point to a string that just contains the current option we
>> are working on, rather than attempting (poorly and incorrectly)
>> to point to the next option within the options string. Although
>> tokenizing the string in this way is probably the better
>> approach, it would be nice to at the same time clean up all the
>> other errors and comments in this code.
>>
>> /**
>> *
>> * The current option will not perform more than one
>> * single option which given, this is due to places explained
>> * in this question.
>> *
>> **/
>>
>> # This current implementation will not parse more than one
>> option. The
>> # reason is explained in comments below.
>>
>> /*
>> * This whole play can be reduced with simple StringTokenizer
>> (strtok).
>> *
>> */
>>
>> # This function can be simplified by using strok().
>>
>> int nsk_jvmti_parseOptions(const char options[]) {
>> size_t len;
>> const char* opt;
>> int success = NSK_TRUE;
>>
>> context.options.string = NULL;
>> context.options.count = 0;
>> context.waittime = 2;
>>
>> if (options == NULL)
>> return NSK_TRUE;
>>
>> len = strlen(options);
>> context.options.string = (char*)malloc(len + 2);
>>
>> if (context.options.string == NULL) {
>> nsk_complain("nsk_jvmti_parseOptions(): out of
>> memory\n");
>> return NSK_FALSE;
>> }
>> strncpy(context.options.string, options, len);
>> context.options.string[len] = '\0';
>> context.options.string[len+1] = '\0';
>>
>> for (opt = context.options.string; ;) {
>> const char* opt_end;
>> const char* val_sep;
>> int opt_len=0;
>> int val_len=0;
>> int exit=1;
>>
>> while (*opt != '\0' && isOptSep(*opt)) opt++;
>> if (*opt == '\0') break;
>>
>> val_sep = NULL;
>> /*
>> This should break when the first option it encounters
>> other wise
>> */
>> # This should break at the end of the first option, before the
>> option value is specified
>> # if there is an option value.
>> for (opt_end = opt, opt_len=0; !(*opt_end == '\0' ||
>> isOptSep(*opt_end)); opt_end++,opt_len++) {
>> if (*opt_end == NSK_JVMTI_OPTION_VAL_SEP) {
>> val_sep = opt_end;
>> exit=0;
>> break;
>> }
>> }
>>
>> if (exit == 1) break;
>>
>> /* now scan for the search for the option value end.
>>
>> */
>> # Now scan for the end of the option value.
>> [Chris: This is a bug because it assumes that there is a value.
>> If we stopped in the above
>> loop due to finding the option separator (which also seems to be
>> broken), then we start
>> reading the next option as the option value.]
>> exit =1;
>> opt_end++;
>> val_sep++;
>> /**
>> * I was expecting this jvmti_parseOptions(),
>> * should be for multiple options as well.
>> * If this break is not there then It will expects
>> * to have. so a space should be sufficient as well.
>> */
>> # I was expecting that nsk_jvmti_parseOptions() would parse
>> multiple options.
>> [Chris: I have no idea what the rest of the comment means. Due to
>> the bug above
>> with handling an option with no value, the code below doesn't do
>> what was expected.
>> The commented out part of it tried to do the right thing by
>> searching for the '-' for the
>> next option, whichis wrong because options don't begin with a
>> '\'. They are separated
>> by a comma, although the code elsewhere wants them separated by a
>> space or a `~`.]
>> for (val_len=0; !(*opt_end == '\0' ||
>> isOptSep(*opt_end)); opt_end++,val_len++) {
>> //if (*opt_end == NSK_JVMTI_OPTION_START) {
>> // break;
>> //}
>> }
>>
>> if (!add_option(opt, opt_len, val_sep, val_len)) {
>> success = NSK_FALSE;
>> break;
>> }
>> opt_end++;
>> opt = opt_end;
>> }
>>
>>
>> On 12/20/18 8:33 AM, Gary Adams wrote:
>>> I prototyped with strsep, because strtok is not safe
>>> and did not want to deal with the strtok_r versus
>>> strtok_s (windows) platform variants.
>>>
>>> ...
>>> #include <stdio.h>
>>> #include <string.h>
>>>
>>> int main(int argc, char **argv){
>>> char *str = strdup("waittime=5,verbose foo bar baz=11");
>>> char *name;
>>> char *value;
>>>
>>> while ((name = strsep (&str, " ,")) != NULL) {
>>> value = index(name, '=');
>>>
>>> if (value == NULL) {
>>> printf ("%s\n", name);
>>> } else {
>>> *value++ = '\0';
>>> printf ("%s=%s\n", name, value);
>>> }
>>> }
>>>
>>> ...
>>>
>>> ./main
>>> waittime=5
>>> verbose
>>> foo
>>> bar
>>> baz=11
>>>
>>>
>>> On 12/20/18, 10:54 AM, JC Beyler wrote:
>>>> Hi Gary,
>>>>
>>>> I had seen that too and forgot to file it! So thanks!
>>>>
>>>> I think the comment you removed is interesting, I'm not sure
>>>> what it means exactly and I've tried re-reading a few times but
>>>> the use of "in this question" is weird :-)
>>>>
>>>> Anyway, the webrev looks good except perhaps for the use of
>>>> strsep, which is BSD, instead of strtok, which is C89/C99/Posix.
>>>>
>>>> Thanks for doing this!
>>>> Jc
>>>>
>>>> On Thu, Dec 20, 2018 at 5:17 AM Gary Adams
>>>> <gary.adams at oracle.com <mailto:gary.adams at oracle.com>> wrote:
>>>>
>>>> During some earlier jvmti test debugging, I noticed that it
>>>> was not
>>>> possible to
>>>> add a quick argument to the current tests and rerun the
>>>> test. e.g.
>>>> expand "waittime=5" to
>>>> "waittime=5,verbose". I tracked down the options parsing in
>>>> jvmti_tools.cpp and saw some
>>>> comments that only a single option could be parsed.
>>>>
>>>> So I filed this bug to revisit the issue for the next release:
>>>>
>>>> Issue: https://bugs.openjdk.java.net/browse/JDK-8211343
>>>>
>>>> I think the option parsing should be ripped out and redone,
>>>> but for now I think a simple tweak to use strsep(), might
>>>> go a long way
>>>> to solving the multiple option support. I just started testing,
>>>> but wanted to know if anyone else has been down this rabbit
>>>> hole
>>>> before.
>>>>
>>>>
>>>> diff --git
>>>> a/test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/jvmti_tools.cpp
>>>>
>>>> b/test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/jvmti_tools.cpp
>>>> ---
>>>> a/test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/jvmti_tools.cpp
>>>> +++
>>>> b/test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/jvmti_tools.cpp
>>>> @@ -196,22 +196,14 @@
>>>> }
>>>>
>>>>
>>>> -/**
>>>> - *
>>>> - * The current option will not perform more than one
>>>> - * single option which given, this is due to places explained
>>>> - * in this question.
>>>> - *
>>>> - **/
>>>> -
>>>> /*
>>>> - * This whole play can be reduced with simple
>>>> StringTokenizer (strtok).
>>>> - *
>>>> + * Parse a comma or space separated list of options.
>>>> */
>>>>
>>>> int nsk_jvmti_parseOptions(const char options[]) {
>>>> size_t len;
>>>> const char* opt;
>>>> + char *str = strdup(options);
>>>> int success = NSK_TRUE;
>>>>
>>>> context.options.string = NULL;
>>>> @@ -232,7 +224,7 @@
>>>> context.options.string[len] = '\0';
>>>> context.options.string[len+1] = '\0';
>>>>
>>>> - for (opt = context.options.string; ;) {
>>>> + while ((opt = strsep(&str, " ,")) != NULL) {
>>>> const char* opt_end;
>>>> const char* val_sep;
>>>> int opt_len=0;
>>>>
>>>>
>>>>
>>>> --
>>>>
>>>> Thanks,
>>>> Jc
>>>
>>
>
>
>
> --
>
> Thanks,
> Jc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20190107/d9d086e0/attachment-0001.html>
More information about the serviceability-dev
mailing list