lkml.org 
[lkml]   [2014]   [Jun]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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@suse.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@redhat.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@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2014-06-27 17:01    [W:3.339 / U:0.084 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site