From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756099AbcEEXZq (ORCPT ); Thu, 5 May 2016 19:25:46 -0400 Received: from mail.kernel.org ([198.145.29.136]:39863 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753293AbcEEXZo (ORCPT ); Thu, 5 May 2016 19:25:44 -0400 Date: Thu, 5 May 2016 20:25:38 -0300 From: Arnaldo Carvalho de Melo To: Masami Hiramatsu Cc: linux-kernel@vger.kernel.org, Namhyung Kim , Peter Zijlstra , Ingo Molnar Subject: Re: [PATCH perf/core v2 1/8] perf: Rewrite strbuf not to die Message-ID: <20160505232538.GK11069@kernel.org> References: <20160429150941.30063.62888.stgit@devbox> <20160429150950.30063.39517.stgit@devbox> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160429150950.30063.39517.stgit@devbox> X-Url: http://acmel.wordpress.com User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > --- > 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 > > @@ -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