On Tue, Oct 27, 2020 at 03:09:02AM +0000, Leo Yan wrote:
> 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
> the caller arm_spe_pkt_desc() will call it and simply check the returns
> value.
>
> 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 <dave.mar...@arm.com>
> Signed-off-by: Leo Yan <leo....@linaro.org>
> Reviewed-by: Andre Przywara <andre.przyw...@arm.com>
> ---
> .../arm-spe-decoder/arm-spe-pkt-decoder.c | 247 ++++++++++--------
> 1 file changed, 135 insertions(+), 112 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..b400636e6da2 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,214 @@ int arm_spe_get_packet(const unsigned char *buf,
> size_t len,
> return ret;
> }
>
> +static int arm_spe_pkt_snprintf(char **buf_p, size_t *blen,
> + const char *fmt, ...)
> +{
> + va_list ap;
> + int ret;
> +
> + va_start(ap, fmt);
> + ret = vsnprintf(*buf_p, *blen, fmt, ap);
> + va_end(ap);
> +
> + if (ret < 0)
> + return ret;
> +
> + *buf_p += ret;
> + *blen -= ret;
> + return ret;
> +}
> +
This approach seems OK, though I wonder whether all the
"if (ret < 0) return;" logic is really needed.
In case of failure, it probably doesn't matter what ends up in buf.
If not, we could just implement a cumulative error:
static int arm_spe_pkt_snprintf(int *err, char **buf_p, size_t *blen,
{
/* ... */
if (ret < 0) {
if (err && !*err)
*err = ret;
} else {
*buf_p += ret;
*blen -= ret;
}
return err ? *err : ret;
}
and just return the final value of err.
> int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
> size_t buf_len)
> {
> int ret, ns, el, idx = packet->index;
> unsigned long long payload = packet->payload;
> const char *name = arm_spe_pkt_name(packet->type);
> + size_t blen = buf_len;
>
> switch (packet->type) {
> case ARM_SPE_BAD:
> case ARM_SPE_PAD:
> case ARM_SPE_END:
> - return snprintf(buf, buf_len, "%s", name);
> - case ARM_SPE_EVENTS: {
> - size_t blen = buf_len;
> -
> - ret = 0;
> - ret = snprintf(buf, buf_len, "EV");
> - buf += ret;
> - blen -= ret;
> + return arm_spe_pkt_snprintf(&buf, &blen, "%s", name);
> + case ARM_SPE_EVENTS:
> + ret = arm_spe_pkt_snprintf(&buf, &blen, "EV");
> + if (ret < 0)
> + return ret;
> +
...
Then this becomes
case ARM_SPE_END:
return arm_spe_pkt_snprintf(&err, &buf, &blen, "%s", name);
case ARM_SPE_EVENTS:
arm_spe_pkt_snprintf(&ret, &buf, &blen, "%s", name);
if (payload & 0x1)
arm_spe_pkt_snprintf(&err, &buf, &blen, "
EXCEPTION-GEN");
if (payload & 0x2)
arm_spe_pkt_snprintf(&err, &buf, &blen, " RETIRED");
/* ... */
This might be over-engineering, but it does help to condense the code.
[...]
Cheers
---Dave
