by Leo Yan

[permalink] [raw]

Subject: Re: [PATCH v8 06/22] perf arm-spe: Refactor printing string to buffer

Hi David,

On Wed, Nov 11, 2020 at 11:03:47PM +0000, David Laight wrote:
> From: Dave Martin
> > Sent: 11 November 2020 17:58
> >
> > On Wed, Nov 11, 2020 at 05:39:22PM +0000, Arnaldo Carvalho de Melo wrote:
> > > Em Wed, Nov 11, 2020 at 03:45:23PM +0000, Andr� Przywara escreveu:
> > > > On 11/11/2020 15:35, Arnaldo Carvalho de Melo wrote:
> > > >
> > > > Hi Arnaldo,
> > > >
> > > > thanks for taking a look!
> > > >
> > > > > Em Wed, Nov 11, 2020 at 03:11:33PM +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_snprintf() which is used to wrap up the complex logics, and
> > > > >> it's used by the caller arm_spe_pkt_desc().
> > > > >>
> > > > >> This patch also moves the variable 'blen' as the function's local
> > > > >> variable, this allows to remove the unnecessary braces and improve the
> > > > >> readability.
> > > > >>
> > > > >> Suggested-by: Dave Martin <[email protected]>
> > > > >> Signed-off-by: Leo Yan <[email protected]>
> > > > >> Reviewed-by: Andre Przywara <[email protected]>
> > > > >> ---
> > > > >> .../arm-spe-decoder/arm-spe-pkt-decoder.c | 260 +++++++++---------
> > > > >> 1 file changed, 126 insertions(+), 134 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 04fd7fd7c15f..1970686f7020 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,183 @@ int arm_spe_get_packet(const unsigned char *buf, size_t len,
> > > > >> return ret;
> > > > >> }
> > > > >>
> > > > >> +static int arm_spe_pkt_snprintf(int *err, char **buf_p, size_t *blen,
> > > > >> + const char *fmt, ...)
> > > > >> +{
> > > > >> + va_list ap;
> > > > >> + int ret;
> > > > >> +
> > > > >> + /* Bail out if any error occurred */
> > > > >> + if (err && *err)
> > > > >> + return *err;
> > > > >> +
> > > > >> + va_start(ap, fmt);
> > > > >> + ret = vsnprintf(*buf_p, *blen, fmt, ap);
> > > > >> + va_end(ap);
> > > > >> +
> > > > >> + if (ret < 0) {
> > > > >> + if (err && !*err)
> > > > >> + *err = ret;
> > > > >> +
> > > > >> + /*
> > > > >> + * A return value of (*blen - 1) or more means that the
> > > > >> + * output was truncated and the buffer is overrun.
> > > > >> + */
> > > > >> + } else if (ret >= ((int)*blen - 1)) {
> > > > >> + (*buf_p)[*blen - 1] = '\0';
> > > > >> +
> > > > >> + /*
> > > > >> + * Set *err to 'ret' to avoid overflow if tries to
> > > > >> + * fill this buffer sequentially.
> > > > >> + */
> > > > >> + if (err && !*err)
> > > > >> + *err = ret;
> > > > >> + } else {
> > > > >> + *buf_p += ret;
> > > > >> + *blen -= ret;
> > > > >> + }
> > > > >> +
> > > > >> + return ret;
> > > > >> +}
> > > > >> +
>
> I'm not entirely sure that snprintf() can actually return a negative value.
>
> Every implementation (except the microsoft one) also always writes a '\0'
> even when the buffer is too short.
>
> A simple wrapper that lets you append output and detect overflow is:
> ret = vsnprintf(buf, len, ...);
> if (ret < 0)
> /* just in case */
> return 0;
> return ret > len ? len : ret;
>
> So on overflow the sum of the lengths is equal to the buffer size
> (ie includes the terminating '\0'.

We had some discussion for the return value in the old patch set, since
we want to keep the same scenmatics for the return value with
vsnprintf(), so the function arm_spe_pkt_snprintf() directly delivers
the return value from vsnprintf().

And if look at the patch 07/22, you could find the 'ret' value is not
really used at the end; the parameter 'err' is used as an accumulative
error value and it will be returned to up stack (0 means success and
non-zero means any failure).

For these two reasons, I'd like to keep as it is rather than converting
to other values.

Thanks for suggestion!
Leo