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
|
- JDK-8211343: nsk_jvmti_parseoptions should handle mult... Gary Adams
- Re: JDK-8211343: nsk_jvmti_parseoptions should ha... JC Beyler
- Re: JDK-8211343: nsk_jvmti_parseoptions shoul... Gary Adams
- Re: JDK-8211343: nsk_jvmti_parseoptions s... Chris Plummer
- Re: JDK-8211343: nsk_jvmti_parseoptio... Gary Adams
- Re: JDK-8211343: nsk_jvmti_parse... Daniel D. Daugherty