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.ad...@oracle.com <mailto:gary.ad...@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.ad...@oracle.com <mailto:gary.ad...@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

Reply via email to