JDK-8211343: nsk_jvmti_parseoptions should handle multiple suboptions
JC Beyler
jcbeyler at google.com
Mon Jan 7 18:32:05 UTC 2019
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> 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> 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/feba9aa3/attachment-0001.html>
More information about the serviceability-dev
mailing list