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

[Leo Yan]

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