From: "Steven Rostedt (Red Hat)" <[email protected]>
The trace_seq functions are rather useful outside of tracing. Instead
of having it be dependent on CONFIG_TRACING, move the code into lib/
and allow other users to have access to it even when tracing is not
configured.
Signed-off-by: Steven Rostedt <[email protected]>
---
include/linux/trace_seq.h | 68 ++--------
kernel/trace/trace.c | 24 ----
kernel/trace/trace_output.c | 268 ---------------------------------------
kernel/trace/trace_output.h | 16 ---
lib/Makefile | 2 +-
lib/trace_seq.c | 303 ++++++++++++++++++++++++++++++++++++++++++++
6 files changed, 316 insertions(+), 365 deletions(-)
create mode 100644 lib/trace_seq.c
diff --git a/include/linux/trace_seq.h b/include/linux/trace_seq.h
index 136116924d8d..3fc21ee71389 100644
--- a/include/linux/trace_seq.h
+++ b/include/linux/trace_seq.h
@@ -25,10 +25,6 @@ trace_seq_init(struct trace_seq *s)
s->full = 0;
}
-/*
- * Currently only defined when tracing is enabled.
- */
-#ifdef CONFIG_TRACING
extern __printf(2, 3)
int trace_seq_printf(struct trace_seq *s, const char *fmt, ...);
extern __printf(2, 0)
@@ -49,59 +45,19 @@ extern int trace_seq_path(struct trace_seq *s, const struct path *path);
extern int trace_seq_bitmask(struct trace_seq *s, const unsigned long *maskp,
int nmaskbits);
-#else /* CONFIG_TRACING */
-static inline int trace_seq_printf(struct trace_seq *s, const char *fmt, ...)
-{
- return 0;
-}
-static inline int
-trace_seq_bprintf(struct trace_seq *s, const char *fmt, const u32 *binary)
-{
- return 0;
-}
+#define MAX_MEMHEX_BYTES 8
-static inline int
-trace_seq_bitmask(struct trace_seq *s, const unsigned long *maskp,
- int nmaskbits)
-{
- return 0;
-}
+#define SEQ_PUT_FIELD_RET(s, x) \
+do { \
+ if (!trace_seq_putmem(s, &(x), sizeof(x))) \
+ return TRACE_TYPE_PARTIAL_LINE; \
+} while (0)
-static inline int trace_print_seq(struct seq_file *m, struct trace_seq *s)
-{
- return 0;
-}
-static inline ssize_t trace_seq_to_user(struct trace_seq *s, char __user *ubuf,
- size_t cnt)
-{
- return 0;
-}
-static inline int trace_seq_puts(struct trace_seq *s, const char *str)
-{
- return 0;
-}
-static inline int trace_seq_putc(struct trace_seq *s, unsigned char c)
-{
- return 0;
-}
-static inline int
-trace_seq_putmem(struct trace_seq *s, const void *mem, size_t len)
-{
- return 0;
-}
-static inline int trace_seq_putmem_hex(struct trace_seq *s, const void *mem,
- size_t len)
-{
- return 0;
-}
-static inline void *trace_seq_reserve(struct trace_seq *s, size_t len)
-{
- return NULL;
-}
-static inline int trace_seq_path(struct trace_seq *s, const struct path *path)
-{
- return 0;
-}
-#endif /* CONFIG_TRACING */
+#define SEQ_PUT_HEX_FIELD_RET(s, x) \
+do { \
+ BUILD_BUG_ON(sizeof(x) > MAX_MEMHEX_BYTES); \
+ if (!trace_seq_putmem_hex(s, &(x), sizeof(x))) \
+ return TRACE_TYPE_PARTIAL_LINE; \
+} while (0)
#endif /* _LINUX_TRACE_SEQ_H */
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 384ede311717..eeb233cbac4f 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -923,30 +923,6 @@ out:
return ret;
}
-ssize_t trace_seq_to_user(struct trace_seq *s, char __user *ubuf, size_t cnt)
-{
- int len;
- int ret;
-
- if (!cnt)
- return 0;
-
- if (s->len <= s->readpos)
- return -EBUSY;
-
- len = s->len - s->readpos;
- if (cnt > len)
- cnt = len;
- ret = copy_to_user(ubuf, s->buffer + s->readpos, cnt);
- if (ret == cnt)
- return -EFAULT;
-
- cnt -= ret;
-
- s->readpos += cnt;
- return cnt;
-}
-
static ssize_t trace_seq_to_buffer(struct trace_seq *s, void *buf, size_t cnt)
{
int len;
diff --git a/kernel/trace/trace_output.c b/kernel/trace/trace_output.c
index f3dad80c20b2..b8930f79a04b 100644
--- a/kernel/trace/trace_output.c
+++ b/kernel/trace/trace_output.c
@@ -20,23 +20,6 @@ static struct hlist_head event_hash[EVENT_HASHSIZE] __read_mostly;
static int next_event_type = __TRACE_LAST_TYPE + 1;
-int trace_print_seq(struct seq_file *m, struct trace_seq *s)
-{
- int len = s->len >= PAGE_SIZE ? PAGE_SIZE - 1 : s->len;
- int ret;
-
- ret = seq_write(m, s->buffer, len);
-
- /*
- * Only reset this buffer if we successfully wrote to the
- * seq_file buffer.
- */
- if (!ret)
- trace_seq_init(s);
-
- return ret;
-}
-
enum print_line_t trace_print_bputs_msg_only(struct trace_iterator *iter)
{
struct trace_seq *s = &iter->seq;
@@ -85,257 +68,6 @@ enum print_line_t trace_print_printk_msg_only(struct trace_iterator *iter)
return TRACE_TYPE_HANDLED;
}
-/**
- * trace_seq_printf - sequence printing of trace information
- * @s: trace sequence descriptor
- * @fmt: printf format string
- *
- * It returns 0 if the trace oversizes the buffer's free
- * space, 1 otherwise.
- *
- * The tracer may use either sequence operations or its own
- * copy to user routines. To simplify formating of a trace
- * trace_seq_printf is used to store strings into a special
- * buffer (@s). Then the output may be either used by
- * the sequencer or pulled into another buffer.
- */
-int
-trace_seq_printf(struct trace_seq *s, const char *fmt, ...)
-{
- int len = (PAGE_SIZE - 1) - s->len;
- va_list ap;
- int ret;
-
- if (s->full || !len)
- return 0;
-
- va_start(ap, fmt);
- ret = vsnprintf(s->buffer + s->len, len, fmt, ap);
- va_end(ap);
-
- /* If we can't write it all, don't bother writing anything */
- if (ret >= len) {
- s->full = 1;
- return 0;
- }
-
- s->len += ret;
-
- return 1;
-}
-EXPORT_SYMBOL_GPL(trace_seq_printf);
-
-/**
- * trace_seq_bitmask - put a list of longs as a bitmask print output
- * @s: trace sequence descriptor
- * @maskp: points to an array of unsigned longs that represent a bitmask
- * @nmaskbits: The number of bits that are valid in @maskp
- *
- * It returns 0 if the trace oversizes the buffer's free
- * space, 1 otherwise.
- *
- * Writes a ASCII representation of a bitmask string into @s.
- */
-int
-trace_seq_bitmask(struct trace_seq *s, const unsigned long *maskp,
- int nmaskbits)
-{
- int len = (PAGE_SIZE - 1) - s->len;
- int ret;
-
- if (s->full || !len)
- return 0;
-
- ret = bitmap_scnprintf(s->buffer, len, maskp, nmaskbits);
- s->len += ret;
-
- return 1;
-}
-EXPORT_SYMBOL_GPL(trace_seq_bitmask);
-
-/**
- * trace_seq_vprintf - sequence printing of trace information
- * @s: trace sequence descriptor
- * @fmt: printf format string
- *
- * The tracer may use either sequence operations or its own
- * copy to user routines. To simplify formating of a trace
- * trace_seq_printf is used to store strings into a special
- * buffer (@s). Then the output may be either used by
- * the sequencer or pulled into another buffer.
- */
-int
-trace_seq_vprintf(struct trace_seq *s, const char *fmt, va_list args)
-{
- int len = (PAGE_SIZE - 1) - s->len;
- int ret;
-
- if (s->full || !len)
- return 0;
-
- ret = vsnprintf(s->buffer + s->len, len, fmt, args);
-
- /* If we can't write it all, don't bother writing anything */
- if (ret >= len) {
- s->full = 1;
- return 0;
- }
-
- s->len += ret;
-
- return len;
-}
-EXPORT_SYMBOL_GPL(trace_seq_vprintf);
-
-int trace_seq_bprintf(struct trace_seq *s, const char *fmt, const u32 *binary)
-{
- int len = (PAGE_SIZE - 1) - s->len;
- int ret;
-
- if (s->full || !len)
- return 0;
-
- ret = bstr_printf(s->buffer + s->len, len, fmt, binary);
-
- /* If we can't write it all, don't bother writing anything */
- if (ret >= len) {
- s->full = 1;
- return 0;
- }
-
- s->len += ret;
-
- return len;
-}
-
-/**
- * trace_seq_puts - trace sequence printing of simple string
- * @s: trace sequence descriptor
- * @str: simple string to record
- *
- * The tracer may use either the sequence operations or its own
- * copy to user routines. This function records a simple string
- * into a special buffer (@s) for later retrieval by a sequencer
- * or other mechanism.
- */
-int trace_seq_puts(struct trace_seq *s, const char *str)
-{
- int len = strlen(str);
-
- if (s->full)
- return 0;
-
- if (len > ((PAGE_SIZE - 1) - s->len)) {
- s->full = 1;
- return 0;
- }
-
- memcpy(s->buffer + s->len, str, len);
- s->len += len;
-
- return len;
-}
-
-int trace_seq_putc(struct trace_seq *s, unsigned char c)
-{
- if (s->full)
- return 0;
-
- if (s->len >= (PAGE_SIZE - 1)) {
- s->full = 1;
- return 0;
- }
-
- s->buffer[s->len++] = c;
-
- return 1;
-}
-EXPORT_SYMBOL(trace_seq_putc);
-
-int trace_seq_putmem(struct trace_seq *s, const void *mem, size_t len)
-{
- if (s->full)
- return 0;
-
- if (len > ((PAGE_SIZE - 1) - s->len)) {
- s->full = 1;
- return 0;
- }
-
- memcpy(s->buffer + s->len, mem, len);
- s->len += len;
-
- return len;
-}
-
-int trace_seq_putmem_hex(struct trace_seq *s, const void *mem, size_t len)
-{
- unsigned char hex[HEX_CHARS];
- const unsigned char *data = mem;
- int i, j;
-
- if (s->full)
- return 0;
-
-#ifdef __BIG_ENDIAN
- for (i = 0, j = 0; i < len; i++) {
-#else
- for (i = len-1, j = 0; i >= 0; i--) {
-#endif
- hex[j++] = hex_asc_hi(data[i]);
- hex[j++] = hex_asc_lo(data[i]);
- }
- hex[j++] = ' ';
-
- return trace_seq_putmem(s, hex, j);
-}
-
-void *trace_seq_reserve(struct trace_seq *s, size_t len)
-{
- void *ret;
-
- if (s->full)
- return NULL;
-
- if (len > ((PAGE_SIZE - 1) - s->len)) {
- s->full = 1;
- return NULL;
- }
-
- ret = s->buffer + s->len;
- s->len += len;
-
- return ret;
-}
-
-int trace_seq_path(struct trace_seq *s, const struct path *path)
-{
- unsigned char *p;
-
- if (s->full)
- return 0;
-
- if (s->len >= (PAGE_SIZE - 1)) {
- s->full = 1;
- return 0;
- }
-
- p = d_path(path, s->buffer + s->len, PAGE_SIZE - s->len);
- if (!IS_ERR(p)) {
- p = mangle_path(s->buffer + s->len, p, "\n");
- if (p) {
- s->len = p - s->buffer;
- return 1;
- }
- } else {
- s->buffer[s->len++] = '?';
- return 1;
- }
-
- s->full = 1;
- return 0;
-}
-
const char *
ftrace_print_flags_seq(struct trace_seq *p, const char *delim,
unsigned long flags,
diff --git a/kernel/trace/trace_output.h b/kernel/trace/trace_output.h
index 127a9d8c8357..89e904540509 100644
--- a/kernel/trace/trace_output.h
+++ b/kernel/trace/trace_output.h
@@ -35,21 +35,5 @@ trace_print_lat_fmt(struct trace_seq *s, struct trace_entry *entry);
extern int __unregister_ftrace_event(struct trace_event *event);
extern struct rw_semaphore trace_event_sem;
-#define MAX_MEMHEX_BYTES 8
-#define HEX_CHARS (MAX_MEMHEX_BYTES*2 + 1)
-
-#define SEQ_PUT_FIELD_RET(s, x) \
-do { \
- if (!trace_seq_putmem(s, &(x), sizeof(x))) \
- return TRACE_TYPE_PARTIAL_LINE; \
-} while (0)
-
-#define SEQ_PUT_HEX_FIELD_RET(s, x) \
-do { \
- BUILD_BUG_ON(sizeof(x) > MAX_MEMHEX_BYTES); \
- if (!trace_seq_putmem_hex(s, &(x), sizeof(x))) \
- return TRACE_TYPE_PARTIAL_LINE; \
-} while (0)
-
#endif
diff --git a/lib/Makefile b/lib/Makefile
index ba967a19edba..fe8d8c1b04aa 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -13,7 +13,7 @@ lib-y := ctype.o string.o vsprintf.o cmdline.o \
sha1.o md5.o irq_regs.o reciprocal_div.o argv_split.o \
proportions.o flex_proportions.o prio_heap.o ratelimit.o show_mem.o \
is_single_threaded.o plist.o decompress.o kobject_uevent.o \
- earlycpio.o
+ earlycpio.o trace_seq.o
obj-$(CONFIG_ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS) += usercopy.o
lib-$(CONFIG_MMU) += ioremap.o
diff --git a/lib/trace_seq.c b/lib/trace_seq.c
new file mode 100644
index 000000000000..5ba99c6cf834
--- /dev/null
+++ b/lib/trace_seq.c
@@ -0,0 +1,303 @@
+/*
+ * trace_seq.c
+ *
+ * Copyright (C) 2008-2014 Red Hat Inc, Steven Rostedt <[email protected]>
+ *
+ */
+#include <linux/uaccess.h>
+#include <linux/seq_file.h>
+#include <linux/trace_seq.h>
+
+int trace_print_seq(struct seq_file *m, struct trace_seq *s)
+{
+ int len = s->len >= PAGE_SIZE ? PAGE_SIZE - 1 : s->len;
+ int ret;
+
+ ret = seq_write(m, s->buffer, len);
+
+ /*
+ * Only reset this buffer if we successfully wrote to the
+ * seq_file buffer.
+ */
+ if (!ret)
+ trace_seq_init(s);
+
+ return ret;
+}
+
+/**
+ * trace_seq_printf - sequence printing of trace information
+ * @s: trace sequence descriptor
+ * @fmt: printf format string
+ *
+ * It returns 0 if the trace oversizes the buffer's free
+ * space, 1 otherwise.
+ *
+ * The tracer may use either sequence operations or its own
+ * copy to user routines. To simplify formating of a trace
+ * trace_seq_printf is used to store strings into a special
+ * buffer (@s). Then the output may be either used by
+ * the sequencer or pulled into another buffer.
+ */
+int
+trace_seq_printf(struct trace_seq *s, const char *fmt, ...)
+{
+ int len = (PAGE_SIZE - 1) - s->len;
+ va_list ap;
+ int ret;
+
+ if (s->full || !len)
+ return 0;
+
+ va_start(ap, fmt);
+ ret = vsnprintf(s->buffer + s->len, len, fmt, ap);
+ va_end(ap);
+
+ /* If we can't write it all, don't bother writing anything */
+ if (ret >= len) {
+ s->full = 1;
+ return 0;
+ }
+
+ s->len += ret;
+
+ return 1;
+}
+EXPORT_SYMBOL_GPL(trace_seq_printf);
+
+/**
+ * trace_seq_bitmask - put a list of longs as a bitmask print output
+ * @s: trace sequence descriptor
+ * @maskp: points to an array of unsigned longs that represent a bitmask
+ * @nmaskbits: The number of bits that are valid in @maskp
+ *
+ * It returns 0 if the trace oversizes the buffer's free
+ * space, 1 otherwise.
+ *
+ * Writes a ASCII representation of a bitmask string into @s.
+ */
+int
+trace_seq_bitmask(struct trace_seq *s, const unsigned long *maskp,
+ int nmaskbits)
+{
+ int len = (PAGE_SIZE - 1) - s->len;
+ int ret;
+
+ if (s->full || !len)
+ return 0;
+
+ ret = bitmap_scnprintf(s->buffer, len, maskp, nmaskbits);
+ s->len += ret;
+
+ return 1;
+}
+EXPORT_SYMBOL_GPL(trace_seq_bitmask);
+
+/**
+ * trace_seq_vprintf - sequence printing of trace information
+ * @s: trace sequence descriptor
+ * @fmt: printf format string
+ *
+ * The tracer may use either sequence operations or its own
+ * copy to user routines. To simplify formating of a trace
+ * trace_seq_printf is used to store strings into a special
+ * buffer (@s). Then the output may be either used by
+ * the sequencer or pulled into another buffer.
+ */
+int
+trace_seq_vprintf(struct trace_seq *s, const char *fmt, va_list args)
+{
+ int len = (PAGE_SIZE - 1) - s->len;
+ int ret;
+
+ if (s->full || !len)
+ return 0;
+
+ ret = vsnprintf(s->buffer + s->len, len, fmt, args);
+
+ /* If we can't write it all, don't bother writing anything */
+ if (ret >= len) {
+ s->full = 1;
+ return 0;
+ }
+
+ s->len += ret;
+
+ return len;
+}
+EXPORT_SYMBOL_GPL(trace_seq_vprintf);
+
+int trace_seq_bprintf(struct trace_seq *s, const char *fmt, const u32 *binary)
+{
+ int len = (PAGE_SIZE - 1) - s->len;
+ int ret;
+
+ if (s->full || !len)
+ return 0;
+
+ ret = bstr_printf(s->buffer + s->len, len, fmt, binary);
+
+ /* If we can't write it all, don't bother writing anything */
+ if (ret >= len) {
+ s->full = 1;
+ return 0;
+ }
+
+ s->len += ret;
+
+ return len;
+}
+
+/**
+ * trace_seq_puts - trace sequence printing of simple string
+ * @s: trace sequence descriptor
+ * @str: simple string to record
+ *
+ * The tracer may use either the sequence operations or its own
+ * copy to user routines. This function records a simple string
+ * into a special buffer (@s) for later retrieval by a sequencer
+ * or other mechanism.
+ */
+int trace_seq_puts(struct trace_seq *s, const char *str)
+{
+ int len = strlen(str);
+
+ if (s->full)
+ return 0;
+
+ if (len > ((PAGE_SIZE - 1) - s->len)) {
+ s->full = 1;
+ return 0;
+ }
+
+ memcpy(s->buffer + s->len, str, len);
+ s->len += len;
+
+ return len;
+}
+
+int trace_seq_putc(struct trace_seq *s, unsigned char c)
+{
+ if (s->full)
+ return 0;
+
+ if (s->len >= (PAGE_SIZE - 1)) {
+ s->full = 1;
+ return 0;
+ }
+
+ s->buffer[s->len++] = c;
+
+ return 1;
+}
+EXPORT_SYMBOL(trace_seq_putc);
+
+int trace_seq_putmem(struct trace_seq *s, const void *mem, size_t len)
+{
+ if (s->full)
+ return 0;
+
+ if (len > ((PAGE_SIZE - 1) - s->len)) {
+ s->full = 1;
+ return 0;
+ }
+
+ memcpy(s->buffer + s->len, mem, len);
+ s->len += len;
+
+ return len;
+}
+
+#define HEX_CHARS (MAX_MEMHEX_BYTES*2 + 1)
+
+int trace_seq_putmem_hex(struct trace_seq *s, const void *mem, size_t len)
+{
+ unsigned char hex[HEX_CHARS];
+ const unsigned char *data = mem;
+ int i, j;
+
+ if (s->full)
+ return 0;
+
+#ifdef __BIG_ENDIAN
+ for (i = 0, j = 0; i < len; i++) {
+#else
+ for (i = len-1, j = 0; i >= 0; i--) {
+#endif
+ hex[j++] = hex_asc_hi(data[i]);
+ hex[j++] = hex_asc_lo(data[i]);
+ }
+ hex[j++] = ' ';
+
+ return trace_seq_putmem(s, hex, j);
+}
+
+void *trace_seq_reserve(struct trace_seq *s, size_t len)
+{
+ void *ret;
+
+ if (s->full)
+ return NULL;
+
+ if (len > ((PAGE_SIZE - 1) - s->len)) {
+ s->full = 1;
+ return NULL;
+ }
+
+ ret = s->buffer + s->len;
+ s->len += len;
+
+ return ret;
+}
+
+int trace_seq_path(struct trace_seq *s, const struct path *path)
+{
+ unsigned char *p;
+
+ if (s->full)
+ return 0;
+
+ if (s->len >= (PAGE_SIZE - 1)) {
+ s->full = 1;
+ return 0;
+ }
+
+ p = d_path(path, s->buffer + s->len, PAGE_SIZE - s->len);
+ if (!IS_ERR(p)) {
+ p = mangle_path(s->buffer + s->len, p, "\n");
+ if (p) {
+ s->len = p - s->buffer;
+ return 1;
+ }
+ } else {
+ s->buffer[s->len++] = '?';
+ return 1;
+ }
+
+ s->full = 1;
+ return 0;
+}
+
+ssize_t trace_seq_to_user(struct trace_seq *s, char __user *ubuf, size_t cnt)
+{
+ int len;
+ int ret;
+
+ if (!cnt)
+ return 0;
+
+ if (s->len <= s->readpos)
+ return -EBUSY;
+
+ len = s->len - s->readpos;
+ if (cnt > len)
+ cnt = len;
+ ret = copy_to_user(ubuf, s->buffer + s->readpos, cnt);
+ if (ret == cnt)
+ return -EFAULT;
+
+ cnt -= ret;
+
+ s->readpos += cnt;
+ return cnt;
+}
--
2.0.0.rc2
On Thu, Jun 19, 2014 at 11:33 AM, Steven Rostedt <[email protected]> wrote:
> From: "Steven Rostedt (Red Hat)" <[email protected]>
>
> The trace_seq functions are rather useful outside of tracing.
They are? Why and where? And why are they still called "trace_xyz()" then?
That commit message is useless and contains no actual information.
Linus
On Thu, 19 Jun 2014 17:33:30 -0400 Steven Rostedt <[email protected]> wrote:
> From: "Steven Rostedt (Red Hat)" <[email protected]>
>
> The trace_seq functions are rather useful outside of tracing. Instead
> of having it be dependent on CONFIG_TRACING, move the code into lib/
> and allow other users to have access to it even when tracing is not
> configured.
What LT said. It's pileon time!
> Signed-off-by: Steven Rostedt <[email protected]>
> ---
> include/linux/trace_seq.h | 68 ++--------
> kernel/trace/trace.c | 24 ----
> kernel/trace/trace_output.c | 268 ---------------------------------------
> kernel/trace/trace_output.h | 16 ---
> lib/Makefile | 2 +-
> lib/trace_seq.c | 303 ++++++++++++++++++++++++++++++++++++++++++++
Putting it in there makes me look at it ;)
> --- a/include/linux/trace_seq.h
> +++ b/include/linux/trace_seq.h
>
> ...
>
> +#define SEQ_PUT_FIELD_RET(s, x) \
> +do { \
> + if (!trace_seq_putmem(s, &(x), sizeof(x))) \
hm, does sizeof(x++) increment x? I guess it does.
> + return TRACE_TYPE_PARTIAL_LINE; \
> +} while (0)
>
>
> ...
>
> +#define SEQ_PUT_HEX_FIELD_RET(s, x) \
> +do { \
> + BUILD_BUG_ON(sizeof(x) > MAX_MEMHEX_BYTES); \
> + if (!trace_seq_putmem_hex(s, &(x), sizeof(x))) \
> + return TRACE_TYPE_PARTIAL_LINE; \
> +} while (0)
Also has side-effects.
> #endif /* _LINUX_TRACE_SEQ_H */
>
> ...
>
> --- /dev/null
> +++ b/lib/trace_seq.c
> @@ -0,0 +1,303 @@
> +/*
> + * trace_seq.c
> + *
> + * Copyright (C) 2008-2014 Red Hat Inc, Steven Rostedt <[email protected]>
> + *
> + */
> +#include <linux/uaccess.h>
> +#include <linux/seq_file.h>
> +#include <linux/trace_seq.h>
> +
> +int trace_print_seq(struct seq_file *m, struct trace_seq *s)
-ENODOC
> +{
> + int len = s->len >= PAGE_SIZE ? PAGE_SIZE - 1 : s->len;
int = uint >= ulong ? ulog : uint
that's spastic. Can we choose a type and stick to it?
Also, min().
> + int ret;
> +
> + ret = seq_write(m, s->buffer, len);
> +
> + /*
> + * Only reset this buffer if we successfully wrote to the
> + * seq_file buffer.
why?
> + */
> + if (!ret)
> + trace_seq_init(s);
> +
> + return ret;
> +}
> +
> +/**
> + * trace_seq_printf - sequence printing of trace information
> + * @s: trace sequence descriptor
> + * @fmt: printf format string
> + *
> + * It returns 0 if the trace oversizes the buffer's free
> + * space, 1 otherwise.
s/oversizes/would overrun/?
> + * The tracer may use either sequence operations or its own
> + * copy to user routines. To simplify formating of a trace
> + * trace_seq_printf is used to store strings into a special
> + * buffer (@s). Then the output may be either used by
> + * the sequencer or pulled into another buffer.
> + */
> +int
> +trace_seq_printf(struct trace_seq *s, const char *fmt, ...)
unneeded newline
> +{
> + int len = (PAGE_SIZE - 1) - s->len;
int = ulong - uint;
> + va_list ap;
> + int ret;
> +
> + if (s->full || !len)
> + return 0;
> +
> + va_start(ap, fmt);
> + ret = vsnprintf(s->buffer + s->len, len, fmt, ap);
> + va_end(ap);
> +
> + /* If we can't write it all, don't bother writing anything */
This is somewhat unusual behavior for a write()-style thing. Comment
should explain "why", not "what".
> + if (ret >= len) {
> + s->full = 1;
> + return 0;
> + }
> +
> + s->len += ret;
> +
> + return 1;
> +}
> +EXPORT_SYMBOL_GPL(trace_seq_printf);
> +
> +/**
> + * trace_seq_bitmask - put a list of longs as a bitmask print output
Is that grammatical?
> + * @s: trace sequence descriptor
> + * @maskp: points to an array of unsigned longs that represent a bitmask
> + * @nmaskbits: The number of bits that are valid in @maskp
> + *
> + * It returns 0 if the trace oversizes the buffer's free
> + * space, 1 otherwise.
Ditto
> + * Writes a ASCII representation of a bitmask string into @s.
> + */
> +int
> +trace_seq_bitmask(struct trace_seq *s, const unsigned long *maskp,
> + int nmaskbits)
> +{
> + int len = (PAGE_SIZE - 1) - s->len;
> + int ret;
> +
> + if (s->full || !len)
> + return 0;
> +
> + ret = bitmap_scnprintf(s->buffer, len, maskp, nmaskbits);
> + s->len += ret;
> +
> + return 1;
> +}
> +EXPORT_SYMBOL_GPL(trace_seq_bitmask);
More dittos.
> +/**
> + * trace_seq_vprintf - sequence printing of trace information
> + * @s: trace sequence descriptor
> + * @fmt: printf format string
> + *
> + * The tracer may use either sequence operations or its own
> + * copy to user routines. To simplify formating of a trace
> + * trace_seq_printf is used to store strings into a special
"trace_seq_printf()". Apparently it makes the kerneldoc output come
out right.
> + * buffer (@s). Then the output may be either used by
> + * the sequencer or pulled into another buffer.
> + */
> +int
> +trace_seq_vprintf(struct trace_seq *s, const char *fmt, va_list args)
> +{
> + int len = (PAGE_SIZE - 1) - s->len;
> + int ret;
> +
> + if (s->full || !len)
> + return 0;
> +
> + ret = vsnprintf(s->buffer + s->len, len, fmt, args);
> +
> + /* If we can't write it all, don't bother writing anything */
> + if (ret >= len) {
> + s->full = 1;
> + return 0;
> + }
> +
> + s->len += ret;
> +
> + return len;
> +}
> +EXPORT_SYMBOL_GPL(trace_seq_vprintf);
Several dittos.
> +int trace_seq_bprintf(struct trace_seq *s, const char *fmt, const u32 *binary)
-ENODOC
> +{
> + int len = (PAGE_SIZE - 1) - s->len;
> + int ret;
> +
> + if (s->full || !len)
> + return 0;
> +
> + ret = bstr_printf(s->buffer + s->len, len, fmt, binary);
> +
> + /* If we can't write it all, don't bother writing anything */
> + if (ret >= len) {
> + s->full = 1;
> + return 0;
> + }
> +
> + s->len += ret;
> +
> + return len;
> +}
Dittos.
> +/**
> + * trace_seq_puts - trace sequence printing of simple string
> + * @s: trace sequence descriptor
> + * @str: simple string to record
> + *
> + * The tracer may use either the sequence operations or its own
> + * copy to user routines. This function records a simple string
> + * into a special buffer (@s) for later retrieval by a sequencer
> + * or other mechanism.
> + */
> +int trace_seq_puts(struct trace_seq *s, const char *str)
> +{
> + int len = strlen(str);
> +
> + if (s->full)
> + return 0;
> +
> + if (len > ((PAGE_SIZE - 1) - s->len)) {
> + s->full = 1;
> + return 0;
> + }
> +
> + memcpy(s->buffer + s->len, str, len);
> + s->len += len;
> +
> + return len;
> +}
Missing EXPORT_SYMBOL?
> +int trace_seq_putc(struct trace_seq *s, unsigned char c)
> +{
> + if (s->full)
> + return 0;
> +
> + if (s->len >= (PAGE_SIZE - 1)) {
> + s->full = 1;
> + return 0;
> + }
> +
> + s->buffer[s->len++] = c;
> +
> + return 1;
> +}
> +EXPORT_SYMBOL(trace_seq_putc);
Mix of EXPORT_SYMBOL() and EXPORT_SYMBOL_GPL()
> +int trace_seq_putmem(struct trace_seq *s, const void *mem, size_t len)
> +{
> + if (s->full)
> + return 0;
> +
> + if (len > ((PAGE_SIZE - 1) - s->len)) {
> + s->full = 1;
> + return 0;
> + }
> +
> + memcpy(s->buffer + s->len, mem, len);
> + s->len += len;
> +
> + return len;
> +}
> +
Lotsa dittos
> +#define HEX_CHARS (MAX_MEMHEX_BYTES*2 + 1)
> +
> +int trace_seq_putmem_hex(struct trace_seq *s, const void *mem, size_t len)
> +{
> + unsigned char hex[HEX_CHARS];
> + const unsigned char *data = mem;
> + int i, j;
> +
> + if (s->full)
> + return 0;
What's this ->full thing all about anyway? Some central comment which
explains the design is needed.
Is this test really needed? trace_seq_putmem() will handle this.
> +#ifdef __BIG_ENDIAN
> + for (i = 0, j = 0; i < len; i++) {
> +#else
> + for (i = len-1, j = 0; i >= 0; i--) {
> +#endif
> + hex[j++] = hex_asc_hi(data[i]);
> + hex[j++] = hex_asc_lo(data[i]);
> + }
> + hex[j++] = ' ';
> +
> + return trace_seq_putmem(s, hex, j);
> +}
-ENODOC, missing EXPORT_SYMBOL.
> +void *trace_seq_reserve(struct trace_seq *s, size_t len)
`len' is a size_t here, a uint in trace_seq and an int when it's a local.
> +{
> + void *ret;
> +
> + if (s->full)
> + return NULL;
> +
> + if (len > ((PAGE_SIZE - 1) - s->len)) {
> + s->full = 1;
> + return NULL;
> + }
> +
> + ret = s->buffer + s->len;
> + s->len += len;
> +
> + return ret;
> +}
Dittos
> +int trace_seq_path(struct trace_seq *s, const struct path *path)
> +{
> + unsigned char *p;
> +
> + if (s->full)
> + return 0;
> +
> + if (s->len >= (PAGE_SIZE - 1)) {
> + s->full = 1;
> + return 0;
> + }
> +
> + p = d_path(path, s->buffer + s->len, PAGE_SIZE - s->len);
> + if (!IS_ERR(p)) {
> + p = mangle_path(s->buffer + s->len, p, "\n");
> + if (p) {
> + s->len = p - s->buffer;
> + return 1;
> + }
> + } else {
> + s->buffer[s->len++] = '?';
> + return 1;
> + }
> +
> + s->full = 1;
> + return 0;
> +}
Dittos
> +ssize_t trace_seq_to_user(struct trace_seq *s, char __user *ubuf, size_t cnt)
> +{
> + int len;
> + int ret;
> +
> + if (!cnt)
> + return 0;
> +
> + if (s->len <= s->readpos)
> + return -EBUSY;
> +
> + len = s->len - s->readpos;
> + if (cnt > len)
> + cnt = len;
> + ret = copy_to_user(ubuf, s->buffer + s->readpos, cnt);
> + if (ret == cnt)
> + return -EFAULT;
> +
> + cnt -= ret;
> +
> + s->readpos += cnt;
> + return cnt;
> +}
Dittos
On Thu, 19 Jun 2014 18:45:57 -1000
Linus Torvalds <[email protected]> wrote:
> On Thu, Jun 19, 2014 at 11:33 AM, Steven Rostedt <[email protected]> wrote:
> > From: "Steven Rostedt (Red Hat)" <[email protected]>
> >
> > The trace_seq functions are rather useful outside of tracing.
>
> They are?
They will be.
> Why and where?
Why? for NMI safe output.
Where? Well, in patch 3. :-)
> And why are they still called "trace_xyz()" then?
Good point, I'd like to rename it too. It was designed like seq_file,
so what about seq_buffer_xxx()?
>
> That commit message is useless and contains no actual information.
Yeah, it was rather weak. It was quick and not a stand alone. It was
dependent on reading the rest of the patches.
OK, I can update the name and make a better change log on the move.
Of course if you hate this patch series, I can just not waste any time
on it.
-- Steve
On Thu, 19 Jun 2014 22:06:07 -0700
Andrew Morton <[email protected]> wrote:
> On Thu, 19 Jun 2014 17:33:30 -0400 Steven Rostedt <[email protected]> wrote:
>
> > From: "Steven Rostedt (Red Hat)" <[email protected]>
> >
> > The trace_seq functions are rather useful outside of tracing. Instead
> > of having it be dependent on CONFIG_TRACING, move the code into lib/
> > and allow other users to have access to it even when tracing is not
> > configured.
>
> What LT said. It's pileon time!
>
> > Signed-off-by: Steven Rostedt <[email protected]>
> > ---
> > include/linux/trace_seq.h | 68 ++--------
> > kernel/trace/trace.c | 24 ----
> > kernel/trace/trace_output.c | 268 ---------------------------------------
> > kernel/trace/trace_output.h | 16 ---
> > lib/Makefile | 2 +-
> > lib/trace_seq.c | 303 ++++++++++++++++++++++++++++++++++++++++++++
>
> Putting it in there makes me look at it ;)
Damn it! If I had known this, I would have moved it there on day one.
You can't say you didn't know about this. I cc you on all my patches :-)
So next time, to get you to review my code, I just need to threaten to
add it to lib?
>
> > --- a/include/linux/trace_seq.h
> > +++ b/include/linux/trace_seq.h
> >
> > ...
> >
> > +#define SEQ_PUT_FIELD_RET(s, x) \
> > +do { \
> > + if (!trace_seq_putmem(s, &(x), sizeof(x))) \
>
> hm, does sizeof(x++) increment x? I guess it does.
I hate this macro. I moved it just to make the kernel build. I would
love to change it. It is more to do with tracing than trace_seq anyway.
>
> > + return TRACE_TYPE_PARTIAL_LINE; \
> > +} while (0)
> >
> >
> > ...
> >
> > +#define SEQ_PUT_HEX_FIELD_RET(s, x) \
> > +do { \
> > + BUILD_BUG_ON(sizeof(x) > MAX_MEMHEX_BYTES); \
> > + if (!trace_seq_putmem_hex(s, &(x), sizeof(x))) \
> > + return TRACE_TYPE_PARTIAL_LINE; \
> > +} while (0)
>
> Also has side-effects.
Yep, and it shouldn't be in the lib anyway. Again, HATE IT!
>
> > #endif /* _LINUX_TRACE_SEQ_H */
> >
> > ...
> >
> > --- /dev/null
> > +++ b/lib/trace_seq.c
> > @@ -0,0 +1,303 @@
> > +/*
> > + * trace_seq.c
> > + *
> > + * Copyright (C) 2008-2014 Red Hat Inc, Steven Rostedt <[email protected]>
> > + *
> > + */
> > +#include <linux/uaccess.h>
> > +#include <linux/seq_file.h>
> > +#include <linux/trace_seq.h>
> > +
> > +int trace_print_seq(struct seq_file *m, struct trace_seq *s)
>
> -ENODOC
Heh, I noticed this too, and almost sent out a 2/4 patch that added the
docs. I should add it first before moving it. But I was lazy and sent
this out because I was more interested in the NMI code feedback than
this.
>
> > +{
> > + int len = s->len >= PAGE_SIZE ? PAGE_SIZE - 1 : s->len;
>
> int = uint >= ulong ? ulog : uint
>
> that's spastic. Can we choose a type and stick to it?
Ah, yeah that should be fixed.
>
> Also, min().
Right.
>
> > + int ret;
> > +
> > + ret = seq_write(m, s->buffer, len);
> > +
> > + /*
> > + * Only reset this buffer if we successfully wrote to the
> > + * seq_file buffer.
>
> why?
Because we might want to try again. I'll update the comment.
>
> > + */
> > + if (!ret)
> > + trace_seq_init(s);
> > +
> > + return ret;
> > +}
> > +
> > +/**
> > + * trace_seq_printf - sequence printing of trace information
> > + * @s: trace sequence descriptor
> > + * @fmt: printf format string
> > + *
> > + * It returns 0 if the trace oversizes the buffer's free
> > + * space, 1 otherwise.
>
> s/oversizes/would overrun/?
It made sense when I first read it (Jiri wrote it). How about:
Returns 0 if the recorded print is bigger than the free space
of the buffer. Returns 1 otherwise.
>
> > + * The tracer may use either sequence operations or its own
> > + * copy to user routines. To simplify formating of a trace
> > + * trace_seq_printf is used to store strings into a special
> > + * buffer (@s). Then the output may be either used by
> > + * the sequencer or pulled into another buffer.
> > + */
> > +int
> > +trace_seq_printf(struct trace_seq *s, const char *fmt, ...)
>
> unneeded newline
OK.
>
> > +{
> > + int len = (PAGE_SIZE - 1) - s->len;
>
> int = ulong - uint;
Maybe it should just be initialized via a macro too?
>
> > + va_list ap;
> > + int ret;
> > +
> > + if (s->full || !len)
> > + return 0;
> > +
> > + va_start(ap, fmt);
> > + ret = vsnprintf(s->buffer + s->len, len, fmt, ap);
> > + va_end(ap);
> > +
> > + /* If we can't write it all, don't bother writing anything */
>
> This is somewhat unusual behavior for a write()-style thing. Comment
> should explain "why", not "what".
Yeah. It's like that because this was driven by the code that calls it,
and is pretty engrained in the process. But taking it out of it's
natural habitat, we need to explain these strange rituals that it does.
The why is, this is a buffer write, and users of it expect it to be all
or nothing, because a partial write is usually solved by flushing and
trying again with the same write arguments. If we did partial writes,
then the user would need another buffer to store the full write to be
able to just write out the rest of the line. That basically destroys
the point of trace_seq as it is suppose to be that extra buffer.
>
> > + if (ret >= len) {
> > + s->full = 1;
> > + return 0;
> > + }
> > +
> > + s->len += ret;
> > +
> > + return 1;
> > +}
> > +EXPORT_SYMBOL_GPL(trace_seq_printf);
> > +
> > +/**
> > + * trace_seq_bitmask - put a list of longs as a bitmask print output
>
> Is that grammatical?
No but it's almost a haiku:
trace_seq_bitmask -
put a list of longs as a
bitmask print output
>
> > + * @s: trace sequence descriptor
> > + * @maskp: points to an array of unsigned longs that represent a bitmask
> > + * @nmaskbits: The number of bits that are valid in @maskp
> > + *
> > + * It returns 0 if the trace oversizes the buffer's free
> > + * space, 1 otherwise.
>
> Ditto
The beauty of cut-and-paste.
>
> > + * Writes a ASCII representation of a bitmask string into @s.
> > + */
> > +int
> > +trace_seq_bitmask(struct trace_seq *s, const unsigned long *maskp,
> > + int nmaskbits)
> > +{
> > + int len = (PAGE_SIZE - 1) - s->len;
> > + int ret;
> > +
> > + if (s->full || !len)
> > + return 0;
> > +
> > + ret = bitmap_scnprintf(s->buffer, len, maskp, nmaskbits);
> > + s->len += ret;
> > +
> > + return 1;
> > +}
> > +EXPORT_SYMBOL_GPL(trace_seq_bitmask);
>
> More dittos.
Confused. What dittos is this dittoing?
>
> > +/**
> > + * trace_seq_vprintf - sequence printing of trace information
> > + * @s: trace sequence descriptor
> > + * @fmt: printf format string
> > + *
> > + * The tracer may use either sequence operations or its own
> > + * copy to user routines. To simplify formating of a trace
> > + * trace_seq_printf is used to store strings into a special
>
> "trace_seq_printf()". Apparently it makes the kerneldoc output come
> out right.
OK.
>
> > + * buffer (@s). Then the output may be either used by
> > + * the sequencer or pulled into another buffer.
> > + */
> > +int
> > +trace_seq_vprintf(struct trace_seq *s, const char *fmt, va_list args)
> > +{
> > + int len = (PAGE_SIZE - 1) - s->len;
> > + int ret;
> > +
> > + if (s->full || !len)
> > + return 0;
> > +
> > + ret = vsnprintf(s->buffer + s->len, len, fmt, args);
> > +
> > + /* If we can't write it all, don't bother writing anything */
> > + if (ret >= len) {
> > + s->full = 1;
> > + return 0;
> > + }
> > +
> > + s->len += ret;
> > +
> > + return len;
> > +}
> > +EXPORT_SYMBOL_GPL(trace_seq_vprintf);
>
> Several dittos.
Oh, just on the function. You're not dittoing a comment about the
EXPORT_SYMBOL_GPL() that you forgot to add, are you?
>
> > +int trace_seq_bprintf(struct trace_seq *s, const char *fmt, const u32 *binary)
>
> -ENODOC
Ditto
>
> > +{
> > + int len = (PAGE_SIZE - 1) - s->len;
> > + int ret;
> > +
> > + if (s->full || !len)
> > + return 0;
> > +
> > + ret = bstr_printf(s->buffer + s->len, len, fmt, binary);
> > +
> > + /* If we can't write it all, don't bother writing anything */
> > + if (ret >= len) {
> > + s->full = 1;
> > + return 0;
> > + }
> > +
> > + s->len += ret;
> > +
> > + return len;
> > +}
>
> Dittos.
Dittos
>
> > +/**
> > + * trace_seq_puts - trace sequence printing of simple string
> > + * @s: trace sequence descriptor
> > + * @str: simple string to record
> > + *
> > + * The tracer may use either the sequence operations or its own
> > + * copy to user routines. This function records a simple string
> > + * into a special buffer (@s) for later retrieval by a sequencer
> > + * or other mechanism.
> > + */
> > +int trace_seq_puts(struct trace_seq *s, const char *str)
> > +{
> > + int len = strlen(str);
> > +
> > + if (s->full)
> > + return 0;
> > +
> > + if (len > ((PAGE_SIZE - 1) - s->len)) {
> > + s->full = 1;
> > + return 0;
> > + }
> > +
> > + memcpy(s->buffer + s->len, str, len);
> > + s->len += len;
> > +
> > + return len;
> > +}
>
> Missing EXPORT_SYMBOL?
The others were used by the tracepoint infrastructure. But I'm not even
sure they are needed anymore because the ftrace.h file now calls
another function that does more work which is exported and then calls
these.
But if I'm going to add this to the library, they probably all should
be exported.
>
> > +int trace_seq_putc(struct trace_seq *s, unsigned char c)
> > +{
> > + if (s->full)
> > + return 0;
> > +
> > + if (s->len >= (PAGE_SIZE - 1)) {
> > + s->full = 1;
> > + return 0;
> > + }
> > +
> > + s->buffer[s->len++] = c;
> > +
> > + return 1;
> > +}
> > +EXPORT_SYMBOL(trace_seq_putc);
>
> Mix of EXPORT_SYMBOL() and EXPORT_SYMBOL_GPL()
That's a mistake. They all should be EXPORT_SYMBOL_GPL(). Hmm, I caught
this once before and made a patch to fix it. That got lost somehow :-/
>
> > +int trace_seq_putmem(struct trace_seq *s, const void *mem, size_t len)
> > +{
> > + if (s->full)
> > + return 0;
> > +
> > + if (len > ((PAGE_SIZE - 1) - s->len)) {
> > + s->full = 1;
> > + return 0;
> > + }
> > +
> > + memcpy(s->buffer + s->len, mem, len);
> > + s->len += len;
> > +
> > + return len;
> > +}
> > +
>
> Lotsa dittos
Ditto back at'cha
>
> > +#define HEX_CHARS (MAX_MEMHEX_BYTES*2 + 1)
> > +
> > +int trace_seq_putmem_hex(struct trace_seq *s, const void *mem, size_t len)
> > +{
> > + unsigned char hex[HEX_CHARS];
> > + const unsigned char *data = mem;
> > + int i, j;
> > +
> > + if (s->full)
> > + return 0;
>
> What's this ->full thing all about anyway? Some central comment which
> explains the design is needed.
Comment? What? Git blame isn't good enough for ya? ;-)
>
> Is this test really needed? trace_seq_putmem() will handle this.
It was added as an optimization, because once it filled up, you could
still have multiple calls to the trace_seq() functions that would waste
time trying to write the buffer.
It seemed like a good idea at the time. I Cc'd Johannes Berg as he's
the one that implemented.
Johannes, is this really needed, should we bother keeping it?
>
> > +#ifdef __BIG_ENDIAN
> > + for (i = 0, j = 0; i < len; i++) {
> > +#else
> > + for (i = len-1, j = 0; i >= 0; i--) {
> > +#endif
> > + hex[j++] = hex_asc_hi(data[i]);
> > + hex[j++] = hex_asc_lo(data[i]);
> > + }
> > + hex[j++] = ' ';
> > +
> > + return trace_seq_putmem(s, hex, j);
> > +}
>
> -ENODOC, missing EXPORT_SYMBOL.
Ditto.
>
> > +void *trace_seq_reserve(struct trace_seq *s, size_t len)
>
> `len' is a size_t here, a uint in trace_seq and an int when it's a local.
We like to be diverse.
>
> > +{
> > + void *ret;
> > +
> > + if (s->full)
> > + return NULL;
> > +
> > + if (len > ((PAGE_SIZE - 1) - s->len)) {
> > + s->full = 1;
> > + return NULL;
> > + }
> > +
> > + ret = s->buffer + s->len;
> > + s->len += len;
> > +
> > + return ret;
> > +}
>
> Dittos
Return dittos
>
> > +int trace_seq_path(struct trace_seq *s, const struct path *path)
> > +{
> > + unsigned char *p;
> > +
> > + if (s->full)
> > + return 0;
> > +
> > + if (s->len >= (PAGE_SIZE - 1)) {
> > + s->full = 1;
> > + return 0;
> > + }
> > +
> > + p = d_path(path, s->buffer + s->len, PAGE_SIZE - s->len);
> > + if (!IS_ERR(p)) {
> > + p = mangle_path(s->buffer + s->len, p, "\n");
> > + if (p) {
> > + s->len = p - s->buffer;
> > + return 1;
> > + }
> > + } else {
> > + s->buffer[s->len++] = '?';
> > + return 1;
> > + }
> > +
> > + s->full = 1;
> > + return 0;
> > +}
>
> Dittos
sottiD
>
> > +ssize_t trace_seq_to_user(struct trace_seq *s, char __user *ubuf, size_t cnt)
> > +{
> > + int len;
> > + int ret;
> > +
> > + if (!cnt)
> > + return 0;
> > +
> > + if (s->len <= s->readpos)
> > + return -EBUSY;
> > +
> > + len = s->len - s->readpos;
> > + if (cnt > len)
> > + cnt = len;
> > + ret = copy_to_user(ubuf, s->buffer + s->readpos, cnt);
> > + if (ret == cnt)
> > + return -EFAULT;
> > +
> > + cnt -= ret;
> > +
> > + s->readpos += cnt;
> > + return cnt;
> > +}
>
> Dittos
Tripple ditto!
Hey! Thanks for the review. Much appreciated. And maybe you should read
those messages in your /dev/null folder that I cc you with. :-)
-- Steve
On Fri, 20 Jun 2014 12:58:23 -0400 Steven Rostedt <[email protected]> wrote:
>
> ...
>
> >
> > > + * Writes a ASCII representation of a bitmask string into @s.
> > > + */
> > > +int
> > > +trace_seq_bitmask(struct trace_seq *s, const unsigned long *maskp,
> > > + int nmaskbits)
> > > +{
> > > + int len = (PAGE_SIZE - 1) - s->len;
> > > + int ret;
> > > +
> > > + if (s->full || !len)
> > > + return 0;
> > > +
> > > + ret = bitmap_scnprintf(s->buffer, len, maskp, nmaskbits);
> > > + s->len += ret;
> > > +
> > > + return 1;
> > > +}
> > > +EXPORT_SYMBOL_GPL(trace_seq_bitmask);
> >
> > More dittos.
>
> Confused. What dittos is this dittoing?
Unneeded newline, poorly considered choice of types.
>
> ...
>
> > > + * buffer (@s). Then the output may be either used by
> > > + * the sequencer or pulled into another buffer.
> > > + */
> > > +int
> > > +trace_seq_vprintf(struct trace_seq *s, const char *fmt, va_list args)
> > > +{
> > > + int len = (PAGE_SIZE - 1) - s->len;
> > > + int ret;
> > > +
> > > + if (s->full || !len)
> > > + return 0;
> > > +
> > > + ret = vsnprintf(s->buffer + s->len, len, fmt, args);
> > > +
> > > + /* If we can't write it all, don't bother writing anything */
> > > + if (ret >= len) {
> > > + s->full = 1;
> > > + return 0;
> > > + }
> > > +
> > > + s->len += ret;
> > > +
> > > + return len;
> > > +}
> > > +EXPORT_SYMBOL_GPL(trace_seq_vprintf);
> >
> > Several dittos.
>
> Oh, just on the function. You're not dittoing a comment about the
> EXPORT_SYMBOL_GPL() that you forgot to add, are you?
yup. Unneded newline, types, EXPORT_ confusion.
>
> ...
>
> >
> > > +#define HEX_CHARS (MAX_MEMHEX_BYTES*2 + 1)
> > > +
> > > +int trace_seq_putmem_hex(struct trace_seq *s, const void *mem, size_t len)
> > > +{
> > > + unsigned char hex[HEX_CHARS];
> > > + const unsigned char *data = mem;
> > > + int i, j;
> > > +
> > > + if (s->full)
> > > + return 0;
> >
> > What's this ->full thing all about anyway? Some central comment which
> > explains the design is needed.
>
> Comment? What? Git blame isn't good enough for ya? ;-)
There's always that. There's also googling for the original list
dicsussion. But it's a bit user-unfriendly, particularly when then
code has aged was subsequently altered many times.
>
> ...
>
>
> Hey! Thanks for the review. Much appreciated. And maybe you should read
> those messages in your /dev/null folder that I cc you with. :-)
I sometimes do.
On Fri, 20 Jun 2014 10:12:44 -0700
Andrew Morton <[email protected]> wrote:
> > Comment? What? Git blame isn't good enough for ya? ;-)
>
> There's always that. There's also googling for the original list
> dicsussion. But it's a bit user-unfriendly, particularly when then
> code has aged was subsequently altered many times.
I was replying mostly in jest as that was what I did to remember why we
added it.
>
> >
> > ...
> >
> >
> > Hey! Thanks for the review. Much appreciated. And maybe you should read
> > those messages in your /dev/null folder that I cc you with. :-)
>
> I sometimes do.
Ah, the lurker.
-- Steve
On Fri, 20 Jun 2014 10:12:44 -0700
Andrew Morton <[email protected]> wrote:
> > > > +#define HEX_CHARS (MAX_MEMHEX_BYTES*2 + 1)
> > > > +
> > > > +int trace_seq_putmem_hex(struct trace_seq *s, const void *mem, size_t len)
> > > > +{
> > > > + unsigned char hex[HEX_CHARS];
> > > > + const unsigned char *data = mem;
> > > > + int i, j;
> > > > +
> > > > + if (s->full)
> > > > + return 0;
> > >
> > > What's this ->full thing all about anyway? Some central comment which
> > > explains the design is needed.
> >
> > Comment? What? Git blame isn't good enough for ya? ;-)
>
> There's always that. There's also googling for the original list
> dicsussion. But it's a bit user-unfriendly, particularly when then
> code has aged was subsequently altered many times.
Although I did not address this because I'm waiting to hear back from
Johannes Berg, I updated for your other comments. I hope I got them all.
Regardless of this patch series, I pulled out the code from
trace_output.c and made a separate file for the trace_seq_*() functions
in kernel/trace/trace_seq.c. I then updated the file with your
comments. I found a bug or two and I will be dealing with them later as
this will only be a clean up patch not a bug fix patch.
Anyway, here's the new file:
-- Steve
/*
* trace_seq.c
*
* Copyright (C) 2008-2014 Red Hat Inc, Steven Rostedt <[email protected]>
*
* The trace_seq is a handy tool that allows you to pass a descriptor around
* to a buffer that other functions can write to. It is similar to the
* seq_file functionality but has some differences.
*
* To use it, the trace_seq must be initialized with trace_seq_init().
* This will set up the counters within the descriptor. You can call
* trace_seq_init() more than once to reset the trace_seq to start
* from scratch.
*
* The buffer size is currently PAGE_SIZE, although it may become dynamic
* in the future.
*
* A write to the buffer will either succed or fail. That is, unlike
* sprintf() there will not be a partial write (well it may write into
* the buffer but it wont update the pointers). This allows users to
* try to write something into the trace_seq buffer and if it fails
* they can flush it and try again.
*
*/
#include <linux/uaccess.h>
#include <linux/seq_file.h>
#include <linux/trace_seq.h>
/* How much buffer is left on the trace_seq? */
#define TRACE_SEQ_BUF_LEFT(s) ((PAGE_SIZE - 1) - (s)->len)
/* How much buffer is written? */
#define TRACE_SEQ_BUF_USED(s) min((s)->len, PAGE_SIZE - 1)
/**
* trace_print_seq - move the contents of trace_seq into a seq_file
* @m: the seq_file descriptor that is the destination
* @s: the trace_seq descriptor that is the source.
*
* Returns 0 on success and non zero on error. If it succeeds to
* write to the seq_file it will reset the trace_seq, otherwise
* it does not modify the trace_seq to let the caller try again.
*/
int trace_print_seq(struct seq_file *m, struct trace_seq *s)
{
unsigned int len = TRACE_SEQ_BUF_USED(s);
int ret;
ret = seq_write(m, s->buffer, len);
/*
* Only reset this buffer if we successfully wrote to the
* seq_file buffer. This lets the caller try again or
* do something else with the contents.
*/
if (!ret)
trace_seq_init(s);
return ret;
}
/**
* trace_seq_printf - sequence printing of trace information
* @s: trace sequence descriptor
* @fmt: printf format string
*
* The tracer may use either sequence operations or its own
* copy to user routines. To simplify formating of a trace
* trace_seq_printf() is used to store strings into a special
* buffer (@s). Then the output may be either used by
* the sequencer or pulled into another buffer.
*
* Returns 1 if we successfully written all the contents to
* the buffer.
* Returns 0 if we the length to write is bigger than the
* reserved buffer space. In this case, nothing gets written.
*/
int trace_seq_printf(struct trace_seq *s, const char *fmt, ...)
{
unsigned int len = TRACE_SEQ_BUF_LEFT(s);
va_list ap;
int ret;
if (s->full || !len)
return 0;
va_start(ap, fmt);
ret = vsnprintf(s->buffer + s->len, len, fmt, ap);
va_end(ap);
/* If we can't write it all, don't bother writing anything */
if (ret >= len) {
s->full = 1;
return 0;
}
s->len += ret;
return 1;
}
EXPORT_SYMBOL_GPL(trace_seq_printf);
/**
* trace_seq_bitmask - write a bitmask array in its ASCII representation
* @s: trace sequence descriptor
* @maskp: points to an array of unsigned longs that represent a bitmask
* @nmaskbits: The number of bits that are valid in @maskp
*
* Writes a ASCII representation of a bitmask string into @s.
*
* Returns 1 if we successfully written all the contents to
* the buffer.
* Returns 0 if we the length to write is bigger than the
* reserved buffer space. In this case, nothing gets written.
*/
int trace_seq_bitmask(struct trace_seq *s, const unsigned long *maskp,
int nmaskbits)
{
unsigned int len = TRACE_SEQ_BUF_LEFT(s);
int ret;
if (s->full || !len)
return 0;
ret = bitmap_scnprintf(s->buffer, len, maskp, nmaskbits);
s->len += ret;
return 1;
}
EXPORT_SYMBOL_GPL(trace_seq_bitmask);
/**
* trace_seq_vprintf - sequence printing of trace information
* @s: trace sequence descriptor
* @fmt: printf format string
*
* The tracer may use either sequence operations or its own
* copy to user routines. To simplify formating of a trace
* trace_seq_printf is used to store strings into a special
* buffer (@s). Then the output may be either used by
* the sequencer or pulled into another buffer.
*
* Returns 1 if we successfully written all the contents to
* the buffer.
* Returns 0 if we the length to write is bigger than the
* reserved buffer space. In this case, nothing gets written.
*/
int trace_seq_vprintf(struct trace_seq *s, const char *fmt, va_list args)
{
unsigned int len = TRACE_SEQ_BUF_LEFT(s);
int ret;
if (s->full || !len)
return 0;
ret = vsnprintf(s->buffer + s->len, len, fmt, args);
/* If we can't write it all, don't bother writing anything */
if (ret >= len) {
s->full = 1;
return 0;
}
s->len += ret;
return len;
}
EXPORT_SYMBOL_GPL(trace_seq_vprintf);
/**
* trace_seq_bprintf - Write the printf string from binary arguments
* @s: trace sequence descriptor
* @fmt: The format string for the @binary arguments
* @binary: The binary arguments for @fmt.
*
* When recording in a fast path, a printf may be recorded with just
* saving the format and the arguments as they were passed to the
* function, instead of wasting cycles converting the arguments into
* ASCII characters. Instead, the arguments are saved in a 32 bit
* word array that is defined by the format string constraints.
*
* This function will take the format and the binary array and finish
* the conversion into the ASCII string within the buffer.
*
* Returns 1 if we successfully written all the contents to
* the buffer.
* Returns 0 if we the length to write is bigger than the
* reserved buffer space. In this case, nothing gets written.
*/
int trace_seq_bprintf(struct trace_seq *s, const char *fmt, const u32 *binary)
{
unsigned int len = TRACE_SEQ_BUF_LEFT(s);
int ret;
if (s->full || !len)
return 0;
ret = bstr_printf(s->buffer + s->len, len, fmt, binary);
/* If we can't write it all, don't bother writing anything */
if (ret >= len) {
s->full = 1;
return 0;
}
s->len += ret;
return len;
}
EXPORT_SYMBOL_GPL(trace_seq_bprintf);
/**
* trace_seq_puts - trace sequence printing of simple string
* @s: trace sequence descriptor
* @str: simple string to record
*
* The tracer may use either the sequence operations or its own
* copy to user routines. This function records a simple string
* into a special buffer (@s) for later retrieval by a sequencer
* or other mechanism.
*
* Returns 1 if we successfully written all the contents to
* the buffer.
* Returns 0 if we the length to write is bigger than the
* reserved buffer space. In this case, nothing gets written.
*/
int trace_seq_puts(struct trace_seq *s, const char *str)
{
unsigned int len = strlen(str);
if (s->full)
return 0;
if (len > TRACE_SEQ_BUF_LEFT(s)) {
s->full = 1;
return 0;
}
memcpy(s->buffer + s->len, str, len);
s->len += len;
return len;
}
EXPORT_SYMBOL_GPL(trace_seq_puts);
/**
* trace_seq_putc - trace sequence printing of simple character
* @s: trace sequence descriptor
* @c: simple character to record
*
* The tracer may use either the sequence operations or its own
* copy to user routines. This function records a simple charater
* into a special buffer (@s) for later retrieval by a sequencer
* or other mechanism.
*
* Returns 1 if we successfully written all the contents to
* the buffer.
* Returns 0 if we the length to write is bigger than the
* reserved buffer space. In this case, nothing gets written.
*/
int trace_seq_putc(struct trace_seq *s, unsigned char c)
{
if (s->full)
return 0;
if (TRACE_SEQ_BUF_LEFT(s) < 1) {
s->full = 1;
return 0;
}
s->buffer[s->len++] = c;
return 1;
}
EXPORT_SYMBOL_GPL(trace_seq_putc);
/**
* trace_seq_putmem - write raw data into the trace_seq buffer
* @s: trace sequence descriptor
* @mem: The raw memory to copy into the buffer
* @len: The length of the raw memory to copy (in bytes)
*
* There may be cases where raw memory needs to be written into the
* buffer and a strcpy() would not work. Using this function allows
* for such cases.
*
* Returns 1 if we successfully written all the contents to
* the buffer.
* Returns 0 if we the length to write is bigger than the
* reserved buffer space. In this case, nothing gets written.
*/
int trace_seq_putmem(struct trace_seq *s, const void *mem, unsigned len len)
{
if (s->full)
return 0;
if (len > TRACE_SEQ_BUF_LEFT(s)) {
s->full = 1;
return 0;
}
memcpy(s->buffer + s->len, mem, len);
s->len += len;
return len;
}
EXPORT_SYMBOL_GPL(trace_seq_putmem);
#define HEX_CHARS (MAX_MEMHEX_BYTES*2 + 1)
/**
* trace_seq_putmem_hex - write raw memory into the buffer in ASCII hex
* @s: trace sequence descriptor
* @mem: The raw memory to write its hex ASCII representation of
* @len: The length of the raw memory to copy (in bytes)
*
* This is similar to trace_seq_putmem() except instead of just copying the
* raw memory into the buffer it writes its ASCII representation of it
* in hex characters.
*
* Returns 1 if we successfully written all the contents to
* the buffer.
* Returns 0 if we the length to write is bigger than the
* reserved buffer space. In this case, nothing gets written.
*/
int trace_seq_putmem_hex(struct trace_seq *s, const void *mem,
unsigned int len)
{
unsigned char hex[HEX_CHARS];
const unsigned char *data = mem;
int i, j;
if (s->full)
return 0;
#ifdef __BIG_ENDIAN
for (i = 0, j = 0; i < len; i++) {
#else
for (i = len-1, j = 0; i >= 0; i--) {
#endif
hex[j++] = hex_asc_hi(data[i]);
hex[j++] = hex_asc_lo(data[i]);
}
hex[j++] = ' ';
return trace_seq_putmem(s, hex, j);
}
EXPORT_SYMBOL_GPL(trace_seq_putmem_hex);
/**
* trace_seq_reserve - reserve space on the sequence buffer
* @s: trace sequence descriptor
* @len: The amount to reserver.
*
* If for some reason there is a need to save some space on the
* buffer to fill in later, this function is used for that purpose.
* The given length will be reserved and the pointer to that
* location on the buffer is returned, unless there is not enough
* buffer left to hold the given length then NULL is returned.
*/
void *trace_seq_reserve(struct trace_seq *s, unsigned int len)
{
void *ret;
if (s->full)
return NULL;
if (len > TRACE_SEQ_BUF_LEFT(s)) {
s->full = 1;
return NULL;
}
ret = s->buffer + s->len;
s->len += len;
return ret;
}
/**
* trace_seq_path - copy a path into the sequence buffer
* @s: trace sequence descriptor
* @path: path to write into the sequence buffer.
*
* Write a path name into the sequence buffer.
*
* Returns 1 if we successfully written all the contents to
* the buffer.
* Returns 0 if we the length to write is bigger than the
* reserved buffer space. In this case, nothing gets written.
*/
int trace_seq_path(struct trace_seq *s, const struct path *path)
{
unsigned char *p;
if (s->full)
return 0;
if (TRACE_SEQ_BUF_LEFT(s) < 1) {
s->full = 1;
return 0;
}
p = d_path(path, s->buffer + s->len, PAGE_SIZE - s->len);
if (!IS_ERR(p)) {
p = mangle_path(s->buffer + s->len, p, "\n");
if (p) {
s->len = p - s->buffer;
return 1;
}
} else {
s->buffer[s->len++] = '?';
return 1;
}
s->full = 1;
return 0;
}
/**
* trace_seq_to_user - copy the squence buffer to user space
* @s: trace sequence descriptor
* @ubuf: The userspace memory location to copy to
* @cnt: The amount to copy
*
* Copies the sequence buffer into the userspace memory pointed to
* by @ubuf. It starts from the last read position (@s->readpos)
* and writes up to @cnt characters or till it reaches the end of
* the content in the buffer (@s->len), which ever comes first.
*
* On success, it returns a positive number of the number of bytes
* it copied.
*
* On failure it returns -EBUSY if all of the content in the
* sequence has been already read, which includes nothing in the
* sequenc (@s->len == @s->readpos).
*
* Returns -EFAULT if the copy to userspace fails.
*/
int trace_seq_to_user(struct trace_seq *s, char __user *ubuf, int cnt)
{
int len;
int ret;
if (!cnt)
return 0;
if (s->len <= s->readpos)
return -EBUSY;
len = s->len - s->readpos;
if (cnt > len)
cnt = len;
ret = copy_to_user(ubuf, s->buffer + s->readpos, cnt);
if (ret == cnt)
return -EFAULT;
cnt -= ret;
s->readpos += cnt;
return cnt;
}
On Fri, 20 Jun 2014 16:28:45 -0400
Steven Rostedt <[email protected]> wrote:
> On Fri, 20 Jun 2014 10:12:44 -0700
> Andrew Morton <[email protected]> wrote:
>
>
> > > > > +#define HEX_CHARS (MAX_MEMHEX_BYTES*2 + 1)
> > > > > +
> > > > > +int trace_seq_putmem_hex(struct trace_seq *s, const void *mem, size_t len)
> > > > > +{
> > > > > + unsigned char hex[HEX_CHARS];
> > > > > + const unsigned char *data = mem;
> > > > > + int i, j;
> > > > > +
> > > > > + if (s->full)
> > > > > + return 0;
> > > >
> > > > What's this ->full thing all about anyway? Some central comment which
> > > > explains the design is needed.
> > >
> > > Comment? What? Git blame isn't good enough for ya? ;-)
> >
> > There's always that. There's also googling for the original list
> > dicsussion. But it's a bit user-unfriendly, particularly when then
> > code has aged was subsequently altered many times.
>
> Although I did not address this because I'm waiting to hear back from
> Johannes Berg, I updated for your other comments. I hope I got them all.
>
> Regardless of this patch series, I pulled out the code from
> trace_output.c and made a separate file for the trace_seq_*() functions
> in kernel/trace/trace_seq.c. I then updated the file with your
> comments. I found a bug or two and I will be dealing with them later as
> this will only be a clean up patch not a bug fix patch.
>
> Anyway, here's the new file:
Would be helpful if I compiled it before posting it :-)
Also, I missed a few EXPORT_SYMBOL_GPL()s along the way.
New update:
-- Steve
/*
* trace_seq.c
*
* Copyright (C) 2008-2014 Red Hat Inc, Steven Rostedt <[email protected]>
*
* The trace_seq is a handy tool that allows you to pass a descriptor around
* to a buffer that other functions can write to. It is similar to the
* seq_file functionality but has some differences.
*
* To use it, the trace_seq must be initialized with trace_seq_init().
* This will set up the counters within the descriptor. You can call
* trace_seq_init() more than once to reset the trace_seq to start
* from scratch.
*
* The buffer size is currently PAGE_SIZE, although it may become dynamic
* in the future.
*
* A write to the buffer will either succed or fail. That is, unlike
* sprintf() there will not be a partial write (well it may write into
* the buffer but it wont update the pointers). This allows users to
* try to write something into the trace_seq buffer and if it fails
* they can flush it and try again.
*
*/
#include <linux/uaccess.h>
#include <linux/seq_file.h>
#include <linux/trace_seq.h>
/* How much buffer is left on the trace_seq? */
#define TRACE_SEQ_BUF_LEFT(s) ((PAGE_SIZE - 1) - (s)->len)
/* How much buffer is written? */
#define TRACE_SEQ_BUF_USED(s) min((s)->len, (unsigned int)(PAGE_SIZE - 1))
/**
* trace_print_seq - move the contents of trace_seq into a seq_file
* @m: the seq_file descriptor that is the destination
* @s: the trace_seq descriptor that is the source.
*
* Returns 0 on success and non zero on error. If it succeeds to
* write to the seq_file it will reset the trace_seq, otherwise
* it does not modify the trace_seq to let the caller try again.
*/
int trace_print_seq(struct seq_file *m, struct trace_seq *s)
{
unsigned int len = TRACE_SEQ_BUF_USED(s);
int ret;
ret = seq_write(m, s->buffer, len);
/*
* Only reset this buffer if we successfully wrote to the
* seq_file buffer. This lets the caller try again or
* do something else with the contents.
*/
if (!ret)
trace_seq_init(s);
return ret;
}
/**
* trace_seq_printf - sequence printing of trace information
* @s: trace sequence descriptor
* @fmt: printf format string
*
* The tracer may use either sequence operations or its own
* copy to user routines. To simplify formating of a trace
* trace_seq_printf() is used to store strings into a special
* buffer (@s). Then the output may be either used by
* the sequencer or pulled into another buffer.
*
* Returns 1 if we successfully written all the contents to
* the buffer.
* Returns 0 if we the length to write is bigger than the
* reserved buffer space. In this case, nothing gets written.
*/
int trace_seq_printf(struct trace_seq *s, const char *fmt, ...)
{
unsigned int len = TRACE_SEQ_BUF_LEFT(s);
va_list ap;
int ret;
if (s->full || !len)
return 0;
va_start(ap, fmt);
ret = vsnprintf(s->buffer + s->len, len, fmt, ap);
va_end(ap);
/* If we can't write it all, don't bother writing anything */
if (ret >= len) {
s->full = 1;
return 0;
}
s->len += ret;
return 1;
}
EXPORT_SYMBOL_GPL(trace_seq_printf);
/**
* trace_seq_bitmask - write a bitmask array in its ASCII representation
* @s: trace sequence descriptor
* @maskp: points to an array of unsigned longs that represent a bitmask
* @nmaskbits: The number of bits that are valid in @maskp
*
* Writes a ASCII representation of a bitmask string into @s.
*
* Returns 1 if we successfully written all the contents to
* the buffer.
* Returns 0 if we the length to write is bigger than the
* reserved buffer space. In this case, nothing gets written.
*/
int trace_seq_bitmask(struct trace_seq *s, const unsigned long *maskp,
int nmaskbits)
{
unsigned int len = TRACE_SEQ_BUF_LEFT(s);
int ret;
if (s->full || !len)
return 0;
ret = bitmap_scnprintf(s->buffer, len, maskp, nmaskbits);
s->len += ret;
return 1;
}
EXPORT_SYMBOL_GPL(trace_seq_bitmask);
/**
* trace_seq_vprintf - sequence printing of trace information
* @s: trace sequence descriptor
* @fmt: printf format string
*
* The tracer may use either sequence operations or its own
* copy to user routines. To simplify formating of a trace
* trace_seq_printf is used to store strings into a special
* buffer (@s). Then the output may be either used by
* the sequencer or pulled into another buffer.
*
* Returns 1 if we successfully written all the contents to
* the buffer.
* Returns 0 if we the length to write is bigger than the
* reserved buffer space. In this case, nothing gets written.
*/
int trace_seq_vprintf(struct trace_seq *s, const char *fmt, va_list args)
{
unsigned int len = TRACE_SEQ_BUF_LEFT(s);
int ret;
if (s->full || !len)
return 0;
ret = vsnprintf(s->buffer + s->len, len, fmt, args);
/* If we can't write it all, don't bother writing anything */
if (ret >= len) {
s->full = 1;
return 0;
}
s->len += ret;
return len;
}
EXPORT_SYMBOL_GPL(trace_seq_vprintf);
/**
* trace_seq_bprintf - Write the printf string from binary arguments
* @s: trace sequence descriptor
* @fmt: The format string for the @binary arguments
* @binary: The binary arguments for @fmt.
*
* When recording in a fast path, a printf may be recorded with just
* saving the format and the arguments as they were passed to the
* function, instead of wasting cycles converting the arguments into
* ASCII characters. Instead, the arguments are saved in a 32 bit
* word array that is defined by the format string constraints.
*
* This function will take the format and the binary array and finish
* the conversion into the ASCII string within the buffer.
*
* Returns 1 if we successfully written all the contents to
* the buffer.
* Returns 0 if we the length to write is bigger than the
* reserved buffer space. In this case, nothing gets written.
*/
int trace_seq_bprintf(struct trace_seq *s, const char *fmt, const u32 *binary)
{
unsigned int len = TRACE_SEQ_BUF_LEFT(s);
int ret;
if (s->full || !len)
return 0;
ret = bstr_printf(s->buffer + s->len, len, fmt, binary);
/* If we can't write it all, don't bother writing anything */
if (ret >= len) {
s->full = 1;
return 0;
}
s->len += ret;
return len;
}
EXPORT_SYMBOL_GPL(trace_seq_bprintf);
/**
* trace_seq_puts - trace sequence printing of simple string
* @s: trace sequence descriptor
* @str: simple string to record
*
* The tracer may use either the sequence operations or its own
* copy to user routines. This function records a simple string
* into a special buffer (@s) for later retrieval by a sequencer
* or other mechanism.
*
* Returns 1 if we successfully written all the contents to
* the buffer.
* Returns 0 if we the length to write is bigger than the
* reserved buffer space. In this case, nothing gets written.
*/
int trace_seq_puts(struct trace_seq *s, const char *str)
{
unsigned int len = strlen(str);
if (s->full)
return 0;
if (len > TRACE_SEQ_BUF_LEFT(s)) {
s->full = 1;
return 0;
}
memcpy(s->buffer + s->len, str, len);
s->len += len;
return len;
}
EXPORT_SYMBOL_GPL(trace_seq_puts);
/**
* trace_seq_putc - trace sequence printing of simple character
* @s: trace sequence descriptor
* @c: simple character to record
*
* The tracer may use either the sequence operations or its own
* copy to user routines. This function records a simple charater
* into a special buffer (@s) for later retrieval by a sequencer
* or other mechanism.
*
* Returns 1 if we successfully written all the contents to
* the buffer.
* Returns 0 if we the length to write is bigger than the
* reserved buffer space. In this case, nothing gets written.
*/
int trace_seq_putc(struct trace_seq *s, unsigned char c)
{
if (s->full)
return 0;
if (TRACE_SEQ_BUF_LEFT(s) < 1) {
s->full = 1;
return 0;
}
s->buffer[s->len++] = c;
return 1;
}
EXPORT_SYMBOL_GPL(trace_seq_putc);
/**
* trace_seq_putmem - write raw data into the trace_seq buffer
* @s: trace sequence descriptor
* @mem: The raw memory to copy into the buffer
* @len: The length of the raw memory to copy (in bytes)
*
* There may be cases where raw memory needs to be written into the
* buffer and a strcpy() would not work. Using this function allows
* for such cases.
*
* Returns 1 if we successfully written all the contents to
* the buffer.
* Returns 0 if we the length to write is bigger than the
* reserved buffer space. In this case, nothing gets written.
*/
int trace_seq_putmem(struct trace_seq *s, const void *mem, unsigned int len)
{
if (s->full)
return 0;
if (len > TRACE_SEQ_BUF_LEFT(s)) {
s->full = 1;
return 0;
}
memcpy(s->buffer + s->len, mem, len);
s->len += len;
return len;
}
EXPORT_SYMBOL_GPL(trace_seq_putmem);
#define HEX_CHARS (MAX_MEMHEX_BYTES*2 + 1)
/**
* trace_seq_putmem_hex - write raw memory into the buffer in ASCII hex
* @s: trace sequence descriptor
* @mem: The raw memory to write its hex ASCII representation of
* @len: The length of the raw memory to copy (in bytes)
*
* This is similar to trace_seq_putmem() except instead of just copying the
* raw memory into the buffer it writes its ASCII representation of it
* in hex characters.
*
* Returns 1 if we successfully written all the contents to
* the buffer.
* Returns 0 if we the length to write is bigger than the
* reserved buffer space. In this case, nothing gets written.
*/
int trace_seq_putmem_hex(struct trace_seq *s, const void *mem,
unsigned int len)
{
unsigned char hex[HEX_CHARS];
const unsigned char *data = mem;
int i, j;
if (s->full)
return 0;
#ifdef __BIG_ENDIAN
for (i = 0, j = 0; i < len; i++) {
#else
for (i = len-1, j = 0; i >= 0; i--) {
#endif
hex[j++] = hex_asc_hi(data[i]);
hex[j++] = hex_asc_lo(data[i]);
}
hex[j++] = ' ';
return trace_seq_putmem(s, hex, j);
}
EXPORT_SYMBOL_GPL(trace_seq_putmem_hex);
/**
* trace_seq_reserve - reserve space on the sequence buffer
* @s: trace sequence descriptor
* @len: The amount to reserver.
*
* If for some reason there is a need to save some space on the
* buffer to fill in later, this function is used for that purpose.
* The given length will be reserved and the pointer to that
* location on the buffer is returned, unless there is not enough
* buffer left to hold the given length then NULL is returned.
*/
void *trace_seq_reserve(struct trace_seq *s, unsigned int len)
{
void *ret;
if (s->full)
return NULL;
if (len > TRACE_SEQ_BUF_LEFT(s)) {
s->full = 1;
return NULL;
}
ret = s->buffer + s->len;
s->len += len;
return ret;
}
EXPORT_SYMBOL_GPL(trace_seq_reserve);
/**
* trace_seq_path - copy a path into the sequence buffer
* @s: trace sequence descriptor
* @path: path to write into the sequence buffer.
*
* Write a path name into the sequence buffer.
*
* Returns 1 if we successfully written all the contents to
* the buffer.
* Returns 0 if we the length to write is bigger than the
* reserved buffer space. In this case, nothing gets written.
*/
int trace_seq_path(struct trace_seq *s, const struct path *path)
{
unsigned char *p;
if (s->full)
return 0;
if (TRACE_SEQ_BUF_LEFT(s) < 1) {
s->full = 1;
return 0;
}
p = d_path(path, s->buffer + s->len, PAGE_SIZE - s->len);
if (!IS_ERR(p)) {
p = mangle_path(s->buffer + s->len, p, "\n");
if (p) {
s->len = p - s->buffer;
return 1;
}
} else {
s->buffer[s->len++] = '?';
return 1;
}
s->full = 1;
return 0;
}
EXPORT_SYMBOL_GPL(trace_seq_path);
/**
* trace_seq_to_user - copy the squence buffer to user space
* @s: trace sequence descriptor
* @ubuf: The userspace memory location to copy to
* @cnt: The amount to copy
*
* Copies the sequence buffer into the userspace memory pointed to
* by @ubuf. It starts from the last read position (@s->readpos)
* and writes up to @cnt characters or till it reaches the end of
* the content in the buffer (@s->len), which ever comes first.
*
* On success, it returns a positive number of the number of bytes
* it copied.
*
* On failure it returns -EBUSY if all of the content in the
* sequence has been already read, which includes nothing in the
* sequenc (@s->len == @s->readpos).
*
* Returns -EFAULT if the copy to userspace fails.
*/
int trace_seq_to_user(struct trace_seq *s, char __user *ubuf, int cnt)
{
int len;
int ret;
if (!cnt)
return 0;
if (s->len <= s->readpos)
return -EBUSY;
len = s->len - s->readpos;
if (cnt > len)
cnt = len;
ret = copy_to_user(ubuf, s->buffer + s->readpos, cnt);
if (ret == cnt)
return -EFAULT;
cnt -= ret;
s->readpos += cnt;
return cnt;
}
EXPORT_SYMBOL_GPL(trace_seq_to_user);
On Fri, 2014-06-20 at 12:58 -0400, Steven Rostedt wrote:
> > > +#define HEX_CHARS (MAX_MEMHEX_BYTES*2 + 1)
> > > +
> > > +int trace_seq_putmem_hex(struct trace_seq *s, const void *mem, size_t len)
> > > +{
> > > + unsigned char hex[HEX_CHARS];
> > > + const unsigned char *data = mem;
> > > + int i, j;
> > > +
> > > + if (s->full)
> > > + return 0;
> >
> > What's this ->full thing all about anyway? Some central comment which
> > explains the design is needed.
>
> Comment? What? Git blame isn't good enough for ya? ;-)
>
> >
> > Is this test really needed? trace_seq_putmem() will handle this.
>
> It was added as an optimization, because once it filled up, you could
> still have multiple calls to the trace_seq() functions that would waste
> time trying to write the buffer.
>
> It seemed like a good idea at the time. I Cc'd Johannes Berg as he's
> the one that implemented.
>
> Johannes, is this really needed, should we bother keeping it?
Honestly, I don't remember, sorry.
Looking at the code though, I'm not sure it's a pure optimisation - if
you do say putc() after a failed puts(), without this code the putc()
would succeed? I can't tell right now if that's really a problem, but it
seems you could get some odd behaviour out of it.
johannes
On Sun, 22 Jun 2014 09:38:05 +0200
Johannes Berg <[email protected]> wrote:
> Looking at the code though, I'm not sure it's a pure optimisation - if
> you do say putc() after a failed puts(), without this code the putc()
> would succeed? I can't tell right now if that's really a problem, but it
> seems you could get some odd behaviour out of it.
How would putc() still succeed? We're just talking about the "full"
field. It would still do the length check:
if (s->len >= (PAGE_SIZE - 1))
return 0;
-- Steve
On Fri 2014-06-20 16:51:45, Steven Rostedt wrote:
> On Fri, 20 Jun 2014 16:28:45 -0400
> Steven Rostedt <[email protected]> wrote:
>
> > On Fri, 20 Jun 2014 10:12:44 -0700
> > Andrew Morton <[email protected]> wrote:
> >
> >
> > > > > > +#define HEX_CHARS (MAX_MEMHEX_BYTES*2 + 1)
> > > > > > +
> > > > > > +int trace_seq_putmem_hex(struct trace_seq *s, const void *mem, size_t len)
> > > > > > +{
> > > > > > + unsigned char hex[HEX_CHARS];
> > > > > > + const unsigned char *data = mem;
> > > > > > + int i, j;
> > > > > > +
> > > > > > + if (s->full)
> > > > > > + return 0;
> > > > >
> > > > > What's this ->full thing all about anyway? Some central comment which
> > > > > explains the design is needed.
> > > >
> > > > Comment? What? Git blame isn't good enough for ya? ;-)
> > >
> > > There's always that. There's also googling for the original list
> > > dicsussion. But it's a bit user-unfriendly, particularly when then
> > > code has aged was subsequently altered many times.
> >
> > Although I did not address this because I'm waiting to hear back from
> > Johannes Berg, I updated for your other comments. I hope I got them all.
> >
> > Regardless of this patch series, I pulled out the code from
> > trace_output.c and made a separate file for the trace_seq_*() functions
> > in kernel/trace/trace_seq.c. I then updated the file with your
> > comments. I found a bug or two and I will be dealing with them later as
> > this will only be a clean up patch not a bug fix patch.
> >
> > Anyway, here's the new file:
>
> Would be helpful if I compiled it before posting it :-)
>
> Also, I missed a few EXPORT_SYMBOL_GPL()s along the way.
>
> New update:
>
> -- Steve
>
> /*
> * trace_seq.c
> *
> * Copyright (C) 2008-2014 Red Hat Inc, Steven Rostedt <[email protected]>
> *
> * The trace_seq is a handy tool that allows you to pass a descriptor around
> * to a buffer that other functions can write to. It is similar to the
> * seq_file functionality but has some differences.
> *
> * To use it, the trace_seq must be initialized with trace_seq_init().
> * This will set up the counters within the descriptor. You can call
> * trace_seq_init() more than once to reset the trace_seq to start
> * from scratch.
> *
> * The buffer size is currently PAGE_SIZE, although it may become dynamic
> * in the future.
> *
> * A write to the buffer will either succed or fail. That is, unlike
> * sprintf() there will not be a partial write (well it may write into
> * the buffer but it wont update the pointers). This allows users to
> * try to write something into the trace_seq buffer and if it fails
> * they can flush it and try again.
We might want to write as much as possible to the buffer if it is used
for the backtrace.
Well, if I understand it correctly, this is only about the last
line. If the trace does not fit, we are in troubles anyway. We might
solve this later when needed.
> */
> #include <linux/uaccess.h>
> #include <linux/seq_file.h>
> #include <linux/trace_seq.h>
>
> /* How much buffer is left on the trace_seq? */
> #define TRACE_SEQ_BUF_LEFT(s) ((PAGE_SIZE - 1) - (s)->len)
>
> /* How much buffer is written? */
> #define TRACE_SEQ_BUF_USED(s) min((s)->len, (unsigned int)(PAGE_SIZE - 1))
[...]
> /**
> * trace_seq_printf - sequence printing of trace information
> * @s: trace sequence descriptor
> * @fmt: printf format string
> *
> * The tracer may use either sequence operations or its own
> * copy to user routines. To simplify formating of a trace
> * trace_seq_printf() is used to store strings into a special
> * buffer (@s). Then the output may be either used by
> * the sequencer or pulled into another buffer.
> *
> * Returns 1 if we successfully written all the contents to
> * the buffer.
> * Returns 0 if we the length to write is bigger than the
> * reserved buffer space. In this case, nothing gets written.
> */
> int trace_seq_printf(struct trace_seq *s, const char *fmt, ...)
> {
> unsigned int len = TRACE_SEQ_BUF_LEFT(s);
> va_list ap;
> int ret;
>
> if (s->full || !len)
> return 0;
>
> va_start(ap, fmt);
> ret = vsnprintf(s->buffer + s->len, len, fmt, ap);
> va_end(ap);
>
> /* If we can't write it all, don't bother writing anything */
> if (ret >= len) {
> s->full = 1;
> return 0;
> }
>
> s->len += ret;
>
> return 1;
> }
> EXPORT_SYMBOL_GPL(trace_seq_printf);
>
> /**
> * trace_seq_bitmask - write a bitmask array in its ASCII representation
> * @s: trace sequence descriptor
> * @maskp: points to an array of unsigned longs that represent a bitmask
> * @nmaskbits: The number of bits that are valid in @maskp
> *
> * Writes a ASCII representation of a bitmask string into @s.
> *
> * Returns 1 if we successfully written all the contents to
> * the buffer.
> * Returns 0 if we the length to write is bigger than the
> * reserved buffer space. In this case, nothing gets written.
> */
> int trace_seq_bitmask(struct trace_seq *s, const unsigned long *maskp,
> int nmaskbits)
> {
> unsigned int len = TRACE_SEQ_BUF_LEFT(s);
> int ret;
>
> if (s->full || !len)
> return 0;
>
> ret = bitmap_scnprintf(s->buffer, len, maskp, nmaskbits);
We do not detect here if the whole bitmap is written. Hmm, the perfect
solution is not easy. What about using the following workaround?
If we wrote until the end of the buffer, the write was most likely
incomplete:
if (ret == len) {
s->full = 1;
return 0;
}
It is more consistent with what we do in the other functions.
> s->len += ret;
>
> return 1;
> }
> EXPORT_SYMBOL_GPL(trace_seq_bitmask);
>
[...]
> /**
> * trace_seq_putmem_hex - write raw memory into the buffer in ASCII hex
> * @s: trace sequence descriptor
> * @mem: The raw memory to write its hex ASCII representation of
> * @len: The length of the raw memory to copy (in bytes)
> *
> * This is similar to trace_seq_putmem() except instead of just copying the
> * raw memory into the buffer it writes its ASCII representation of it
> * in hex characters.
> *
> * Returns 1 if we successfully written all the contents to
> * the buffer.
> * Returns 0 if we the length to write is bigger than the
> * reserved buffer space. In this case, nothing gets written.
> */
> int trace_seq_putmem_hex(struct trace_seq *s, const void *mem,
> unsigned int len)
> {
> unsigned char hex[HEX_CHARS];
> const unsigned char *data = mem;
> int i, j;
>
> if (s->full)
> return 0;
>
> #ifdef __BIG_ENDIAN
> for (i = 0, j = 0; i < len; i++) {
> #else
> for (i = len-1, j = 0; i >= 0; i--) {
> #endif
This might overflow when "len" > (HEX_CHARS-1)/2.
I think that we should either limit "len" or make it in two cycles.
Hmm, if we use two cycles, it might be problem how to solve the endianity.
IMHO, it depends on what type of data is written into the buffer
(single vs. set of values).
> hex[j++] = hex_asc_hi(data[i]);
> hex[j++] = hex_asc_lo(data[i]);
> }
> hex[j++] = ' ';
>
> return trace_seq_putmem(s, hex, j);
> }
> EXPORT_SYMBOL_GPL(trace_seq_putmem_hex);
>
> /**
> * trace_seq_reserve - reserve space on the sequence buffer
> * @s: trace sequence descriptor
> * @len: The amount to reserver.
> *
> * If for some reason there is a need to save some space on the
> * buffer to fill in later, this function is used for that purpose.
> * The given length will be reserved and the pointer to that
> * location on the buffer is returned, unless there is not enough
> * buffer left to hold the given length then NULL is returned.
> */
> void *trace_seq_reserve(struct trace_seq *s, unsigned int len)
> {
> void *ret;
>
> if (s->full)
> return NULL;
>
> if (len > TRACE_SEQ_BUF_LEFT(s)) {
> s->full = 1;
> return NULL;
> }
>
> ret = s->buffer + s->len;
> s->len += len;
>
> return ret;
> }
> EXPORT_SYMBOL_GPL(trace_seq_reserve);
This sounds like a slightly dangerous operation. The reserve is valid only
until the buffer is re-initialized. Is this really intended?
Do we really want to export it?
Well, the whole buffer need to be used carefully. IMHO, it can't be accessed
in parallel without extra locks. So, this might be fine after all. We
could only warn about this in the function description.
[...]
The rest looks fine to me.
Also the proposed name "seq_buf" sounds fine to me.
Best Regards,
Petr
On Mon, 23 Jun 2014 18:33:06 +0200
Petr Mládek <[email protected]> wrote:
> > * A write to the buffer will either succed or fail. That is, unlike
> > * sprintf() there will not be a partial write (well it may write into
> > * the buffer but it wont update the pointers). This allows users to
> > * try to write something into the trace_seq buffer and if it fails
> > * they can flush it and try again.
>
> We might want to write as much as possible to the buffer if it is used
> for the backtrace.
Understood, but there is a reason for this way of doing things.
>
> Well, if I understand it correctly, this is only about the last
> line. If the trace does not fit, we are in troubles anyway. We might
> solve this later when needed.
>
> > */
> > #include <linux/uaccess.h>
> > #include <linux/seq_file.h>
> > #include <linux/trace_seq.h>
> >
> > /* How much buffer is left on the trace_seq? */
> > #define TRACE_SEQ_BUF_LEFT(s) ((PAGE_SIZE - 1) - (s)->len)
> >
> > /* How much buffer is written? */
> > #define TRACE_SEQ_BUF_USED(s) min((s)->len, (unsigned int)(PAGE_SIZE - 1))
>
> [...]
>
> > /**
> > * trace_seq_printf - sequence printing of trace information
> > * @s: trace sequence descriptor
> > * @fmt: printf format string
> > *
> > * The tracer may use either sequence operations or its own
> > * copy to user routines. To simplify formating of a trace
> > * trace_seq_printf() is used to store strings into a special
> > * buffer (@s). Then the output may be either used by
> > * the sequencer or pulled into another buffer.
> > *
> > * Returns 1 if we successfully written all the contents to
> > * the buffer.
> > * Returns 0 if we the length to write is bigger than the
> > * reserved buffer space. In this case, nothing gets written.
> > */
> > int trace_seq_printf(struct trace_seq *s, const char *fmt, ...)
> > {
> > unsigned int len = TRACE_SEQ_BUF_LEFT(s);
> > va_list ap;
> > int ret;
> >
> > if (s->full || !len)
> > return 0;
> >
> > va_start(ap, fmt);
> > ret = vsnprintf(s->buffer + s->len, len, fmt, ap);
> > va_end(ap);
> >
> > /* If we can't write it all, don't bother writing anything */
> > if (ret >= len) {
> > s->full = 1;
> > return 0;
> > }
> >
> > s->len += ret;
> >
> > return 1;
> > }
> > EXPORT_SYMBOL_GPL(trace_seq_printf);
> >
> > /**
> > * trace_seq_bitmask - write a bitmask array in its ASCII representation
> > * @s: trace sequence descriptor
> > * @maskp: points to an array of unsigned longs that represent a bitmask
> > * @nmaskbits: The number of bits that are valid in @maskp
> > *
> > * Writes a ASCII representation of a bitmask string into @s.
> > *
> > * Returns 1 if we successfully written all the contents to
> > * the buffer.
> > * Returns 0 if we the length to write is bigger than the
> > * reserved buffer space. In this case, nothing gets written.
> > */
> > int trace_seq_bitmask(struct trace_seq *s, const unsigned long *maskp,
> > int nmaskbits)
> > {
> > unsigned int len = TRACE_SEQ_BUF_LEFT(s);
> > int ret;
> >
> > if (s->full || !len)
> > return 0;
> >
> > ret = bitmap_scnprintf(s->buffer, len, maskp, nmaskbits);
>
> We do not detect here if the whole bitmap is written. Hmm, the perfect
> solution is not easy. What about using the following workaround?
Ah right. Nice catch.
>
> If we wrote until the end of the buffer, the write was most likely
> incomplete:
>
> if (ret == len) {
> s->full = 1;
> return 0;
> }
>
> It is more consistent with what we do in the other functions.
>
> > s->len += ret;
> >
> > return 1;
> > }
I'll have to look into this too. Yeah, the results should be more
consistent. Maybe have zero be it fit, and return how much buffer is
left when it doesn't fit?
> > EXPORT_SYMBOL_GPL(trace_seq_bitmask);
> >
>
> [...]
>
> > /**
> > * trace_seq_putmem_hex - write raw memory into the buffer in ASCII hex
> > * @s: trace sequence descriptor
> > * @mem: The raw memory to write its hex ASCII representation of
> > * @len: The length of the raw memory to copy (in bytes)
> > *
> > * This is similar to trace_seq_putmem() except instead of just copying the
> > * raw memory into the buffer it writes its ASCII representation of it
> > * in hex characters.
> > *
> > * Returns 1 if we successfully written all the contents to
> > * the buffer.
> > * Returns 0 if we the length to write is bigger than the
> > * reserved buffer space. In this case, nothing gets written.
> > */
> > int trace_seq_putmem_hex(struct trace_seq *s, const void *mem,
> > unsigned int len)
> > {
> > unsigned char hex[HEX_CHARS];
> > const unsigned char *data = mem;
> > int i, j;
> >
> > if (s->full)
> > return 0;
> >
> > #ifdef __BIG_ENDIAN
> > for (i = 0, j = 0; i < len; i++) {
> > #else
> > for (i = len-1, j = 0; i >= 0; i--) {
> > #endif
>
> This might overflow when "len" > (HEX_CHARS-1)/2.
Yes I caught this while doing updates per Andrew's replies. Currently
it's fine because the only user of it is wrapped in the macro. It
originally had a BUG_ON() if len > HEX_CHARS but that was moved to the
macro to make it a build bug on. I hate this function, but we need to
keep it to keep backward compatibility for what was in the original -rt
latency tracer :-p
Anyway, last week I rewrote this function to be much more robust. You
can look at my branch ftrace/next (caution, it rebases constantly).
>
> I think that we should either limit "len" or make it in two cycles.
>
> Hmm, if we use two cycles, it might be problem how to solve the endianity.
> IMHO, it depends on what type of data is written into the buffer
> (single vs. set of values).
>
> > hex[j++] = hex_asc_hi(data[i]);
> > hex[j++] = hex_asc_lo(data[i]);
> > }
> > hex[j++] = ' ';
> >
> > return trace_seq_putmem(s, hex, j);
> > }
> > EXPORT_SYMBOL_GPL(trace_seq_putmem_hex);
> >
> > /**
> > * trace_seq_reserve - reserve space on the sequence buffer
> > * @s: trace sequence descriptor
> > * @len: The amount to reserver.
> > *
> > * If for some reason there is a need to save some space on the
> > * buffer to fill in later, this function is used for that purpose.
> > * The given length will be reserved and the pointer to that
> > * location on the buffer is returned, unless there is not enough
> > * buffer left to hold the given length then NULL is returned.
> > */
> > void *trace_seq_reserve(struct trace_seq *s, unsigned int len)
> > {
> > void *ret;
> >
> > if (s->full)
> > return NULL;
> >
> > if (len > TRACE_SEQ_BUF_LEFT(s)) {
> > s->full = 1;
> > return NULL;
> > }
> >
> > ret = s->buffer + s->len;
> > s->len += len;
> >
> > return ret;
> > }
> > EXPORT_SYMBOL_GPL(trace_seq_reserve);
>
> This sounds like a slightly dangerous operation. The reserve is valid only
> until the buffer is re-initialized. Is this really intended?
Well, the only on reserving it should be whoever is using it. It is
like allocating memory and then letting something else free it.
> Do we really want to export it?
Honestly, I don't know why this was added. It was created by Eduard and
pulled in by Ingo. There's no users in the kernel. I think I may just
nuke it.
>
> Well, the whole buffer need to be used carefully. IMHO, it can't be accessed
> in parallel without extra locks. So, this might be fine after all. We
> could only warn about this in the function description.
>
Well, neither can seq_file. It's based on that. Take a look at some of
my changes in ftrace/next and see .
> [...]
>
> The rest looks fine to me.
>
> Also the proposed name "seq_buf" sounds fine to me.
Yeah, that may change soon too. Right now I'm working on cleaning up
the code within the trace directory, and then I'll push it to lib if
this is the way we are going.
-- Steve
>
>
> Best Regards,
> Petr
On Mon, 2014-06-23 at 12:08 -0400, Steven Rostedt wrote:
> On Sun, 22 Jun 2014 09:38:05 +0200
> Johannes Berg <[email protected]> wrote:
>
>
> > Looking at the code though, I'm not sure it's a pure optimisation - if
> > you do say putc() after a failed puts(), without this code the putc()
> > would succeed? I can't tell right now if that's really a problem, but it
> > seems you could get some odd behaviour out of it.
>
> How would putc() still succeed? We're just talking about the "full"
> field. It would still do the length check:
>
> if (s->len >= (PAGE_SIZE - 1))
> return 0;
Right, but the puts() could fail if not all of the string fits, and a
subsequent putc() might fit.
johannes
On Mon, 23 Jun 2014 19:38:14 +0200
Johannes Berg <[email protected]> wrote:
> On Mon, 2014-06-23 at 12:08 -0400, Steven Rostedt wrote:
> > On Sun, 22 Jun 2014 09:38:05 +0200
> > Johannes Berg <[email protected]> wrote:
> >
> >
> > > Looking at the code though, I'm not sure it's a pure optimisation - if
> > > you do say putc() after a failed puts(), without this code the putc()
> > > would succeed? I can't tell right now if that's really a problem, but it
> > > seems you could get some odd behaviour out of it.
> >
> > How would putc() still succeed? We're just talking about the "full"
> > field. It would still do the length check:
> >
> > if (s->len >= (PAGE_SIZE - 1))
> > return 0;
>
> Right, but the puts() could fail if not all of the string fits, and a
> subsequent putc() might fit.
>
Ah! I see what you are saying. You are saying that once you are full
you need to flush. Honestly, the trace_puts() should check the return.
Perhaps it wants to try again with a shorter string?
The full flag means that once we hit full, we are done, no more can be
written till the buffer is flushed.
Is that what we would want from this utility? Or do we want to allow
the user to try again with a smaller string?
I was thinking of reversing the error code and have the trace_seq_*()
return 0 as success and the remaining buffer size on error.
-- Steve
On Mon, 2014-06-23 at 14:04 -0400, Steven Rostedt wrote:
> Ah! I see what you are saying. You are saying that once you are full
> you need to flush. Honestly, the trace_puts() should check the return.
> Perhaps it wants to try again with a shorter string?
Perhaps, but perhaps not? I don't remember exactly the issue that I saw
without the full flag, and it may very well be a purely theoretical
thing too. I don't think I mind either way.
> The full flag means that once we hit full, we are done, no more can be
> written till the buffer is flushed.
Right. Arguably, it was already full or overflowed, but as it now stands
we make sure the data is consistent with the input (or rejected if the
input didn't fit.)
> Is that what we would want from this utility? Or do we want to allow
> the user to try again with a smaller string?
That's a good question - right now of course the first attempt would
make it full, and then any further attempt would fail.
> I was thinking of reversing the error code and have the trace_seq_*()
> return 0 as success and the remaining buffer size on error.
That would make some sense if we wanted to make it post-determined (you
fail, and then you can use the remaining buffer). Maybe it would make
sense to pre-determine that, e.g. by having a function that can reserve
a range, say
void trace_seq_put_buf(..., int min_len, int max_len,
int *out_avail, void *out_data_ptr)
(of course that prototype needs to be adjusted appropriately)
If less than min_len is available, we could still have the "full"
semantics.
Either way, I don't really know or even care. I'm only using this
indirectly through the tracing, where I don't think it makes a lot of
sense without the full semantics since afaik the buffers need to be in
the right order in the page(s), and where you likely don't want to cut a
string short since you couldn't continue it on the next page, but you're
clearly the expert on this and I'm probably wrong anyway :)
johannes