| Message ID | 1402996306-6811-3-git-send-email-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
-- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Oops, I just click send before I type up the reply. Here we go again. On Tue, Jun 17, 2014 at 2:11 AM, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > Very dumb patch to just skip --param allow-store-data-races=0 introduced in > newer GCC versions. > > +static char **handle_param(char *arg, char **next) > +{ > + const char *value = NULL; > + > + /* For now just skip any '--param=*' or '--param *' */ > + value = split_value_from_arg(arg, value); > + if (!value) > + ++next; > + > + return ++next; > +} I think this is problematic.There are three possible input from args: 1) "--parm", you need to ++next skip to next arg, which is the value for parm. 2) "--parm=x", you don't need to skip to next arg. 3) "--parm-with-crap", invalid argument. You don't need to skip next arg. I think the patch is wrong on case 2) and case 3). In case 2), the patch skip two arguments and make next point points to out of bound memory. The split_value_from_arg function is not a good abstraction for this job. Its return value can only indicate 2 possible out come. Also, returning the default value force the test against the input default value. That make the logic a bit complicate. > struct switches { > const char *name; > char **(*fn)(char *, char **); > @@ -686,13 +698,14 @@ struct switches { > static char **handle_long_options(char *arg, char **next) > { > static struct switches cmd[] = { > + { "param", handle_param }, > { "version", handle_version }, > { NULL, NULL } > }; > struct switches *s = cmd; > > while (s->name) { > - if (!strcmp(s->name, arg)) > + if (!strncmp(arg, s->name, strlen(s->name))) This will allow "--version-with-crap" as valid arguments. I think we can have one extra member in "struct switch" to indicate this option is a prefix rather than a whole word. For "parm", it need to set that prefix member to non zero. Please let me know if there is a V3 coming. Chris -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, 2014-06-28 at 09:59 -0700, Christopher Li wrote: > Oops, I just click send before I type up the reply. Here we go again. > > On Tue, Jun 17, 2014 at 2:11 AM, Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > Very dumb patch to just skip --param allow-store-data-races=0 introduced in > > newer GCC versions. > > > > +static char **handle_param(char *arg, char **next) > > +{ > > + const char *value = NULL; > > + > > + /* For now just skip any '--param=*' or '--param *' */ > > + value = split_value_from_arg(arg, value); > > + if (!value) > > + ++next; > > + > > + return ++next; > > +} > > I think this is problematic.There are three possible input > from args: > 1) "--parm", you need to ++next skip to next arg, which is the value for parm. > 2) "--parm=x", you don't need to skip to next arg. > 3) "--parm-with-crap", invalid argument. You don't need to skip next arg. > > I think the patch is wrong on case 2) and case 3). > In case 2), the patch skip two arguments and make next point > points to out of bound memory. Hmm... I'd just added test printf to the handle_param() and see if I print *next, it is either --param or --param=*. So, using return (next + 2) helps, otherwise we end up with the same situation as before patch. What did I miss? > > The split_value_from_arg function is not a good abstraction for this job. > Its return value can only indicate 2 possible out come. > Also, returning the default value force the test against the input > default value. That make the logic a bit complicate. > > > struct switches { > > const char *name; > > char **(*fn)(char *, char **); > > @@ -686,13 +698,14 @@ struct switches { > > static char **handle_long_options(char *arg, char **next) > > { > > static struct switches cmd[] = { > > + { "param", handle_param }, > > { "version", handle_version }, > > { NULL, NULL } > > }; > > struct switches *s = cmd; > > > > while (s->name) { > > - if (!strcmp(s->name, arg)) > > + if (!strncmp(arg, s->name, strlen(s->name))) > > This will allow "--version-with-crap" as valid arguments. Which was explicitly mentioned in the commit message. > > I think we can have one extra member in "struct switch" > to indicate this option is a prefix rather than a whole word. > For "parm", it need to set that prefix member to non zero. No objections about this approach. > Please let me know if there is a V3 coming. Apparently you did this on weekend.
On Mon, Jun 30, 2014 at 1:32 AM, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > Hmm... I'd just added test printf to the handle_param() and see if I > print *next, it is either --param or --param=*. So, using return (next + > 2) helps, otherwise we end up with the same situation as before patch. The return value from handle_switch() is a bit tricky. It is actually points to the current args which about to be expired. Take a look at this code which invoke the handle_switch(). for (;;) { char *arg = *++args; <---------------- notice the ++ before the fetch if (!arg) break; if (arg[0] == '-' && arg[1]) { args = handle_switch(arg+1, args); <-------- args return here. continue; } add_ptr_list_notag(filelist, arg); } > > What did I miss? So the caller loop will perform 1 pointer advance before fetch. Your code can advance 2 pointer, so that is total 3 pointer advance. > > Which was explicitly mentioned in the commit message. Sorry about that, I jump to the code first. I later notice that in the commit message as well. Any way, the change I push should fix all that. Chris -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2014-06-30 at 01:51 -0700, Christopher Li wrote: > On Mon, Jun 30, 2014 at 1:32 AM, Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > > Hmm... I'd just added test printf to the handle_param() and see if I > > print *next, it is either --param or --param=*. So, using return (next + > > 2) helps, otherwise we end up with the same situation as before patch. > > The return value from handle_switch() is a bit tricky. It is actually points to > the current args which about to be expired. > > Take a look at this code which invoke the handle_switch(). > for (;;) { > char *arg = *++args; <---------------- notice the ++ > before the fetch > if (!arg) > break; > > if (arg[0] == '-' && arg[1]) { > args = handle_switch(arg+1, args); <-------- args return here. > continue; > } > add_ptr_list_notag(filelist, arg); > } > > > > > What did I miss? > > So the caller loop will perform 1 pointer advance before fetch. > Your code can advance 2 pointer, so that is total 3 pointer advance. Yeah, thanks for explanation. Just noticed this after send a message. > > > > > Which was explicitly mentioned in the commit message. > > Sorry about that, I jump to the code first. I later notice that in > the commit message as well. > > Any way, the change I push should fix all that. Yup. Thank you.
diff --git a/lib.c b/lib.c index 4e5a846..d5b94c3 100644 --- a/lib.c +++ b/lib.c @@ -678,6 +678,18 @@ static char **handle_version(char *arg, char **next) exit(0); } +static char **handle_param(char *arg, char **next) +{ + const char *value = NULL; + + /* For now just skip any '--param=*' or '--param *' */ + value = split_value_from_arg(arg, value); + if (!value) + ++next; + + return ++next; +} + struct switches { const char *name; char **(*fn)(char *, char **); @@ -686,13 +698,14 @@ struct switches { static char **handle_long_options(char *arg, char **next) { static struct switches cmd[] = { + { "param", handle_param }, { "version", handle_version }, { NULL, NULL } }; struct switches *s = cmd; while (s->name) { - if (!strcmp(s->name, arg)) + if (!strncmp(arg, s->name, strlen(s->name))) return s->fn(arg, next); s++; }
Very dumb patch to just skip --param allow-store-data-races=0 introduced in newer GCC versions. Without this patch sparse recognizes parameter of the --param option as a file name which obviously couldn't be found. The patch for easy implementation's sake slightly changed behaviour of --version. Instead of exact keyword it will recognize anything starting with --version as a correct option. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- lib.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)