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

[Masami Hiramatsu]

On Thu, 5 May 2016 20:25:38 -0300
Arnaldo Carvalho de Melo <acme@xxxxxxxxxx> wrote:

> 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.

Oops, right. it seems I misunderstood the return value of realloc.
> 
> 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?

Yes, agreed.
I'll update to do so.

Thanks!




-- 
Masami Hiramatsu <mhiramat@xxxxxxxxxx>