Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 26 Nov 2020 22:31:33 +0800
From:   Leo Yan <leo.yan@linaro.org>
To:     Arnaldo Carvalho de Melo <acme@kernel.org>
Cc:     Andre Przywara <andre.przywara@arm.com>,
        Dave Martin <Dave.Martin@arm.com>,
        James Clark <james.clark@arm.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Mark Rutland <mark.rutland@arm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Jiri Olsa <jolsa@redhat.com>,
        Namhyung Kim <namhyung@kernel.org>,
        Al Grant <Al.Grant@arm.com>, Wei Li <liwei391@huawei.com>,
        John Garry <john.garry@huawei.com>,
        Will Deacon <will@kernel.org>,
        Mathieu Poirier <mathieu.poirier@linaro.org>,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v9 01/16] perf arm-spe: Refactor printing string to buffer
Message-ID: <20201126143133.GB27722@leoy-ThinkPad-X240s>
References: <20201119152441.6972-1-leo.yan@linaro.org>
 <20201119152441.6972-2-leo.yan@linaro.org>
 <20201126121158.GB53384@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20201126121158.GB53384@kernel.org>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

On Thu, Nov 26, 2020 at 09:11:58AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Thu, Nov 19, 2020 at 11:24:26PM +0800, Leo Yan escreveu:
> > When outputs strings to the decoding buffer with function snprintf(),
> > SPE decoder needs to detects if any error returns from snprintf() and if
> > so needs to directly bail out.  If snprintf() returns success, it needs
> > to update buffer pointer and reduce the buffer length so can continue to
> > output the next string into the consequent memory space.
> > 
> > This complex logics are spreading in the function arm_spe_pkt_desc() so
> > there has many duplicate codes for handling error detecting, increment
> > buffer pointer and decrement buffer size.
> > 
> > To avoid the duplicate code, this patch introduces a new helper function
> > arm_spe_pkt_out_string() which is used to wrap up the complex logics,
> > and it's used by the caller arm_spe_pkt_desc().  This patch moves the
> > variable 'blen' as the function's local variable so allows to remove
> > the unnecessary braces and improve the readability.
> > 
> > This patch simplifies the return value for arm_spe_pkt_desc(): '0' means
> > success and other values mean an error has occurred.  To realize this,
> > it relies on arm_spe_pkt_out_string()'s parameter 'err', the 'err' is a
> > cumulative value, returns its final value if printing buffer is called
> > for one time or multiple times.  Finally, the error is handled in a
> > central place, rather than directly bailing out in switch-cases, it
> > returns error at the end of arm_spe_pkt_desc().
> > 
> > This patch changes the caller arm_spe_dump() to respect the updated
> > return value semantics of arm_spe_pkt_desc().
> > 
> > Suggested-by: Dave Martin <Dave.Martin@arm.com>
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > Reviewed-by: Andre Przywara <andre.przywara@arm.com>
> > Reviewed-by: Dave Martin <Dave.Martin@arm.com>
> > ---
> >  .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 302 +++++++++---------
> >  tools/perf/util/arm-spe.c                     |   2 +-
> >  2 files changed, 151 insertions(+), 153 deletions(-)
> > 
> > diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> > index 671a4763fb47..fbededc1bcd4 100644
> > --- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> > +++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> > @@ -9,6 +9,7 @@
> >  #include <endian.h>
> >  #include <byteswap.h>
> >  #include <linux/bitops.h>
> > +#include <stdarg.h>
> >  
> >  #include "arm-spe-pkt-decoder.h"
> >  
> > @@ -258,192 +259,189 @@ int arm_spe_get_packet(const unsigned char *buf, size_t len,
> >  	return ret;
> >  }
> >  
> > +static int arm_spe_pkt_out_string(int *err, char **buf_p, size_t *blen,
> > +				  const char *fmt, ...)
> > +{
> > +	va_list ap;
> > +	int ret;
> 
> 
> Ok, from a quick look this continues confusing, but at least its not
> named scnprintf() and then people won't expect the usual semantics.

Shame for the confusion :(

> Applying.

Thanks, Arnaldo.