Re: [PATCH 1/3] perf probe: Remove length limitation for showing available variables

[Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>]

On 2015/05/08 21:23, He Kuang wrote:
> Use struct strbuf instead of bare char[] to remove the length limitation
> of variables in variable_list, so they will not disappear due to
> overlength, and make preparation for adding more description for
> variables.

Looks good to me, except one memory leak. please see below.

> 
> Signed-off-by: He Kuang <hekuang@huawei.com>
> ---
>  tools/perf/util/dwarf-aux.c    | 50 +++++++++++++++++++-----------------------
>  tools/perf/util/dwarf-aux.h    |  4 ++--
>  tools/perf/util/probe-finder.c | 16 ++++++++------
>  3 files changed, 33 insertions(+), 37 deletions(-)
> 
> diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
> index c34e024..75d264e 100644
> --- a/tools/perf/util/dwarf-aux.c
> +++ b/tools/perf/util/dwarf-aux.c
> @@ -832,19 +832,17 @@ Dwarf_Die *die_find_member(Dwarf_Die *st_die, const char *name,
>  /**
>   * die_get_typename - Get the name of given variable DIE
>   * @vr_die: a variable DIE
> - * @buf: a buffer for result type name
> - * @len: a max-length of @buf
> + * @buf: a strbuf for result type name
>   *
> - * Get the name of @vr_die and stores it to @buf. Return the actual length
> - * of type name if succeeded. Return -E2BIG if @len is not enough long, and
> - * Return -ENOENT if failed to find type name.
> + * Get the name of @vr_die and stores it to @buf. Return 0 if succeeded.
> + * and Return -ENOENT if failed to find type name.
>   * Note that the result will stores typedef name if possible, and stores
>   * "*(function_type)" if the type is a function pointer.
>   */
> -int die_get_typename(Dwarf_Die *vr_die, char *buf, int len)
> +int die_get_typename(Dwarf_Die *vr_die, struct strbuf *buf)
>  {
>  	Dwarf_Die type;
> -	int tag, ret, ret2;
> +	int tag, ret;
>  	const char *tmp = "";
>  
>  	if (__die_get_real_type(vr_die, &type) == NULL)
> @@ -855,8 +853,8 @@ int die_get_typename(Dwarf_Die *vr_die, char *buf, int len)
>  		tmp = "*";
>  	else if (tag == DW_TAG_subroutine_type) {
>  		/* Function pointer */
> -		ret = snprintf(buf, len, "(function_type)");
> -		return (ret >= len) ? -E2BIG : ret;
> +		strbuf_addf(buf, "(function_type)");
> +		return 0;
>  	} else {
>  		if (!dwarf_diename(&type))
>  			return -ENOENT;
> @@ -867,39 +865,35 @@ int die_get_typename(Dwarf_Die *vr_die, char *buf, int len)
>  		else if (tag == DW_TAG_enumeration_type)
>  			tmp = "enum ";
>  		/* Write a base name */
> -		ret = snprintf(buf, len, "%s%s", tmp, dwarf_diename(&type));
> -		return (ret >= len) ? -E2BIG : ret;
> -	}
> -	ret = die_get_typename(&type, buf, len);
> -	if (ret > 0) {
> -		ret2 = snprintf(buf + ret, len - ret, "%s", tmp);
> -		ret = (ret2 >= len - ret) ? -E2BIG : ret2 + ret;
> +		strbuf_addf(buf, "%s%s", tmp, dwarf_diename(&type));
> +		return 0;
>  	}
> +	ret = die_get_typename(&type, buf);
> +	if (ret == 0)
> +		strbuf_addf(buf, "%s", tmp);
> +
>  	return ret;
>  }
>  
>  /**
>   * die_get_varname - Get the name and type of given variable DIE
>   * @vr_die: a variable DIE
> - * @buf: a buffer for type and variable name
> - * @len: the max-length of @buf
> + * @buf: a strbuf for type and variable name
>   *
>   * Get the name and type of @vr_die and stores it in @buf as "type\tname".
>   */
> -int die_get_varname(Dwarf_Die *vr_die, char *buf, int len)
> +int die_get_varname(Dwarf_Die *vr_die, struct strbuf *buf)
>  {
> -	int ret, ret2;
> +	int ret;
>  
> -	ret = die_get_typename(vr_die, buf, len);
> +	ret = die_get_typename(vr_die, buf);
>  	if (ret < 0) {
>  		pr_debug("Failed to get type, make it unknown.\n");
> -		ret = snprintf(buf, len, "(unknown_type)");
> -	}
> -	if (ret > 0) {
> -		ret2 = snprintf(buf + ret, len - ret, "\t%s",
> -				dwarf_diename(vr_die));
> -		ret = (ret2 >= len - ret) ? -E2BIG : ret2 + ret;
> +		strbuf_addf(buf, "(unknown_type)");
>  	}
> -	return ret;
> +
> +	strbuf_addf(buf, "\t%s", dwarf_diename(vr_die));
> +
> +	return 0;
>  }
>  
> diff --git a/tools/perf/util/dwarf-aux.h b/tools/perf/util/dwarf-aux.h
> index af7dbcd..c0d6173 100644
> --- a/tools/perf/util/dwarf-aux.h
> +++ b/tools/perf/util/dwarf-aux.h
> @@ -114,8 +114,8 @@ extern Dwarf_Die *die_find_member(Dwarf_Die *st_die, const char *name,
>  				  Dwarf_Die *die_mem);
>  
>  /* Get the name of given variable DIE */
> -extern int die_get_typename(Dwarf_Die *vr_die, char *buf, int len);
> +extern int die_get_typename(Dwarf_Die *vr_die, struct strbuf *buf);
>  
>  /* Get the name and type of given variable DIE, stored as "type\tname" */
> -extern int die_get_varname(Dwarf_Die *vr_die, char *buf, int len);
> +extern int die_get_varname(Dwarf_Die *vr_die, struct strbuf *buf);
>  #endif
> diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
> index 63d3389..7b645ff40 100644
> --- a/tools/perf/util/probe-finder.c
> +++ b/tools/perf/util/probe-finder.c
> @@ -1238,17 +1238,16 @@ int debuginfo__find_trace_events(struct debuginfo *dbg,
>  	return (ret < 0) ? ret : tf.ntevs;
>  }
>  
> -#define MAX_VAR_LEN 64
> -
>  /* Collect available variables in this scope */
>  static int collect_variables_cb(Dwarf_Die *die_mem, void *data)
>  {
>  	struct available_var_finder *af = data;
>  	struct variable_list *vl;
> -	char buf[MAX_VAR_LEN];
> +	struct strbuf buf;
>  	int tag, ret;
>  
>  	vl = &af->vls[af->nvls - 1];
> +	strbuf_init(&buf, 64);
>  
>  	tag = dwarf_tag(die_mem);
>  	if (tag == DW_TAG_formal_parameter ||
> @@ -1257,10 +1256,13 @@ static int collect_variables_cb(Dwarf_Die *die_mem, void *data)
>  						af->pf.fb_ops, &af->pf.sp_die,
>  						NULL);
>  		if (ret == 0) {
> -			ret = die_get_varname(die_mem, buf, MAX_VAR_LEN);
> -			pr_debug2("Add new var: %s\n", buf);
> -			if (ret > 0)
> -				strlist__add(vl->vars, buf);
> +			ret = die_get_varname(die_mem, &buf);
> +			pr_debug2("Add new var: %s\n", buf.buf);
> +			if (ret == 0) {
> +				strlist__add(vl->vars,
> +					strbuf_detach(&buf, NULL));
> +			}
> +			strbuf_release(&buf);

It seems that this strbuf_release() should be called in any case,
since strbuf_init already allocated buffer.

>  		}
>  	}

so here is the good place to call, isn't it?

Thank you,

>  
> 


-- 
Masami HIRAMATSU
Linux Technology Research Center, System Productivity Research Dept.
Center for Technology Innovation - Systems Engineering
Hitachi, Ltd., Research & Development Group
E-mail: masami.hiramatsu.pt@hitachi.com