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?
>
> 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
>
--
Masami Hiramatsu <mhira...@kernel.org>
