| 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
| ||
|
Message-ID: <YTcyXmLpbL0BWbU+@alley>
Date: Tue, 7 Sep 2021 11:35:26 +0200
From: Petr Mladek <pmladek@...e.com>
To: Rasmus Villemoes <linux@...musvillemoes.dk>
Cc: Yury Norov <yury.norov@...il.com>,
Andrew Morton <akpm@...ux-foundation.org>,
Andy Shevchenko <andy.shevchenko@...il.com>,
Bartosz Golaszewski <bgolaszewski@...libre.com>,
Chris Down <chris@...isdown.name>,
Gilles Muller <Gilles.Muller@...ia.fr>,
Ingo Molnar <mingo@...nel.org>,
Jacob Keller <jacob.e.keller@...el.com>,
Joe Perches <joe@...ches.com>,
Julia Lawall <Julia.Lawall@...ia.fr>,
Michal Marek <michal.lkml@...kovi.net>,
Nicolas Palix <nicolas.palix@...g.fr>,
Peter Zijlstra <peterz@...radead.org>,
Sergey Senozhatsky <senozhatsky@...omium.org>,
Stephen Boyd <swboyd@...omium.org>,
Steven Rostedt <rostedt@...dmis.org>,
Thomas Gleixner <tglx@...utronix.de>,
linux-kernel@...r.kernel.org, cocci@...teme.lip6.fr
Subject: Re: [PATCH v2 1/2] lib: add sputchar() helper
On Mon 2021-09-06 13:51:59, Rasmus Villemoes wrote:
> On 05/09/2021 01.10, Yury Norov wrote:
> > There are 47 occurrences of the code snippet like this:
> > if (buf < end)
> > *buf = ' ';
> > ++buf;
> >
> > This patch adds a helper function sputchar() to replace opencoding.
> > It adds a lot to readability, and also saves 43 bytes of text on x86.
> >
> > v2: cleanup cases discovered with coccinelle script.
> >
> > Signed-off-by: Yury Norov <yury.norov@...il.com>
> > ---
> > include/linux/kernel.h | 8 ++
>
> Sorry, but 47 cases, mostly in one .c file, is not enough justification
> for adding yet another piece of random code to
> the-dumping-ground-which-is-kernel.h, especially since this helper is
> very specific to the needs of the vsnprintf() implementation, so
> extremely unlikely to find other users.
What about putting it into include/linux/string_helpers.h ?
Or create include/linux/vsprintf.h ?
> I'm also not a fan of the sputchar name - it's unreadable at first
> glance, and when you figure out it's "a _s_tring version of putchar",
> that doesn't help, because its semantics are nothing like the stdio putchar.
I do not like the name either.
What about vsprintf_putc(buf, end, c) or vsp_putc(buf, end, c)?
Well, the ugly thing are the three parameters. Which brings an idea of
struct vsprintf_buffer { // or struct vsp_buf
char *buf,
char *end,
}
and using vprintf_putc(vsp_buf, c) or vsp_putc(vsp_buf, c).
The change would look like:
>From 32119545392f560be42c7042721811a3177fb1dc Mon Sep 17 00:00:00 2001
From: Petr Mladek <pmladek@...e.com>
Date: Tue, 7 Sep 2021 11:31:22 +0200
Subject: [PATCH] vsprintf: Sample use of struct vsp_buf
This is just partial replacement of [buf,end] couple with
struct vsp_buf.
The intention is to see how the code would look like. It does
not compile.
---
lib/vsprintf.c | 85 +++++++++++++++++++++-----------------------------
1 file changed, 35 insertions(+), 50 deletions(-)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 3bcb7be03f93..963c9212141d 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -446,8 +446,9 @@ static_assert(sizeof(struct printf_spec) == 8);
#define PRECISION_MAX ((1 << 15) - 1)
static noinline_for_stack
-char *number(char *buf, char *end, unsigned long long num,
- struct printf_spec spec)
+strcut vsp_buf *number(struct vsp_buf *vsp_buf,
+ unsigned long long num,
+ struct printf_spec spec)
{
/* put_dec requires 2-byte alignment of the buffer. */
char tmp[3 * sizeof(num)] __aligned(2);
@@ -506,68 +507,52 @@ char *number(char *buf, char *end, unsigned long long num,
/* printing 100 using %2d gives "100", not "00" */
if (i > precision)
precision = i;
+
/* leading space padding */
field_width -= precision;
if (!(spec.flags & (ZEROPAD | LEFT))) {
- while (--field_width >= 0) {
- if (buf < end)
- *buf = ' ';
- ++buf;
- }
+ while (--field_width >= 0)
+ vsp_putc(vsp_buf, ' ');
}
+
/* sign */
- if (sign) {
- if (buf < end)
- *buf = sign;
- ++buf;
- }
+ if (sign)
+ vsp_putc(vsp_buf, sign);
+
/* "0x" / "0" prefix */
if (need_pfx) {
- if (spec.base == 16 || !is_zero) {
- if (buf < end)
- *buf = '0';
- ++buf;
- }
+ if (spec.base == 16 || !is_zero)
+ vsp_putc(vps_buf, '0');
if (spec.base == 16) {
- if (buf < end)
- *buf = ('X' | locase);
- ++buf;
- }
+ vsp_putc(vps_buf, 'X' | locase);
}
+
/* zero or space padding */
if (!(spec.flags & LEFT)) {
char c = ' ' + (spec.flags & ZEROPAD);
- while (--field_width >= 0) {
- if (buf < end)
- *buf = c;
- ++buf;
- }
+ while (--field_width >= 0)
+ vsp_putc(vps_buf, c);
}
+
/* hmm even more zero padding? */
- while (i <= --precision) {
- if (buf < end)
- *buf = '0';
- ++buf;
- }
+ while (i <= --precision)
+ vsp_putc(vps_buf, '0');
+
/* actual digits of result */
- while (--i >= 0) {
- if (buf < end)
- *buf = tmp[i];
- ++buf;
- }
+ while (--i >= 0)
+ vsp_putc(vps_buf, tmp[i]);
+
/* trailing space padding */
- while (--field_width >= 0) {
- if (buf < end)
- *buf = ' ';
- ++buf;
- }
+ while (--field_width >= 0)
+ vsp_putc(vps_buf, ' ');
return buf;
}
static noinline_for_stack
-char *special_hex_number(char *buf, char *end, unsigned long long num, int size)
+ struct vsp_buf* *special_hex_number(struct vsp_buf *vsp_buf,
+ unsigned long long num, int size)
{
struct printf_spec spec;
@@ -577,7 +562,7 @@ char *special_hex_number(char *buf, char *end, unsigned long long num, int size)
spec.base = 16;
spec.precision = -1;
- return number(buf, end, num, spec);
+ return number(vsp_buf, num, spec);
}
static void move_right(char *buf, char *end, unsigned len, unsigned spaces)
@@ -646,14 +631,14 @@ static char *string_nocheck(char *buf, char *end, const char *s,
return widen_string(buf, len, end, spec);
}
-static char *err_ptr(char *buf, char *end, void *ptr,
+static struct vsp_buf *err_ptr(struct vsp_buf *vsp_buf, void *ptr,
struct printf_spec spec)
{
int err = PTR_ERR(ptr);
const char *sym = errname(err);
if (sym)
- return string_nocheck(buf, end, sym, spec);
+ return string_nocheck(vsp_buf, sym, spec);
/*
* Somebody passed ERR_PTR(-1234) or some other non-existing
@@ -662,7 +647,7 @@ static char *err_ptr(char *buf, char *end, void *ptr,
*/
spec.flags |= SIGN;
spec.base = 10;
- return number(buf, end, err, spec);
+ return number(vsp_buf, err, spec);
}
/* Be careful: error messages must fit into the given buffer. */
@@ -720,9 +705,9 @@ char *string(char *buf, char *end, const char *s,
return string_nocheck(buf, end, s, spec);
}
-static char *pointer_string(char *buf, char *end,
- const void *ptr,
- struct printf_spec spec)
+static vsp_buf *pointer_string(struct vsp_buf *vsp_buf,
+ const void *ptr,
+ struct printf_spec spec)
{
spec.base = 16;
spec.flags |= SMALL;
@@ -731,7 +716,7 @@ static char *pointer_string(char *buf, char *end,
spec.flags |= ZEROPAD;
}
- return number(buf, end, (unsigned long int)ptr, spec);
+ return number(vsp_buf, (unsigned long int)ptr, spec);
}
/* Make pointers available for printing early in the boot sequence. */
--
2.26.2
Powered by blists - more mailing lists