Re: [PATCH perf/core v2 1/8] perf: Rewrite strbuf not to die

[Arnaldo Carvalho de Melo]

Em Sat, Apr 30, 2016 at 12:09:50AM +0900, Masami Hiramatsu escreveu:
> Rewrite strbuf implementation not to use die() nor xrealloc().
> Instead of die, now most of the API returns error code or 0 if
> succeeded.
> 
> Signed-off-by: Masami Hiramatsu <mhiramat@xxxxxxxxxx>
> ---
>  tools/perf/util/strbuf.c |   93 +++++++++++++++++++++++++++++++++-------------
>  tools/perf/util/strbuf.h |   25 +++++++-----
>  2 files changed, 82 insertions(+), 36 deletions(-)
> 
> diff --git a/tools/perf/util/strbuf.c b/tools/perf/util/strbuf.c
> index 8fb7329..a98bb60 100644
> --- a/tools/perf/util/strbuf.c
> +++ b/tools/perf/util/strbuf.c
> @@ -1,3 +1,4 @@
> +#include "debug.h"
>  #include "cache.h"
>  #include <linux/kernel.h>
>  
> @@ -17,12 +18,13 @@ int prefixcmp(const char *str, const char *prefix)
>   */
>  char strbuf_slopbuf[1];
>  
> -void strbuf_init(struct strbuf *sb, ssize_t hint)
> +int strbuf_init(struct strbuf *sb, ssize_t hint)
>  {
>  	sb->alloc = sb->len = 0;
>  	sb->buf = strbuf_slopbuf;
>  	if (hint)
> -		strbuf_grow(sb, hint);
> +		return strbuf_grow(sb, hint);
> +	return 0;
>  }
>  
>  void strbuf_release(struct strbuf *sb)
> @@ -42,67 +44,104 @@ char *strbuf_detach(struct strbuf *sb, size_t *sz)
>  	return res;
>  }
>  
> -void strbuf_grow(struct strbuf *sb, size_t extra)
> +int strbuf_grow(struct strbuf *sb, size_t extra)
>  {
> -	if (sb->len + extra + 1 <= sb->len)
> -		die("you want to use way too much memory");
> -	if (!sb->alloc)
> -		sb->buf = NULL;
> -	ALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);
> +	char *buf;
> +	size_t nr = sb->len + extra + 1;
> +
> +	if (nr < sb->alloc)
> +		return 0;
> +
> +	if (nr <= sb->len)
> +		return -E2BIG;
> +
> +	if (alloc_nr(sb->alloc) > nr)
> +		nr = alloc_nr(sb->alloc);
> +
> +	buf = malloc(nr * sizeof(*buf));
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	if (sb->alloc) {
> +		memcpy(buf, sb->buf, sb->alloc);
> +		free(sb->buf);
> +	}

Why not use realloc? I.e. as the old code did, the problem was not that,
was just the panic when the realloc operation fails, that you are
removing here.

I.e. the above would be:

	buf = realloc(sb->buf, nr * sizeof(*buf));
	if (!buf)
		return -ENOMEM;

> +	sb->buf = buf;
> +	sb->alloc = nr;
> +	return 0;

I.e. no need to do the memcpy() nor the free(), no?

>  ssize_t strbuf_read(struct strbuf *sb, int fd, ssize_t hint)
>  {
>  	size_t oldlen = sb->len;
>  	size_t oldalloc = sb->alloc;
> +	int ret;
> +
> +	ret = strbuf_grow(sb, hint ? hint : 8192);
> +	if (ret)
> +		return ret;
>  
> -	strbuf_grow(sb, hint ? hint : 8192);
>  	for (;;) {
>  		ssize_t cnt;
>  
> @@ -112,12 +151,14 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, ssize_t hint)
>  				strbuf_release(sb);
>  			else
>  				strbuf_setlen(sb, oldlen);
> -			return -1;
> +			return cnt;

This is unrelated, no?

I.e. this _was_ already returning a failure code, but then you are
propagating the read() return, which may even be a good idea, haven't
thought about that, but is unrelated to what this patch is doing, please
put it in a separate patch if you think it is a good idea.

All the rest seems ok, going over the other patches now.

- Arnaldo