[PATCH] perf: fix a segfault problem.

[PATCH] perf: fix a segfault problem.

[Wang Nan]

Without this patch, perf report cause segfault if pass "" as '-t':

  $ perf report -t ""

    # To display the perf.data header info, please use --header/--header-only options.
    #
    # Samples: 37  of event 'syscalls:sys_enter_write'
    # Event count (approx.): 37
    #
    # Children    SelfCommand   Shared Object         Symbol
    Segmentation fault

This patch avoid the segfault by checking empty string for
'symbol_conf.field_sep'.

Signed-off-by: Wang Nan <wangnan0@huawei.com>
---
 tools/perf/util/sort.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 4593f36..7f563a0 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -31,7 +31,8 @@ static int repsep_snprintf(char *bf, size_t size, const char *fmt, ...)
 
 	va_start(ap, fmt);
 	n = vsnprintf(bf, size, fmt, ap);
-	if (symbol_conf.field_sep && n > 0) {
+	if (symbol_conf.field_sep && n > 0 &&
+			(symbol_conf.field_sep[0] != '\0')) {
 		char *sep = bf;
 
 		while (1) {
-- 
1.8.3.4

Re: [PATCH] perf: fix a segfault problem.

[Namhyung Kim]

On Fri, Mar 13, 2015 at 08:41:32AM +0000, Wang Nan wrote:
> Without this patch, perf report cause segfault if pass "" as '-t':
> 
>   $ perf report -t ""
> 
>     # To display the perf.data header info, please use --header/--header-only options.
>     #
>     # Samples: 37  of event 'syscalls:sys_enter_write'
>     # Event count (approx.): 37
>     #
>     # Children    SelfCommand   Shared Object         Symbol
>     Segmentation fault
> 
> This patch avoid the segfault by checking empty string for
> 'symbol_conf.field_sep'.

What about resetting it to NULL if empty string was given?

Thanks,
Namhyung


> 
> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> ---
>  tools/perf/util/sort.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
> index 4593f36..7f563a0 100644
> --- a/tools/perf/util/sort.c
> +++ b/tools/perf/util/sort.c
> @@ -31,7 +31,8 @@ static int repsep_snprintf(char *bf, size_t size, const char *fmt, ...)
>  
>  	va_start(ap, fmt);
>  	n = vsnprintf(bf, size, fmt, ap);
> -	if (symbol_conf.field_sep && n > 0) {
> +	if (symbol_conf.field_sep && n > 0 &&
> +			(symbol_conf.field_sep[0] != '\0')) {
>  		char *sep = bf;
>  
>  		while (1) {
> -- 
> 1.8.3.4
> 

Re: [PATCH] perf: fix a segfault problem.

[Wang Nan]

On 2015/3/13 17:46, Namhyung Kim wrote:
> On Fri, Mar 13, 2015 at 08:41:32AM +0000, Wang Nan wrote:
>> Without this patch, perf report cause segfault if pass "" as '-t':
>>
>>   $ perf report -t ""
>>
>>     # To display the perf.data header info, please use --header/--header-only options.
>>     #
>>     # Samples: 37  of event 'syscalls:sys_enter_write'
>>     # Event count (approx.): 37
>>     #
>>     # Children    SelfCommand   Shared Object         Symbol
>>     Segmentation fault
>>
>> This patch avoid the segfault by checking empty string for
>> 'symbol_conf.field_sep'.
> 
> What about resetting it to NULL if empty string was given?
> 

In fact I'm not very clear why we need such 'symbol_conf.field_sep', so I'm
not sure whether '-t ""' is totally meanless or not.

-t option replaces a group of character with '.' and appends them after a field.
With -t 'abc' I get something like:

 #
 # OverheadabcCommand   abcShared Object    abcSymbol
  100.00%abcb.beltr.ceabc[kernel.k.llsyms]abc[k] 0xffffffff810118f0
  ...

Hard to read...

I read docs and your commit messages, but still not understand the option. Could you
please explain the goal and usage of that option again?

Thank you.

> Thanks,
> Namhyung
> 
> 
>>
>> Signed-off-by: Wang Nan <wangnan0@huawei.com>
>> ---
>>  tools/perf/util/sort.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
>> index 4593f36..7f563a0 100644
>> --- a/tools/perf/util/sort.c
>> +++ b/tools/perf/util/sort.c
>> @@ -31,7 +31,8 @@ static int repsep_snprintf(char *bf, size_t size, const char *fmt, ...)
>>  
>>  	va_start(ap, fmt);
>>  	n = vsnprintf(bf, size, fmt, ap);
>> -	if (symbol_conf.field_sep && n > 0) {
>> +	if (symbol_conf.field_sep && n > 0 &&
>> +			(symbol_conf.field_sep[0] != '\0')) {
>>  		char *sep = bf;
>>  
>>  		while (1) {
>> -- 
>> 1.8.3.4
>>

Re: [PATCH] perf: fix a segfault problem.

[Namhyung Kim]

On Fri, Mar 13, 2015 at 06:07:34PM +0800, Wang Nan wrote:
> On 2015/3/13 17:46, Namhyung Kim wrote:
> > On Fri, Mar 13, 2015 at 08:41:32AM +0000, Wang Nan wrote:
> >> Without this patch, perf report cause segfault if pass "" as '-t':
> >>
> >>   $ perf report -t ""
> >>
> >>     # To display the perf.data header info, please use --header/--header-only options.
> >>     #
> >>     # Samples: 37  of event 'syscalls:sys_enter_write'
> >>     # Event count (approx.): 37
> >>     #
> >>     # Children    SelfCommand   Shared Object         Symbol
> >>     Segmentation fault
> >>
> >> This patch avoid the segfault by checking empty string for
> >> 'symbol_conf.field_sep'.
> > 
> > What about resetting it to NULL if empty string was given?
> > 
> 
> In fact I'm not very clear why we need such 'symbol_conf.field_sep', so I'm
> not sure whether '-t ""' is totally meanless or not.
> 
> -t option replaces a group of character with '.' and appends them after a field.
> With -t 'abc' I get something like:
> 
>  #
>  # OverheadabcCommand   abcShared Object    abcSymbol
>   100.00%abcb.beltr.ceabc[kernel.k.llsyms]abc[k] 0xffffffff810118f0
>   ...
> 
> Hard to read...
> 
> I read docs and your commit messages, but still not understand the option. Could you
> please explain the goal and usage of that option again?

Well, I'm not the person who wrote the doc and introduced this
option. ;-)

Anyway AFAIK it's to generate a CSV file so usual value would be ','.
To reduce possible confusion due to the separation character in the
original output, it replaces the character during the generation.

Thanks,
Namhyung

[PATCH] perf: report: don't allow empty argument for '-t'.

[Wang Nan]

Without this patch, perf report cause segfault if pass "" as '-t':

  $ perf report -t ""

   # To display the perf.data header info, please use --header/--header-only options.
   #
   # Samples: 37  of event 'syscalls:sys_enter_write'
   # Event count (approx.): 37
   #
   # Children    SelfCommand   Shared Object         Symbol
   Segmentation fault

Since -t is used to add field-separator for generate table, -t "" is
actually meanless. This patch defines a new OPT_STRING_NOEMPTY() option
generator to ensure user never pass empty string to that option.

Signed-off-by: Wang Nan <wangnan0@huawei.com>
---
 tools/perf/builtin-report.c     |  2 +-
 tools/perf/util/parse-options.c | 21 +++++++++++++++++++--
 tools/perf/util/parse-options.h |  2 ++
 3 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index fb35034..2652e52 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -676,7 +676,7 @@ int cmd_report(int argc, const char **argv, const char *prefix __maybe_unused)
 	OPT_STRING('w', "column-widths", &symbol_conf.col_width_list_str,
 		   "width[,width...]",
 		   "don't try to adjust column width, use these fixed values"),
-	OPT_STRING('t', "field-separator", &symbol_conf.field_sep, "separator",
+	OPT_STRING_NOEMPTY('t', "field-separator", &symbol_conf.field_sep, "separator",
 		   "separator for columns, no spaces will be added between "
 		   "columns '.' is reserved."),
 	OPT_BOOLEAN('U', "hide-unresolved", &report.hide_unresolved,
diff --git a/tools/perf/util/parse-options.c b/tools/perf/util/parse-options.c
index 1457d66..01626be 100644
--- a/tools/perf/util/parse-options.c
+++ b/tools/perf/util/parse-options.c
@@ -37,6 +37,7 @@ static int get_value(struct parse_opt_ctx_t *p,
 {
 	const char *s, *arg = NULL;
 	const int unset = flags & OPT_UNSET;
+	int err;
 
 	if (unset && p->opt)
 		return opterror(opt, "takes no value", flags);
@@ -114,13 +115,29 @@ static int get_value(struct parse_opt_ctx_t *p,
 		return 0;
 
 	case OPTION_STRING:
+		err = 0;
 		if (unset)
 			*(const char **)opt->value = NULL;
 		else if (opt->flags & PARSE_OPT_OPTARG && !p->opt)
 			*(const char **)opt->value = (const char *)opt->defval;
 		else
-			return get_arg(p, opt, flags, (const char **)opt->value);
-		return 0;
+			err = get_arg(p, opt, flags, (const char **)opt->value);
+
+		/* PARSE_OPT_NOEMPTY: Allow NULL but disallow empty string. */
+		if (opt->flags & PARSE_OPT_NOEMPTY) {
+			const char *val = *(const char **)opt->value;
+
+			if (!val)
+				return err;
+
+			/* Similar to unset if we are given an empty string. */
+			if (val[0] == '\0') {
+				*(const char **)opt->value = NULL;
+				return 0;
+			}
+		}
+
+		return err;
 
 	case OPTION_CALLBACK:
 		if (unset)
diff --git a/tools/perf/util/parse-options.h b/tools/perf/util/parse-options.h
index 97b153f..59561fd 100644
--- a/tools/perf/util/parse-options.h
+++ b/tools/perf/util/parse-options.h
@@ -40,6 +40,7 @@ enum parse_opt_option_flags {
 	PARSE_OPT_LASTARG_DEFAULT = 16,
 	PARSE_OPT_DISABLED = 32,
 	PARSE_OPT_EXCLUSIVE = 64,
+	PARSE_OPT_NOEMPTY  = 128,
 };
 
 struct option;
@@ -122,6 +123,7 @@ struct option {
 #define OPT_LONG(s, l, v, h)        { .type = OPTION_LONG, .short_name = (s), .long_name = (l), .value = check_vtype(v, long *), .help = (h) }
 #define OPT_U64(s, l, v, h)         { .type = OPTION_U64, .short_name = (s), .long_name = (l), .value = check_vtype(v, u64 *), .help = (h) }
 #define OPT_STRING(s, l, v, a, h)   { .type = OPTION_STRING,  .short_name = (s), .long_name = (l), .value = check_vtype(v, const char **), (a), .help = (h) }
+#define OPT_STRING_NOEMPTY(s, l, v, a, h)   { .type = OPTION_STRING,  .short_name = (s), .long_name = (l), .value = check_vtype(v, const char **), (a), .help = (h), .flags = PARSE_OPT_NOEMPTY}
 #define OPT_DATE(s, l, v, h) \
 	{ .type = OPTION_CALLBACK, .short_name = (s), .long_name = (l), .value = (v), .argh = "time", .help = (h), .callback = parse_opt_approxidate_cb }
 #define OPT_CALLBACK(s, l, v, a, h, f) \
-- 
1.8.3.4

Re: [PATCH] perf: report: don't allow empty argument for '-t'.

[Namhyung Kim]

On Fri, Mar 13, 2015 at 12:51:54PM +0000, Wang Nan wrote:
> Without this patch, perf report cause segfault if pass "" as '-t':
> 
>   $ perf report -t ""
> 
>    # To display the perf.data header info, please use --header/--header-only options.
>    #
>    # Samples: 37  of event 'syscalls:sys_enter_write'
>    # Event count (approx.): 37
>    #
>    # Children    SelfCommand   Shared Object         Symbol
>    Segmentation fault
> 
> Since -t is used to add field-separator for generate table, -t "" is
> actually meanless. This patch defines a new OPT_STRING_NOEMPTY() option
> generator to ensure user never pass empty string to that option.
> 
> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> ---
>  tools/perf/builtin-report.c     |  2 +-

I think perf 'diff' and 'mem' commands also have same problem..

Thanks,
Namhyung


>  tools/perf/util/parse-options.c | 21 +++++++++++++++++++--
>  tools/perf/util/parse-options.h |  2 ++
>  3 files changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> index fb35034..2652e52 100644
> --- a/tools/perf/builtin-report.c
> +++ b/tools/perf/builtin-report.c
> @@ -676,7 +676,7 @@ int cmd_report(int argc, const char **argv, const char *prefix __maybe_unused)
>  	OPT_STRING('w', "column-widths", &symbol_conf.col_width_list_str,
>  		   "width[,width...]",
>  		   "don't try to adjust column width, use these fixed values"),
> -	OPT_STRING('t', "field-separator", &symbol_conf.field_sep, "separator",
> +	OPT_STRING_NOEMPTY('t', "field-separator", &symbol_conf.field_sep, "separator",
>  		   "separator for columns, no spaces will be added between "
>  		   "columns '.' is reserved."),
>  	OPT_BOOLEAN('U', "hide-unresolved", &report.hide_unresolved,
> diff --git a/tools/perf/util/parse-options.c b/tools/perf/util/parse-options.c
> index 1457d66..01626be 100644
> --- a/tools/perf/util/parse-options.c
> +++ b/tools/perf/util/parse-options.c
> @@ -37,6 +37,7 @@ static int get_value(struct parse_opt_ctx_t *p,
>  {
>  	const char *s, *arg = NULL;
>  	const int unset = flags & OPT_UNSET;
> +	int err;
>  
>  	if (unset && p->opt)
>  		return opterror(opt, "takes no value", flags);
> @@ -114,13 +115,29 @@ static int get_value(struct parse_opt_ctx_t *p,
>  		return 0;
>  
>  	case OPTION_STRING:
> +		err = 0;
>  		if (unset)
>  			*(const char **)opt->value = NULL;
>  		else if (opt->flags & PARSE_OPT_OPTARG && !p->opt)
>  			*(const char **)opt->value = (const char *)opt->defval;
>  		else
> -			return get_arg(p, opt, flags, (const char **)opt->value);
> -		return 0;
> +			err = get_arg(p, opt, flags, (const char **)opt->value);
> +
> +		/* PARSE_OPT_NOEMPTY: Allow NULL but disallow empty string. */
> +		if (opt->flags & PARSE_OPT_NOEMPTY) {
> +			const char *val = *(const char **)opt->value;
> +
> +			if (!val)
> +				return err;
> +
> +			/* Similar to unset if we are given an empty string. */
> +			if (val[0] == '\0') {
> +				*(const char **)opt->value = NULL;
> +				return 0;
> +			}
> +		}
> +
> +		return err;
>  
>  	case OPTION_CALLBACK:
>  		if (unset)
> diff --git a/tools/perf/util/parse-options.h b/tools/perf/util/parse-options.h
> index 97b153f..59561fd 100644
> --- a/tools/perf/util/parse-options.h
> +++ b/tools/perf/util/parse-options.h
> @@ -40,6 +40,7 @@ enum parse_opt_option_flags {
>  	PARSE_OPT_LASTARG_DEFAULT = 16,
>  	PARSE_OPT_DISABLED = 32,
>  	PARSE_OPT_EXCLUSIVE = 64,
> +	PARSE_OPT_NOEMPTY  = 128,
>  };
>  
>  struct option;
> @@ -122,6 +123,7 @@ struct option {
>  #define OPT_LONG(s, l, v, h)        { .type = OPTION_LONG, .short_name = (s), .long_name = (l), .value = check_vtype(v, long *), .help = (h) }
>  #define OPT_U64(s, l, v, h)         { .type = OPTION_U64, .short_name = (s), .long_name = (l), .value = check_vtype(v, u64 *), .help = (h) }
>  #define OPT_STRING(s, l, v, a, h)   { .type = OPTION_STRING,  .short_name = (s), .long_name = (l), .value = check_vtype(v, const char **), (a), .help = (h) }
> +#define OPT_STRING_NOEMPTY(s, l, v, a, h)   { .type = OPTION_STRING,  .short_name = (s), .long_name = (l), .value = check_vtype(v, const char **), (a), .help = (h), .flags = PARSE_OPT_NOEMPTY}
>  #define OPT_DATE(s, l, v, h) \
>  	{ .type = OPTION_CALLBACK, .short_name = (s), .long_name = (l), .value = (v), .argh = "time", .help = (h), .callback = parse_opt_approxidate_cb }
>  #define OPT_CALLBACK(s, l, v, a, h, f) \
> -- 
> 1.8.3.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] perf: report: don't allow empty argument for '-t'.

[Wang Nan]

Hi Namhyung Kim,

Do you have any comment on it?

Thank you.

On 2015/3/13 20:51, Wang Nan wrote:
> Without this patch, perf report cause segfault if pass "" as '-t':
> 
>   $ perf report -t ""
> 
>    # To display the perf.data header info, please use --header/--header-only options.
>    #
>    # Samples: 37  of event 'syscalls:sys_enter_write'
>    # Event count (approx.): 37
>    #
>    # Children    SelfCommand   Shared Object         Symbol
>    Segmentation fault
> 
> Since -t is used to add field-separator for generate table, -t "" is
> actually meanless. This patch defines a new OPT_STRING_NOEMPTY() option
> generator to ensure user never pass empty string to that option.
> 
> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> ---
>  tools/perf/builtin-report.c     |  2 +-
>  tools/perf/util/parse-options.c | 21 +++++++++++++++++++--
>  tools/perf/util/parse-options.h |  2 ++
>  3 files changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> index fb35034..2652e52 100644
> --- a/tools/perf/builtin-report.c
> +++ b/tools/perf/builtin-report.c
> @@ -676,7 +676,7 @@ int cmd_report(int argc, const char **argv, const char *prefix __maybe_unused)
>  	OPT_STRING('w', "column-widths", &symbol_conf.col_width_list_str,
>  		   "width[,width...]",
>  		   "don't try to adjust column width, use these fixed values"),
> -	OPT_STRING('t', "field-separator", &symbol_conf.field_sep, "separator",
> +	OPT_STRING_NOEMPTY('t', "field-separator", &symbol_conf.field_sep, "separator",
>  		   "separator for columns, no spaces will be added between "
>  		   "columns '.' is reserved."),
>  	OPT_BOOLEAN('U', "hide-unresolved", &report.hide_unresolved,
> diff --git a/tools/perf/util/parse-options.c b/tools/perf/util/parse-options.c
> index 1457d66..01626be 100644
> --- a/tools/perf/util/parse-options.c
> +++ b/tools/perf/util/parse-options.c
> @@ -37,6 +37,7 @@ static int get_value(struct parse_opt_ctx_t *p,
>  {
>  	const char *s, *arg = NULL;
>  	const int unset = flags & OPT_UNSET;
> +	int err;
>  
>  	if (unset && p->opt)
>  		return opterror(opt, "takes no value", flags);
> @@ -114,13 +115,29 @@ static int get_value(struct parse_opt_ctx_t *p,
>  		return 0;
>  
>  	case OPTION_STRING:
> +		err = 0;
>  		if (unset)
>  			*(const char **)opt->value = NULL;
>  		else if (opt->flags & PARSE_OPT_OPTARG && !p->opt)
>  			*(const char **)opt->value = (const char *)opt->defval;
>  		else
> -			return get_arg(p, opt, flags, (const char **)opt->value);
> -		return 0;
> +			err = get_arg(p, opt, flags, (const char **)opt->value);
> +
> +		/* PARSE_OPT_NOEMPTY: Allow NULL but disallow empty string. */
> +		if (opt->flags & PARSE_OPT_NOEMPTY) {
> +			const char *val = *(const char **)opt->value;
> +
> +			if (!val)
> +				return err;
> +
> +			/* Similar to unset if we are given an empty string. */
> +			if (val[0] == '\0') {
> +				*(const char **)opt->value = NULL;
> +				return 0;
> +			}
> +		}
> +
> +		return err;
>  
>  	case OPTION_CALLBACK:
>  		if (unset)
> diff --git a/tools/perf/util/parse-options.h b/tools/perf/util/parse-options.h
> index 97b153f..59561fd 100644
> --- a/tools/perf/util/parse-options.h
> +++ b/tools/perf/util/parse-options.h
> @@ -40,6 +40,7 @@ enum parse_opt_option_flags {
>  	PARSE_OPT_LASTARG_DEFAULT = 16,
>  	PARSE_OPT_DISABLED = 32,
>  	PARSE_OPT_EXCLUSIVE = 64,
> +	PARSE_OPT_NOEMPTY  = 128,
>  };
>  
>  struct option;
> @@ -122,6 +123,7 @@ struct option {
>  #define OPT_LONG(s, l, v, h)        { .type = OPTION_LONG, .short_name = (s), .long_name = (l), .value = check_vtype(v, long *), .help = (h) }
>  #define OPT_U64(s, l, v, h)         { .type = OPTION_U64, .short_name = (s), .long_name = (l), .value = check_vtype(v, u64 *), .help = (h) }
>  #define OPT_STRING(s, l, v, a, h)   { .type = OPTION_STRING,  .short_name = (s), .long_name = (l), .value = check_vtype(v, const char **), (a), .help = (h) }
> +#define OPT_STRING_NOEMPTY(s, l, v, a, h)   { .type = OPTION_STRING,  .short_name = (s), .long_name = (l), .value = check_vtype(v, const char **), (a), .help = (h), .flags = PARSE_OPT_NOEMPTY}
>  #define OPT_DATE(s, l, v, h) \
>  	{ .type = OPTION_CALLBACK, .short_name = (s), .long_name = (l), .value = (v), .argh = "time", .help = (h), .callback = parse_opt_approxidate_cb }
>  #define OPT_CALLBACK(s, l, v, a, h, f) \
> 

Re: [PATCH] perf: report: don't allow empty argument for '-t'.

[Namhyung Kim]

On Thu, Mar 19, 2015 at 02:41:42PM +0800, Wang Nan wrote:
> Hi Namhyung Kim,

Hello,

> 
> Do you have any comment on it?

As I said in previous reply, I think perf 'diff' and 'mem' have same
problem.  So how about fix them altogether?

Thanks,
Namhyung


> 
> Thank you.
> 
> On 2015/3/13 20:51, Wang Nan wrote:
> > Without this patch, perf report cause segfault if pass "" as '-t':
> > 
> >   $ perf report -t ""
> > 
> >    # To display the perf.data header info, please use --header/--header-only options.
> >    #
> >    # Samples: 37  of event 'syscalls:sys_enter_write'
> >    # Event count (approx.): 37
> >    #
> >    # Children    SelfCommand   Shared Object         Symbol
> >    Segmentation fault
> > 
> > Since -t is used to add field-separator for generate table, -t "" is
> > actually meanless. This patch defines a new OPT_STRING_NOEMPTY() option
> > generator to ensure user never pass empty string to that option.
> > 
> > Signed-off-by: Wang Nan <wangnan0@huawei.com>
> > ---
> >  tools/perf/builtin-report.c     |  2 +-
> >  tools/perf/util/parse-options.c | 21 +++++++++++++++++++--
> >  tools/perf/util/parse-options.h |  2 ++
> >  3 files changed, 22 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> > index fb35034..2652e52 100644
> > --- a/tools/perf/builtin-report.c
> > +++ b/tools/perf/builtin-report.c
> > @@ -676,7 +676,7 @@ int cmd_report(int argc, const char **argv, const char *prefix __maybe_unused)
> >  	OPT_STRING('w', "column-widths", &symbol_conf.col_width_list_str,
> >  		   "width[,width...]",
> >  		   "don't try to adjust column width, use these fixed values"),
> > -	OPT_STRING('t', "field-separator", &symbol_conf.field_sep, "separator",
> > +	OPT_STRING_NOEMPTY('t', "field-separator", &symbol_conf.field_sep, "separator",
> >  		   "separator for columns, no spaces will be added between "
> >  		   "columns '.' is reserved."),
> >  	OPT_BOOLEAN('U', "hide-unresolved", &report.hide_unresolved,
> > diff --git a/tools/perf/util/parse-options.c b/tools/perf/util/parse-options.c
> > index 1457d66..01626be 100644
> > --- a/tools/perf/util/parse-options.c
> > +++ b/tools/perf/util/parse-options.c
> > @@ -37,6 +37,7 @@ static int get_value(struct parse_opt_ctx_t *p,
> >  {
> >  	const char *s, *arg = NULL;
> >  	const int unset = flags & OPT_UNSET;
> > +	int err;
> >  
> >  	if (unset && p->opt)
> >  		return opterror(opt, "takes no value", flags);
> > @@ -114,13 +115,29 @@ static int get_value(struct parse_opt_ctx_t *p,
> >  		return 0;
> >  
> >  	case OPTION_STRING:
> > +		err = 0;
> >  		if (unset)
> >  			*(const char **)opt->value = NULL;
> >  		else if (opt->flags & PARSE_OPT_OPTARG && !p->opt)
> >  			*(const char **)opt->value = (const char *)opt->defval;
> >  		else
> > -			return get_arg(p, opt, flags, (const char **)opt->value);
> > -		return 0;
> > +			err = get_arg(p, opt, flags, (const char **)opt->value);
> > +
> > +		/* PARSE_OPT_NOEMPTY: Allow NULL but disallow empty string. */
> > +		if (opt->flags & PARSE_OPT_NOEMPTY) {
> > +			const char *val = *(const char **)opt->value;
> > +
> > +			if (!val)
> > +				return err;
> > +
> > +			/* Similar to unset if we are given an empty string. */
> > +			if (val[0] == '\0') {
> > +				*(const char **)opt->value = NULL;
> > +				return 0;
> > +			}
> > +		}
> > +
> > +		return err;
> >  
> >  	case OPTION_CALLBACK:
> >  		if (unset)
> > diff --git a/tools/perf/util/parse-options.h b/tools/perf/util/parse-options.h
> > index 97b153f..59561fd 100644
> > --- a/tools/perf/util/parse-options.h
> > +++ b/tools/perf/util/parse-options.h
> > @@ -40,6 +40,7 @@ enum parse_opt_option_flags {
> >  	PARSE_OPT_LASTARG_DEFAULT = 16,
> >  	PARSE_OPT_DISABLED = 32,
> >  	PARSE_OPT_EXCLUSIVE = 64,
> > +	PARSE_OPT_NOEMPTY  = 128,
> >  };
> >  
> >  struct option;
> > @@ -122,6 +123,7 @@ struct option {
> >  #define OPT_LONG(s, l, v, h)        { .type = OPTION_LONG, .short_name = (s), .long_name = (l), .value = check_vtype(v, long *), .help = (h) }
> >  #define OPT_U64(s, l, v, h)         { .type = OPTION_U64, .short_name = (s), .long_name = (l), .value = check_vtype(v, u64 *), .help = (h) }
> >  #define OPT_STRING(s, l, v, a, h)   { .type = OPTION_STRING,  .short_name = (s), .long_name = (l), .value = check_vtype(v, const char **), (a), .help = (h) }
> > +#define OPT_STRING_NOEMPTY(s, l, v, a, h)   { .type = OPTION_STRING,  .short_name = (s), .long_name = (l), .value = check_vtype(v, const char **), (a), .help = (h), .flags = PARSE_OPT_NOEMPTY}
> >  #define OPT_DATE(s, l, v, h) \
> >  	{ .type = OPTION_CALLBACK, .short_name = (s), .long_name = (l), .value = (v), .argh = "time", .help = (h), .callback = parse_opt_approxidate_cb }
> >  #define OPT_CALLBACK(s, l, v, a, h, f) \
> > 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] perf: report: don't allow empty argument for '-t'.

[Arnaldo Carvalho de Melo]

Em Thu, Mar 19, 2015 at 04:26:24PM +0900, Namhyung Kim escreveu:
> On Thu, Mar 19, 2015 at 02:41:42PM +0800, Wang Nan wrote:
> > Hi Namhyung Kim,
> 
> Hello,
> 
> > 
> > Do you have any comment on it?
> 
> As I said in previous reply, I think perf 'diff' and 'mem' have same
> problem.  So how about fix them altogether?

So you are ok with this change, right? Can I cound that as an ack? I.e.
he could send the other fixes as separate patch(es).

- Arnaldo

Re: [PATCH] perf: report: don't allow empty argument for '-t'.

[Namhyung Kim]

Hi Arnaldo,

On Thu, Mar 19, 2015 at 11:00 PM, Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
> Em Thu, Mar 19, 2015 at 04:26:24PM +0900, Namhyung Kim escreveu:
>> On Thu, Mar 19, 2015 at 02:41:42PM +0800, Wang Nan wrote:
>> > Hi Namhyung Kim,
>>
>> Hello,
>>
>> >
>> > Do you have any comment on it?
>>
>> As I said in previous reply, I think perf 'diff' and 'mem' have same
>> problem.  So how about fix them altogether?
>
> So you are ok with this change, right? Can I cound that as an ack? I.e.
> he could send the other fixes as separate patch(es).

I'm okay with it

Acked-by: Namhyung Kim <namhyung@kernel.org>

Thanks,
Namhyung

[tip:perf/core] perf report: Don't allow empty argument for '-t'.

[tip-bot for Wang Nan]

Commit-ID:  0c8c20779c5d56b93b8cb4cd30ba129a927ab437
Gitweb:     http://git.kernel.org/tip/0c8c20779c5d56b93b8cb4cd30ba129a927ab437
Author:     Wang Nan <wangnan0@huawei.com>
AuthorDate: Fri, 13 Mar 2015 12:51:54 +0000
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Thu, 19 Mar 2015 13:53:28 -0300

perf report: Don't allow empty argument for '-t'.

Without this patch, perf report cause segfault if pass "" as '-t':

  $ perf report -t ""

   # To display the perf.data header info, please use --header/--header-only options.
   #
   # Samples: 37  of event 'syscalls:sys_enter_write'
   # Event count (approx.): 37
   #
   # Children    SelfCommand   Shared Object         Symbol
   Segmentation fault

Since -t is used to add field-separator for generate table, -t "" is
actually meanless. This patch defines a new OPT_STRING_NOEMPTY() option
generator to ensure user never pass empty string to that option.

Signed-off-by: Wang Nan <wangnan0@huawei.com>
Acked-by: Namhyung Kim <namhyung@kernel.org>
Cc: pi3orama@163.com
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Zefan Li <lizefan@huawei.com>
Link: http://lkml.kernel.org/r/1426251114-198991-1-git-send-email-wangnan0@huawei.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-report.c     |  2 +-
 tools/perf/util/parse-options.c | 21 +++++++++++++++++++--
 tools/perf/util/parse-options.h |  2 ++
 3 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 52f74e1..0ae4826 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -676,7 +676,7 @@ int cmd_report(int argc, const char **argv, const char *prefix __maybe_unused)
 	OPT_STRING('w', "column-widths", &symbol_conf.col_width_list_str,
 		   "width[,width...]",
 		   "don't try to adjust column width, use these fixed values"),
-	OPT_STRING('t', "field-separator", &symbol_conf.field_sep, "separator",
+	OPT_STRING_NOEMPTY('t', "field-separator", &symbol_conf.field_sep, "separator",
 		   "separator for columns, no spaces will be added between "
 		   "columns '.' is reserved."),
 	OPT_BOOLEAN('U', "hide-unresolved", &report.hide_unresolved,
diff --git a/tools/perf/util/parse-options.c b/tools/perf/util/parse-options.c
index 1457d66..01626be 100644
--- a/tools/perf/util/parse-options.c
+++ b/tools/perf/util/parse-options.c
@@ -37,6 +37,7 @@ static int get_value(struct parse_opt_ctx_t *p,
 {
 	const char *s, *arg = NULL;
 	const int unset = flags & OPT_UNSET;
+	int err;
 
 	if (unset && p->opt)
 		return opterror(opt, "takes no value", flags);
@@ -114,13 +115,29 @@ static int get_value(struct parse_opt_ctx_t *p,
 		return 0;
 
 	case OPTION_STRING:
+		err = 0;
 		if (unset)
 			*(const char **)opt->value = NULL;
 		else if (opt->flags & PARSE_OPT_OPTARG && !p->opt)
 			*(const char **)opt->value = (const char *)opt->defval;
 		else
-			return get_arg(p, opt, flags, (const char **)opt->value);
-		return 0;
+			err = get_arg(p, opt, flags, (const char **)opt->value);
+
+		/* PARSE_OPT_NOEMPTY: Allow NULL but disallow empty string. */
+		if (opt->flags & PARSE_OPT_NOEMPTY) {
+			const char *val = *(const char **)opt->value;
+
+			if (!val)
+				return err;
+
+			/* Similar to unset if we are given an empty string. */
+			if (val[0] == '\0') {
+				*(const char **)opt->value = NULL;
+				return 0;
+			}
+		}
+
+		return err;
 
 	case OPTION_CALLBACK:
 		if (unset)
diff --git a/tools/perf/util/parse-options.h b/tools/perf/util/parse-options.h
index 97b153f..59561fd 100644
--- a/tools/perf/util/parse-options.h
+++ b/tools/perf/util/parse-options.h
@@ -40,6 +40,7 @@ enum parse_opt_option_flags {
 	PARSE_OPT_LASTARG_DEFAULT = 16,
 	PARSE_OPT_DISABLED = 32,
 	PARSE_OPT_EXCLUSIVE = 64,
+	PARSE_OPT_NOEMPTY  = 128,
 };
 
 struct option;
@@ -122,6 +123,7 @@ struct option {
 #define OPT_LONG(s, l, v, h)        { .type = OPTION_LONG, .short_name = (s), .long_name = (l), .value = check_vtype(v, long *), .help = (h) }
 #define OPT_U64(s, l, v, h)         { .type = OPTION_U64, .short_name = (s), .long_name = (l), .value = check_vtype(v, u64 *), .help = (h) }
 #define OPT_STRING(s, l, v, a, h)   { .type = OPTION_STRING,  .short_name = (s), .long_name = (l), .value = check_vtype(v, const char **), (a), .help = (h) }
+#define OPT_STRING_NOEMPTY(s, l, v, a, h)   { .type = OPTION_STRING,  .short_name = (s), .long_name = (l), .value = check_vtype(v, const char **), (a), .help = (h), .flags = PARSE_OPT_NOEMPTY}
 #define OPT_DATE(s, l, v, h) \
 	{ .type = OPTION_CALLBACK, .short_name = (s), .long_name = (l), .value = (v), .argh = "time", .help = (h), .callback = parse_opt_approxidate_cb }
 #define OPT_CALLBACK(s, l, v, a, h, f) \