linux-kernel - Re: [PATCH v9 01/16] perf arm-spe: Refactor printing string to buffer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20201126143133.GB27722@leoy-ThinkPad-X240s>
Date:   Thu, 26 Nov 2020 22:31:33 +0800
From:   Leo Yan <leo.yan@...aro.org>
To:     Arnaldo Carvalho de Melo <acme@...nel.org>
Cc:     Andre Przywara <andre.przywara@....com>,
        Dave Martin <Dave.Martin@....com>,
        James Clark <james.clark@....com>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...hat.com>,
        Namhyung Kim <namhyung@...nel.org>,
        Al Grant <Al.Grant@....com>, Wei Li <liwei391@...wei.com>,
        John Garry <john.garry@...wei.com>,
        Will Deacon <will@...nel.org>,
        Mathieu Poirier <mathieu.poirier@...aro.org>,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v9 01/16] perf arm-spe: Refactor printing string to buffer

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@....com>
> > Signed-off-by: Leo Yan <leo.yan@...aro.org>
> > Reviewed-by: Andre Przywara <andre.przywara@....com>
> > Reviewed-by: Dave Martin <Dave.Martin@....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.