Printbufs, your new data structure for all your string-building and outputting
needs!
git repo: https://evilpiepirate.org/git/bcachefs.git/log/?h=printbuf_v3
Benefits:
- Replaces passing & returning raw char * pointers and lengths in a ton of
places, including especially vsprintf.c, with a much saner calling convention
- New helpers which greatly simplify and cleanup aforementioned vsprintf.c
- New standard calling convention/naming for pretty printers!
- New printf format string - %pf(%p) - for calling pretty printers by passing
them to sprintf instead of sticking them in vsprintf.c behind weird dispatch
code
- printbufs can auto-heap allocate! No need for statically sized buffers,
unless you want to do that
- Tabstops and indenting, for greatly improved formatting of multi-line output
...and probably more that I've forgotten to mention.
Changes since last patche:
New namespace prefixes:
-----------------------
We're not overloading pr_* anymore: any standard library code that outputs to a
printbuf should use the prt_ prefix. This is not for printbuf control code -
that uses the printbuf_ prefix; prt_ is just for things that print.
string_escape_mem():
--------------------
string_escape_mem() has now been properly converted to printbufs instead of just
adding a printbuf-style wrapper; the new printbuf helpers simplify that code
quite a bit.
hexdump:
--------
The hexdump code has been converted to printbuf and also reorganized and cleaned
up quite a bit, with better naming too. We now have
- prt_hex_bytes(), for printing a few hex bytes, with optional grouping
- prt_hex_line(), for printing a whole line of hex output with ascii characters
at the end
- prt_hex_dump(), for printing a whole multiline hex dump
Important behaviour change:
Previously, the hex dump code would _byte swap the output on little endian_.
Since this is not exactly standard behaviour for a hex dumper (binutils doesn't
do this), and is confusing as hell if you're trying to map byte offsets in
structs to your hex output, I've dropped it. Since we only use the hex dumper in
debug output, nothing should break, but to avoid confusion I've put this front
and center in the commit message for that patch.
tracing:
--------
This iteration of the patch series finally converts tracing to printbufs, which
is the last seq_buf user and that code is now also deleted. The tracing
conversion was pretty uneventful, not much to say here (except that it had its
own unique implementation of hex dumping with byte swabbing on little endian;
this is now replaced with just a call to prt_hex_bytes()).
Kent Overstreet (33):
lib/printbuf: New data structure for printing strings
lib/string_helpers: Convert string_escape_mem() to printbuf
vsprintf: Convert to printbuf
lib/hexdump: 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: prt_string_option(), prt_bitflags()
vsprintf: Improve number()
vsprintf: prt_u64_minwidth(), prt_u64()
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
tracing: trace_events_synth: Convert to printbuf
d_path: prt_path()
tracing: Convert to printbuf
Delete seq_buf
Documentation/core-api/printk-formats.rst | 22 +
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 | 23 +-
drivers/pci/p2pdma.c | 17 +-
fs/d_path.c | 34 +
include/linux/dcache.h | 1 +
include/linux/kernel.h | 12 +
include/linux/pretty-printers.h | 10 +
include/linux/printbuf.h | 245 +++
include/linux/seq_buf.h | 162 --
include/linux/string.h | 5 +
include/linux/string_helpers.h | 8 +-
include/linux/trace_events.h | 2 +-
include/linux/trace_seq.h | 14 +-
kernel/trace/trace.c | 45 +-
kernel/trace/trace_dynevent.c | 34 +-
kernel/trace/trace_events_filter.c | 2 +-
kernel/trace/trace_events_synth.c | 32 +-
kernel/trace/trace_functions_graph.c | 6 +-
kernel/trace/trace_kprobe.c | 2 +-
kernel/trace/trace_seq.c | 111 +-
lib/Makefile | 4 +-
lib/hexdump.c | 246 +--
lib/pretty-printers.c | 59 +
lib/printbuf.c | 252 +++
lib/seq_buf.c | 397 -----
lib/string_helpers.c | 141 +-
lib/test_hexdump.c | 30 +-
lib/test_printf.c | 26 +-
lib/vsprintf.c | 1716 ++++++++++-----------
mm/memcontrol.c | 68 +-
tools/testing/nvdimm/test/ndtest.c | 22 +-
36 files changed, 1943 insertions(+), 1967 deletions(-)
create mode 100644 include/linux/pretty-printers.h
create mode 100644 include/linux/printbuf.h
delete mode 100644 include/linux/seq_buf.h
create mode 100644 lib/pretty-printers.c
create mode 100644 lib/printbuf.c
delete mode 100644 lib/seq_buf.c
--
2.36.0
- eliminate on-stack buffer in device_node_string
- eliminate unnecessary uses of printf_spec, lift format string
precision/field width to pointer()
Signed-off-by: Kent Overstreet <[email protected]>
---
lib/vsprintf.c | 73 ++++++++++++++++++++------------------------------
1 file changed, 29 insertions(+), 44 deletions(-)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 1cd08d0d5b..a0a17a2aac 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1988,25 +1988,20 @@ void fwnode_full_name_string(struct printbuf *out,
static noinline_for_stack
void device_node_string(struct printbuf *out, struct device_node *dn,
- struct printf_spec spec, const char *fmt)
+ const char *fmt)
{
- char tbuf[sizeof("xxxx") + 1];
const char *p;
int ret;
- unsigned start = out->pos;
struct property *prop;
bool has_mult, pass;
- struct printf_spec str_spec = spec;
- str_spec.field_width = -1;
-
if (fmt[0] != 'F')
- return error_string_spec(out, "(%pO?)", spec);
+ return error_string(out, "(%pO?)");
if (!IS_ENABLED(CONFIG_OF))
- return error_string_spec(out, "(%pOF?)", spec);
+ return error_string(out, "(%pOF?)");
- if (check_pointer_spec(out, dn, spec))
+ if (check_pointer(out, dn))
return;
/* simple case without anything any more format specifiers */
@@ -2015,7 +2010,6 @@ void device_node_string(struct printbuf *out, struct device_node *dn,
fmt = "f";
for (pass = false; strspn(fmt,"fnpPFcC"); fmt++, pass = true) {
- int precision;
if (pass)
prt_char(out, ':');
@@ -2023,43 +2017,41 @@ void device_node_string(struct printbuf *out, struct device_node *dn,
case 'f': /* full_name */
fwnode_full_name_string(out, of_fwnode_handle(dn));
break;
- case 'n': /* name */
- p = fwnode_get_name(of_fwnode_handle(dn));
- precision = str_spec.precision;
- str_spec.precision = strchrnul(p, '@') - p;
- string_spec(out, p, str_spec);
- str_spec.precision = precision;
+ case 'n': { /* name */
+ const char *name = fwnode_get_name(of_fwnode_handle(dn));
+ unsigned len = strchrnul(name, '@') - name;
+
+ prt_bytes(out, name, len);
break;
+ }
case 'p': /* phandle */
- prt_u64(out, (unsigned int)dn->phandle);
+ prt_u64(out, dn->phandle);
break;
case 'P': /* path-spec */
p = fwnode_get_name(of_fwnode_handle(dn));
if (!p[1])
p = "/";
- string_spec(out, p, str_spec);
+ string(out, p);
break;
case 'F': /* flags */
- tbuf[0] = of_node_check_flag(dn, OF_DYNAMIC) ? 'D' : '-';
- tbuf[1] = of_node_check_flag(dn, OF_DETACHED) ? 'd' : '-';
- tbuf[2] = of_node_check_flag(dn, OF_POPULATED) ? 'P' : '-';
- tbuf[3] = of_node_check_flag(dn, OF_POPULATED_BUS) ? 'B' : '-';
- tbuf[4] = 0;
- string_nocheck(out, tbuf, str_spec);
+ prt_char(out, of_node_check_flag(dn, OF_DYNAMIC) ? 'D' : '-');
+ prt_char(out, of_node_check_flag(dn, OF_DETACHED) ? 'd' : '-');
+ prt_char(out, of_node_check_flag(dn, OF_POPULATED) ? 'P' : '-');
+ prt_char(out, of_node_check_flag(dn, OF_POPULATED_BUS) ? 'B' : '-');
break;
case 'c': /* major compatible string_spec */
ret = of_property_read_string(dn, "compatible", &p);
if (!ret)
- string_spec(out, p, str_spec);
+ string(out, p);
break;
case 'C': /* full compatible string_spec */
has_mult = false;
of_property_for_each_string(dn, "compatible", prop, p) {
if (has_mult)
- string_nocheck(out, ",", str_spec);
- string_nocheck(out, "\"", str_spec);
- string_spec(out, p, str_spec);
- string_nocheck(out, "\"", str_spec);
+ prt_char(out, ',');
+ prt_char(out, '\"');
+ string(out, p);
+ prt_char(out, '\"');
has_mult = true;
}
@@ -2068,39 +2060,30 @@ void device_node_string(struct printbuf *out, struct device_node *dn,
break;
}
}
-
- widen_string(out, out->pos - start, spec);
}
static noinline_for_stack
void fwnode_string(struct printbuf *out,
struct fwnode_handle *fwnode,
- struct printf_spec spec, const char *fmt)
+ const char *fmt)
{
- struct printf_spec str_spec = spec;
- unsigned start = out->pos;
-
- str_spec.field_width = -1;
-
if (*fmt != 'w')
- return error_string_spec(out, "(%pf?)", spec);
+ return error_string(out, "(%pf?)");
- if (check_pointer_spec(out, fwnode, spec))
+ if (check_pointer(out, fwnode))
return;
fmt++;
switch (*fmt) {
case 'P': /* name */
- string_spec(out, fwnode_get_name(fwnode), str_spec);
+ string(out, fwnode_get_name(fwnode));
break;
case 'f': /* full_name */
default:
fwnode_full_name_string(out, fwnode);
break;
}
-
- widen_string(out, out->pos - start, spec);
}
int __init no_hash_pointers_enable(char *str)
@@ -2338,9 +2321,11 @@ void pointer(struct printbuf *out, const char *fmt,
flags_string(out, ptr, fmt);
return do_width_precision(out, prev_pos, spec);
case 'O':
- return device_node_string(out, ptr, spec, fmt + 1);
+ device_node_string(out, ptr, fmt + 1);
+ return do_width_precision(out, prev_pos, spec);
case 'f':
- return fwnode_string(out, ptr, spec, fmt + 1);
+ fwnode_string(out, ptr, fmt + 1);
+ return do_width_precision(out, prev_pos, spec);
case 'x':
return pointer_string(out, ptr, spec);
case 'e':
--
2.36.0
Like the upcoming vsprintf.c conversion, this converts string_escape_mem
to prt_escaped_string(), which uses and outputs to a printbuf, and makes
string_escape_mem() a smaller wrapper to support existing users.
The new printbuf helpers greatly simplify the code.
Signed-off-by: Kent Overstreet <[email protected]>
---
include/linux/string_helpers.h | 4 +
lib/string_helpers.c | 134 +++++++++++----------------------
2 files changed, 49 insertions(+), 89 deletions(-)
diff --git a/include/linux/string_helpers.h b/include/linux/string_helpers.h
index 4d72258d42..67de398944 100644
--- a/include/linux/string_helpers.h
+++ b/include/linux/string_helpers.h
@@ -10,6 +10,7 @@
struct device;
struct file;
struct task_struct;
+struct printbuf;
/* Descriptions of the types of units to
* print in */
@@ -62,6 +63,8 @@ static inline int string_unescape_any_inplace(char *buf)
#define ESCAPE_ALL_MASK GENMASK(8, 0)
+void prt_escaped_string(struct printbuf *out, const char *src, size_t isz,
+ unsigned int flags, const char *only);
int string_escape_mem(const char *src, size_t isz, char *dst, size_t osz,
unsigned int flags, const char *only);
@@ -71,6 +74,7 @@ static inline int string_escape_mem_any_np(const char *src, size_t isz,
return string_escape_mem(src, isz, dst, osz, ESCAPE_ANY_NP, only);
}
+
static inline int string_escape_str(const char *src, char *dst, size_t sz,
unsigned int flags, const char *only)
{
diff --git a/lib/string_helpers.c b/lib/string_helpers.c
index 4f877e9551..3b1118337e 100644
--- a/lib/string_helpers.c
+++ b/lib/string_helpers.c
@@ -15,6 +15,7 @@
#include <linux/fs.h>
#include <linux/limits.h>
#include <linux/mm.h>
+#include <linux/printbuf.h>
#include <linux/slab.h>
#include <linux/string.h>
#include <linux/string_helpers.h>
@@ -301,19 +302,14 @@ int string_unescape(char *src, char *dst, size_t size, unsigned int flags)
}
EXPORT_SYMBOL(string_unescape);
-static bool escape_passthrough(unsigned char c, char **dst, char *end)
+static bool escape_passthrough(struct printbuf *out, unsigned char c)
{
- char *out = *dst;
-
- if (out < end)
- *out = c;
- *dst = out + 1;
+ prt_char(out, c);
return true;
}
-static bool escape_space(unsigned char c, char **dst, char *end)
+static bool escape_space(struct printbuf *out, unsigned char c)
{
- char *out = *dst;
unsigned char to;
switch (c) {
@@ -336,20 +332,13 @@ static bool escape_space(unsigned char c, char **dst, char *end)
return false;
}
- if (out < end)
- *out = '\\';
- ++out;
- if (out < end)
- *out = to;
- ++out;
-
- *dst = out;
+ prt_char(out, '\\');
+ prt_char(out, to);
return true;
}
-static bool escape_special(unsigned char c, char **dst, char *end)
+static bool escape_special(struct printbuf *out, unsigned char c)
{
- char *out = *dst;
unsigned char to;
switch (c) {
@@ -369,83 +358,43 @@ static bool escape_special(unsigned char c, char **dst, char *end)
return false;
}
- if (out < end)
- *out = '\\';
- ++out;
- if (out < end)
- *out = to;
- ++out;
-
- *dst = out;
+ prt_char(out, '\\');
+ prt_char(out, to);
return true;
}
-static bool escape_null(unsigned char c, char **dst, char *end)
+static bool escape_null(struct printbuf *out, unsigned char c)
{
- char *out = *dst;
-
if (c)
return false;
- if (out < end)
- *out = '\\';
- ++out;
- if (out < end)
- *out = '0';
- ++out;
-
- *dst = out;
+ prt_char(out, '\\');
+ prt_char(out, '0');
return true;
}
-static bool escape_octal(unsigned char c, char **dst, char *end)
+static bool escape_octal(struct printbuf *out, unsigned char c)
{
- char *out = *dst;
-
- if (out < end)
- *out = '\\';
- ++out;
- if (out < end)
- *out = ((c >> 6) & 0x07) + '0';
- ++out;
- if (out < end)
- *out = ((c >> 3) & 0x07) + '0';
- ++out;
- if (out < end)
- *out = ((c >> 0) & 0x07) + '0';
- ++out;
-
- *dst = out;
+ prt_char(out, '\\');
+ prt_char(out, ((c >> 6) & 0x07) + '0');
+ prt_char(out, ((c >> 3) & 0x07) + '0');
+ prt_char(out, ((c >> 0) & 0x07) + '0');
return true;
}
-static bool escape_hex(unsigned char c, char **dst, char *end)
+static bool escape_hex(struct printbuf *out, unsigned char c)
{
- char *out = *dst;
-
- if (out < end)
- *out = '\\';
- ++out;
- if (out < end)
- *out = 'x';
- ++out;
- if (out < end)
- *out = hex_asc_hi(c);
- ++out;
- if (out < end)
- *out = hex_asc_lo(c);
- ++out;
-
- *dst = out;
+ prt_char(out, '\\');
+ prt_char(out, 'x');
+ prt_hex_byte(out, c);
return true;
}
/**
- * string_escape_mem - quote characters in the given memory buffer
+ * prt_escaped_string - quote characters in the given memory buffer
+ * @out: printbuf to output to (escaped)
* @src: source buffer (unescaped)
* @isz: source buffer size
- * @dst: destination buffer (escaped)
- * @osz: destination buffer size
* @flags: combination of the flags
* @only: NULL-terminated string containing characters used to limit
* the selected escape class. If characters are included in @only
@@ -517,11 +466,10 @@ static bool escape_hex(unsigned char c, char **dst, char *end)
* truncated, compare the return value to osz. There is room left in
* dst for a '\0' terminator if and only if ret < osz.
*/
-int string_escape_mem(const char *src, size_t isz, char *dst, size_t osz,
- unsigned int flags, const char *only)
+void prt_escaped_string(struct printbuf *out,
+ const char *src, size_t isz,
+ unsigned int flags, const char *only)
{
- char *p = dst;
- char *end = p + osz;
bool is_dict = only && *only;
bool is_append = flags & ESCAPE_APPEND;
@@ -549,41 +497,49 @@ int string_escape_mem(const char *src, size_t isz, char *dst, size_t osz,
* %ESCAPE_NA cases.
*/
if (!(is_append || in_dict) && is_dict &&
- escape_passthrough(c, &p, end))
+ escape_passthrough(out, c))
continue;
if (!(is_append && in_dict) && isascii(c) && isprint(c) &&
- flags & ESCAPE_NAP && escape_passthrough(c, &p, end))
+ flags & ESCAPE_NAP && escape_passthrough(out, c))
continue;
if (!(is_append && in_dict) && isprint(c) &&
- flags & ESCAPE_NP && escape_passthrough(c, &p, end))
+ flags & ESCAPE_NP && escape_passthrough(out, c))
continue;
if (!(is_append && in_dict) && isascii(c) &&
- flags & ESCAPE_NA && escape_passthrough(c, &p, end))
+ flags & ESCAPE_NA && escape_passthrough(out, c))
continue;
- if (flags & ESCAPE_SPACE && escape_space(c, &p, end))
+ if (flags & ESCAPE_SPACE && escape_space(out, c))
continue;
- if (flags & ESCAPE_SPECIAL && escape_special(c, &p, end))
+ if (flags & ESCAPE_SPECIAL && escape_special(out, c))
continue;
- if (flags & ESCAPE_NULL && escape_null(c, &p, end))
+ if (flags & ESCAPE_NULL && escape_null(out, c))
continue;
/* ESCAPE_OCTAL and ESCAPE_HEX always go last */
- if (flags & ESCAPE_OCTAL && escape_octal(c, &p, end))
+ if (flags & ESCAPE_OCTAL && escape_octal(out, c))
continue;
- if (flags & ESCAPE_HEX && escape_hex(c, &p, end))
+ if (flags & ESCAPE_HEX && escape_hex(out, c))
continue;
- escape_passthrough(c, &p, end);
+ escape_passthrough(out, c);
}
+}
+EXPORT_SYMBOL(prt_escaped_string);
+
+int string_escape_mem(const char *src, size_t isz, char *dst, size_t osz,
+ unsigned int flags, const char *only)
+{
+ struct printbuf out = PRINTBUF_EXTERN(dst, osz);
- return p - dst;
+ prt_escaped_string(&out, src, isz, flags, only);
+ return out.pos;
}
EXPORT_SYMBOL(string_escape_mem);
--
2.36.0
Two changes:
- We're attempting to consolidate printf_spec and format string
handling in the top level vpr_buf(), this changes resource_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 | 51 ++++++++++++++++++++++++--------------------------
1 file changed, 24 insertions(+), 27 deletions(-)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 863040fdaa..4256b1d3a5 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1057,7 +1057,7 @@ static const struct printf_spec default_dec_spec = {
static noinline_for_stack
void resource_string(struct printbuf *out, struct resource *res,
- struct printf_spec spec, const char *fmt)
+ int decode)
{
#ifndef IO_RSRC_PRINTK_SIZE
#define IO_RSRC_PRINTK_SIZE 6
@@ -1096,62 +1096,58 @@ void resource_string(struct printbuf *out, struct resource *res,
#define FLAG_BUF_SIZE (2 * sizeof(res->flags))
#define DECODED_BUF_SIZE sizeof("[mem - 64bit pref window disabled]")
#define RAW_BUF_SIZE sizeof("[mem - flags 0x]")
- char sym_buf[max(2*RSRC_BUF_SIZE + DECODED_BUF_SIZE,
- 2*RSRC_BUF_SIZE + FLAG_BUF_SIZE + RAW_BUF_SIZE)];
- struct printbuf sym = PRINTBUF_EXTERN(sym_buf, sizeof(sym_buf));
- int decode = (fmt[0] == 'R') ? 1 : 0;
const struct printf_spec *specp;
- if (check_pointer_spec(out, res, spec))
+ if (check_pointer(out, res))
return;
- prt_char(&sym, '[');
+ prt_char(out, '[');
if (res->flags & IORESOURCE_IO) {
- string_nocheck(&sym, "io ", str_spec);
+ string_nocheck(out, "io ", str_spec);
specp = &io_spec;
} else if (res->flags & IORESOURCE_MEM) {
- string_nocheck(&sym, "mem ", str_spec);
+ string_nocheck(out, "mem ", str_spec);
specp = &mem_spec;
} else if (res->flags & IORESOURCE_IRQ) {
- string_nocheck(&sym, "irq ", str_spec);
+ string_nocheck(out, "irq ", str_spec);
specp = &default_dec_spec;
} else if (res->flags & IORESOURCE_DMA) {
- string_nocheck(&sym, "dma ", str_spec);
+ string_nocheck(out, "dma ", str_spec);
specp = &default_dec_spec;
} else if (res->flags & IORESOURCE_BUS) {
- string_nocheck(&sym, "bus ", str_spec);
+ string_nocheck(out, "bus ", str_spec);
specp = &bus_spec;
} else {
- string_nocheck(&sym, "??? ", str_spec);
+ string_nocheck(out, "??? ", str_spec);
specp = &mem_spec;
decode = 0;
}
if (decode && res->flags & IORESOURCE_UNSET) {
- string_nocheck(&sym, "size ", str_spec);
- number(&sym, resource_size(res), *specp);
+ string_nocheck(out, "size ", str_spec);
+ number(out, resource_size(res), *specp);
} else {
- number(&sym, res->start, *specp);
+ number(out, res->start, *specp);
if (res->start != res->end) {
- prt_char(&sym, '-');
- number(&sym, res->end, *specp);
+ prt_char(out, '-');
+ number(out, res->end, *specp);
}
}
if (decode) {
if (res->flags & IORESOURCE_MEM_64)
- string_nocheck(&sym, " 64bit", str_spec);
+ string_nocheck(out, " 64bit", str_spec);
if (res->flags & IORESOURCE_PREFETCH)
- string_nocheck(&sym, " pref", str_spec);
+ string_nocheck(out, " pref", str_spec);
if (res->flags & IORESOURCE_WINDOW)
- string_nocheck(&sym, " window", str_spec);
+ string_nocheck(out, " window", str_spec);
if (res->flags & IORESOURCE_DISABLED)
- string_nocheck(&sym, " disabled", str_spec);
+ string_nocheck(out, " disabled", str_spec);
} else {
- string_nocheck(&sym, " flags ", str_spec);
- number(&sym, res->flags, default_flag_spec);
+ string_nocheck(out, " flags ", str_spec);
+ number(out, res->flags, default_flag_spec);
}
- prt_char(&sym, ']');
+ prt_char(out, ']');
- string_nocheck(out, sym_buf, spec);
+ printbuf_nul_terminate(out);
}
static noinline_for_stack
@@ -2326,7 +2322,8 @@ void pointer(struct printbuf *out, const char *fmt,
return do_width_precision(out, prev_pos, spec);
case 'R':
case 'r':
- return resource_string(out, ptr, spec, fmt);
+ resource_string(out, ptr, fmt[0] == 'R');
+ return do_width_precision(out, prev_pos, spec);
case 'h':
return hex_string(out, ptr, spec, fmt);
case 'b':
--
2.36.0
This adds printbufs: a printbuf points to a char * buffer and knows the
size of the output buffer as well as the current output position.
Future patches will be adding more features to printbuf, but initially
printbufs are targeted at refactoring and improving our existing code in
lib/vsprintf.c - so this initial printbuf patch has the features
required for that.
Signed-off-by: Kent Overstreet <[email protected]>
Reviewed-by: Matthew Wilcox (Oracle) <[email protected]>
---
include/linux/printbuf.h | 118 +++++++++++++++++++++++++++++++++++++++
1 file changed, 118 insertions(+)
create mode 100644 include/linux/printbuf.h
diff --git a/include/linux/printbuf.h b/include/linux/printbuf.h
new file mode 100644
index 0000000000..8b3797dc4b
--- /dev/null
+++ b/include/linux/printbuf.h
@@ -0,0 +1,118 @@
+/* SPDX-License-Identifier: LGPL-2.1+ */
+/* Copyright (C) 2022 Kent Overstreet */
+
+#ifndef _LINUX_PRINTBUF_H
+#define _LINUX_PRINTBUF_H
+
+#include <linux/string.h>
+
+/*
+ * Printbufs: String buffer for outputting (printing) to, for vsnprintf
+ */
+
+struct printbuf {
+ char *buf;
+ unsigned size;
+ unsigned pos;
+};
+
+/*
+ * Returns size remaining of output buffer:
+ */
+static inline unsigned printbuf_remaining_size(struct printbuf *out)
+{
+ return out->pos < out->size ? out->size - out->pos : 0;
+}
+
+/*
+ * Returns number of characters we can print to the output buffer - i.e.
+ * excluding the terminating nul:
+ */
+static inline unsigned printbuf_remaining(struct printbuf *out)
+{
+ return out->pos < out->size ? out->size - out->pos - 1 : 0;
+}
+
+static inline unsigned printbuf_written(struct printbuf *out)
+{
+ return min(out->pos, out->size);
+}
+
+/*
+ * Returns true if output was truncated:
+ */
+static inline bool printbuf_overflowed(struct printbuf *out)
+{
+ return out->pos >= out->size;
+}
+
+static inline void printbuf_nul_terminate(struct printbuf *out)
+{
+ if (out->pos < out->size)
+ out->buf[out->pos] = 0;
+ else if (out->size)
+ out->buf[out->size - 1] = 0;
+}
+
+static inline void __prt_chars(struct printbuf *out, char c, unsigned n)
+{
+ memset(out->buf + out->pos,
+ c,
+ min(n, printbuf_remaining(out)));
+ out->pos += n;
+}
+
+static inline void prt_chars(struct printbuf *out, char c, unsigned n)
+{
+ __prt_chars(out, c, n);
+ printbuf_nul_terminate(out);
+}
+
+static inline void __prt_char(struct printbuf *out, char c)
+{
+ if (printbuf_remaining(out))
+ out->buf[out->pos] = c;
+ out->pos++;
+}
+
+static inline void prt_char(struct printbuf *out, char c)
+{
+ __prt_char(out, c);
+ printbuf_nul_terminate(out);
+}
+
+static inline void prt_bytes(struct printbuf *out, const void *b, unsigned n)
+{
+ memcpy(out->buf + out->pos,
+ b,
+ min(n, printbuf_remaining(out)));
+ out->pos += n;
+ printbuf_nul_terminate(out);
+}
+
+static inline void prt_str(struct printbuf *out, const char *str)
+{
+ prt_bytes(out, str, strlen(str));
+}
+
+static inline void prt_hex_byte(struct printbuf *out, u8 byte)
+{
+ __prt_char(out, hex_asc_hi(byte));
+ __prt_char(out, hex_asc_lo(byte));
+ printbuf_nul_terminate(out);
+}
+
+static inline void prt_hex_byte_upper(struct printbuf *out, u8 byte)
+{
+ __prt_char(out, hex_asc_upper_hi(byte));
+ __prt_char(out, hex_asc_upper_lo(byte));
+ printbuf_nul_terminate(out);
+}
+
+#define PRINTBUF_EXTERN(_buf, _size) \
+((struct printbuf) { \
+ .buf = _buf, \
+ .size = _size, \
+})
+
+#endif /* _LINUX_PRINTBUF_H */
--
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:
%pf(%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]>
Reviewed-by: Matthew Wilcox (Oracle) <[email protected]>
---
Documentation/core-api/printk-formats.rst | 22 ++++++
lib/test_printf.c | 20 ++++++
lib/vsprintf.c | 81 ++++++++++++++++++++++-
3 files changed, 121 insertions(+), 2 deletions(-)
diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
index 5e89497ba3..8fc0b62af1 100644
--- a/Documentation/core-api/printk-formats.rst
+++ b/Documentation/core-api/printk-formats.rst
@@ -625,6 +625,28 @@ Examples::
%p4cc Y10 little-endian (0x20303159)
%p4cc NV12 big-endian (0xb231564e)
+Calling a pretty printer function
+---------------------------------
+
+::
+
+ %pf(%p) pretty printer function taking one argument
+ %pf(%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("%pf(%p)", foo_to_text, foo);
+
+Note that a pretty-printer may not sleep, if called from printk(). If called
+from pr_buf() or sprintf() there are no such restrictions.
+
Thanks
======
diff --git a/lib/test_printf.c b/lib/test_printf.c
index 07309c45f3..ff833870f5 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -783,6 +783,25 @@ test_pointer(void)
fourcc_pointer();
}
+static void printf_test_fn(struct printbuf *out, void *p)
+{
+ int *i = p;
+
+ prt_printf(out, "%i", *i);
+}
+
+static void __init
+test_fn(void)
+{
+ int i = 1;
+
+ test("1", "%pf(%p)", printf_test_fn, &i);
+ /*
+ * Not tested, so we don't fail the build with -Werror:
+ */
+ //test("1", "%(%p)", printf_test_fn, &i);
+}
+
static void __init selftest(void)
{
alloced_buffer = kmalloc(BUF_SIZE + 2*PAD_SIZE, GFP_KERNEL);
@@ -794,6 +813,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 b686dafb2f..118d6f65c3 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -436,7 +436,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 {
@@ -2519,7 +2520,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 '%':
@@ -2601,6 +2611,49 @@ set_precision(struct printf_spec *spec, int prec)
}
}
+static void call_prt_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;
+ }
+}
+
/**
* prt_vprintf - Format a string, outputting to a printbuf
* @out: The printbuf to output to
@@ -2664,6 +2717,30 @@ void prt_vprintf(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_prt_fn(out, fn, fn_args, nr_args);
+ fmt++; /* past trailing ) */
+ break;
+ }
+
case FORMAT_TYPE_PERCENT_CHAR:
__prt_char(out, '%');
break;
--
2.36.0
This converts from seq_buf to printbuf. Here we're using printbuf with
an external buffer, meaning it's a direct conversion.
Signed-off-by: Kent Overstreet <[email protected]>
Cc: Dan Williams <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: [email protected]
---
tools/testing/nvdimm/test/ndtest.c | 22 ++++++++++------------
1 file changed, 10 insertions(+), 12 deletions(-)
diff --git a/tools/testing/nvdimm/test/ndtest.c b/tools/testing/nvdimm/test/ndtest.c
index 4d1a947367..a2097955da 100644
--- a/tools/testing/nvdimm/test/ndtest.c
+++ b/tools/testing/nvdimm/test/ndtest.c
@@ -12,7 +12,7 @@
#include <linux/ndctl.h>
#include <nd-core.h>
#include <linux/printk.h>
-#include <linux/seq_buf.h>
+#include <linux/printbuf.h>
#include "../watermark.h"
#include "nfit_test.h"
@@ -740,32 +740,30 @@ static ssize_t flags_show(struct device *dev,
{
struct nvdimm *nvdimm = to_nvdimm(dev);
struct ndtest_dimm *dimm = nvdimm_provider_data(nvdimm);
- struct seq_buf s;
+ struct printbuf s = PRINTBUF_EXTERN(buf, PAGE_SIZE);
u64 flags;
flags = dimm->flags;
- seq_buf_init(&s, buf, PAGE_SIZE);
if (flags & PAPR_PMEM_UNARMED_MASK)
- seq_buf_printf(&s, "not_armed ");
+ prt_printf(&s, "not_armed ");
if (flags & PAPR_PMEM_BAD_SHUTDOWN_MASK)
- seq_buf_printf(&s, "flush_fail ");
+ prt_printf(&s, "flush_fail ");
if (flags & PAPR_PMEM_BAD_RESTORE_MASK)
- seq_buf_printf(&s, "restore_fail ");
+ prt_printf(&s, "restore_fail ");
if (flags & PAPR_PMEM_SAVE_MASK)
- seq_buf_printf(&s, "save_fail ");
+ prt_printf(&s, "save_fail ");
if (flags & PAPR_PMEM_SMART_EVENT_MASK)
- seq_buf_printf(&s, "smart_notify ");
+ prt_printf(&s, "smart_notify ");
+ if (printbuf_written(&s))
+ prt_printf(&s, "\n");
- if (seq_buf_used(&s))
- seq_buf_printf(&s, "\n");
-
- return seq_buf_used(&s);
+ return printbuf_written(&s);
}
static DEVICE_ATTR_RO(flags);
--
2.36.0
The current test code checks that sprintf never writes past the
terminating nul. This is a rather strange requirement, completely
separate from writing past the end of the buffer, which of course we
can't do: writing anywhere to the buffer passed to snprintf, within size
of course, should be perfectly fine.
Since this check has no documentation as to where it comes from or what
depends on it, and it's getting in the way of further refactoring
(printf_spec handling is right now scattered massively throughout the
code, and we'd like to consolidate it) - delete it.
Also, many current pretty-printers building up their output on the
stack, and then copy it to the actual output buffer - by eliminating
this requirement we can kill those extra buffers.
Signed-off-by: Kent Overstreet <[email protected]>
---
lib/test_printf.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/lib/test_printf.c b/lib/test_printf.c
index ff833870f5..a702dd5dc0 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -78,12 +78,6 @@ do_test(int bufsize, const char *expect, int elen,
return 1;
}
- if (memchr_inv(test_buffer + written + 1, FILL_CHAR, BUF_SIZE + PAD_SIZE - (written + 1))) {
- pr_warn("vsnprintf(buf, %d, \"%s\", ...) wrote beyond the nul-terminator\n",
- bufsize, fmt);
- return 1;
- }
-
if (memcmp(test_buffer, expect, written)) {
pr_warn("vsnprintf(buf, %d, \"%s\", ...) wrote '%s', expected '%.*s'\n",
bufsize, fmt, test_buffer, written, expect);
--
2.36.0
- We're attempting to consolidate printf_spec and format string
handling in the top level ptr_vprintf(), this changes
mac_address_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 | 18 +++++++-----------
1 file changed, 7 insertions(+), 11 deletions(-)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 964e00b6a2..6020f55fc0 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1261,15 +1261,13 @@ void bitmap_list_string(struct printbuf *out, unsigned long *bitmap,
static noinline_for_stack
void mac_address_string(struct printbuf *out, u8 *addr,
- struct printf_spec spec, const char *fmt)
+ const char *fmt)
{
- char mac_addr[sizeof("xx:xx:xx:xx:xx:xx")];
- char *p = mac_addr;
int i;
char separator;
bool reversed = false;
- if (check_pointer_spec(out, addr, spec))
+ if (check_pointer(out, addr))
return;
switch (fmt[1]) {
@@ -1288,16 +1286,13 @@ void mac_address_string(struct printbuf *out, u8 *addr,
for (i = 0; i < 6; i++) {
if (reversed)
- p = hex_byte_pack(p, addr[5 - i]);
+ prt_hex_byte(out, addr[5 - i]);
else
- p = hex_byte_pack(p, addr[i]);
+ prt_hex_byte(out, addr[i]);
if (fmt[0] == 'M' && i != 5)
- *p++ = separator;
+ prt_char(out, separator);
}
- *p = '\0';
-
- string_nocheck(out, mac_addr, spec);
}
static noinline_for_stack
@@ -2292,7 +2287,8 @@ void pointer(struct printbuf *out, const char *fmt,
case 'm': /* Contiguous: 000102030405 */
/* [mM]F (FDDI) */
/* [mM]R (Reverse order; Bluetooth) */
- return mac_address_string(out, ptr, spec, fmt);
+ mac_address_string(out, ptr, fmt);
+ return do_width_precision(out, prev_pos, spec);
case 'I': /* Formatted IP supported
* 4: 1.2.3.4
* 6: 0001:0203:...:0708
--
2.36.0
seq_buf is being deprecated, this converts to printbuf.
Signed-off-by: Kent Overstreet <[email protected]>
---
drivers/input/joystick/analog.c | 23 ++++++++++-------------
1 file changed, 10 insertions(+), 13 deletions(-)
diff --git a/drivers/input/joystick/analog.c b/drivers/input/joystick/analog.c
index 3088c5b829..a8c5f90e82 100644
--- a/drivers/input/joystick/analog.c
+++ b/drivers/input/joystick/analog.c
@@ -19,7 +19,7 @@
#include <linux/input.h>
#include <linux/gameport.h>
#include <linux/jiffies.h>
-#include <linux/seq_buf.h>
+#include <linux/printbuf.h>
#include <linux/timex.h>
#include <linux/timekeeping.h>
@@ -339,24 +339,21 @@ static void analog_calibrate_timer(struct analog_port *port)
static void analog_name(struct analog *analog)
{
- struct seq_buf s;
+ struct printbuf buf = PRINTBUF_EXTERN(analog->name, sizeof(analog->name));
- seq_buf_init(&s, analog->name, sizeof(analog->name));
- seq_buf_printf(&s, "Analog %d-axis %d-button",
- hweight8(analog->mask & ANALOG_AXES_STD),
- hweight8(analog->mask & ANALOG_BTNS_STD) + !!(analog->mask & ANALOG_BTNS_CHF) * 2 +
- hweight16(analog->mask & ANALOG_BTNS_GAMEPAD) + !!(analog->mask & ANALOG_HBTN_CHF) * 4);
+ prt_printf(&buf, "Analog %d-axis %d-button",
+ hweight8(analog->mask & ANALOG_AXES_STD),
+ hweight8(analog->mask & ANALOG_BTNS_STD) + !!(analog->mask & ANALOG_BTNS_CHF) * 2 +
+ hweight16(analog->mask & ANALOG_BTNS_GAMEPAD) + !!(analog->mask & ANALOG_HBTN_CHF) * 4);
if (analog->mask & ANALOG_HATS_ALL)
- seq_buf_printf(&s, " %d-hat",
- hweight16(analog->mask & ANALOG_HATS_ALL));
-
+ prt_printf(&buf, " %d-hat", hweight16(analog->mask & ANALOG_HATS_ALL));
if (analog->mask & ANALOG_HAT_FCS)
- seq_buf_printf(&s, " FCS");
+ prt_printf(&buf, " FCS");
if (analog->mask & ANALOG_ANY_CHF)
- seq_buf_printf(&s, (analog->mask & ANALOG_SAITEK) ? " Saitek" : " CHF");
+ prt_printf(&buf, (analog->mask & ANALOG_SAITEK) ? " Saitek" : " CHF");
- seq_buf_printf(&s, (analog->mask & ANALOG_GAMEPAD) ? " gamepad" : " joystick");
+ prt_printf(&buf, (analog->mask & ANALOG_GAMEPAD) ? " gamepad" : " joystick");
}
/*
--
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 6020f55fc0..5de25723a3 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1761,14 +1761,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]) {
@@ -1806,7 +1806,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;
@@ -1824,21 +1824,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?)");
}
}
@@ -2322,7 +2321,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
On Sat, 4 Jun 2022 15:30:09 -0400
Kent Overstreet <[email protected]> wrote:
> Printbufs, your new data structure for all your string-building and outputting
> needs!
>
> git repo: https://evilpiepirate.org/git/bcachefs.git/log/?h=printbuf_v3
Hi Kent,
Just wanted to let you know that I want to review this and I'm not
ignoring it. But there's a still quite a bit in my queue that I must
review before I get to this, so it may still be a while. :-/
-- Steve
This patch refactors number() to make it a bit clearer, and it also
changes it to call printbuf_make_room() only once at the start, instead
of in the printbuf output helpers.
Signed-off-by: Kent Overstreet <[email protected]>
---
lib/vsprintf.c | 82 +++++++++++++++++++++++++-------------------------
1 file changed, 41 insertions(+), 41 deletions(-)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 118d6f65c3..1c5ffc7f28 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -458,92 +458,92 @@ void number(struct printbuf *out, unsigned long long num,
{
/* put_dec requires 2-byte alignment of the buffer. */
char tmp[3 * sizeof(num)] __aligned(2);
- char sign;
- char locase;
+ char sign = 0;
+ /* locase = 0 or 0x20. ORing digits or letters with 'locase'
+ * produces same digits or (maybe lowercased) letters */
+ char locase = (spec.flags & SMALL);
int need_pfx = ((spec.flags & SPECIAL) && spec.base != 10);
- int i;
bool is_zero = num == 0LL;
int field_width = spec.field_width;
int precision = spec.precision;
+ int nr_digits = 0;
+ int output_bytes = 0;
- /* locase = 0 or 0x20. ORing digits or letters with 'locase'
- * produces same digits or (maybe lowercased) letters */
- locase = (spec.flags & SMALL);
if (spec.flags & LEFT)
spec.flags &= ~ZEROPAD;
- sign = 0;
if (spec.flags & SIGN) {
if ((signed long long)num < 0) {
sign = '-';
num = -(signed long long)num;
- field_width--;
+ output_bytes++;
} else if (spec.flags & PLUS) {
sign = '+';
- field_width--;
+ output_bytes++;
} else if (spec.flags & SPACE) {
sign = ' ';
- field_width--;
+ output_bytes++;
}
}
if (need_pfx) {
if (spec.base == 16)
- field_width -= 2;
+ output_bytes += 2;
else if (!is_zero)
- field_width--;
+ output_bytes++;
}
/* generate full string in tmp[], in reverse order */
- i = 0;
- if (num < spec.base)
- tmp[i++] = hex_asc_upper[num] | locase;
- else if (spec.base != 10) { /* 8 or 16 */
+ if (spec.base == 10) {
+ nr_digits = put_dec(tmp, num) - tmp;
+ } else { /* 8 or 16 */
int mask = spec.base - 1;
- int shift = 3;
+ int shift = ilog2((unsigned) spec.base);
- if (spec.base == 16)
- shift = 4;
do {
- tmp[i++] = (hex_asc_upper[((unsigned char)num) & mask] | locase);
+ tmp[nr_digits++] = (hex_asc_upper[((unsigned char)num) & mask] | locase);
num >>= shift;
} while (num);
- } else { /* base 10 */
- i = put_dec(tmp, num) - tmp;
}
/* printing 100 using %2d gives "100", not "00" */
- if (i > precision)
- precision = i;
+ precision = max(nr_digits, precision);
+ output_bytes += precision;
+ field_width = max(0, field_width - output_bytes);
+
+ printbuf_make_room(out, field_width + output_bytes);
+
/* leading space padding */
- field_width = max(0, field_width - precision);
if (!(spec.flags & (ZEROPAD | LEFT))) {
- __prt_chars(out, ' ', field_width);
+ __prt_chars_reserved(out, ' ', field_width);
field_width = 0;
}
+
/* sign */
if (sign)
- __prt_char(out, sign);
+ __prt_char_reserved(out, sign);
+
/* "0x" / "0" prefix */
if (need_pfx) {
if (spec.base == 16 || !is_zero)
- __prt_char(out, '0');
+ __prt_char_reserved(out, '0');
if (spec.base == 16)
- __prt_char(out, 'X' | locase);
+ __prt_char_reserved(out, 'X' | locase);
}
- /* zero or space padding */
- if (!(spec.flags & LEFT)) {
- char c = ' ' + (spec.flags & ZEROPAD);
- __prt_chars(out, c, field_width);
- field_width = 0;
- }
- /* hmm even more zero padding? */
- if (precision > i)
- __prt_chars(out, '0', precision - i);
+ /* zero padding */
+ if (!(spec.flags & LEFT))
+ __prt_chars_reserved(out, '0', field_width);
+
+ /* zero padding from precision */
+ if (precision > nr_digits)
+ __prt_chars_reserved(out, '0', precision - nr_digits);
+
/* actual digits of result */
- while (--i >= 0)
- __prt_char(out, tmp[i]);
+ while (--nr_digits >= 0)
+ __prt_char_reserved(out, tmp[nr_digits]);
+
/* trailing space padding */
- __prt_chars(out, ' ', field_width);
+ if (spec.flags & LEFT)
+ __prt_chars_reserved(out, ' ', field_width);
printbuf_nul_terminate(out);
}
--
2.36.0
printf_spec is right now something of a mess - it's a grab-bag of state
that's interpreted inconsistently by different code, and it's scattered
throughout vsprintf.c.
We'd like to get it out of the pretty-printers, and have it be solely
the responsibility of vsprintf()/vpr_buf(), the code that parses and
handles format strings.
Most of the code that uses printf_spec is only using it for a minimum &
maximum field width - that can be done at the toplevel by checking how
much we just printed, and padding or truncating it as necessary. This
patch takes those "simple" uses of printf_spec and moves them as far up
the call stack as possible.
This patch also renames some helpers and creates new ones that don't
take printf_spec:
- do_width_precision: new helper that handles with/precision of
printf_spec
- error_string -> error_string_spec
- check_pointer -> check_pointer_spec
- string -> string_spec
Next patches will be reducing/eliminating uses of the *_spec versions.
Signed-off-by: Kent Overstreet <[email protected]>
---
lib/vsprintf.c | 245 ++++++++++++++++++++++++++++---------------------
1 file changed, 138 insertions(+), 107 deletions(-)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 7b49a998d3..863040fdaa 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -617,6 +617,19 @@ void widen_string(struct printbuf *out, int n,
prt_chars(out, ' ', spaces);
}
+static void do_width_precision(struct printbuf *out, unsigned prev_pos,
+ struct printf_spec spec)
+{
+ unsigned n = out->pos - prev_pos;
+
+ if (n > spec.precision) {
+ out->pos -= n - spec.precision;
+ n = spec.precision;
+ }
+
+ widen_string(out, n, spec);
+}
+
/* Handle string from a well known address. */
static void string_nocheck(struct printbuf *out,
const char *s,
@@ -649,7 +662,7 @@ static void err_ptr(struct printbuf *out, void *ptr,
}
/* Be careful: error messages must fit into the given buffer. */
-static void error_string(struct printbuf *out, const char *s,
+static void error_string_spec(struct printbuf *out, const char *s,
struct printf_spec spec)
{
/*
@@ -679,7 +692,7 @@ static const char *check_pointer_msg(const void *ptr)
return NULL;
}
-static int check_pointer(struct printbuf *out,
+static int check_pointer_spec(struct printbuf *out,
const void *ptr,
struct printf_spec spec)
{
@@ -687,7 +700,7 @@ static int check_pointer(struct printbuf *out,
err_msg = check_pointer_msg(ptr);
if (err_msg) {
- error_string(out, err_msg, spec);
+ error_string_spec(out, err_msg, spec);
return -EFAULT;
}
@@ -695,16 +708,47 @@ static int check_pointer(struct printbuf *out,
}
static noinline_for_stack
-void string(struct printbuf *out,
+void string_spec(struct printbuf *out,
const char *s,
struct printf_spec spec)
{
- if (check_pointer(out, s, spec))
+ if (check_pointer_spec(out, s, spec))
return;
string_nocheck(out, s, spec);
}
+static void error_string(struct printbuf *out, const char *s)
+{
+ /*
+ * Hard limit to avoid a completely insane messages. It actually
+ * works pretty well because most error messages are in
+ * the many pointer format modifiers.
+ */
+ prt_bytes(out, s, min(strlen(s), 2 * sizeof(void *)));
+}
+
+static int check_pointer(struct printbuf *out, const void *ptr)
+{
+ const char *err_msg;
+
+ err_msg = check_pointer_msg(ptr);
+ if (err_msg) {
+ error_string(out, err_msg);
+ return -EFAULT;
+ }
+
+ return 0;
+}
+
+static void string(struct printbuf *out, const char *s)
+{
+ if (check_pointer(out, s))
+ return;
+
+ prt_str(out, s);
+}
+
static void pointer_string(struct printbuf *out,
const void *ptr,
struct printf_spec spec)
@@ -830,7 +874,7 @@ static void ptr_to_id(struct printbuf *out,
if (ret) {
spec.field_width = 2 * sizeof(ptr);
/* string length must be less than default_width */
- return error_string(out, str, spec);
+ return error_string_spec(out, str, spec);
}
pointer_string(out, (const void *)hashval, spec);
@@ -871,7 +915,7 @@ void restricted_pointer(struct printbuf *out,
if (in_irq() || in_serving_softirq() || in_nmi()) {
if (spec.field_width == -1)
spec.field_width = 2 * sizeof(ptr);
- return error_string(out, "pK-error", spec);
+ return error_string_spec(out, "pK-error", spec);
}
/*
@@ -901,14 +945,12 @@ void restricted_pointer(struct printbuf *out,
}
static noinline_for_stack
-void dentry_name(struct printbuf *out,
- const struct dentry *d, struct printf_spec spec,
+void dentry_name(struct printbuf *out, const struct dentry *d,
const char *fmt)
{
- const char *array[4], *s;
+ const char *array[4];
const struct dentry *p;
- int depth;
- int i, n;
+ int i, depth;
switch (fmt[1]) {
case '2': case '3': case '4':
@@ -920,7 +962,7 @@ void dentry_name(struct printbuf *out,
rcu_read_lock();
for (i = 0; i < depth; i++, d = p) {
- if (check_pointer(out, d, spec)) {
+ if (check_pointer(out, d)) {
rcu_read_unlock();
return;
}
@@ -934,56 +976,46 @@ void dentry_name(struct printbuf *out,
break;
}
}
- s = array[--i];
- for (n = 0; n != spec.precision; n++) {
- char c = *s++;
- if (!c) {
- if (!i)
- break;
- c = '/';
- s = array[--i];
- }
- prt_char(out, c);
+ while (1) {
+ prt_str(out, array[--i]);
+ if (!i)
+ break;
+ prt_char(out, '/');
}
rcu_read_unlock();
-
- widen_string(out, n, spec);
}
static noinline_for_stack
-void file_dentry_name(struct printbuf *out,
- const struct file *f,
- struct printf_spec spec, const char *fmt)
+void file_dentry_name(struct printbuf *out, const struct file *f,
+ const char *fmt)
{
- if (check_pointer(out, f, spec))
+ if (check_pointer(out, f))
return;
- return dentry_name(out, f->f_path.dentry, spec, fmt);
+ return dentry_name(out, f->f_path.dentry, fmt);
}
#ifdef CONFIG_BLOCK
static noinline_for_stack
-void bdev_name(struct printbuf *out,
- struct block_device *bdev,
- struct printf_spec spec, const char *fmt)
+void bdev_name(struct printbuf *out, struct block_device *bdev)
{
struct gendisk *hd;
- if (check_pointer(out, bdev, spec))
+ if (check_pointer(out, bdev))
return;
hd = bdev->bd_disk;
- string(out, hd->disk_name, spec);
+ string(out, hd->disk_name);
if (bdev->bd_partno) {
if (isdigit(hd->disk_name[strlen(hd->disk_name)-1]))
prt_char(out, 'p');
- number(out, bdev->bd_partno, spec);
+ prt_u64(out, bdev->bd_partno);
}
}
#endif
static noinline_for_stack
void symbol_string(struct printbuf *out, void *ptr,
- struct printf_spec spec, const char *fmt)
+ const char *fmt)
{
unsigned long value;
#ifdef CONFIG_KALLSYMS
@@ -1006,17 +1038,12 @@ void symbol_string(struct printbuf *out, void *ptr,
else
sprint_symbol_no_offset(sym, value);
- string_nocheck(out, sym, spec);
+ prt_str(out, sym);
#else
special_hex_number(out, value, sizeof(void *));
#endif
}
-static const struct printf_spec default_str_spec = {
- .field_width = -1,
- .precision = -1,
-};
-
static const struct printf_spec default_flag_spec = {
.base = 16,
.precision = -1,
@@ -1075,7 +1102,7 @@ void resource_string(struct printbuf *out, struct resource *res,
int decode = (fmt[0] == 'R') ? 1 : 0;
const struct printf_spec *specp;
- if (check_pointer(out, res, spec))
+ if (check_pointer_spec(out, res, spec))
return;
prt_char(&sym, '[');
@@ -1139,7 +1166,7 @@ void hex_string(struct printbuf *out, u8 *addr,
/* nothing to print */
return;
- if (check_pointer(out, addr, spec))
+ if (check_pointer_spec(out, addr, spec))
return;
switch (fmt[1]) {
@@ -1180,7 +1207,7 @@ void bitmap_string(struct printbuf *out, unsigned long *bitmap,
int i, chunksz;
bool first = true;
- if (check_pointer(out, bitmap, spec))
+ if (check_pointer_spec(out, bitmap, spec))
return;
/* reused to print numbers */
@@ -1219,7 +1246,7 @@ void bitmap_list_string(struct printbuf *out, unsigned long *bitmap,
bool first = true;
int rbot, rtop;
- if (check_pointer(out, bitmap, spec))
+ if (check_pointer_spec(out, bitmap, spec))
return ;
for_each_set_bitrange(rbot, rtop, bitmap, nr_bits) {
@@ -1246,7 +1273,7 @@ void mac_address_string(struct printbuf *out, u8 *addr,
char separator;
bool reversed = false;
- if (check_pointer(out, addr, spec))
+ if (check_pointer_spec(out, addr, spec))
return;
switch (fmt[1]) {
@@ -1547,7 +1574,7 @@ void ip_addr_string(struct printbuf *out, const void *ptr,
{
char *err_fmt_msg;
- if (check_pointer(out, ptr, spec))
+ if (check_pointer_spec(out, ptr, spec))
return;
switch (fmt[1]) {
@@ -1568,12 +1595,12 @@ void ip_addr_string(struct printbuf *out, const void *ptr,
case AF_INET6:
return ip6_addr_string_sa(out, &sa->v6, spec, fmt);
default:
- return error_string(out, "(einval)", spec);
+ return error_string_spec(out, "(einval)", spec);
}}
}
err_fmt_msg = fmt[0] == 'i' ? "(%pi?)" : "(%pI?)";
- return error_string(out, err_fmt_msg, spec);
+ return error_string_spec(out, err_fmt_msg, spec);
}
static noinline_for_stack
@@ -1588,7 +1615,7 @@ void escaped_string(struct printbuf *out, u8 *addr,
if (spec.field_width == 0)
return; /* nothing to print */
- if (check_pointer(out, addr, spec))
+ if (check_pointer_spec(out, addr, spec))
return;
do {
@@ -1633,7 +1660,7 @@ static void va_format(struct printbuf *out,
{
va_list va;
- if (check_pointer(out, va_fmt, spec))
+ if (check_pointer_spec(out, va_fmt, spec))
return;
va_copy(va, *va_fmt->va);
@@ -1642,16 +1669,13 @@ static void va_format(struct printbuf *out,
}
static noinline_for_stack
-void uuid_string(struct printbuf *out, const u8 *addr,
- struct printf_spec spec, const char *fmt)
+void uuid_string(struct printbuf *out, const u8 *addr, const char *fmt)
{
- char uuid_buf[UUID_STRING_LEN + 1];
- struct printbuf uuid = PRINTBUF_EXTERN(uuid_buf, sizeof(uuid_buf));
int i;
const u8 *index = uuid_index;
bool uc = false;
- if (check_pointer(out, addr, spec))
+ if (check_pointer(out, addr))
return;
switch (*(++fmt)) {
@@ -1668,30 +1692,28 @@ void uuid_string(struct printbuf *out, const u8 *addr,
for (i = 0; i < 16; i++) {
if (uc)
- prt_hex_byte_upper(&uuid, addr[index[i]]);
+ prt_hex_byte_upper(out, addr[index[i]]);
else
- prt_hex_byte(&uuid, addr[index[i]]);
+ prt_hex_byte(out, addr[index[i]]);
switch (i) {
case 3:
case 5:
case 7:
case 9:
- prt_char(&uuid, '-');
+ prt_char(out, '-');
break;
}
}
-
- string_nocheck(out, uuid_buf, spec);
}
static noinline_for_stack
void netdev_bits(struct printbuf *out, const void *addr,
- struct printf_spec spec, const char *fmt)
+ const char *fmt)
{
unsigned long long num;
int size;
- if (check_pointer(out, addr, spec))
+ if (check_pointer(out, addr))
return;
switch (fmt[1]) {
@@ -1701,7 +1723,7 @@ void netdev_bits(struct printbuf *out, const void *addr,
special_hex_number(out, num, size);
break;
default:
- error_string(out, "(%pN?)", spec);
+ error_string(out, "(%pN?)");
break;
}
}
@@ -1716,9 +1738,9 @@ void fourcc_string(struct printbuf *out, const u32 *fourcc,
u32 orig, val;
if (fmt[1] != 'c' || fmt[2] != 'c')
- return error_string(out, "(%p4?)", spec);
+ return error_string_spec(out, "(%p4?)", spec);
- if (check_pointer(out, fourcc, spec))
+ if (check_pointer_spec(out, fourcc, spec))
return;
orig = get_unaligned(fourcc);
@@ -1739,17 +1761,17 @@ void fourcc_string(struct printbuf *out, const u32 *fourcc,
special_hex_number(&output, orig, sizeof(u32));
prt_char(&output, ')');
- string(out, output_buf, spec);
+ string_spec(out, output_buf, spec);
}
static noinline_for_stack
void address_val(struct printbuf *out, const void *addr,
- struct printf_spec spec, const char *fmt)
+ const char *fmt)
{
unsigned long long num;
int size;
- if (check_pointer(out, addr, spec))
+ if (check_pointer(out, addr))
return;
switch (fmt[1]) {
@@ -1800,7 +1822,7 @@ void rtc_str(struct printbuf *out, const struct rtc_time *tm,
bool found = true;
int count = 2;
- if (check_pointer(out, tm, spec))
+ if (check_pointer_spec(out, tm, spec))
return;
switch (fmt[count]) {
@@ -1870,7 +1892,7 @@ void time_and_date(struct printbuf *out,
case 'T':
return time64_str(out, *(const time64_t *)ptr, spec, fmt);
default:
- return error_string(out, "(%pt?)", spec);
+ return error_string_spec(out, "(%pt?)", spec);
}
}
@@ -1879,16 +1901,16 @@ void clock(struct printbuf *out, struct clk *clk,
struct printf_spec spec, const char *fmt)
{
if (!IS_ENABLED(CONFIG_HAVE_CLK))
- return error_string(out, "(%pC?)", spec);
+ return error_string_spec(out, "(%pC?)", spec);
- if (check_pointer(out, clk, spec))
+ if (check_pointer_spec(out, clk, spec))
return;
switch (fmt[1]) {
case 'n':
default:
#ifdef CONFIG_COMMON_CLK
- return string(out, __clk_get_name(clk), spec);
+ return string_spec(out, __clk_get_name(clk), spec);
#else
return ptr_to_id(out, clk, spec);
#endif
@@ -1906,7 +1928,7 @@ void format_flags(struct printbuf *out, unsigned long flags,
if ((flags & mask) != mask)
continue;
- string(out, names->name, default_str_spec);
+ string(out, names->name);
flags &= ~mask;
if (flags)
@@ -1964,7 +1986,7 @@ void format_page_flags(struct printbuf *out, unsigned long flags)
if (append)
prt_char(out, '|');
- string(out, pff[i].name, default_str_spec);
+ string(out, pff[i].name);
prt_char(out, '=');
number(out, (flags >> pff[i].shift) & pff[i].mask, *pff[i].spec);
@@ -1980,7 +2002,7 @@ void flags_string(struct printbuf *out, void *flags_ptr,
unsigned long flags;
const struct trace_print_flags *names;
- if (check_pointer(out, flags_ptr, spec))
+ if (check_pointer_spec(out, flags_ptr, spec))
return;
switch (fmt[1]) {
@@ -1995,7 +2017,7 @@ void flags_string(struct printbuf *out, void *flags_ptr,
names = gfpflag_names;
break;
default:
- return error_string(out, "(%pG?)", spec);
+ return error_string_spec(out, "(%pG?)", spec);
}
return format_flags(out, flags, names);
@@ -2012,10 +2034,8 @@ void fwnode_full_name_string(struct printbuf *out,
struct fwnode_handle *__fwnode =
fwnode_get_nth_parent(fwnode, depth);
- string(out, fwnode_get_name_prefix(__fwnode),
- default_str_spec);
- string(out, fwnode_get_name(__fwnode),
- default_str_spec);
+ string(out, fwnode_get_name_prefix(__fwnode));
+ string(out, fwnode_get_name(__fwnode));
fwnode_handle_put(__fwnode);
}
@@ -2036,12 +2056,12 @@ void device_node_string(struct printbuf *out, struct device_node *dn,
str_spec.field_width = -1;
if (fmt[0] != 'F')
- return error_string(out, "(%pO?)", spec);
+ return error_string_spec(out, "(%pO?)", spec);
if (!IS_ENABLED(CONFIG_OF))
- return error_string(out, "(%pOF?)", spec);
+ return error_string_spec(out, "(%pOF?)", spec);
- if (check_pointer(out, dn, spec))
+ if (check_pointer_spec(out, dn, spec))
return;
/* simple case without anything any more format specifiers */
@@ -2062,7 +2082,7 @@ void device_node_string(struct printbuf *out, struct device_node *dn,
p = fwnode_get_name(of_fwnode_handle(dn));
precision = str_spec.precision;
str_spec.precision = strchrnul(p, '@') - p;
- string(out, p, str_spec);
+ string_spec(out, p, str_spec);
str_spec.precision = precision;
break;
case 'p': /* phandle */
@@ -2072,7 +2092,7 @@ void device_node_string(struct printbuf *out, struct device_node *dn,
p = fwnode_get_name(of_fwnode_handle(dn));
if (!p[1])
p = "/";
- string(out, p, str_spec);
+ string_spec(out, p, str_spec);
break;
case 'F': /* flags */
tbuf[0] = of_node_check_flag(dn, OF_DYNAMIC) ? 'D' : '-';
@@ -2082,18 +2102,18 @@ void device_node_string(struct printbuf *out, struct device_node *dn,
tbuf[4] = 0;
string_nocheck(out, tbuf, str_spec);
break;
- case 'c': /* major compatible string */
+ case 'c': /* major compatible string_spec */
ret = of_property_read_string(dn, "compatible", &p);
if (!ret)
- string(out, p, str_spec);
+ string_spec(out, p, str_spec);
break;
- case 'C': /* full compatible string */
+ case 'C': /* full compatible string_spec */
has_mult = false;
of_property_for_each_string(dn, "compatible", prop, p) {
if (has_mult)
string_nocheck(out, ",", str_spec);
string_nocheck(out, "\"", str_spec);
- string(out, p, str_spec);
+ string_spec(out, p, str_spec);
string_nocheck(out, "\"", str_spec);
has_mult = true;
@@ -2118,16 +2138,16 @@ void fwnode_string(struct printbuf *out,
str_spec.field_width = -1;
if (*fmt != 'w')
- return error_string(out, "(%pf?)", spec);
+ return error_string_spec(out, "(%pf?)", spec);
- if (check_pointer(out, fwnode, spec))
+ if (check_pointer_spec(out, fwnode, spec))
return;
fmt++;
switch (*fmt) {
case 'P': /* name */
- string(out, fwnode_get_name(fwnode), str_spec);
+ string_spec(out, fwnode_get_name(fwnode), str_spec);
break;
case 'f': /* full_name */
default:
@@ -2294,13 +2314,16 @@ static noinline_for_stack
void pointer(struct printbuf *out, const char *fmt,
void *ptr, struct printf_spec spec)
{
+ unsigned prev_pos = out->pos;
+
switch (*fmt) {
case 'S':
case 's':
ptr = dereference_symbol_descriptor(ptr);
fallthrough;
case 'B':
- return symbol_string(out, ptr, spec, fmt);
+ symbol_string(out, ptr, fmt);
+ return do_width_precision(out, prev_pos, spec);
case 'R':
case 'r':
return resource_string(out, ptr, spec, fmt);
@@ -2331,28 +2354,34 @@ void pointer(struct printbuf *out, const char *fmt,
case 'E':
return escaped_string(out, ptr, spec, fmt);
case 'U':
- return uuid_string(out, ptr, spec, fmt);
+ uuid_string(out, ptr, fmt);
+ return do_width_precision(out, prev_pos, spec);
case 'V':
return va_format(out, ptr, spec, fmt);
case 'K':
return restricted_pointer(out, ptr, spec);
case 'N':
- return netdev_bits(out, ptr, spec, fmt);
+ netdev_bits(out, ptr, fmt);
+ return do_width_precision(out, prev_pos, spec);
case '4':
return fourcc_string(out, ptr, spec, fmt);
case 'a':
- return address_val(out, ptr, spec, fmt);
+ address_val(out, ptr, fmt);
+ return do_width_precision(out, prev_pos, spec);
case 'd':
- return dentry_name(out, ptr, spec, fmt);
+ dentry_name(out, ptr, fmt);
+ return do_width_precision(out, prev_pos, spec);
case 't':
return time_and_date(out, ptr, spec, fmt);
case 'C':
return clock(out, ptr, spec, fmt);
case 'D':
- return file_dentry_name(out, ptr, spec, fmt);
+ file_dentry_name(out, ptr, fmt);
+ return do_width_precision(out, prev_pos, spec);
#ifdef CONFIG_BLOCK
case 'g':
- return bdev_name(out, ptr, spec, fmt);
+ bdev_name(out, ptr);
+ return do_width_precision(out, prev_pos, spec);
#endif
case 'G':
@@ -2372,9 +2401,9 @@ void pointer(struct printbuf *out, const char *fmt,
case 'k':
switch (fmt[1]) {
case 's':
- return string(out, ptr, spec);
+ return string_spec(out, ptr, spec);
default:
- return error_string(out, "(einval)", spec);
+ return error_string_spec(out, "(einval)", spec);
}
default:
return default_pointer(out, ptr, spec);
@@ -2675,6 +2704,7 @@ void prt_vprintf(struct printbuf *out, const char *fmt, va_list args)
while (*fmt) {
const char *old_fmt = fmt;
+ unsigned prev_pos = out->pos;
int read = format_decode(fmt, &spec);
fmt += read;
@@ -2704,7 +2734,8 @@ void prt_vprintf(struct printbuf *out, const char *fmt, va_list args)
break;
case FORMAT_TYPE_STR:
- string(out, va_arg(args, char *), spec);
+ string(out, va_arg(args, char *));
+ do_width_precision(out, prev_pos, spec);
break;
case FORMAT_TYPE_PTR:
@@ -3197,7 +3228,7 @@ void prt_bstrprintf(struct printbuf *out, const char *fmt, const u32 *bin_buf)
case FORMAT_TYPE_STR: {
const char *str_arg = args;
args += strlen(str_arg) + 1;
- string(out, (char *)str_arg, spec);
+ string_spec(out, (char *)str_arg, spec);
break;
}
--
2.36.0
This converts memory_stat_format() from seq_buf to printbuf. Printbuf is
simalar to seq_buf except that it heap allocates the string buffer:
here, we were already heap allocating the buffer with kmalloc() so the
conversion is trivial.
Signed-off-by: Kent Overstreet <[email protected]>
---
mm/memcontrol.c | 68 ++++++++++++++++++++++++-------------------------
1 file changed, 33 insertions(+), 35 deletions(-)
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 598fece89e..57861dc9fe 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -62,7 +62,7 @@
#include <linux/file.h>
#include <linux/resume_user_mode.h>
#include <linux/psi.h>
-#include <linux/seq_buf.h>
+#include <linux/printbuf.h>
#include "internal.h"
#include <net/sock.h>
#include <net/ip.h>
@@ -1461,13 +1461,9 @@ static inline unsigned long memcg_page_state_output(struct mem_cgroup *memcg,
static char *memory_stat_format(struct mem_cgroup *memcg)
{
- struct seq_buf s;
+ struct printbuf buf = PRINTBUF;
int i;
- seq_buf_init(&s, kmalloc(PAGE_SIZE, GFP_KERNEL), PAGE_SIZE);
- if (!s.buffer)
- return NULL;
-
/*
* Provide statistics on the state of the memory subsystem as
* well as cumulative event counters that show past behavior.
@@ -1484,49 +1480,51 @@ static char *memory_stat_format(struct mem_cgroup *memcg)
u64 size;
size = memcg_page_state_output(memcg, memory_stats[i].idx);
- seq_buf_printf(&s, "%s %llu\n", memory_stats[i].name, size);
+ prt_printf(&buf, "%s %llu\n", memory_stats[i].name, size);
if (unlikely(memory_stats[i].idx == NR_SLAB_UNRECLAIMABLE_B)) {
size += memcg_page_state_output(memcg,
NR_SLAB_RECLAIMABLE_B);
- seq_buf_printf(&s, "slab %llu\n", size);
+ prt_printf(&buf, "slab %llu\n", size);
}
}
/* Accumulated memory events */
- seq_buf_printf(&s, "%s %lu\n", vm_event_name(PGFAULT),
- memcg_events(memcg, PGFAULT));
- seq_buf_printf(&s, "%s %lu\n", vm_event_name(PGMAJFAULT),
- memcg_events(memcg, PGMAJFAULT));
- seq_buf_printf(&s, "%s %lu\n", vm_event_name(PGREFILL),
- memcg_events(memcg, PGREFILL));
- seq_buf_printf(&s, "pgscan %lu\n",
- memcg_events(memcg, PGSCAN_KSWAPD) +
- memcg_events(memcg, PGSCAN_DIRECT));
- seq_buf_printf(&s, "pgsteal %lu\n",
- memcg_events(memcg, PGSTEAL_KSWAPD) +
- memcg_events(memcg, PGSTEAL_DIRECT));
- seq_buf_printf(&s, "%s %lu\n", vm_event_name(PGACTIVATE),
- memcg_events(memcg, PGACTIVATE));
- seq_buf_printf(&s, "%s %lu\n", vm_event_name(PGDEACTIVATE),
- memcg_events(memcg, PGDEACTIVATE));
- seq_buf_printf(&s, "%s %lu\n", vm_event_name(PGLAZYFREE),
- memcg_events(memcg, PGLAZYFREE));
- seq_buf_printf(&s, "%s %lu\n", vm_event_name(PGLAZYFREED),
- memcg_events(memcg, PGLAZYFREED));
+ prt_printf(&buf, "%s %lu\n", vm_event_name(PGFAULT),
+ memcg_events(memcg, PGFAULT));
+ prt_printf(&buf, "%s %lu\n", vm_event_name(PGMAJFAULT),
+ memcg_events(memcg, PGMAJFAULT));
+ prt_printf(&buf, "%s %lu\n", vm_event_name(PGREFILL),
+ memcg_events(memcg, PGREFILL));
+ prt_printf(&buf, "pgscan %lu\n",
+ memcg_events(memcg, PGSCAN_KSWAPD) +
+ memcg_events(memcg, PGSCAN_DIRECT));
+ prt_printf(&buf, "pgsteal %lu\n",
+ memcg_events(memcg, PGSTEAL_KSWAPD) +
+ memcg_events(memcg, PGSTEAL_DIRECT));
+ prt_printf(&buf, "%s %lu\n", vm_event_name(PGACTIVATE),
+ memcg_events(memcg, PGACTIVATE));
+ prt_printf(&buf, "%s %lu\n", vm_event_name(PGDEACTIVATE),
+ memcg_events(memcg, PGDEACTIVATE));
+ prt_printf(&buf, "%s %lu\n", vm_event_name(PGLAZYFREE),
+ memcg_events(memcg, PGLAZYFREE));
+ prt_printf(&buf, "%s %lu\n", vm_event_name(PGLAZYFREED),
+ memcg_events(memcg, PGLAZYFREED));
#ifdef CONFIG_TRANSPARENT_HUGEPAGE
- seq_buf_printf(&s, "%s %lu\n", vm_event_name(THP_FAULT_ALLOC),
- memcg_events(memcg, THP_FAULT_ALLOC));
- seq_buf_printf(&s, "%s %lu\n", vm_event_name(THP_COLLAPSE_ALLOC),
- memcg_events(memcg, THP_COLLAPSE_ALLOC));
+ prt_printf(&buf, "%s %lu\n", vm_event_name(THP_FAULT_ALLOC),
+ memcg_events(memcg, THP_FAULT_ALLOC));
+ prt_printf(&buf, "%s %lu\n", vm_event_name(THP_COLLAPSE_ALLOC),
+ memcg_events(memcg, THP_COLLAPSE_ALLOC));
#endif /* CONFIG_TRANSPARENT_HUGEPAGE */
- /* The above should easily fit into one page */
- WARN_ON_ONCE(seq_buf_has_overflowed(&s));
+ if (buf.allocation_failure) {
+ printbuf_exit(&buf);
+ return NULL;
+ }
- return s.buffer;
+ return buf.buf;
}
#define K(x) ((x) << (PAGE_SHIFT-10))
--
2.36.0
This patch adds two new features to printbuf for structured formatting:
- Indent level: the indent level, as a number of spaces, may be
increased with pr_indent_add() and decreased with pr_indent_sub().
Subsequent lines, when started with pr_newline() (not "\n", although
that may change) will then be intended according to the current
indent level. This helps with pretty-printers that structure a large
amonut of data across multiple lines and multiple functions.
- Tabstops: Tabstops may be set by assigning to the printbuf->tabstops
array.
Then, pr_tab() may be used to advance to the next tabstop, printing
as many spaces as required - leaving previous output left justified
to the previous tabstop. pr_tab_rjust() advances to the next tabstop
but inserts the spaces just after the previous tabstop - right
justifying the previously-outputted text to the next tabstop.
Signed-off-by: Kent Overstreet <[email protected]>
---
include/linux/printbuf.h | 28 +++++++++
lib/printbuf.c | 125 +++++++++++++++++++++++++++++++++++++++
2 files changed, 153 insertions(+)
diff --git a/include/linux/printbuf.h b/include/linux/printbuf.h
index 35236f774a..8fb074eb67 100644
--- a/include/linux/printbuf.h
+++ b/include/linux/printbuf.h
@@ -36,6 +36,23 @@
* memory allocation failure we usually don't want to bail out and unwind - we
* want to print what we've got, on a best-effort basis. But code that does want
* to return -ENOMEM may check printbuf.allocation_failure.
+ *
+ * Indenting, tabstops:
+ *
+ * To aid is writing multi-line pretty printers spread across multiple
+ * functions, printbufs track the current indent level.
+ *
+ * printbuf_indent_push() and printbuf_indent_pop() increase and decrease the current indent
+ * level, respectively.
+ *
+ * To use tabstops, set printbuf->tabstops[]; they are in units of spaces, from
+ * start of line. Once set, prt_tab() will output spaces up to the next tabstop.
+ * prt_tab_rjust() will also advance the current line of text up to the next
+ * tabstop, but it does so by shifting text since the previous tabstop up to the
+ * next tabstop - right justifying it.
+ *
+ * Make sure you use prt_newline() instead of \n in the format string for indent
+ * level and tabstops to work corretly.
*/
#include <linux/string.h>
@@ -44,18 +61,29 @@ struct printbuf {
char *buf;
unsigned size;
unsigned pos;
+ unsigned last_newline;
+ unsigned last_field;
+ unsigned indent;
/*
* If nonzero, allocations will be done with GFP_ATOMIC:
*/
u8 atomic;
bool allocation_failure:1;
bool heap_allocated:1;
+ u8 tabstop;
+ u8 tabstops[4];
};
int printbuf_make_room(struct printbuf *, unsigned);
const char *printbuf_str(const struct printbuf *);
void printbuf_exit(struct printbuf *);
+void prt_newline(struct printbuf *);
+void printbuf_indent_add(struct printbuf *, unsigned);
+void printbuf_indent_sub(struct printbuf *, unsigned);
+void prt_tab(struct printbuf *);
+void prt_tab_rjust(struct printbuf *);
+
/* Initializer for a heap allocated printbuf: */
#define PRINTBUF ((struct printbuf) { .heap_allocated = true })
diff --git a/lib/printbuf.c b/lib/printbuf.c
index 0093b34158..e11a504abf 100644
--- a/lib/printbuf.c
+++ b/lib/printbuf.c
@@ -11,6 +11,11 @@
#include <linux/slab.h>
#include <linux/printbuf.h>
+static inline size_t printbuf_linelen(struct printbuf *buf)
+{
+ return buf->pos - buf->last_newline;
+}
+
int printbuf_make_room(struct printbuf *out, unsigned extra)
{
unsigned new_size;
@@ -68,3 +73,123 @@ void printbuf_exit(struct printbuf *buf)
}
}
EXPORT_SYMBOL(printbuf_exit);
+
+void prt_newline(struct printbuf *buf)
+{
+ unsigned i;
+
+ printbuf_make_room(buf, 1 + buf->indent);
+
+ __prt_char(buf, '\n');
+
+ buf->last_newline = buf->pos;
+
+ for (i = 0; i < buf->indent; i++)
+ __prt_char(buf, ' ');
+
+ printbuf_nul_terminate(buf);
+
+ buf->last_field = buf->pos;
+ buf->tabstop = 0;
+}
+EXPORT_SYMBOL(prt_newline);
+
+/**
+ * printbuf_indent_add - add to the current indent level
+ *
+ * @buf: printbuf to control
+ * @spaces: number of spaces to add to the current indent level
+ *
+ * Subsequent lines, and the current line if the output position is at the start
+ * of the current line, will be indented by @spaces more spaces.
+ */
+void printbuf_indent_add(struct printbuf *buf, unsigned spaces)
+{
+ if (WARN_ON_ONCE(buf->indent + spaces < buf->indent))
+ spaces = 0;
+
+ buf->indent += spaces;
+ while (spaces--)
+ prt_char(buf, ' ');
+}
+EXPORT_SYMBOL(printbuf_indent_add);
+
+/**
+ * printbuf_indent_sub - subtract from the current indent level
+ *
+ * @buf: printbuf to control
+ * @spaces: number of spaces to subtract from the current indent level
+ *
+ * Subsequent lines, and the current line if the output position is at the start
+ * of the current line, will be indented by @spaces less spaces.
+ */
+void printbuf_indent_sub(struct printbuf *buf, unsigned spaces)
+{
+ if (WARN_ON_ONCE(spaces > buf->indent))
+ spaces = buf->indent;
+
+ if (buf->last_newline + buf->indent == buf->pos) {
+ buf->pos -= spaces;
+ printbuf_nul_terminate(buf);
+ }
+ buf->indent -= spaces;
+}
+EXPORT_SYMBOL(printbuf_indent_sub);
+
+/**
+ * prt_tab - Advance printbuf to the next tabstop
+ *
+ * @buf: printbuf to control
+ *
+ * Advance output to the next tabstop by printing spaces.
+ */
+void prt_tab(struct printbuf *out)
+{
+ int spaces = max_t(int, 0, out->tabstops[out->tabstop] - printbuf_linelen(out));
+
+ BUG_ON(out->tabstop > ARRAY_SIZE(out->tabstops));
+
+ prt_chars(out, ' ', spaces);
+
+ out->last_field = out->pos;
+ out->tabstop++;
+}
+EXPORT_SYMBOL(prt_tab);
+
+/**
+ * prt_tab_rjust - Advance printbuf to the next tabstop, right justifying
+ * previous output
+ *
+ * @buf: printbuf to control
+ *
+ * Advance output to the next tabstop by inserting spaces immediately after the
+ * previous tabstop, right justifying previously outputted text.
+ */
+void prt_tab_rjust(struct printbuf *buf)
+{
+ BUG_ON(buf->tabstop > ARRAY_SIZE(buf->tabstops));
+
+ if (printbuf_linelen(buf) < buf->tabstops[buf->tabstop]) {
+ unsigned move = buf->pos - buf->last_field;
+ unsigned shift = buf->tabstops[buf->tabstop] -
+ printbuf_linelen(buf);
+
+ printbuf_make_room(buf, shift);
+
+ if (buf->last_field + shift < buf->size)
+ memmove(buf->buf + buf->last_field + shift,
+ buf->buf + buf->last_field,
+ min(move, buf->size - 1 - buf->last_field - shift));
+
+ if (buf->last_field < buf->size)
+ memset(buf->buf + buf->last_field, ' ',
+ min(shift, buf->size - buf->last_field));
+
+ buf->pos += shift;
+ printbuf_nul_terminate(buf);
+ }
+
+ buf->last_field = buf->pos;
+ buf->tabstop++;
+}
+EXPORT_SYMBOL(prt_tab_rjust);
--
2.36.0
On Sun, Jun 05, 2022 at 12:21:10PM -0400, Steven Rostedt wrote:
> On Sat, 4 Jun 2022 15:30:09 -0400
> Kent Overstreet <[email protected]> wrote:
>
> > Printbufs, your new data structure for all your string-building and outputting
> > needs!
> >
> > git repo: https://evilpiepirate.org/git/bcachefs.git/log/?h=printbuf_v3
>
> Hi Kent,
>
> Just wanted to let you know that I want to review this and I'm not
> ignoring it. But there's a still quite a bit in my queue that I must
> review before I get to this, so it may still be a while. :-/
Could you just review the parts that touch your code then? The rest has already
seen some review and Petr's going to do more, and I'd like to get this patchset
done and squared away so I can go back to building my OOM debugging improvements
on top of this stuff :)
This adds two new-style printbuf helpers for printing simple u64s, and
converts num_to_str() to be a simple wrapper around prt_u64_minwidth().
Signed-off-by: Kent Overstreet <[email protected]>
---
include/linux/kernel.h | 4 +-
lib/vsprintf.c | 94 ++++++++++++++++++++----------------------
2 files changed, 48 insertions(+), 50 deletions(-)
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 1906861ece..9ba5a53c6a 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -202,12 +202,14 @@ static inline void might_fault(void) { }
void do_exit(long error_code) __noreturn;
+struct printbuf;
+extern void prt_u64_minwidth(struct printbuf *out, u64 num, unsigned width);
+extern void prt_u64(struct printbuf *out, u64 num);
extern int num_to_str(char *buf, int size,
unsigned long long num, unsigned int width);
/* lib/printf utilities */
-struct printbuf;
extern __printf(2, 3) void prt_printf(struct printbuf *out, const char *fmt, ...);
extern __printf(2, 0) void prt_vprintf(struct printbuf *out, const char *fmt, va_list);
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 1c5ffc7f28..7b49a998d3 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -368,41 +368,51 @@ char *put_dec(char *buf, unsigned long long n)
#endif
-/*
- * Convert passed number to decimal string.
- * Returns the length of string. On buffer overflow, returns 0.
- *
- * If speed is not important, use snprintf(). It's easy to read the code.
+/**
+ * prt_u64_minwidth - print a u64, in decimal, with zero padding
+ * @out: printbuf to output to
+ * @num: u64 to print
+ * @width: minimum width
*/
-int num_to_str(char *buf, int size, unsigned long long num, unsigned int width)
+void prt_u64_minwidth(struct printbuf *out, u64 num, unsigned width)
{
/* put_dec requires 2-byte alignment of the buffer. */
char tmp[sizeof(num) * 3] __aligned(2);
- int idx, len;
+ unsigned len = put_dec(tmp, num) - tmp;
- /* put_dec() may work incorrectly for num = 0 (generate "", not "0") */
- if (num <= 9) {
- tmp[0] = '0' + num;
- len = 1;
- } else {
- len = put_dec(tmp, num) - tmp;
- }
+ printbuf_make_room(out, max(len, width));
- if (len > size || width > size)
- return 0;
+ if (width > len)
+ __prt_chars_reserved(out, '0', width - len);
- if (width > len) {
- width = width - len;
- for (idx = 0; idx < width; idx++)
- buf[idx] = ' ';
- } else {
- width = 0;
- }
+ while (len)
+ __prt_char_reserved(out, tmp[--len]);
+ printbuf_nul_terminate(out);
+}
- for (idx = 0; idx < len; ++idx)
- buf[idx + width] = tmp[len - idx - 1];
+/**
+ * prt_u64 - print a simple u64, in decimal
+ * @out: printbuf to output to
+ * @num: u64 to print
+ */
+void prt_u64(struct printbuf *out, u64 num)
+{
+ prt_u64_minwidth(out, num, 0);
+}
- return len + width;
+/*
+ * Convert passed number to decimal string.
+ * Returns the length of string. On buffer overflow, returns 0.
+ *
+ * Consider switching to printbufs and using prt_u64() or prt_u64_minwith()
+ * instead.
+ */
+int num_to_str(char *buf, int size, unsigned long long num, unsigned int width)
+{
+ struct printbuf out = PRINTBUF_EXTERN(buf, size);
+
+ prt_u64_minwidth(&out, num, width);
+ return out.pos;
}
#define SIGN 1 /* unsigned/signed, must be 1 */
@@ -1018,20 +1028,6 @@ static const struct printf_spec default_dec_spec = {
.precision = -1,
};
-static const struct printf_spec default_dec02_spec = {
- .base = 10,
- .field_width = 2,
- .precision = -1,
- .flags = ZEROPAD,
-};
-
-static const struct printf_spec default_dec04_spec = {
- .base = 10,
- .field_width = 4,
- .precision = -1,
- .flags = ZEROPAD,
-};
-
static noinline_for_stack
void resource_string(struct printbuf *out, struct resource *res,
struct printf_spec spec, const char *fmt)
@@ -1231,12 +1227,12 @@ void bitmap_list_string(struct printbuf *out, unsigned long *bitmap,
prt_char(out, ',');
first = false;
- number(out, rbot, default_dec_spec);
+ prt_u64(out, rbot);
if (rtop == rbot + 1)
continue;
prt_char(out, '-');
- number(out, rtop - 1, default_dec_spec);
+ prt_u64(out, rtop - 1);
}
}
@@ -1778,21 +1774,21 @@ void date_str(struct printbuf *out,
int year = tm->tm_year + (r ? 0 : 1900);
int mon = tm->tm_mon + (r ? 0 : 1);
- number(out, year, default_dec04_spec);
+ prt_u64_minwidth(out, year, 4);
prt_char(out, '-');
- number(out, mon, default_dec02_spec);
+ prt_u64_minwidth(out, mon, 2);
prt_char(out, '-');
- number(out, tm->tm_mday, default_dec02_spec);
+ prt_u64_minwidth(out, tm->tm_mday, 2);
}
static noinline_for_stack
void time_str(struct printbuf *out, const struct rtc_time *tm, bool r)
{
- number(out, tm->tm_hour, default_dec02_spec);
+ prt_u64_minwidth(out, tm->tm_hour, 2);
prt_char(out, ':');
- number(out, tm->tm_min, default_dec02_spec);
+ prt_u64_minwidth(out, tm->tm_min, 2);
prt_char(out, ':');
- number(out, tm->tm_sec, default_dec02_spec);
+ prt_u64_minwidth(out, tm->tm_sec, 2);
}
static noinline_for_stack
@@ -2070,7 +2066,7 @@ void device_node_string(struct printbuf *out, struct device_node *dn,
str_spec.precision = precision;
break;
case 'p': /* phandle */
- number(out, (unsigned int)dn->phandle, default_dec_spec);
+ prt_u64(out, (unsigned int)dn->phandle);
break;
case 'P': /* path-spec */
p = fwnode_get_name(of_fwnode_handle(dn));
--
2.36.0
This converts most of the hexdump code to printbufs, along with some
significant cleanups and a bit of reorganization. The old non-printbuf
functions are mostly left as wrappers around the new printbuf versions.
Big note: byte swabbing behaviour
Previously, hex_dump_to_buffer() would byteswab the groups of bytes
being printed on little endian machines. This behaviour is... not
standard or typical for a hex dumper, and this behaviour was silently
added/changed without documentation (in 2007).
Given that the hex dumpers are just used for debugging output, nothing
is likely to break, and hopefully by reverting to more standard
behaviour the end result will be _less_ confusion, modulo a few kernel
developers who will certainly be annoyed by their tools changing.
Signed-off-by: Kent Overstreet <[email protected]>
---
include/linux/kernel.h | 6 +
lib/hexdump.c | 246 ++++++++++++++++++++++++-----------------
lib/test_hexdump.c | 30 +----
3 files changed, 159 insertions(+), 123 deletions(-)
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 5c4f4b6d36..1906861ece 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -293,6 +293,12 @@ extern int hex_to_bin(unsigned char ch);
extern int __must_check hex2bin(u8 *dst, const char *src, size_t count);
extern char *bin2hex(char *dst, const void *src, size_t count);
+struct printbuf;
+void prt_hex_bytes(struct printbuf *, const void *, unsigned, unsigned, unsigned);
+void prt_hex_line(struct printbuf *, const void *, size_t, int, int, bool);
+void prt_hex_dump(struct printbuf *, const void *, size_t,
+ const char *, int, unsigned, unsigned, bool);
+
bool mac_pton(const char *s, u8 *mac);
/*
diff --git a/lib/hexdump.c b/lib/hexdump.c
index 06833d4043..9556f15ad2 100644
--- a/lib/hexdump.c
+++ b/lib/hexdump.c
@@ -9,6 +9,7 @@
#include <linux/kernel.h>
#include <linux/minmax.h>
#include <linux/export.h>
+#include <linux/printbuf.h>
#include <asm/unaligned.h>
const char hex_asc[] = "0123456789abcdef";
@@ -79,32 +80,40 @@ int hex2bin(u8 *dst, const char *src, size_t count)
EXPORT_SYMBOL(hex2bin);
/**
- * bin2hex - convert binary data to an ascii hexadecimal string
- * @dst: ascii hexadecimal result
- * @src: binary data
- * @count: binary data length
+ * prt_hex_bytes - Print a string of hex bytes, with optional separator
+ *
+ * @out: The printbuf to output to
+ * @addr: Buffer to print
+ * @nr: Number of bytes to print
+ * @separator: Optional separator character between each byte
*/
-char *bin2hex(char *dst, const void *src, size_t count)
+void prt_hex_bytes(struct printbuf *out, const void *buf, unsigned len,
+ unsigned groupsize, unsigned separator)
{
- const unsigned char *_src = src;
+ const u8 *ptr = buf;
+ unsigned i;
- while (count--)
- dst = hex_byte_pack(dst, *_src++);
- return dst;
+ if (!groupsize)
+ groupsize = 1;
+
+ for (i = 0; i < len ; ++i) {
+ if (i && separator && !(i % groupsize))
+ __prt_char(out, separator);
+ prt_hex_byte(out, ptr[i]);
+ }
}
-EXPORT_SYMBOL(bin2hex);
+EXPORT_SYMBOL(prt_hex_bytes);
/**
- * hex_dump_to_buffer - convert a blob of data to "hex ASCII" in memory
+ * prt_hex_line - convert a blob of data to "hex ASCII" in memory
+ * @out: printbuf to output to
* @buf: data blob to dump
* @len: number of bytes in the @buf
* @rowsize: number of bytes to print per line; must be 16 or 32
* @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
- * @linebuf: where to put the converted data
- * @linebuflen: total size of @linebuf, including space for terminating NUL
* @ascii: include ASCII after the hex output
*
- * hex_dump_to_buffer() works on one "line" of output at a time, i.e.,
+ * prt_hex_line() works on one "line" of output at a time, i.e.,
* 16 or 32 bytes of input data converted to hex + ASCII output.
*
* Given a buffer of u8 data, hex_dump_to_buffer() converts the input data
@@ -117,22 +126,13 @@ EXPORT_SYMBOL(bin2hex);
*
* example output buffer:
* 40 41 42 43 44 45 46 47 48 49 4a 4b 4c 4d 4e 4f @ABCDEFGHIJKLMNO
- *
- * Return:
- * The amount of bytes placed in the buffer without terminating NUL. If the
- * output was truncated, then the return value is the number of bytes
- * (excluding the terminating NUL) which would have been written to the final
- * string if enough space had been available.
*/
-int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
- char *linebuf, size_t linebuflen, bool ascii)
+void prt_hex_line(struct printbuf *out, const void *buf, size_t len,
+ int rowsize, int groupsize, bool ascii)
{
+ unsigned saved_pos = out->pos;
const u8 *ptr = buf;
- int ngroups;
- u8 ch;
- int j, lx = 0;
- int ascii_column;
- int ret;
+ int i, ngroups;
if (rowsize != 16 && rowsize != 32)
rowsize = 16;
@@ -145,84 +145,127 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
groupsize = 1;
ngroups = len / groupsize;
- ascii_column = rowsize * 2 + rowsize / groupsize + 1;
-
- if (!linebuflen)
- goto overflow1;
if (!len)
- goto nil;
-
- if (groupsize == 8) {
- const u64 *ptr8 = buf;
-
- for (j = 0; j < ngroups; j++) {
- ret = snprintf(linebuf + lx, linebuflen - lx,
- "%s%16.16llx", j ? " " : "",
- get_unaligned(ptr8 + j));
- if (ret >= linebuflen - lx)
- goto overflow1;
- lx += ret;
- }
- } else if (groupsize == 4) {
- const u32 *ptr4 = buf;
-
- for (j = 0; j < ngroups; j++) {
- ret = snprintf(linebuf + lx, linebuflen - lx,
- "%s%8.8x", j ? " " : "",
- get_unaligned(ptr4 + j));
- if (ret >= linebuflen - lx)
- goto overflow1;
- lx += ret;
- }
- } else if (groupsize == 2) {
- const u16 *ptr2 = buf;
-
- for (j = 0; j < ngroups; j++) {
- ret = snprintf(linebuf + lx, linebuflen - lx,
- "%s%4.4x", j ? " " : "",
- get_unaligned(ptr2 + j));
- if (ret >= linebuflen - lx)
- goto overflow1;
- lx += ret;
- }
- } else {
- for (j = 0; j < len; j++) {
- if (linebuflen < lx + 2)
- goto overflow2;
- ch = ptr[j];
- linebuf[lx++] = hex_asc_hi(ch);
- if (linebuflen < lx + 2)
- goto overflow2;
- linebuf[lx++] = hex_asc_lo(ch);
- if (linebuflen < lx + 2)
- goto overflow2;
- linebuf[lx++] = ' ';
+ return;
+
+ prt_hex_bytes(out, ptr, len, groupsize, ' ');
+
+ if (ascii) {
+ unsigned ascii_column = rowsize * 2 + rowsize / groupsize + 1;
+
+ prt_chars(out, ' ', max_t(int, 0, ascii_column - (out->pos - saved_pos)));
+
+ for (i = 0; i < len; i++) {
+ u8 ch = ptr[i];
+ prt_char(out, isascii(ch) && isprint(ch) ? ch : '.');
}
- if (j)
- lx--;
}
- if (!ascii)
- goto nil;
+}
+EXPORT_SYMBOL(prt_hex_line);
- while (lx < ascii_column) {
- if (linebuflen < lx + 2)
- goto overflow2;
- linebuf[lx++] = ' ';
- }
- for (j = 0; j < len; j++) {
- if (linebuflen < lx + 2)
- goto overflow2;
- ch = ptr[j];
- linebuf[lx++] = (isascii(ch) && isprint(ch)) ? ch : '.';
+/**
+ * prt_hex_dump - print multiline formatted hex dump
+ * @out: printbuf to output to
+ * @buf: data blob to dump
+ * @len: number of bytes in the @buf
+ * @prefix_str: string to prefix each line with;
+ * caller supplies trailing spaces for alignment if desired
+ * @prefix_type: controls whether prefix of an offset, address, or none
+ * is printed (%DUMP_PREFIX_OFFSET, %DUMP_PREFIX_ADDRESS, %DUMP_PREFIX_NONE)
+ * @rowsize: number of bytes to print per line; must be 16 or 32
+ * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
+ * @ascii: include ASCII after the hex output
+ *
+ * Function is an analogue of print_hex_dump() and thus has similar interface.
+ *
+ * linebuf size is maximal length for one line.
+ * 32 * 3 - maximum bytes per line, each printed into 2 chars + 1 for
+ * separating space
+ * 2 - spaces separating hex dump and ascii representation
+ * 32 - ascii representation
+ * 1 - terminating '\0'
+ */
+void prt_hex_dump(struct printbuf *out, const void *buf, size_t len,
+ const char *prefix_str, int prefix_type,
+ unsigned rowsize, unsigned groupsize, bool ascii)
+{
+ const u8 *ptr = buf;
+ size_t i;
+
+ if (rowsize != 16 && rowsize != 32)
+ rowsize = 16;
+
+ for (i = 0; i < len; i += rowsize) {
+ prt_str(out, prefix_str);
+
+ switch (prefix_type) {
+ case DUMP_PREFIX_ADDRESS:
+ prt_printf(out, "%p: ", ptr + i);
+ break;
+ case DUMP_PREFIX_OFFSET:
+ prt_printf(out, "%.8zx: ", i);
+ break;
+ }
+
+ prt_hex_line(out, ptr + i, min_t(size_t, len - i, rowsize),
+ rowsize, groupsize, ascii);
+ prt_char(out, '\n');
}
-nil:
- linebuf[lx] = '\0';
- return lx;
-overflow2:
- linebuf[lx++] = '\0';
-overflow1:
- return ascii ? ascii_column + len : (groupsize * 2 + 1) * ngroups - 1;
+}
+
+/**
+ * bin2hex - convert binary data to an ascii hexadecimal string
+ * @dst: ascii hexadecimal result
+ * @src: binary data
+ * @count: binary data length
+ */
+char *bin2hex(char *dst, const void *src, size_t count)
+{
+ struct printbuf out = PRINTBUF_EXTERN(dst, count * 4);
+
+ prt_hex_bytes(&out, src, count, 0, 0);
+ return dst + out.pos;
+}
+EXPORT_SYMBOL(bin2hex);
+
+/**
+ * hex_dump_to_buffer - convert a blob of data to "hex ASCII" in memory
+ * @buf: data blob to dump
+ * @len: number of bytes in the @buf
+ * @rowsize: number of bytes to print per line; must be 16 or 32
+ * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
+ * @linebuf: where to put the converted data
+ * @linebuflen: total size of @linebuf, including space for terminating NUL
+ * @ascii: include ASCII after the hex output
+ *
+ * hex_dump_to_buffer() works on one "line" of output at a time, i.e.,
+ * 16 or 32 bytes of input data converted to hex + ASCII output.
+ *
+ * Given a buffer of u8 data, hex_dump_to_buffer() converts the input data
+ * to a hex + ASCII dump at the supplied memory location.
+ * The converted output is always NUL-terminated.
+ *
+ * E.g.:
+ * hex_dump_to_buffer(frame->data, frame->len, 16, 1,
+ * linebuf, sizeof(linebuf), true);
+ *
+ * example output buffer:
+ * 40 41 42 43 44 45 46 47 48 49 4a 4b 4c 4d 4e 4f @ABCDEFGHIJKLMNO
+ *
+ * Return:
+ * The amount of bytes placed in the buffer without terminating NUL. If the
+ * output was truncated, then the return value is the number of bytes
+ * (excluding the terminating NUL) which would have been written to the final
+ * string if enough space had been available.
+ */
+int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
+ char *linebuf, size_t linebuflen, bool ascii)
+{
+ struct printbuf out = PRINTBUF_EXTERN(linebuf, linebuflen);
+
+ prt_hex_line(&out, buf, len, rowsize, groupsize, ascii);
+ return out.pos;
}
EXPORT_SYMBOL(hex_dump_to_buffer);
@@ -262,6 +305,11 @@ void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
int rowsize, int groupsize,
const void *buf, size_t len, bool ascii)
{
+ /*
+ * XXX: this code does the exact same thing as prt_hex_dump(): we should
+ * be able to call that and printk() the result, except printk is
+ * restricted to 1024 bytes of output per call
+ */
const u8 *ptr = buf;
int i, linelen, remaining = len;
unsigned char linebuf[32 * 3 + 2 + 32 + 1];
diff --git a/lib/test_hexdump.c b/lib/test_hexdump.c
index 5144899d3c..f9e97879dc 100644
--- a/lib/test_hexdump.c
+++ b/lib/test_hexdump.c
@@ -25,36 +25,19 @@ static const char * const test_data_1[] __initconst = {
"4c", "d1", "19", "99", "43", "b1", "af", "0c",
};
-static const char * const test_data_2_le[] __initconst = {
- "32be", "7bdb", "180a", "b293",
- "ba70", "24c4", "837d", "9b34",
- "9ca6", "ad31", "0f9c", "e9ac",
- "d14c", "9919", "b143", "0caf",
-};
-
-static const char * const test_data_2_be[] __initconst = {
+static const char * const test_data_2[] __initconst = {
"be32", "db7b", "0a18", "93b2",
"70ba", "c424", "7d83", "349b",
"a69c", "31ad", "9c0f", "ace9",
"4cd1", "1999", "43b1", "af0c",
};
-static const char * const test_data_4_le[] __initconst = {
- "7bdb32be", "b293180a", "24c4ba70", "9b34837d",
- "ad319ca6", "e9ac0f9c", "9919d14c", "0cafb143",
-};
-
-static const char * const test_data_4_be[] __initconst = {
+static const char * const test_data_4[] __initconst = {
"be32db7b", "0a1893b2", "70bac424", "7d83349b",
"a69c31ad", "9c0face9", "4cd11999", "43b1af0c",
};
-static const char * const test_data_8_le[] __initconst = {
- "b293180a7bdb32be", "9b34837d24c4ba70",
- "e9ac0f9cad319ca6", "0cafb1439919d14c",
-};
-
-static const char * const test_data_8_be[] __initconst = {
+static const char * const test_data_8[] __initconst = {
"be32db7b0a1893b2", "70bac4247d83349b",
"a69c31ad9c0face9", "4cd1199943b1af0c",
};
@@ -73,7 +56,6 @@ static void __init test_hexdump_prepare_test(size_t len, int rowsize,
size_t l = len;
int gs = groupsize, rs = rowsize;
unsigned int i;
- const bool is_be = IS_ENABLED(CONFIG_CPU_BIG_ENDIAN);
if (rs != 16 && rs != 32)
rs = 16;
@@ -85,11 +67,11 @@ static void __init test_hexdump_prepare_test(size_t len, int rowsize,
gs = 1;
if (gs == 8)
- result = is_be ? test_data_8_be : test_data_8_le;
+ result = test_data_8;
else if (gs == 4)
- result = is_be ? test_data_4_be : test_data_4_le;
+ result = test_data_4;
else if (gs == 2)
- result = is_be ? test_data_2_be : test_data_2_le;
+ result = test_data_2;
else
result = test_data_1;
--
2.36.0
Signed-off-by: Kent Overstreet <[email protected]>
---
include/linux/pretty-printers.h | 10 ++++++
lib/Makefile | 2 +-
lib/pretty-printers.c | 59 +++++++++++++++++++++++++++++++++
3 files changed, 70 insertions(+), 1 deletion(-)
create mode 100644 include/linux/pretty-printers.h
create mode 100644 lib/pretty-printers.c
diff --git a/include/linux/pretty-printers.h b/include/linux/pretty-printers.h
new file mode 100644
index 0000000000..f39d8edfba
--- /dev/null
+++ b/include/linux/pretty-printers.h
@@ -0,0 +1,10 @@
+/* SPDX-License-Identifier: LGPL-2.1+ */
+/* Copyright (C) 2022 Kent Overstreet */
+
+#ifndef _LINUX_PRETTY_PRINTERS_H
+#define _LINUX_PRETTY_PRINTERS_H
+
+void prt_string_option(struct printbuf *, const char * const[], size_t);
+void prt_bitflags(struct printbuf *, const char * const[], u64);
+
+#endif /* _LINUX_PRETTY_PRINTERS_H */
diff --git a/lib/Makefile b/lib/Makefile
index b4609a4258..b520024852 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -34,7 +34,7 @@ lib-y := ctype.o string.o vsprintf.o cmdline.o \
is_single_threaded.o plist.o decompress.o kobject_uevent.o \
earlycpio.o seq_buf.o siphash.o dec_and_lock.o \
nmi_backtrace.o nodemask.o win_minmax.o memcat_p.o \
- buildid.o printbuf.o
+ buildid.o printbuf.o pretty-printers.o
lib-$(CONFIG_PRINTK) += dump_stack.o
lib-$(CONFIG_SMP) += cpumask.o
diff --git a/lib/pretty-printers.c b/lib/pretty-printers.c
new file mode 100644
index 0000000000..8794eb3824
--- /dev/null
+++ b/lib/pretty-printers.c
@@ -0,0 +1,59 @@
+// SPDX-License-Identifier: LGPL-2.1+
+/* Copyright (C) 2022 Kent Overstreet */
+
+#include <linux/kernel.h>
+#include <linux/printbuf.h>
+#include <linux/pretty-printers.h>
+
+/**
+ * prt_string_option - Given a list of strings, print out the list and indicate
+ * which option is selected, with square brackets (sysfs style)
+ *
+ * @out: The printbuf to output to
+ * @list: List of strings to choose from
+ * @selected: The option to highlight, with square brackets
+ */
+void prt_string_option(struct printbuf *out,
+ const char * const list[],
+ size_t selected)
+{
+ size_t i;
+
+ for (i = 0; list[i]; i++) {
+ if (i)
+ prt_char(out, ' ');
+ if (i == selected)
+ prt_char(out, '[');
+ prt_str(out, list[i]);
+ if (i == selected)
+ prt_char(out, ']');
+ }
+}
+EXPORT_SYMBOL(prt_string_option);
+
+/**
+ * prt_bitflags: Given a bitmap and a list of names for each bit, print out which
+ * bits are on, comma separated
+ *
+ * @out: The printbuf to output to
+ * @list: List of names for each bit
+ * @flags: Bits to print
+ */
+void prt_bitflags(struct printbuf *out,
+ const char * const list[], u64 flags)
+{
+ unsigned bit, nr = 0;
+ bool first = true;
+
+ while (list[nr])
+ nr++;
+
+ while (flags && (bit = __ffs(flags)) < nr) {
+ if (!first)
+ prt_char(out, ',');
+ first = false;
+ prt_str(out, list[bit]);
+ flags ^= 1 << bit;
+ }
+}
+EXPORT_SYMBOL(prt_bitflags);
--
2.36.0
This makes printbufs optionally heap allocated: a printbuf initialized
with the PRINTBUF initializer will automatically heap allocate and
resize as needed.
Allocations are done with GFP_KERNEL: code should use e.g.
memalloc_nofs_save()/restore() as needed. Since we do not currently have
memalloc_nowait_save()/restore(), in contexts where it is not safe to
block we provide the helpers
printbuf_atomic_inc()
printbuf_atomic_dec()
When the atomic count is nonzero, memory allocations will be done with
GFP_NOWAIT.
On memory allocation failure, output will be truncated. Code that wishes
to check for memory allocation failure (in contexts where we should
return -ENOMEM) should check if printbuf->allocation_failure is set.
Since printbufs are expected to be typically used for log messages and
on a best effort basis, we don't return errors directly.
Other helpers provided by this patch:
- printbuf_make_room(buf, extra)
Reallocates if necessary to make room for @extra bytes (not including
terminating null).
- printbuf_str(buf)
Returns a null terminated string equivalent to the contents of @buf.
If @buf was never allocated (or allocation failed), returns a
constant empty string.
- printbuf_exit(buf)
Releases memory allocated by a printbuf.
Signed-off-by: Kent Overstreet <[email protected]>
---
include/linux/printbuf.h | 114 +++++++++++++++++++++++++++++++++------
lib/Makefile | 2 +-
lib/printbuf.c | 70 ++++++++++++++++++++++++
3 files changed, 170 insertions(+), 16 deletions(-)
create mode 100644 lib/printbuf.c
diff --git a/include/linux/printbuf.h b/include/linux/printbuf.h
index 8b3797dc4b..35236f774a 100644
--- a/include/linux/printbuf.h
+++ b/include/linux/printbuf.h
@@ -4,18 +4,68 @@
#ifndef _LINUX_PRINTBUF_H
#define _LINUX_PRINTBUF_H
-#include <linux/string.h>
-
/*
- * Printbufs: String buffer for outputting (printing) to, for vsnprintf
+ * Printbufs: Simple strings for printing to, with optional heap allocation
+ *
+ * This code has provisions for use in userspace, to aid in making other code
+ * portable between kernelspace and userspace.
+ *
+ * Basic example:
+ * struct printbuf buf = PRINTBUF;
+ *
+ * prt_printf(&buf, "foo=");
+ * foo_to_text(&buf, foo);
+ * printk("%s", buf.buf);
+ * printbuf_exit(&buf);
+ *
+ * Or
+ * struct printbuf buf = PRINTBUF_EXTERN(char_buf, char_buf_size)
+ *
+ * We can now write pretty printers instead of writing code that dumps
+ * everything to the kernel log buffer, and then those pretty-printers can be
+ * used by other code that outputs to kernel log, sysfs, debugfs, etc.
+ *
+ * Memory allocation: Outputing to a printbuf may allocate memory. This
+ * allocation is done with GFP_KERNEL, by default: use the newer
+ * memalloc_*_(save|restore) functions as needed.
+ *
+ * Since no equivalent yet exists for GFP_ATOMIC/GFP_NOWAIT, memory allocations
+ * will be done with GFP_NOWAIT if printbuf->atomic is nonzero.
+ *
+ * Memory allocation failures: We don't return errors directly, because on
+ * memory allocation failure we usually don't want to bail out and unwind - we
+ * want to print what we've got, on a best-effort basis. But code that does want
+ * to return -ENOMEM may check printbuf.allocation_failure.
*/
+#include <linux/string.h>
+
struct printbuf {
char *buf;
unsigned size;
unsigned pos;
+ /*
+ * If nonzero, allocations will be done with GFP_ATOMIC:
+ */
+ u8 atomic;
+ bool allocation_failure:1;
+ bool heap_allocated:1;
};
+int printbuf_make_room(struct printbuf *, unsigned);
+const char *printbuf_str(const struct printbuf *);
+void printbuf_exit(struct printbuf *);
+
+/* Initializer for a heap allocated printbuf: */
+#define PRINTBUF ((struct printbuf) { .heap_allocated = true })
+
+/* Initializer a printbuf that points to an external buffer: */
+#define PRINTBUF_EXTERN(_buf, _size) \
+((struct printbuf) { \
+ .buf = _buf, \
+ .size = _size, \
+})
+
/*
* Returns size remaining of output buffer:
*/
@@ -48,13 +98,15 @@ static inline bool printbuf_overflowed(struct printbuf *out)
static inline void printbuf_nul_terminate(struct printbuf *out)
{
+ printbuf_make_room(out, 1);
+
if (out->pos < out->size)
out->buf[out->pos] = 0;
else if (out->size)
out->buf[out->size - 1] = 0;
}
-static inline void __prt_chars(struct printbuf *out, char c, unsigned n)
+static inline void __prt_chars_reserved(struct printbuf *out, char c, unsigned n)
{
memset(out->buf + out->pos,
c,
@@ -64,17 +116,26 @@ static inline void __prt_chars(struct printbuf *out, char c, unsigned n)
static inline void prt_chars(struct printbuf *out, char c, unsigned n)
{
- __prt_chars(out, c, n);
+ printbuf_make_room(out, n);
+ __prt_chars_reserved(out, c, n);
printbuf_nul_terminate(out);
}
-static inline void __prt_char(struct printbuf *out, char c)
+/* Doesn't call printbuf_make_room(), doesn't nul terminate: */
+static inline void __prt_char_reserved(struct printbuf *out, char c)
{
if (printbuf_remaining(out))
out->buf[out->pos] = c;
out->pos++;
}
+/* Doesn't nul terminate: */
+static inline void __prt_char(struct printbuf *out, char c)
+{
+ printbuf_make_room(out, 1);
+ __prt_char_reserved(out, c);
+}
+
static inline void prt_char(struct printbuf *out, char c)
{
__prt_char(out, c);
@@ -83,6 +144,8 @@ static inline void prt_char(struct printbuf *out, char c)
static inline void prt_bytes(struct printbuf *out, const void *b, unsigned n)
{
+ printbuf_make_room(out, n);
+
memcpy(out->buf + out->pos,
b,
min(n, printbuf_remaining(out)));
@@ -97,22 +160,43 @@ static inline void prt_str(struct printbuf *out, const char *str)
static inline void prt_hex_byte(struct printbuf *out, u8 byte)
{
- __prt_char(out, hex_asc_hi(byte));
- __prt_char(out, hex_asc_lo(byte));
+ printbuf_make_room(out, 2);
+ __prt_char_reserved(out, hex_asc_hi(byte));
+ __prt_char_reserved(out, hex_asc_lo(byte));
printbuf_nul_terminate(out);
}
static inline void prt_hex_byte_upper(struct printbuf *out, u8 byte)
{
- __prt_char(out, hex_asc_upper_hi(byte));
- __prt_char(out, hex_asc_upper_lo(byte));
+ printbuf_make_room(out, 2);
+ __prt_char_reserved(out, hex_asc_upper_hi(byte));
+ __prt_char_reserved(out, hex_asc_upper_lo(byte));
printbuf_nul_terminate(out);
}
-#define PRINTBUF_EXTERN(_buf, _size) \
-((struct printbuf) { \
- .buf = _buf, \
- .size = _size, \
-})
+/**
+ * printbuf_reset - re-use a printbuf without freeing and re-initializing it:
+ */
+static inline void printbuf_reset(struct printbuf *buf)
+{
+ buf->pos = 0;
+ buf->allocation_failure = 0;
+}
+
+/**
+ * printbuf_atomic_inc - mark as entering an atomic section
+ */
+static inline void printbuf_atomic_inc(struct printbuf *buf)
+{
+ buf->atomic++;
+}
+
+/**
+ * printbuf_atomic_inc - mark as leaving an atomic section
+ */
+static inline void printbuf_atomic_dec(struct printbuf *buf)
+{
+ buf->atomic--;
+}
#endif /* _LINUX_PRINTBUF_H */
diff --git a/lib/Makefile b/lib/Makefile
index 6b9ffc1bd1..b4609a4258 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -34,7 +34,7 @@ lib-y := ctype.o string.o vsprintf.o cmdline.o \
is_single_threaded.o plist.o decompress.o kobject_uevent.o \
earlycpio.o seq_buf.o siphash.o dec_and_lock.o \
nmi_backtrace.o nodemask.o win_minmax.o memcat_p.o \
- buildid.o
+ buildid.o printbuf.o
lib-$(CONFIG_PRINTK) += dump_stack.o
lib-$(CONFIG_SMP) += cpumask.o
diff --git a/lib/printbuf.c b/lib/printbuf.c
new file mode 100644
index 0000000000..0093b34158
--- /dev/null
+++ b/lib/printbuf.c
@@ -0,0 +1,70 @@
+// SPDX-License-Identifier: LGPL-2.1+
+/* Copyright (C) 2022 Kent Overstreet */
+
+#ifdef __KERNEL__
+#include <linux/export.h>
+#include <linux/kernel.h>
+#else
+#define EXPORT_SYMBOL(x)
+#endif
+
+#include <linux/slab.h>
+#include <linux/printbuf.h>
+
+int printbuf_make_room(struct printbuf *out, unsigned extra)
+{
+ unsigned new_size;
+ char *buf;
+
+ if (!out->heap_allocated)
+ return 0;
+
+ /* Reserved space for terminating nul: */
+ extra += 1;
+
+ if (out->pos + extra < out->size)
+ return 0;
+
+ new_size = roundup_pow_of_two(out->size + extra);
+ buf = krealloc(out->buf, new_size, !out->atomic ? GFP_KERNEL : GFP_NOWAIT);
+
+ if (!buf) {
+ out->allocation_failure = true;
+ return -ENOMEM;
+ }
+
+ out->buf = buf;
+ out->size = new_size;
+ return 0;
+}
+EXPORT_SYMBOL(printbuf_make_room);
+
+/**
+ * printbuf_str - returns printbuf's buf as a C string, guaranteed to be null
+ * terminated
+ */
+const char *printbuf_str(const struct printbuf *buf)
+{
+ /*
+ * If we've written to a printbuf then it's guaranteed to be a null
+ * terminated string - but if we haven't, then we might not have
+ * allocated a buffer at all:
+ */
+ return buf->pos
+ ? buf->buf
+ : "";
+}
+EXPORT_SYMBOL(printbuf_str);
+
+/**
+ * printbuf_exit - exit a printbuf, freeing memory it owns and poisoning it
+ * against accidental use.
+ */
+void printbuf_exit(struct printbuf *buf)
+{
+ if (buf->heap_allocated) {
+ kfree(buf->buf);
+ buf->buf = ERR_PTR(-EINTR); /* poison value */
+ }
+}
+EXPORT_SYMBOL(printbuf_exit);
--
2.36.0
This adds options to printbuf for specifying whether units should be
printed raw (default) or with human readable units, and for controlling
whether human-readable units should be base 2 (default), or base 10.
This also adds new helpers that obey these options:
- pr_human_readable_u64
- pr_human_readable_s64
These obey printbuf->si_units
- pr_units_u64
- pr_units_s64
These obey both printbuf-human_readable_units and printbuf->si_units
Signed-off-by: Kent Overstreet <[email protected]>
---
include/linux/printbuf.h | 15 +++++++++++
lib/printbuf.c | 57 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 72 insertions(+)
diff --git a/include/linux/printbuf.h b/include/linux/printbuf.h
index 8fb074eb67..9d6c951626 100644
--- a/include/linux/printbuf.h
+++ b/include/linux/printbuf.h
@@ -53,10 +53,19 @@
*
* Make sure you use prt_newline() instead of \n in the format string for indent
* level and tabstops to work corretly.
+ *
+ * Output units: printbuf->units exists to tell pretty-printers how to output
+ * numbers: a raw value (e.g. directly from a superblock field), as bytes, or as
+ * human readable bytes. prt_units() obeys it.
*/
#include <linux/string.h>
+enum printbuf_si {
+ PRINTBUF_UNITS_2, /* use binary powers of 2^10 */
+ PRINTBUF_UNITS_10, /* use powers of 10^3 (standard SI) */
+};
+
struct printbuf {
char *buf;
unsigned size;
@@ -70,6 +79,8 @@ struct printbuf {
u8 atomic;
bool allocation_failure:1;
bool heap_allocated:1;
+ enum printbuf_si si_units:1;
+ bool human_readable_units:1;
u8 tabstop;
u8 tabstops[4];
};
@@ -83,6 +94,10 @@ void printbuf_indent_add(struct printbuf *, unsigned);
void printbuf_indent_sub(struct printbuf *, unsigned);
void prt_tab(struct printbuf *);
void prt_tab_rjust(struct printbuf *);
+void prt_human_readable_u64(struct printbuf *, u64);
+void prt_human_readable_s64(struct printbuf *, s64);
+void prt_units_u64(struct printbuf *, u64);
+void prt_units_s64(struct printbuf *, s64);
/* Initializer for a heap allocated printbuf: */
#define PRINTBUF ((struct printbuf) { .heap_allocated = true })
diff --git a/lib/printbuf.c b/lib/printbuf.c
index e11a504abf..88f1f34fe6 100644
--- a/lib/printbuf.c
+++ b/lib/printbuf.c
@@ -9,6 +9,7 @@
#endif
#include <linux/slab.h>
+#include <linux/string_helpers.h>
#include <linux/printbuf.h>
static inline size_t printbuf_linelen(struct printbuf *buf)
@@ -193,3 +194,59 @@ void prt_tab_rjust(struct printbuf *buf)
buf->tabstop++;
}
EXPORT_SYMBOL(prt_tab_rjust);
+
+/**
+ * prt_human_readable_u64 - Print out a u64 in human readable units
+ *
+ * Units of 2^10 (default) or 10^3 are controlled via @buf->si_units
+ */
+void prt_human_readable_u64(struct printbuf *buf, u64 v)
+{
+ printbuf_make_room(buf, 10);
+ buf->pos += string_get_size(v, 1, !buf->si_units,
+ buf->buf + buf->pos,
+ printbuf_remaining_size(buf));
+}
+EXPORT_SYMBOL(prt_human_readable_u64);
+
+/**
+ * prt_human_readable_s64 - Print out a s64 in human readable units
+ *
+ * Units of 2^10 (default) or 10^3 are controlled via @buf->si_units
+ */
+void prt_human_readable_s64(struct printbuf *buf, s64 v)
+{
+ if (v < 0)
+ prt_char(buf, '-');
+ prt_human_readable_u64(buf, abs(v));
+}
+EXPORT_SYMBOL(prt_human_readable_s64);
+
+/**
+ * prt_units_u64 - Print out a u64 according to printbuf unit options
+ *
+ * Units are either raw (default), or human reabable units (controlled via
+ * @buf->human_readable_units)
+ */
+void prt_units_u64(struct printbuf *out, u64 v)
+{
+ if (out->human_readable_units)
+ prt_human_readable_u64(out, v);
+ else
+ prt_printf(out, "%llu", v);
+}
+EXPORT_SYMBOL(prt_units_u64);
+
+/**
+ * prt_units_s64 - Print out a s64 according to printbuf unit options
+ *
+ * Units are either raw (default), or human reabable units (controlled via
+ * @buf->human_readable_units)
+ */
+void prt_units_s64(struct printbuf *out, s64 v)
+{
+ if (v < 0)
+ prt_char(out, '-');
+ prt_units_u64(out, abs(v));
+}
+EXPORT_SYMBOL(prt_units_s64);
--
2.36.0
On Sat 2022-06-04 15:30:10, Kent Overstreet wrote:
> This adds printbufs: a printbuf points to a char * buffer and knows the
> size of the output buffer as well as the current output position.
>
> Future patches will be adding more features to printbuf, but initially
> printbufs are targeted at refactoring and improving our existing code in
> lib/vsprintf.c - so this initial printbuf patch has the features
> required for that.
>
> Signed-off-by: Kent Overstreet <[email protected]>
> Reviewed-by: Matthew Wilcox (Oracle) <[email protected]>
> ---
> include/linux/printbuf.h | 118 +++++++++++++++++++++++++++++++++++++++
> 1 file changed, 118 insertions(+)
> create mode 100644 include/linux/printbuf.h
>
> diff --git a/include/linux/printbuf.h b/include/linux/printbuf.h
> new file mode 100644
> index 0000000000..8b3797dc4b
> --- /dev/null
> +++ b/include/linux/printbuf.h
> @@ -0,0 +1,118 @@
> +/* SPDX-License-Identifier: LGPL-2.1+ */
> +/* Copyright (C) 2022 Kent Overstreet */
> +
> +#ifndef _LINUX_PRINTBUF_H
> +#define _LINUX_PRINTBUF_H
> +
> +#include <linux/string.h>
> +
> +/*
> + * Printbufs: String buffer for outputting (printing) to, for vsnprintf
It might be worth to mention:
@pos is the position that would be used to write into the buffer.
The string is truncated when @pos is greater or equal to
the buffer @size.
@buf might be NULL and @size zero when the buffer is used just
to count the needed buffer size.
It is visible in the code. But it would be nice to explicitly
mention it because it is one important feature of this buffer.
> + */
> +
> +struct printbuf {
> + char *buf;
> + unsigned size;
> + unsigned pos;
> +};
> +
> +static inline void __prt_chars(struct printbuf *out, char c, unsigned n)
> +{
> + memset(out->buf + out->pos,
> + c,
> + min(n, printbuf_remaining(out)));
> + out->pos += n;
> +}
Thanks for using "prt_" prefix. It is better than "pr_", definitely.
Though, I wonder if you considered using:
pb_ aka "Print to Buffer"
ppb_ aka "Print to PrintBuffer"
bpr_ aka "Buffer Print"
to make it more obvious that it is printing into some buffer.
I promise that this is my last try to bikeshed about the prefix.
I just want to get it "right" before you extend the patchset even
more.
Otherwise, the code looks good to me.
Best Regards,
Petr
On Sat 2022-06-04 15:30:11, Kent Overstreet wrote:
> Like the upcoming vsprintf.c conversion, this converts string_escape_mem
> to prt_escaped_string(), which uses and outputs to a printbuf, and makes
> string_escape_mem() a smaller wrapper to support existing users.
>
> The new printbuf helpers greatly simplify the code.
>
> --- a/lib/string_helpers.c
> +++ b/lib/string_helpers.c
> @@ -15,6 +15,7 @@
> #include <linux/fs.h>
> #include <linux/limits.h>
> #include <linux/mm.h>
> +#include <linux/printbuf.h>
> #include <linux/slab.h>
> #include <linux/string.h>
> #include <linux/string_helpers.h>
> @@ -301,19 +302,14 @@ int string_unescape(char *src, char *dst, size_t size, unsigned int flags)
> }
> EXPORT_SYMBOL(string_unescape);
>
> -static bool escape_passthrough(unsigned char c, char **dst, char *end)
> +static bool escape_passthrough(struct printbuf *out, unsigned char c)
> {
> - char *out = *dst;
> -
> - if (out < end)
> - *out = c;
> - *dst = out + 1;
> + prt_char(out, c);
This modifies the behavior. The original code did not add
the trailing '\0'.
I agree that the original behavior is ugly but it is documented
see the comment:
* Return:
* The total size of the escaped output that would be generated for
* the given input and flags. To check whether the output was
* truncated, compare the return value to osz. There is room left in
* dst for a '\0' terminator if and only if ret < osz.
^^^^^^^^^^^^^^
I am all for changing the behavior but it would require checking
all callers.
Anyway, adding the trailing '\0' all is not much effective.
I suggest to use __prt_char() and add the trailing '\0' when
the string is complete.
We must make sure that __prt_char() is able to add the last
character even when there will not longer be space for
the trailing '\0'.
> return true;
> }
>
[...]
>
> /**
> - * string_escape_mem - quote characters in the given memory buffer
> + * prt_escaped_string - quote characters in the given memory buffer
> + * @out: printbuf to output to (escaped)
> * @src: source buffer (unescaped)
> * @isz: source buffer size
> - * @dst: destination buffer (escaped)
> - * @osz: destination buffer size
> * @flags: combination of the flags
> * @only: NULL-terminated string containing characters used to limit
> * the selected escape class. If characters are included in @only
> @@ -517,11 +466,10 @@ static bool escape_hex(unsigned char c, char **dst, char *end)
> * truncated, compare the return value to osz. There is room left in
> * dst for a '\0' terminator if and only if ret < osz.
> */
> -int string_escape_mem(const char *src, size_t isz, char *dst, size_t osz,
> - unsigned int flags, const char *only)
> +void prt_escaped_string(struct printbuf *out,
> + const char *src, size_t isz,
> + unsigned int flags, const char *only)
> {
> - char *p = dst;
> - char *end = p + osz;
> bool is_dict = only && *only;
> bool is_append = flags & ESCAPE_APPEND;
>
> @@ -549,41 +497,49 @@ int string_escape_mem(const char *src, size_t isz, char *dst, size_t osz,
> * %ESCAPE_NA cases.
> */
> if (!(is_append || in_dict) && is_dict &&
> - escape_passthrough(c, &p, end))
> + escape_passthrough(out, c))
> continue;
>
> if (!(is_append && in_dict) && isascii(c) && isprint(c) &&
> - flags & ESCAPE_NAP && escape_passthrough(c, &p, end))
> + flags & ESCAPE_NAP && escape_passthrough(out, c))
> continue;
>
> if (!(is_append && in_dict) && isprint(c) &&
> - flags & ESCAPE_NP && escape_passthrough(c, &p, end))
> + flags & ESCAPE_NP && escape_passthrough(out, c))
> continue;
>
> if (!(is_append && in_dict) && isascii(c) &&
> - flags & ESCAPE_NA && escape_passthrough(c, &p, end))
> + flags & ESCAPE_NA && escape_passthrough(out, c))
> continue;
>
> - if (flags & ESCAPE_SPACE && escape_space(c, &p, end))
> + if (flags & ESCAPE_SPACE && escape_space(out, c))
> continue;
>
> - if (flags & ESCAPE_SPECIAL && escape_special(c, &p, end))
> + if (flags & ESCAPE_SPECIAL && escape_special(out, c))
> continue;
>
> - if (flags & ESCAPE_NULL && escape_null(c, &p, end))
> + if (flags & ESCAPE_NULL && escape_null(out, c))
> continue;
>
> /* ESCAPE_OCTAL and ESCAPE_HEX always go last */
> - if (flags & ESCAPE_OCTAL && escape_octal(c, &p, end))
> + if (flags & ESCAPE_OCTAL && escape_octal(out, c))
> continue;
>
> - if (flags & ESCAPE_HEX && escape_hex(c, &p, end))
> + if (flags & ESCAPE_HEX && escape_hex(out, c))
> continue;
>
> - escape_passthrough(c, &p, end);
> + escape_passthrough(out, c);
> }
> +}
> +EXPORT_SYMBOL(prt_escaped_string);
> +
> +int string_escape_mem(const char *src, size_t isz, char *dst, size_t osz,
> + unsigned int flags, const char *only)
We need keep the comment above this API as long as it is public.
Note that it uses the docbook comment format, starging with /** ...
And the symbol is exported.
It is even more important because of the crazy behavior where
it does not add the trailing '\0'.
> +{
> + struct printbuf out = PRINTBUF_EXTERN(dst, osz);
>
> - return p - dst;
> + prt_escaped_string(&out, src, isz, flags, only);
> + return out.pos;
> }
> EXPORT_SYMBOL(string_escape_mem);
Note: If you decided to change/fix the behavior then please do so
in a separate patch(set). I was able to catch the problem
because the patch was straightforward and "easy" to review.
Best Regards,
Petr
On 6/9/22 10:25, Petr Mladek wrote:
> On Sat 2022-06-04 15:30:11, Kent Overstreet wrote:
>> Like the upcoming vsprintf.c conversion, this converts string_escape_mem
>> to prt_escaped_string(), which uses and outputs to a printbuf, and makes
>> string_escape_mem() a smaller wrapper to support existing users.
>>
>> The new printbuf helpers greatly simplify the code.
>>
>> --- a/lib/string_helpers.c
>> +++ b/lib/string_helpers.c
>> @@ -15,6 +15,7 @@
>> #include <linux/fs.h>
>> #include <linux/limits.h>
>> #include <linux/mm.h>
>> +#include <linux/printbuf.h>
>> #include <linux/slab.h>
>> #include <linux/string.h>
>> #include <linux/string_helpers.h>
>> @@ -301,19 +302,14 @@ int string_unescape(char *src, char *dst, size_t size, unsigned int flags)
>> }
>> EXPORT_SYMBOL(string_unescape);
>>
>> -static bool escape_passthrough(unsigned char c, char **dst, char *end)
>> +static bool escape_passthrough(struct printbuf *out, unsigned char c)
>> {
>> - char *out = *dst;
>> -
>> - if (out < end)
>> - *out = c;
>> - *dst = out + 1;
>> + prt_char(out, c);
>
> This modifies the behavior. The original code did not add
> the trailing '\0'.
>
> I agree that the original behavior is ugly but it is documented
> see the comment:
>
> * Return:
> * The total size of the escaped output that would be generated for
> * the given input and flags. To check whether the output was
> * truncated, compare the return value to osz. There is room left in
> * dst for a '\0' terminator if and only if ret < osz.
> ^^^^^^^^^^^^^^
>
>
> I am all for changing the behavior but it would require checking
> all callers.
>
> Anyway, adding the trailing '\0' all is not much effective.
> I suggest to use __prt_char() and add the trailing '\0' when
> the string is complete.
>
> We must make sure that __prt_char() is able to add the last
> character even when there will not longer be space for
> the trailing '\0'.
You really think there's going to be code depending on an _absence_ of a
trailing nul? I should've updated the comment (I missed that originally,
doing it now) - but this seems excessively nitpicky.
And I'm looking at the callers now:
- seq_file, which doesn't commit the results if it overflowed the buffer
- net/sunrpc/cache.c, which also appears to turn an overflow into an error
and that's it, aside from test cases.
The nul termination is an explicit thing: as I convert to printbuf, I
don't want functions that output to printbufs returning printbufs that
aren't nul terminated unless explicitly documented - and there's no
reason to be doing this except for a few fast path helpers.
>
>> +int string_escape_mem(const char *src, size_t isz, char *dst, size_t osz,
>> + unsigned int flags, const char *only)
>
> We need keep the comment above this API as long as it is public.
Will do.