| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20140627102134.5d2cae41@gandalf.local.home>
Date: Fri, 27 Jun 2014 10:21:34 -0400
From: Steven Rostedt <rostedt@...dmis.org>
To: Petr Mládek <pmladek@...e.cz>
Cc: linux-kernel@...r.kernel.org,
Linus Torvalds <torvalds@...ux-foundation.org>,
Ingo Molnar <mingo@...nel.org>,
Andrew Morton <akpm@...ux-foundation.org>,
Jiri Kosina <jkosina@...e.cz>, Michal Hocko <mhocko@...e.cz>,
Jan Kara <jack@...e.cz>,
Frederic Weisbecker <fweisbec@...il.com>,
Dave Anderson <anderson@...hat.com>,
"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
Konstantin Khlebnikov <koct9i@...il.com>
Subject: Re: [RFC][PATCH 2/5 v2] tracing: Create seq_buf layer in trace_seq
On Fri, 27 Jun 2014 15:45:38 +0200
Petr Mládek <pmladek@...e.cz> wrote:
> ad 4th:
>
> Both "full" and "overflow" flags seems to have the same meaning.
> For example, trace_seq_printf() sets "full" on failure even
> when s->seq.len != s->size.
>
>
> Best Regards,
> Petr
>
> [...]
BTW, you shouldn't sign off if you have more comments, I usually stop
reading when I see someone's signature. Or at the very least, state
that you have more comments below.
> > --- /dev/null
> > +++ b/kernel/trace/seq_buf.c
> > @@ -0,0 +1,348 @@
> > +/*
> > + * seq_buf.c
> > + *
> > + * Copyright (C) 2014 Red Hat Inc, Steven Rostedt <srostedt@...hat.com>
> > + *
> > + * The seq_buf 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 seq_buf must be initialized with seq_buf_init().
> > + * This will set up the counters within the descriptor. You can call
> > + * seq_buf_init() more than once to reset the seq_buf to start
> > + * from scratch.
> > + *
> > + */
> > +#include <linux/uaccess.h>
> > +#include <linux/seq_file.h>
> > +#include <linux/seq_buf.h>
> > +
> > +/* How much buffer is left on the seq_buf? */
> > +#define SEQ_BUF_LEFT(s) (((s)->size - 1) - (s)->len)
> > +
> > +/* How much buffer is written? */
> > +#define SEQ_BUF_USED(s) min((s)->len, (s)->size - 1)
> > +
> > +static inline void seq_buf_check_len(struct seq_buf *s)
> > +{
> > + if (unlikely(s->len > (s->size - 1))) {
>
> I would create macro for this check. It is repeated many times. I mean
>
> #define SEQ_BUF_OVERFLOW(s) (s->len > (s->size - 1))
>
> if (unlikely(SEQ_BUF_OVERFLOW))
Yeah, I was hoping that the static inline would be enough, but then it
didn't fit into the functions at the end. I never got around to then
adding a macro.
Everything was open coded when I first created it.
>
> > + s->len = s->size - 1;
> > + s->overflow = 1;
> > + }
> > +}
> > +
>
> [...]
>
> > +#define MAX_MEMHEX_BYTES 8U
> > +#define HEX_CHARS (MAX_MEMHEX_BYTES*2 + 1)
> > +
> > +/**
> > + * seq_buf_putmem_hex - write raw memory into the buffer in ASCII hex
> > + * @s: seq_buf 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 seq_buf_putmem() except instead of just copying the
> > + * raw memory into the buffer it writes its ASCII representation of it
> > + * in hex characters.
> > + *
> > + * Returns how much it wrote to the buffer.
> > + */
> > +int seq_buf_putmem_hex(struct seq_buf *s, const void *mem,
> > + unsigned int len)
> > +{
> > + unsigned char hex[HEX_CHARS];
> > + const unsigned char *data = mem;
> > + unsigned int start_len;
> > + int i, j;
> > + int cnt = 0;
> > +
> > + while (len) {
> > + start_len = min(len, HEX_CHARS - 1);
>
> I would do s/start_len/end_len/
>
> I know that it depends on the point of view. But iteration between "0"
> and "end" is better understandable for me :-)
Yeah, I didn't like the name of that variable. I guess end_len is
better, but still not exactly what I want to call it. Unfortunately, I
don't know what exactly I want to call it ;-)
>
> > +#ifdef __BIG_ENDIAN
> > + for (i = 0, j = 0; i < start_len; i++) {
> > +#else
> > + for (i = start_len-1, j = 0; i >= 0; i--) {
> > +#endif
> > + hex[j++] = hex_asc_hi(data[i]);
> > + hex[j++] = hex_asc_lo(data[i]);
> > + }
> > + if (WARN_ON_ONCE(j == 0 || j/2 > len))
> > + break;
> > +
> > + /* j increments twice per loop */
> > + len -= j / 2;
> > + hex[j++] = ' ';
> > +
> > + cnt += seq_buf_putmem(s, hex, j);
> > + }
> > + return cnt;
> > +}
> > +
> > +/**
> > + * seq_buf_path - copy a path into the sequence buffer
> > + * @s: seq_buf descriptor
> > + * @path: path to write into the sequence buffer.
> > + *
> > + * Write a path name into the sequence buffer.
> > + *
> > + * Returns the number of bytes written into the buffer.
> > + */
> > +int seq_buf_path(struct seq_buf *s, const struct path *path)
> > +{
> > + unsigned int len = SEQ_BUF_LEFT(s);
> > + unsigned char *p;
> > + unsigned int start = s->len;
> > +
> > + WARN_ON((int)len < 0);
> > + p = d_path(path, s->buffer + s->len, len);
> > + if (!IS_ERR(p)) {
> > + p = mangle_path(s->buffer + s->len, p, "\n");
> > + if (p) {
> > + s->len = p - s->buffer;
> > + WARN_ON(s->len > (s->size - 1));
>
> strange indentation
>
> > + return s->len - start;
> > + }
> > + } else {
> > + s->buffer[s->len++] = '?';
> > + WARN_ON(s->len > (s->size - 1));
>
> same here
Heh, I added those for debugging, and then decided to keep it. Never
went back to clean it up :-/
-- Steve
>
> > + return s->len - start;
> > + }
> > +
> > + s->overflow = 1;
> > + return 0;
> > +}
> > +
>
>
> Best Regards,
> Petr
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists