Re: JDK-8211343: nsk_jvmti_parseoptions should handle multiple suboptions
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 > 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
#include
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
Re: RFR: JDK-8211343: nsk_jvmti_parseoptions should handle multiple suboptions
Hi Gary, Looks good to me. Thanks, Serguei On 12/31/18 10:06, gary.ad...@oracle.com wrote: Here's a revised webrev. Webrev: http://bussund0416.us.oracle.com/export/users/gradams/work/webrevs/8211343/webrev.01/ Updates in this round of changes : - replaced index() with strchr() to avoid platform dependent issues with strings.h include - removed NSK_JVMTI_OPTION_VAL_SEP - removed temporary debugging print statements - removed empty options string from SetNativeMethodPrefix001 - added free for temporary strdup buffer - updated copyright for 2019 On 12/21/18 1:52 PM, 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. ...
Re: RFR: JDK-8211343: nsk_jvmti_parseoptions should handle multiple suboptions
Ok. Chris On 12/31/18 12:30 PM, gary.ad...@oracle.com wrote: The one test that failed is changed in this webrev. When it failed, it was the check in add_option that does not allow an "empty" option name. On 12/31/18 2:25 PM, Chris Plummer wrote: Looks good. Are empty options strings still allowed after your changes? Chris On 12/31/18 10:06 AM, gary.ad...@oracle.com wrote: Here's a revised webrev. Webrev: http://bussund0416.us.oracle.com/export/users/gradams/work/webrevs/8211343/webrev.01/ Updates in this round of changes : - replaced index() with strchr() to avoid platform dependent issues with strings.h include - removed NSK_JVMTI_OPTION_VAL_SEP - removed temporary debugging print statements - removed empty options string from SetNativeMethodPrefix001 - added free for temporary strdup buffer - updated copyright for 2019 On 12/21/18 1:52 PM, 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. ...
Re: RFR: JDK-8211343: nsk_jvmti_parseoptions should handle multiple suboptions
The one test that failed is changed in this webrev. When it failed, it was the check in add_option that does not allow an "empty" option name. On 12/31/18 2:25 PM, Chris Plummer wrote: Looks good. Are empty options strings still allowed after your changes? Chris On 12/31/18 10:06 AM, gary.ad...@oracle.com wrote: Here's a revised webrev. Webrev: http://bussund0416.us.oracle.com/export/users/gradams/work/webrevs/8211343/webrev.01/ Updates in this round of changes : - replaced index() with strchr() to avoid platform dependent issues with strings.h include - removed NSK_JVMTI_OPTION_VAL_SEP - removed temporary debugging print statements - removed empty options string from SetNativeMethodPrefix001 - added free for temporary strdup buffer - updated copyright for 2019 On 12/21/18 1:52 PM, 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. ...
Re: RFR: JDK-8211343: nsk_jvmti_parseoptions should handle multiple suboptions
Looks good. Are empty options strings still allowed after your changes? Chris On 12/31/18 10:06 AM, gary.ad...@oracle.com wrote: Here's a revised webrev. Webrev: http://bussund0416.us.oracle.com/export/users/gradams/work/webrevs/8211343/webrev.01/ Updates in this round of changes : - replaced index() with strchr() to avoid platform dependent issues with strings.h include - removed NSK_JVMTI_OPTION_VAL_SEP - removed temporary debugging print statements - removed empty options string from SetNativeMethodPrefix001 - added free for temporary strdup buffer - updated copyright for 2019 On 12/21/18 1:52 PM, 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. ...
Re: RFR: JDK-8211343: nsk_jvmti_parseoptions should handle multiple suboptions
Here's a revised webrev. Webrev: http://bussund0416.us.oracle.com/export/users/gradams/work/webrevs/8211343/webrev.01/ Updates in this round of changes : - replaced index() with strchr() to avoid platform dependent issues with strings.h include - removed NSK_JVMTI_OPTION_VAL_SEP - removed temporary debugging print statements - removed empty options string from SetNativeMethodPrefix001 - added free for temporary strdup buffer - updated copyright for 2019 On 12/21/18 1:52 PM, 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. ...
Re: RFR: JDK-8211343: nsk_jvmti_parseoptions should handle multiple suboptions
On 12/21/18 12:55 PM, gary.ad...@oracle.com wrote:
On 12/21/18 3:33 PM, serguei.spit...@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.
...
Re: RFR: JDK-8211343: nsk_jvmti_parseoptions should handle multiple suboptions
On 12/21/18 3:33 PM, serguei.spit...@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.
...
Re: RFR: JDK-8211343: nsk_jvmti_parseoptions should handle multiple suboptions
On 12/21/18 2:31 PM, 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)? I choose to believe the author was confused. 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++; I believe it is a common idiom for C programs, to set a null and increment a pointer. If you think it is confusing I can change it. 235 /* Create a temporay copy of the options string to be tokenized. */ "temporay" typo. Done. I did run into solaris and windows build failures, because the index() prototype is in strings.h on other platforms. I'll also try substituting strchr() so we can just include string.h I've been checking other places in the jdk sources and noticed checks for _MSC_VER, _WIN32, defined(__linix__), and NEED_BSD_STRINGS when including strings.h I'll post a fresh webrev when I get clean builds. ... 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. ...
Re: RFR: JDK-8211343: nsk_jvmti_parseoptions should handle multiple suboptions
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);
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';
}
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.
...
Re: RFR: JDK-8211343: nsk_jvmti_parseoptions should handle multiple suboptions
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. ...
RFR: JDK-8211343: nsk_jvmti_parseoptions should handle multiple suboptions
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. ...
Re: JDK-8211343: nsk_jvmti_parseoptions should handle multiple suboptions
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
#include
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;
//}
Re: JDK-8211343: nsk_jvmti_parseoptions should handle multiple suboptions
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:
Re: JDK-8211343: nsk_jvmti_parseoptions should handle multiple suboptions
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
#include
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 > 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
Re: JDK-8211343: nsk_jvmti_parseoptions should handle multiple suboptions
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 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
JDK-8211343: nsk_jvmti_parseoptions should handle multiple suboptions
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;
