Hi Masami,
On Fri, 2019-06-28 at 14:39 +0900, Masami Hiramatsu wrote:
> On Thu, 27 Jun 2019 10:35:16 -0500
> Tom Zanussi <zanu...@kernel.org> wrote:
>
> > In the process of adding better error messages for sorting, I
> > realized
> > that strsep was being used incorrectly and some of the error paths
> > I
> > was expecting to be hit weren't and just fell through to the common
> > invalid key error case.
>
> Would you mean this includes a bugfix too?
>
Yes, though not actually a problem or visible to the user. This
basically cleans things up so that the next patch adding the error
messages works as expected.
Tom
> >
> > It also became obvious that for keyword assignments, it wasn't
> > necessary to save the full assignment and reparse it later, and
> > having
> > a common empty-assignment check would also make more sense in terms
> > of
> > error processing.
> >
> > Change the code to fix these problems and simplify it for new error
> > message changes in a subsequent patch.
> >
> > Signed-off-by: Tom Zanussi <zanu...@kernel.org>
>
> Anyway looks good to me.
>
> Reviewed-by: Masami Hiramatsu <mhira...@kernel.org>
>
> Thanks,
>
> > ---
> > kernel/trace/trace_events_hist.c | 70 ++++++++++++++++----------
> > --------------
> > 1 file changed, 27 insertions(+), 43 deletions(-)
> >
> > diff --git a/kernel/trace/trace_events_hist.c
> > b/kernel/trace/trace_events_hist.c
> > index ca6b0dff60c5..964d032f51c6 100644
> > --- a/kernel/trace/trace_events_hist.c
> > +++ b/kernel/trace/trace_events_hist.c
> > @@ -1985,12 +1985,6 @@ static int parse_map_size(char *str)
> > unsigned long size, map_bits;
> > int ret;
> >
> > - strsep(&str, "=");
> > - if (!str) {
> > - ret = -EINVAL;
> > - goto out;
> > - }
> > -
> > ret = kstrtoul(str, 0, &size);
> > if (ret)
> > goto out;
> > @@ -2050,25 +2044,25 @@ static int parse_action(char *str, struct
> > hist_trigger_attrs *attrs)
> > static int parse_assignment(struct trace_array *tr,
> > char *str, struct hist_trigger_attrs
> > *attrs)
> > {
> > - int ret = 0;
> > + int len, ret = 0;
> >
> > - if ((str_has_prefix(str, "key=")) ||
> > - (str_has_prefix(str, "keys="))) {
> > - attrs->keys_str = kstrdup(str, GFP_KERNEL);
> > + if ((len = str_has_prefix(str, "key=")) ||
> > + (len = str_has_prefix(str, "keys="))) {
> > + attrs->keys_str = kstrdup(str + len, GFP_KERNEL);
> > if (!attrs->keys_str) {
> > ret = -ENOMEM;
> > goto out;
> > }
> > - } else if ((str_has_prefix(str, "val=")) ||
> > - (str_has_prefix(str, "vals=")) ||
> > - (str_has_prefix(str, "values="))) {
> > - attrs->vals_str = kstrdup(str, GFP_KERNEL);
> > + } else if ((len = str_has_prefix(str, "val=")) ||
> > + (len = str_has_prefix(str, "vals=")) ||
> > + (len = str_has_prefix(str, "values="))) {
> > + attrs->vals_str = kstrdup(str + len, GFP_KERNEL);
> > if (!attrs->vals_str) {
> > ret = -ENOMEM;
> > goto out;
> > }
> > - } else if (str_has_prefix(str, "sort=")) {
> > - attrs->sort_key_str = kstrdup(str, GFP_KERNEL);
> > + } else if ((len = str_has_prefix(str, "sort="))) {
> > + attrs->sort_key_str = kstrdup(str + len,
> > GFP_KERNEL);
> > if (!attrs->sort_key_str) {
> > ret = -ENOMEM;
> > goto out;
> > @@ -2079,12 +2073,8 @@ static int parse_assignment(struct
> > trace_array *tr,
> > ret = -ENOMEM;
> > goto out;
> > }
> > - } else if (str_has_prefix(str, "clock=")) {
> > - strsep(&str, "=");
> > - if (!str) {
> > - ret = -EINVAL;
> > - goto out;
> > - }
> > + } else if ((len = str_has_prefix(str, "clock="))) {
> > + str += len;
> >
> > str = strstrip(str);
> > attrs->clock = kstrdup(str, GFP_KERNEL);
> > @@ -2092,8 +2082,8 @@ static int parse_assignment(struct
> > trace_array *tr,
> > ret = -ENOMEM;
> > goto out;
> > }
> > - } else if (str_has_prefix(str, "size=")) {
> > - int map_bits = parse_map_size(str);
> > + } else if ((len = str_has_prefix(str, "size="))) {
> > + int map_bits = parse_map_size(str + len);
> >
> > if (map_bits < 0) {
> > ret = map_bits;
> > @@ -2133,8 +2123,14 @@ parse_hist_trigger_attrs(struct trace_array
> > *tr, char *trigger_str)
> >
> > while (trigger_str) {
> > char *str = strsep(&trigger_str, ":");
> > + char *rhs;
> >
> > - if (strchr(str, '=')) {
> > + rhs = strchr(str, '=');
> > + if (rhs) {
> > + if (!strlen(++rhs)) {
> > + ret = -EINVAL;
> > + goto free;
> > + }
> > ret = parse_assignment(tr, str, attrs);
> > if (ret)
> > goto free;
> > @@ -4459,10 +4455,6 @@ static int create_val_fields(struct
> > hist_trigger_data *hist_data,
> > if (!fields_str)
> > goto out;
> >
> > - strsep(&fields_str, "=");
> > - if (!fields_str)
> > - goto out;
> > -
> > for (i = 0, j = 1; i < TRACING_MAP_VALS_MAX &&
> > j < TRACING_MAP_VALS_MAX; i++) {
> > field_str = strsep(&fields_str, ",");
> > @@ -4557,10 +4549,6 @@ static int create_key_fields(struct
> > hist_trigger_data *hist_data,
> > if (!fields_str)
> > goto out;
> >
> > - strsep(&fields_str, "=");
> > - if (!fields_str)
> > - goto out;
> > -
> > for (i = n_vals; i < n_vals + TRACING_MAP_KEYS_MAX; i++) {
> > field_str = strsep(&fields_str, ",");
> > if (!field_str)
> > @@ -4718,12 +4706,6 @@ static int create_sort_keys(struct
> > hist_trigger_data *hist_data)
> > if (!fields_str)
> > goto out;
> >
> > - strsep(&fields_str, "=");
> > - if (!fields_str) {
> > - ret = -EINVAL;
> > - goto out;
> > - }
> > -
> > for (i = 0; i < TRACING_MAP_SORT_KEYS_MAX; i++) {
> > struct hist_field *hist_field;
> > char *field_str, *field_name;
> > @@ -4732,9 +4714,11 @@ static int create_sort_keys(struct
> > hist_trigger_data *hist_data)
> > sort_key = &hist_data->sort_keys[i];
> >
> > field_str = strsep(&fields_str, ",");
> > - if (!field_str) {
> > - if (i == 0)
> > - ret = -EINVAL;
> > + if (!field_str)
> > + break;
> > +
> > + if (!*field_str) {
> > + ret = -EINVAL;
> > break;
> > }
> >
> > @@ -4744,7 +4728,7 @@ static int create_sort_keys(struct
> > hist_trigger_data *hist_data)
> > }
> >
> > field_name = strsep(&field_str, ".");
> > - if (!field_name) {
> > + if (!field_name || !*field_name) {
> > ret = -EINVAL;
> > break;
> > }
> > --
> > 2.14.1
> >
>
>
