So there's a lot of new stuff since the first posting:
- Printbufs have been broken up into multiple patches that each add distinct
functionality - this is intended to make it easier to review and to see
what's used for what
- Printbufs now support both auto-heap allocated buffers, and external/static
buffers: this was required for the vsprintf.c refactoring, and means they're
(almost) a direct replacement for seq_buf
- The big thing: a new %pf(%p) format string extension for calling pretty
printers from printf (idea from Matthew Wilcox)
This is intended to replace most of our other format string extensions: e.g.
instead of writing
printf("%pg", bdev);
You'd now write
printf("%pf(%p)", bdev_name, bdev);
The advantage of this is that pretty printers no longer have to live in
lib/vsprintf.c, and they're much more discoverable - you can cscope to the
pretty printer!
And my hope is that this will help induce people to write lots more pretty
printers; since they can now live with the code they're printing and don't
require touching code in vsprintf.c, there's less friction to creating new
ones.
We hope to standardize this extension as %(%p), but since gcc's printf format
checking doesn't yet understand that we're going with %pf(%p) for now.
Currently, we only support pointer arguments to pretty-printers. I think we
can improve this in the future to support at least integer arguments as well,
i.e. "%(%u)" will eventually work. This will require using libffi to do it
correctly, but it looks like libffi is nearly suitable for in kernel use (it
supports all linux architectures, and configured with the features we want it
compiles down to practically nothing).
- Massive vsprintf.c refactoring
printbufs are now the core data structure used by vsprintf.c - we're not
passing around a bunch of raw char pointers anymore! yay!
This gets us a sane standard calling convention for pretty printers - i.e.,
we need this for %pf(%p).
Couple notes on the refactoring:
- printf_spec has become a dumping ground of state, passed everywhere and
used inconsistently. The refactoring attempts to improve this, and
centralize printf_spec handling as much as possible near/in the top level
code that handles format strings. Some %p extensions use
field/width/precision in nonstandard ways; the refactoring patches make it
clearer where this is going on.
- a _lot_ of pretty printers were allocating secondary buffers on the stack,
mainly to avoid ever writing past the terminating null in the output
buffer. There was a test that checked for this, but it had no documentation
where the requirement came from nor does that requirement make any sense,
so I deleted it (if anyone knows anything about it, speak up!). The code
now takes the approach of just writing to the output buffer and then
truncating afterwards if required by the precision argument.
Yay, less stack usage!
- format string parsing is still a mess: I'd like to consolidate that to only
happen in one place, but that's going to be a much more involved
refactoring - and if we just switch to new-style calling pretty printers
directly, we'll be able to just delete all that code.
- More seq_buf conversions
Using printbufs to clean up vsprintf.c meant adding a second, non
heap-allocated mode, so printbufs now do almost everything seq_buf does -
seq_buf has a read_pos member for some reason (tracing?) that I didn't get
into.
So now seq_buf is just used by the tracing code, and that can also probably
be converted to printbuf, but seq_buf is Steven's thing so I'll let him take
a look before getting into that.
Kent Overstreet (28):
lib/printbuf: New data structure for printing strings
vsprintf: Convert to printbuf
vsprintf: %pf(%p)
lib/string_helpers: string_get_size() now returns characters wrote
lib/printbuf: Heap allocation
lib/printbuf: Tabstops, indenting
lib/printbuf: Unit specifiers
lib/pretty-printers: pr_string_option(), pr_bitflags()
vsprintf: Improve number()
vsprintf: pr_u64_minwidth(), pr_u64()
vsprintf: Lift pr_hex_bytes() out from hex_string()
test_printf: Drop requirement that sprintf not write past nul
vsprintf: Start consolidating printf_spec handling
vsprintf: Refactor resource_string()
vsprintf: Refactor fourcc_string()
vsprintf: Refactor ip_addr_string()
vsprintf: Refactor mac_address_string()
vsprintf: time_and_date() no longer takes printf_spec
vsprintf: flags_string() no longer takes printf_spec
vsprintf: Refactor device_node_string, fwnode_string
vsprintf: Refactor hex_string, bitmap_string_list, bitmap_string
Input/joystick/analog: Convert from seq_buf -> printbuf
mm/memcontrol.c: Convert to printbuf
clk: tegra: bpmp: Convert to printbuf
tools/testing/nvdimm: Convert to printbuf
powerpc: Convert to printbuf
x86/resctrl: Convert to printbuf
PCI/P2PDMA: Convert to printbuf
Documentation/core-api/printk-formats.rst | 19 +
arch/powerpc/kernel/process.c | 16 +-
arch/powerpc/kernel/security.c | 75 +-
arch/powerpc/platforms/pseries/papr_scm.c | 34 +-
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 16 +-
drivers/clk/tegra/clk-bpmp.c | 21 +-
drivers/input/joystick/analog.c | 37 +-
drivers/pci/p2pdma.c | 17 +-
include/linux/kernel.h | 4 +
include/linux/pretty-printers.h | 11 +
include/linux/printbuf.h | 225 +++
include/linux/string_helpers.h | 8 +-
lib/Makefile | 2 +-
lib/pretty-printers.c | 81 ++
lib/printbuf.c | 252 ++++
lib/string_helpers.c | 18 +-
lib/test_printf.c | 23 +-
lib/vsprintf.c | 1612 ++++++++++-----------
mm/memcontrol.c | 68 +-
tools/testing/nvdimm/test/ndtest.c | 22 +-
20 files changed, 1527 insertions(+), 1034 deletions(-)
create mode 100644 include/linux/pretty-printers.h
create mode 100644 include/linux/printbuf.h
create mode 100644 lib/pretty-printers.c
create mode 100644 lib/printbuf.c
--
2.36.0
This implements two new format strings: both do the same thing, one more
compatible with current gcc format string checking, the other that we'd
like to standardize:
%p(%p) - more compatible
%(%p) - more prettier
Both can take variable numbers of arguments, i.e. %(%p,%p,%p).
They're used to indicate that snprintf or pr_buf should interpret the
next argument as a pretty-printer function to call, and subsequent
arguments within the parentheses should be passed to the pretty-printer.
A pretty printer takes as its first argument a printbuf, and then zero
or more pointer arguments - integer arguments are not (currently) supported.
Example usage:
static void foo_to_text(struct printbuf *out, struct foo *foo)
{
pr_buf(out, "bar=%u baz=%u", foo->bar, foo->baz);
}
printf("%(%p)", foo_to_text, foo);
The goal is to replace most of our %p format extensions with this
interface, and to move pretty-printers out of the core vsprintf.c code -
this will get us better organization and better discoverability (you'll
be able to cscope to pretty printer calls!), as well as eliminate a lot
of dispatch code in vsprintf.c.
Currently, we can only call pretty printers with pointer arguments. This
could be changed to also allow at least integer arguments in the future
by using libffi.
Signed-off-by: Kent Overstreet <[email protected]>
---
Documentation/core-api/printk-formats.rst | 19 ++++++
lib/test_printf.c | 17 +++++
lib/vsprintf.c | 81 ++++++++++++++++++++++-
3 files changed, 115 insertions(+), 2 deletions(-)
diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
index 5e89497ba3..e4ed40960e 100644
--- a/Documentation/core-api/printk-formats.rst
+++ b/Documentation/core-api/printk-formats.rst
@@ -625,6 +625,25 @@ Examples::
%p4cc Y10 little-endian (0x20303159)
%p4cc NV12 big-endian (0xb231564e)
+Calling a pretty printer function
+---------------------------------
+
+::
+
+ %p(%p) pretty printer function taking one argument
+ %p(%p,%p) pretty printer function taking two arguments
+
+For calling generic pretty printers. A pretty printer is a function that takes
+as its first argument a pointer to a printbuf, and then zero or more additional
+pointer arguments. For example:
+
+ void foo_to_text(struct printbuf *out, struct foo *foo)
+ {
+ pr_buf(out, "bar=%u baz=%u", foo->bar, foo->baz);
+ }
+
+ printf("%p(%p)", foo_to_text, foo);
+
Thanks
======
diff --git a/lib/test_printf.c b/lib/test_printf.c
index 07309c45f3..1e1604ef1a 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -783,6 +783,22 @@ test_pointer(void)
fourcc_pointer();
}
+static void printf_test_fn(struct printbuf *out, void *p)
+{
+ int *i = p;
+
+ pr_buf(out, "%i", *i);
+}
+
+static void __init
+test_fn(void)
+{
+ int i = 1;
+
+ test("1", "%pf(%p)", printf_test_fn, &i);
+ test("1", "%(%p)", printf_test_fn, &i);
+}
+
static void __init selftest(void)
{
alloced_buffer = kmalloc(BUF_SIZE + 2*PAD_SIZE, GFP_KERNEL);
@@ -794,6 +810,7 @@ static void __init selftest(void)
test_number();
test_string();
test_pointer();
+ test_fn();
kfree(alloced_buffer);
}
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 09b259e030..7fbeaf50d1 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -431,7 +431,8 @@ enum format_type {
FORMAT_TYPE_UINT,
FORMAT_TYPE_INT,
FORMAT_TYPE_SIZE_T,
- FORMAT_TYPE_PTRDIFF
+ FORMAT_TYPE_PTRDIFF,
+ FORMAT_TYPE_FN,
};
struct printf_spec {
@@ -2512,7 +2513,16 @@ int format_decode(const char *fmt, struct printf_spec *spec)
return ++fmt - start;
case 'p':
- spec->type = FORMAT_TYPE_PTR;
+ fmt++;
+ if (fmt[0] == 'f' &&
+ fmt[1] == '(') {
+ fmt += 2;
+ spec->type = FORMAT_TYPE_FN;
+ } else
+ spec->type = FORMAT_TYPE_PTR;
+ return fmt - start;
+ case '(':
+ spec->type = FORMAT_TYPE_FN;
return ++fmt - start;
case '%':
@@ -2594,6 +2604,49 @@ set_precision(struct printf_spec *spec, int prec)
}
}
+static void call_pr_fn(struct printbuf *out, void *fn, void **fn_args, unsigned nr_args)
+{
+ typedef void (*printf_fn_0)(struct printbuf *);
+ typedef void (*printf_fn_1)(struct printbuf *, void *);
+ typedef void (*printf_fn_2)(struct printbuf *, void *, void *);
+ typedef void (*printf_fn_3)(struct printbuf *, void *, void *, void *);
+ typedef void (*printf_fn_4)(struct printbuf *, void *, void *, void *, void *);
+ typedef void (*printf_fn_5)(struct printbuf *, void *, void *, void *, void *, void *);
+ typedef void (*printf_fn_6)(struct printbuf *, void *, void *, void *, void *, void *, void *);
+ typedef void (*printf_fn_7)(struct printbuf *, void *, void *, void *, void *, void *, void *, void *);
+ typedef void (*printf_fn_8)(struct printbuf *, void *, void *, void *, void *, void *, void *, void *, void *);
+
+ switch (nr_args) {
+ case 0:
+ ((printf_fn_0)fn)(out);
+ break;
+ case 1:
+ ((printf_fn_1)fn)(out, fn_args[0]);
+ break;
+ case 2:
+ ((printf_fn_2)fn)(out, fn_args[0], fn_args[1]);
+ break;
+ case 3:
+ ((printf_fn_3)fn)(out, fn_args[0], fn_args[1], fn_args[2]);
+ break;
+ case 4:
+ ((printf_fn_4)fn)(out, fn_args[0], fn_args[1], fn_args[2], fn_args[3]);
+ break;
+ case 5:
+ ((printf_fn_5)fn)(out, fn_args[0], fn_args[1], fn_args[2], fn_args[3], fn_args[4]);
+ break;
+ case 6:
+ ((printf_fn_6)fn)(out, fn_args[0], fn_args[1], fn_args[2], fn_args[3], fn_args[4], fn_args[5]);
+ break;
+ case 7:
+ ((printf_fn_7)fn)(out, fn_args[0], fn_args[1], fn_args[2], fn_args[3], fn_args[4], fn_args[5], fn_args[6]);
+ break;
+ case 8:
+ ((printf_fn_8)fn)(out, fn_args[0], fn_args[1], fn_args[2], fn_args[3], fn_args[4], fn_args[5], fn_args[6], fn_args[7]);
+ break;
+ }
+}
+
/**
* vpr_buf - Format a string, outputting to a printbuf
* @out: The printbuf to output to
@@ -2657,6 +2710,30 @@ void vpr_buf(struct printbuf *out, const char *fmt, va_list args)
fmt++;
break;
+ case FORMAT_TYPE_FN: {
+ unsigned nr_args = 0;
+ void *fn_args[8];
+ void *fn = va_arg(args, void *);
+
+ while (1) {
+ if (WARN_ON_ONCE(nr_args == ARRAY_SIZE(fn_args)))
+ goto out;
+ if (*fmt++ != '%')
+ goto out;
+ if (*fmt++ != 'p')
+ goto out;
+ fn_args[nr_args++] = va_arg(args, void *);
+ if (*fmt == ')')
+ break;
+ if (*fmt++ != ',')
+ goto out;
+ }
+
+ call_pr_fn(out, fn, fn_args, nr_args);
+ fmt++; /* past trailing ) */
+ break;
+ }
+
case FORMAT_TYPE_PERCENT_CHAR:
__pr_char(out, '%');
break;
--
2.36.0
We're attempting to consolidate printf_spec and format string handling
in the top level vpr_buf(), this changes time_and_date() to not
take printf_spec.
Signed-off-by: Kent Overstreet <[email protected]>
---
lib/vsprintf.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 76947e0d30..837a3f967e 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1735,14 +1735,14 @@ void time_str(struct printbuf *out, const struct rtc_time *tm, bool r)
static noinline_for_stack
void rtc_str(struct printbuf *out, const struct rtc_time *tm,
- struct printf_spec spec, const char *fmt)
+ const char *fmt)
{
bool have_t = true, have_d = true;
bool raw = false, iso8601_separator = true;
bool found = true;
int count = 2;
- if (check_pointer_spec(out, tm, spec))
+ if (check_pointer(out, tm))
return;
switch (fmt[count]) {
@@ -1780,7 +1780,7 @@ void rtc_str(struct printbuf *out, const struct rtc_time *tm,
static noinline_for_stack
void time64_str(struct printbuf *out, const time64_t time,
- struct printf_spec spec, const char *fmt)
+ const char *fmt)
{
struct rtc_time rtc_time;
struct tm tm;
@@ -1798,21 +1798,20 @@ void time64_str(struct printbuf *out, const time64_t time,
rtc_time.tm_isdst = 0;
- rtc_str(out, &rtc_time, spec, fmt);
+ rtc_str(out, &rtc_time, fmt);
}
static noinline_for_stack
-void time_and_date(struct printbuf *out,
- void *ptr, struct printf_spec spec,
+void time_and_date(struct printbuf *out, void *ptr,
const char *fmt)
{
switch (fmt[1]) {
case 'R':
- return rtc_str(out, (const struct rtc_time *)ptr, spec, fmt);
+ return rtc_str(out, (const struct rtc_time *)ptr, fmt);
case 'T':
- return time64_str(out, *(const time64_t *)ptr, spec, fmt);
+ return time64_str(out, *(const time64_t *)ptr, fmt);
default:
- return error_string_spec(out, "(%pt?)", spec);
+ return error_string(out, "(%pt?)");
}
}
@@ -2300,7 +2299,8 @@ void pointer(struct printbuf *out, const char *fmt,
dentry_name(out, ptr, fmt);
return do_width_precision(out, prev_pos, spec);
case 't':
- return time_and_date(out, ptr, spec, fmt);
+ time_and_date(out, ptr, fmt);
+ return do_width_precision(out, prev_pos, spec);
case 'C':
return clock(out, ptr, spec, fmt);
case 'D':
--
2.36.0
- We're attempting to consolidate printf_spec and format string
handling in the top level vpr_buf(), this changes ip_addr_string() to
not take printf_spec
- With the new printbuf helpers there's no need to use a separate stack
allocated buffer, so this patch deletes it.
Signed-off-by: Kent Overstreet <[email protected]>
---
lib/vsprintf.c | 116 ++++++++++++++++---------------------------------
1 file changed, 37 insertions(+), 79 deletions(-)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 3d17ddad31..62a221522c 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1273,13 +1273,13 @@ void mac_address_string(struct printbuf *out, u8 *addr,
}
static noinline_for_stack
-void ip4_string(struct printbuf *out,
- const u8 *addr, const char *fmt)
+void ip4_string(struct printbuf *out, const u8 *addr, const char *fmt)
{
- int i;
- bool leading_zeros = (fmt[0] == 'i');
- int index;
- int step;
+ struct printf_spec spec = default_dec_spec;
+ int i, index, step;
+
+ if (fmt[0] == 'i')
+ spec.precision = 3;
switch (fmt[2]) {
case 'h':
@@ -1303,19 +1303,9 @@ void ip4_string(struct printbuf *out,
break;
}
for (i = 0; i < 4; i++) {
- char temp[4] __aligned(2); /* hold each IP quad in reverse order */
- int digits = put_dec_trunc8(temp, addr[index]) - temp;
- if (leading_zeros) {
- if (digits < 3)
- __pr_char(out, '0');
- if (digits < 2)
- __pr_char(out, '0');
- }
- /* reverse the digits in the quad */
- while (digits--)
- __pr_char(out, temp[digits]);
- if (i < 3)
+ if (i)
__pr_char(out, '.');
+ number(out, addr[index], spec);
index += step;
}
}
@@ -1398,8 +1388,6 @@ void ip6_compressed_string(struct printbuf *out, const char *addr)
__pr_char(out, ':');
ip4_string(out, &in6.s6_addr[12], "I4");
}
-
- printbuf_nul_terminate(out);
}
static noinline_for_stack
@@ -1419,41 +1407,20 @@ void ip6_string(struct printbuf *out, const char *addr, const char *fmt)
static noinline_for_stack
void ip6_addr_string(struct printbuf *out, const u8 *addr,
- struct printf_spec spec, const char *fmt)
+ const char *fmt)
{
- char ip6_addr_buf[sizeof("xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:255.255.255.255")];
- struct printbuf ip6_addr = PRINTBUF_EXTERN(ip6_addr_buf, sizeof(ip6_addr_buf));
-
if (fmt[0] == 'I' && fmt[2] == 'c')
- ip6_compressed_string(&ip6_addr, addr);
+ ip6_compressed_string(out, addr);
else
- ip6_string(&ip6_addr, addr, fmt);
-
- string_nocheck(out, ip6_addr_buf, spec);
-}
-
-static noinline_for_stack
-void ip4_addr_string(struct printbuf *out, const u8 *addr,
- struct printf_spec spec, const char *fmt)
-{
- char ip4_addr_buf[sizeof("255.255.255.255")];
- struct printbuf ip4_addr = PRINTBUF_EXTERN(ip4_addr_buf, sizeof(ip4_addr_buf));
-
- ip4_string(&ip4_addr, addr, fmt);
-
- string_nocheck(out, ip4_addr_buf, spec);
+ ip6_string(out, addr, fmt);
}
static noinline_for_stack
void ip6_addr_string_sa(struct printbuf *out,
const struct sockaddr_in6 *sa,
- struct printf_spec spec, const char *fmt)
+ const char *fmt)
{
bool have_p = false, have_s = false, have_f = false, have_c = false;
- char ip6_addr_buf[sizeof("[xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:255.255.255.255]") +
- sizeof(":12345") + sizeof("/123456789") +
- sizeof("%1234567890")];
- struct printbuf ip6_addr = PRINTBUF_EXTERN(ip6_addr_buf, sizeof(ip6_addr_buf));
const u8 *addr = (const u8 *) &sa->sin6_addr;
char fmt6[2] = { fmt[0], '6' };
@@ -1476,42 +1443,35 @@ void ip6_addr_string_sa(struct printbuf *out,
}
if (have_p || have_s || have_f)
- __pr_char(&ip6_addr, '[');
+ __pr_char(out, '[');
if (fmt6[0] == 'I' && have_c)
- ip6_compressed_string(&ip6_addr, addr);
+ ip6_compressed_string(out, addr);
else
- ip6_string(&ip6_addr, addr, fmt6);
+ ip6_string(out, addr, fmt6);
if (have_p || have_s || have_f)
- __pr_char(&ip6_addr, ']');
+ __pr_char(out, ']');
if (have_p) {
- __pr_char(&ip6_addr, ':');
- number(&ip6_addr, ntohs(sa->sin6_port), spec);
+ __pr_char(out, ':');
+ pr_u64(out, ntohs(sa->sin6_port));
}
if (have_f) {
- __pr_char(&ip6_addr, '/');
- number(&ip6_addr, ntohl(sa->sin6_flowinfo &
- IPV6_FLOWINFO_MASK), spec);
+ __pr_char(out, '/');
+ pr_u64(out, ntohl(sa->sin6_flowinfo & IPV6_FLOWINFO_MASK));
}
if (have_s) {
- __pr_char(&ip6_addr, '%');
- number(&ip6_addr, sa->sin6_scope_id, spec);
+ __pr_char(out, '%');
+ pr_u64(out, sa->sin6_scope_id);
}
- printbuf_nul_terminate(&ip6_addr);
-
- string_nocheck(out, ip6_addr_buf, spec);
}
static noinline_for_stack
-void ip4_addr_string_sa(struct printbuf *out,
- const struct sockaddr_in *sa,
- struct printf_spec spec, const char *fmt)
+void ip4_addr_string_sa(struct printbuf *out, const struct sockaddr_in *sa,
+ const char *fmt)
{
bool have_p = false;
- char ip4_addr_buf[sizeof("255.255.255.255") + sizeof(":12345")];
- struct printbuf ip4_addr = PRINTBUF_EXTERN(ip4_addr_buf, sizeof(ip4_addr_buf));
const u8 *addr = (const u8 *) &sa->sin_addr.s_addr;
char fmt4[3] = { fmt[0], '4', 0 };
@@ -1530,30 +1490,27 @@ void ip4_addr_string_sa(struct printbuf *out,
}
}
- ip4_string(&ip4_addr, addr, fmt4);
+ ip4_string(out, addr, fmt4);
if (have_p) {
- __pr_char(&ip4_addr, ':');
- number(&ip4_addr, ntohs(sa->sin_port), spec);
+ __pr_char(out, ':');
+ pr_u64(out, ntohs(sa->sin_port));
}
- printbuf_nul_terminate(&ip4_addr);
-
- string_nocheck(out, ip4_addr_buf, spec);
}
static noinline_for_stack
void ip_addr_string(struct printbuf *out, const void *ptr,
- struct printf_spec spec, const char *fmt)
+ const char *fmt)
{
char *err_fmt_msg;
- if (check_pointer_spec(out, ptr, spec))
+ if (check_pointer(out, ptr))
return;
switch (fmt[1]) {
case '6':
- return ip6_addr_string(out, ptr, spec, fmt);
+ return ip6_addr_string(out, ptr, fmt);
case '4':
- return ip4_addr_string(out, ptr, spec, fmt);
+ return ip4_string(out, ptr, fmt);
case 'S': {
const union {
struct sockaddr raw;
@@ -1563,16 +1520,16 @@ void ip_addr_string(struct printbuf *out, const void *ptr,
switch (sa->raw.sa_family) {
case AF_INET:
- return ip4_addr_string_sa(out, &sa->v4, spec, fmt);
+ return ip4_addr_string_sa(out, &sa->v4, fmt);
case AF_INET6:
- return ip6_addr_string_sa(out, &sa->v6, spec, fmt);
+ return ip6_addr_string_sa(out, &sa->v6, fmt);
default:
- return error_string_spec(out, "(einval)", spec);
+ return error_string(out, "(einval)");
}}
}
err_fmt_msg = fmt[0] == 'i' ? "(%pi?)" : "(%pI?)";
- return error_string_spec(out, err_fmt_msg, spec);
+ error_string(out, err_fmt_msg);
}
static noinline_for_stack
@@ -2323,7 +2280,8 @@ void pointer(struct printbuf *out, const char *fmt,
* 4: 001.002.003.004
* 6: 000102...0f
*/
- return ip_addr_string(out, ptr, spec, fmt);
+ ip_addr_string(out, ptr, fmt);
+ return do_width_precision(out, prev_pos, spec);
case 'E':
return escaped_string(out, ptr, spec, fmt);
case 'U':
--
2.36.0
On Thu, May 26, 2022 at 04:44:20PM +0200, Petr Mladek wrote:
> On Thu 2022-05-19 13:23:53, Kent Overstreet wrote:
> > So there's a lot of new stuff since the first posting:
>
> > - Printbufs have been broken up into multiple patches that each add distinct
> > functionality - this is intended to make it easier to review and to see
> > what's used for what
>
> It is great that it is split. Also it is great to see all the ideas.
> But I would really prefer to somehow split it to make it easier
> for review and rebasing.
>
> I see the following "independent" parts:
>
> 1. Add simple API that allows to replace @len, @buf, @end in vsprintf.c
> by @printbuf. I agree that the code looks better and more safe.
>
> 2. Clean up of printf_spec. It would be great. But I do not like
> some parts. For example, si_units, human_readable_units
> are not property of the buffer. They are specific for a
> particular substring.
Not in conventional usage - these are properties that are set globally when
building up a string: consider an -h flag to a userspace utility.
>
> 3. New %p(%p) format. It really needs deep thinking. It is a
> ticket for potential big troubles. It is one patch that
> might be introduced and discussed anytime once we have
> the simple buffer API.
It's split out into a separate patch - discuss away!
> 3. Replace seq_buf. Steven Rostedt has to agree with it. Honestly,
> I do not see any improvement. The patches mostly do 1:1 replacement
> of one API with another.
It was necessary to avoid code duplication, which Christoph didn't like, and
seq_buf wasn't quite right for what I was trying to do and Steven didn't want to
change it. (read_pos is tracing specific and doesn't have anything to do with
printing, some of the semantics had to be tweaked to support snprintf, and the
name was wrong... :)
> 4. Heap allocated buffer. I am not sure if it is really needed.
> The patchset adds 3 users. IMHO, small static buffer would be
> perfectly fine for 2 of them. I personally do not like the error
> handling and the need to call exit.
>
> 5. All the fancy stuff (pr_tab(), pr_string_option()). The patchset
> does not add any user for them.
Heap allocated buffers and tabstops: these are things that I've been using in
bcachefs, and have quickly become necessities - I included them because I think
others will soon find them valuable (e.g. /proc has a lot of files that would be
more readable if formatted with tabstops).
pr_string_option(): this isn't anything really new, we've got a lot of pretty
printers and string helpers of this nature that need to be better organized and
given a common calling convention, I included this as code I've got that's been
useful and I think does it cleanly. I have more future plans for pretty printer
cleanup.
On Thu 2022-05-19 13:23:53, Kent Overstreet wrote:
> So there's a lot of new stuff since the first posting:
> - Printbufs have been broken up into multiple patches that each add distinct
> functionality - this is intended to make it easier to review and to see
> what's used for what
It is great that it is split. Also it is great to see all the ideas.
But I would really prefer to somehow split it to make it easier
for review and rebasing.
I see the following "independent" parts:
1. Add simple API that allows to replace @len, @buf, @end in vsprintf.c
by @printbuf. I agree that the code looks better and more safe.
2. Clean up of printf_spec. It would be great. But I do not like
some parts. For example, si_units, human_readable_units
are not property of the buffer. They are specific for a
particular substring.
3. New %p(%p) format. It really needs deep thinking. It is a
ticket for potential big troubles. It is one patch that
might be introduced and discussed anytime once we have
the simple buffer API.
3. Replace seq_buf. Steven Rostedt has to agree with it. Honestly,
I do not see any improvement. The patches mostly do 1:1 replacement
of one API with another.
4. Heap allocated buffer. I am not sure if it is really needed.
The patchset adds 3 users. IMHO, small static buffer would be
perfectly fine for 2 of them. I personally do not like the error
handling and the need to call exit.
5. All the fancy stuff (pr_tab(), pr_string_option()). The patchset
does not add any user for them.
I am going to comment the particular patches. It might take some
time. The patchset is really huge. It would really help to split it.
Best Regards,
Petr