Re: [PATCH v3 1/5] seq_file: provide an analogue of print_hex_dump()
On Mon, Sep 1, 2014 at 2:49 PM, Geert Uytterhoeven wrote: > Hi Andy, > > On Mon, Sep 1, 2014 at 1:33 PM, Andy Shevchenko > wrote: >>> >> However, to protect against overflows if hex_dump_to_buffer() ever >>> >> changes, >>> >> I think it would be better to let hex_dump_to_buffer() indicate if the >>> >> passed buffer was to small (it already checks the passed linebuflen). >>> >> Then you can just check for that. >>> > >>> > I thought about that. We may introduce either new call and make >>> > current one the user of it or change all occurrences. >>> > Nevertheless, currently it will print only one groupsize if there is >>> > enough room for it but for two or more. >>> > Thus, I prefer to keep the behaviour "print until we can". >>> >>> The idea of seq_*() is that it will retry with a bigger bufsize if there's >>> not enough space. >> >> Fair enough. >> >> So, summarize above, the check before hex_dump_to_buffer() should care >> about maximum possible buffer needed for one line. But we have already >> rowsize calculated (we actually can't rely on groupsize in this case). >> >> Do you agree with formula rowsize * 3 + 2 + rowsize + 1? > > That's the _current_ formula. The day hex_dump_to_buffer() is changed, > it will break silently. Do we have any other option except modify hex_dump_to_buffer() to return an error when buffer is not enough? In this case we require hex_dump_to_buffer() to follow this agreement even if it's changed once. -- With Best Regards, Andy Shevchenko -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/5] seq_file: provide an analogue of print_hex_dump()
Hi Andy,
On Mon, Sep 1, 2014 at 1:33 PM, Andy Shevchenko
wrote:
>> >> However, to protect against overflows if hex_dump_to_buffer() ever
>> >> changes,
>> >> I think it would be better to let hex_dump_to_buffer() indicate if the
>> >> passed buffer was to small (it already checks the passed linebuflen).
>> >> Then you can just check for that.
>> >
>> > I thought about that. We may introduce either new call and make
>> > current one the user of it or change all occurrences.
>> > Nevertheless, currently it will print only one groupsize if there is
>> > enough room for it but for two or more.
>> > Thus, I prefer to keep the behaviour "print until we can".
>>
>> The idea of seq_*() is that it will retry with a bigger bufsize if there's
>> not enough space.
>
> Fair enough.
>
> So, summarize above, the check before hex_dump_to_buffer() should care
> about maximum possible buffer needed for one line. But we have already
> rowsize calculated (we actually can't rely on groupsize in this case).
>
> Do you agree with formula rowsize * 3 + 2 + rowsize + 1?
That's the _current_ formula. The day hex_dump_to_buffer() is changed,
it will break silently.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/5] seq_file: provide an analogue of print_hex_dump()
On Mon, 2014-09-01 at 12:59 +0200, Geert Uytterhoeven wrote: > Hi Andy, > > On Mon, Sep 1, 2014 at 12:15 PM, Andy Shevchenko > wrote: > I think it needs a call to seq_set_overflow() in case the buffer is too > small, > so the caller will retry with a bigger buffer. > >>> > >>> Yes, in two places it would be useful to do. > >> > >> Two places? I see only one, just before calling hex_dump_to_buffer. > > > > seq_putc doesn't set it as I can see. > > > >>> But what the condition for "buffer is too small", the same groupsize * 2 > >>> + 1 or you mean something else? > >> > >> "groupsize * 2 + 1" is not the amount of bytes hex_dump_to_buffer() wants > >> to write. It's only the size for one word. > >> > >> You could check if there are at least "32 * 3 + 2 + 32 + 1" bytes (your > >> old linebuf size) available. > > > > This is a good question why this number? What if we have to print only > > one byte (as different groupsize)? > > I don't think complaining about a too-small buffer prematurely hurts. > > > I think the requirement for one groupsize is quite okay. > > Then you will loose data if the buffer is too small. > > >> However, to protect against overflows if hex_dump_to_buffer() ever changes, > >> I think it would be better to let hex_dump_to_buffer() indicate if the > >> passed buffer was to small (it already checks the passed linebuflen). > >> Then you can just check for that. > > > > I thought about that. We may introduce either new call and make > > current one the user of it or change all occurrences. > > Nevertheless, currently it will print only one groupsize if there is > > enough room for it but for two or more. > > Thus, I prefer to keep the behaviour "print until we can". > > The idea of seq_*() is that it will retry with a bigger bufsize if there's > not enough space. Fair enough. So, summarize above, the check before hex_dump_to_buffer() should care about maximum possible buffer needed for one line. But we have already rowsize calculated (we actually can't rely on groupsize in this case). Do you agree with formula rowsize * 3 + 2 + rowsize + 1? -- Andy Shevchenko Intel Finland Oy -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/5] seq_file: provide an analogue of print_hex_dump()
Hi Andy,
On Mon, Sep 1, 2014 at 12:15 PM, Andy Shevchenko
wrote:
I think it needs a call to seq_set_overflow() in case the buffer is too
small,
so the caller will retry with a bigger buffer.
>>>
>>> Yes, in two places it would be useful to do.
>>
>> Two places? I see only one, just before calling hex_dump_to_buffer.
>
> seq_putc doesn't set it as I can see.
>
>>> But what the condition for "buffer is too small", the same groupsize * 2
>>> + 1 or you mean something else?
>>
>> "groupsize * 2 + 1" is not the amount of bytes hex_dump_to_buffer() wants
>> to write. It's only the size for one word.
>>
>> You could check if there are at least "32 * 3 + 2 + 32 + 1" bytes (your
>> old linebuf size) available.
>
> This is a good question why this number? What if we have to print only
> one byte (as different groupsize)?
I don't think complaining about a too-small buffer prematurely hurts.
> I think the requirement for one groupsize is quite okay.
Then you will loose data if the buffer is too small.
>> However, to protect against overflows if hex_dump_to_buffer() ever changes,
>> I think it would be better to let hex_dump_to_buffer() indicate if the
>> passed buffer was to small (it already checks the passed linebuflen).
>> Then you can just check for that.
>
> I thought about that. We may introduce either new call and make
> current one the user of it or change all occurrences.
> Nevertheless, currently it will print only one groupsize if there is
> enough room for it but for two or more.
> Thus, I prefer to keep the behaviour "print until we can".
The idea of seq_*() is that it will retry with a bigger bufsize if there's
not enough space.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/5] seq_file: provide an analogue of print_hex_dump()
On Mon, Sep 1, 2014 at 12:25 PM, Geert Uytterhoeven wrote: > Hi Andy, > > On Mon, Sep 1, 2014 at 11:09 AM, Andy Shevchenko > wrote: >> On Mon, 2014-09-01 at 10:58 +0200, Geert Uytterhoeven wrote: >>> On Mon, Sep 1, 2014 at 10:36 AM, Andy Shevchenko >>> wrote: >>> >> ... and extra copying for no good reason. Why not check that we have >>> >> enough space in buffer and generate directly into it? See what e.g. >>> >> seq_escape() is doing... >>> > >>> > What about this variant? >>> >>> I think it needs a call to seq_set_overflow() in case the buffer is too >>> small, >>> so the caller will retry with a bigger buffer. >> >> Yes, in two places it would be useful to do. > > Two places? I see only one, just before calling hex_dump_to_buffer. seq_putc doesn't set it as I can see. >> But what the condition for "buffer is too small", the same groupsize * 2 >> + 1 or you mean something else? > > "groupsize * 2 + 1" is not the amount of bytes hex_dump_to_buffer() wants > to write. It's only the size for one word. > > You could check if there are at least "32 * 3 + 2 + 32 + 1" bytes (your > old linebuf size) available. This is a good question why this number? What if we have to print only one byte (as different groupsize)? I think the requirement for one groupsize is quite okay. > However, to protect against overflows if hex_dump_to_buffer() ever changes, > I think it would be better to let hex_dump_to_buffer() indicate if the > passed buffer was to small (it already checks the passed linebuflen). > Then you can just check for that. I thought about that. We may introduce either new call and make current one the user of it or change all occurrences. Nevertheless, currently it will print only one groupsize if there is enough room for it but for two or more. Thus, I prefer to keep the behaviour "print until we can". -- With Best Regards, Andy Shevchenko -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/5] seq_file: provide an analogue of print_hex_dump()
Hi Andy,
On Mon, Sep 1, 2014 at 11:09 AM, Andy Shevchenko
wrote:
> On Mon, 2014-09-01 at 10:58 +0200, Geert Uytterhoeven wrote:
>> On Mon, Sep 1, 2014 at 10:36 AM, Andy Shevchenko
>> wrote:
>> >> ... and extra copying for no good reason. Why not check that we have
>> >> enough space in buffer and generate directly into it? See what e.g.
>> >> seq_escape() is doing...
>> >
>> > What about this variant?
>>
>> I think it needs a call to seq_set_overflow() in case the buffer is too
>> small,
>> so the caller will retry with a bigger buffer.
>
> Yes, in two places it would be useful to do.
Two places? I see only one, just before calling hex_dump_to_buffer.
> But what the condition for "buffer is too small", the same groupsize * 2
> + 1 or you mean something else?
"groupsize * 2 + 1" is not the amount of bytes hex_dump_to_buffer() wants
to write. It's only the size for one word.
You could check if there are at least "32 * 3 + 2 + 32 + 1" bytes (your
old linebuf size) available.
However, to protect against overflows if hex_dump_to_buffer() ever changes,
I think it would be better to let hex_dump_to_buffer() indicate if the
passed buffer was to small (it already checks the passed linebuflen).
Then you can just check for that.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/5] seq_file: provide an analogue of print_hex_dump()
On Mon, 2014-09-01 at 10:58 +0200, Geert Uytterhoeven wrote:
> Hi Andy,
>
> On Mon, Sep 1, 2014 at 10:36 AM, Andy Shevchenko
> wrote:
> >> ... and extra copying for no good reason. Why not check that we have
> >> enough space in buffer and generate directly into it? See what e.g.
> >> seq_escape() is doing...
> >
> > What about this variant?
>
> I think it needs a call to seq_set_overflow() in case the buffer is too small,
> so the caller will retry with a bigger buffer.
Yes, in two places it would be useful to do.
But what the condition for "buffer is too small", the same groupsize * 2
+ 1 or you mean something else?
>
> >
> > @@ -794,6 +795,47 @@ void seq_pad(struct seq_file *m, char c)
> > }
> > EXPORT_SYMBOL(seq_pad);
> >
> > +/* Analogue of print_hex_dump() */
> > +void seq_hex_dump(struct seq_file *m, const char *prefix_str, int
> > prefix_type,
> > + int rowsize, int groupsize, const void *buf, size_t len,
> > + bool ascii)
> > +{
> > + const u8 *ptr = buf;
> > + int i, linelen, remaining = len;
> > + int ret;
> > +
> > + if (rowsize != 16 && rowsize != 32)
> > + rowsize = 16;
> > +
> > + for (i = 0; i < len; i += rowsize) {
> > + linelen = min(remaining, rowsize);
> > + remaining -= rowsize;
> > +
> > + ret = seq_printf(m, "%s", prefix_str);
> > + if (ret < 0)
> > + break;
> > +
> > + if (prefix_type == DUMP_PREFIX_ADDRESS)
> > + ret = seq_printf(m, "%p: ", ptr + i);
> > + else if (prefix_type == DUMP_PREFIX_OFFSET)
> > + ret = seq_printf(m, "%.8x: ", i);
> > + if (ret < 0)
> > + break;
> > +
> > + if (m->size < m->count + groupsize * 2 + 1)
> > + break;
> > +
> > + hex_dump_to_buffer(ptr + i, linelen, rowsize, groupsize,
> > + m->buf + m->count, m->size - m->count,
> > + ascii);
> > +
> > + ret = seq_putc(m, '\n');
> > + if (ret < 0)
> > + break;
> > + }
> > +}
> > +EXPORT_SYMBOL(seq_hex_dump);
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> ge...@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like
> that.
> -- Linus Torvalds
--
Andy Shevchenko
Intel Finland Oy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/5] seq_file: provide an analogue of print_hex_dump()
Hi Andy,
On Mon, Sep 1, 2014 at 10:36 AM, Andy Shevchenko
wrote:
>> ... and extra copying for no good reason. Why not check that we have
>> enough space in buffer and generate directly into it? See what e.g.
>> seq_escape() is doing...
>
> What about this variant?
I think it needs a call to seq_set_overflow() in case the buffer is too small,
so the caller will retry with a bigger buffer.
>
> @@ -794,6 +795,47 @@ void seq_pad(struct seq_file *m, char c)
> }
> EXPORT_SYMBOL(seq_pad);
>
> +/* Analogue of print_hex_dump() */
> +void seq_hex_dump(struct seq_file *m, const char *prefix_str, int
> prefix_type,
> + int rowsize, int groupsize, const void *buf, size_t len,
> + bool ascii)
> +{
> + const u8 *ptr = buf;
> + int i, linelen, remaining = len;
> + int ret;
> +
> + if (rowsize != 16 && rowsize != 32)
> + rowsize = 16;
> +
> + for (i = 0; i < len; i += rowsize) {
> + linelen = min(remaining, rowsize);
> + remaining -= rowsize;
> +
> + ret = seq_printf(m, "%s", prefix_str);
> + if (ret < 0)
> + break;
> +
> + if (prefix_type == DUMP_PREFIX_ADDRESS)
> + ret = seq_printf(m, "%p: ", ptr + i);
> + else if (prefix_type == DUMP_PREFIX_OFFSET)
> + ret = seq_printf(m, "%.8x: ", i);
> + if (ret < 0)
> + break;
> +
> + if (m->size < m->count + groupsize * 2 + 1)
> + break;
> +
> + hex_dump_to_buffer(ptr + i, linelen, rowsize, groupsize,
> + m->buf + m->count, m->size - m->count,
> + ascii);
> +
> + ret = seq_putc(m, '\n');
> + if (ret < 0)
> + break;
> + }
> +}
> +EXPORT_SYMBOL(seq_hex_dump);
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/5] seq_file: provide an analogue of print_hex_dump()
On Sat, 2014-08-30 at 23:54 +0100, Al Viro wrote:
> On Mon, Aug 25, 2014 at 12:03:11PM +0300, Andy Shevchenko wrote:
> > The new seq_hex_dump() is a complete analogue of print_hex_dump().
> >
> > We have few users of this functionality already. It allows to reduce their
> > codebase.
>
> I really don't like the stack footprint.
>
> > + unsigned char linebuf[32 * 3 + 2 + 32 + 1];
>
> ... and extra copying for no good reason. Why not check that we have
> enough space in buffer and generate directly into it? See what e.g.
> seq_escape() is doing...
What about this variant?
@@ -794,6 +795,47 @@ void seq_pad(struct seq_file *m, char c)
}
EXPORT_SYMBOL(seq_pad);
+/* Analogue of print_hex_dump() */
+void seq_hex_dump(struct seq_file *m, const char *prefix_str, int prefix_type,
+ int rowsize, int groupsize, const void *buf, size_t len,
+ bool ascii)
+{
+ const u8 *ptr = buf;
+ int i, linelen, remaining = len;
+ int ret;
+
+ if (rowsize != 16 && rowsize != 32)
+ rowsize = 16;
+
+ for (i = 0; i < len; i += rowsize) {
+ linelen = min(remaining, rowsize);
+ remaining -= rowsize;
+
+ ret = seq_printf(m, "%s", prefix_str);
+ if (ret < 0)
+ break;
+
+ if (prefix_type == DUMP_PREFIX_ADDRESS)
+ ret = seq_printf(m, "%p: ", ptr + i);
+ else if (prefix_type == DUMP_PREFIX_OFFSET)
+ ret = seq_printf(m, "%.8x: ", i);
+ if (ret < 0)
+ break;
+
+ if (m->size < m->count + groupsize * 2 + 1)
+ break;
+
+ hex_dump_to_buffer(ptr + i, linelen, rowsize, groupsize,
+ m->buf + m->count, m->size - m->count,
+ ascii);
+
+ ret = seq_putc(m, '\n');
+ if (ret < 0)
+ break;
+ }
+}
+EXPORT_SYMBOL(seq_hex_dump);
--
Andy Shevchenko
Intel Finland Oy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/5] seq_file: provide an analogue of print_hex_dump()
On Sat, 2014-08-30 at 23:54 +0100, Al Viro wrote:
On Mon, Aug 25, 2014 at 12:03:11PM +0300, Andy Shevchenko wrote:
The new seq_hex_dump() is a complete analogue of print_hex_dump().
We have few users of this functionality already. It allows to reduce their
codebase.
I really don't like the stack footprint.
+ unsigned char linebuf[32 * 3 + 2 + 32 + 1];
... and extra copying for no good reason. Why not check that we have
enough space in buffer and generate directly into it? See what e.g.
seq_escape() is doing...
What about this variant?
@@ -794,6 +795,47 @@ void seq_pad(struct seq_file *m, char c)
}
EXPORT_SYMBOL(seq_pad);
+/* Analogue of print_hex_dump() */
+void seq_hex_dump(struct seq_file *m, const char *prefix_str, int prefix_type,
+ int rowsize, int groupsize, const void *buf, size_t len,
+ bool ascii)
+{
+ const u8 *ptr = buf;
+ int i, linelen, remaining = len;
+ int ret;
+
+ if (rowsize != 16 rowsize != 32)
+ rowsize = 16;
+
+ for (i = 0; i len; i += rowsize) {
+ linelen = min(remaining, rowsize);
+ remaining -= rowsize;
+
+ ret = seq_printf(m, %s, prefix_str);
+ if (ret 0)
+ break;
+
+ if (prefix_type == DUMP_PREFIX_ADDRESS)
+ ret = seq_printf(m, %p: , ptr + i);
+ else if (prefix_type == DUMP_PREFIX_OFFSET)
+ ret = seq_printf(m, %.8x: , i);
+ if (ret 0)
+ break;
+
+ if (m-size m-count + groupsize * 2 + 1)
+ break;
+
+ hex_dump_to_buffer(ptr + i, linelen, rowsize, groupsize,
+ m-buf + m-count, m-size - m-count,
+ ascii);
+
+ ret = seq_putc(m, '\n');
+ if (ret 0)
+ break;
+ }
+}
+EXPORT_SYMBOL(seq_hex_dump);
--
Andy Shevchenko andriy.shevche...@intel.com
Intel Finland Oy
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/5] seq_file: provide an analogue of print_hex_dump()
Hi Andy,
On Mon, Sep 1, 2014 at 10:36 AM, Andy Shevchenko
andriy.shevche...@linux.intel.com wrote:
... and extra copying for no good reason. Why not check that we have
enough space in buffer and generate directly into it? See what e.g.
seq_escape() is doing...
What about this variant?
I think it needs a call to seq_set_overflow() in case the buffer is too small,
so the caller will retry with a bigger buffer.
@@ -794,6 +795,47 @@ void seq_pad(struct seq_file *m, char c)
}
EXPORT_SYMBOL(seq_pad);
+/* Analogue of print_hex_dump() */
+void seq_hex_dump(struct seq_file *m, const char *prefix_str, int
prefix_type,
+ int rowsize, int groupsize, const void *buf, size_t len,
+ bool ascii)
+{
+ const u8 *ptr = buf;
+ int i, linelen, remaining = len;
+ int ret;
+
+ if (rowsize != 16 rowsize != 32)
+ rowsize = 16;
+
+ for (i = 0; i len; i += rowsize) {
+ linelen = min(remaining, rowsize);
+ remaining -= rowsize;
+
+ ret = seq_printf(m, %s, prefix_str);
+ if (ret 0)
+ break;
+
+ if (prefix_type == DUMP_PREFIX_ADDRESS)
+ ret = seq_printf(m, %p: , ptr + i);
+ else if (prefix_type == DUMP_PREFIX_OFFSET)
+ ret = seq_printf(m, %.8x: , i);
+ if (ret 0)
+ break;
+
+ if (m-size m-count + groupsize * 2 + 1)
+ break;
+
+ hex_dump_to_buffer(ptr + i, linelen, rowsize, groupsize,
+ m-buf + m-count, m-size - m-count,
+ ascii);
+
+ ret = seq_putc(m, '\n');
+ if (ret 0)
+ break;
+ }
+}
+EXPORT_SYMBOL(seq_hex_dump);
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say programmer or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/5] seq_file: provide an analogue of print_hex_dump()
On Mon, 2014-09-01 at 10:58 +0200, Geert Uytterhoeven wrote:
Hi Andy,
On Mon, Sep 1, 2014 at 10:36 AM, Andy Shevchenko
andriy.shevche...@linux.intel.com wrote:
... and extra copying for no good reason. Why not check that we have
enough space in buffer and generate directly into it? See what e.g.
seq_escape() is doing...
What about this variant?
I think it needs a call to seq_set_overflow() in case the buffer is too small,
so the caller will retry with a bigger buffer.
Yes, in two places it would be useful to do.
But what the condition for buffer is too small, the same groupsize * 2
+ 1 or you mean something else?
@@ -794,6 +795,47 @@ void seq_pad(struct seq_file *m, char c)
}
EXPORT_SYMBOL(seq_pad);
+/* Analogue of print_hex_dump() */
+void seq_hex_dump(struct seq_file *m, const char *prefix_str, int
prefix_type,
+ int rowsize, int groupsize, const void *buf, size_t len,
+ bool ascii)
+{
+ const u8 *ptr = buf;
+ int i, linelen, remaining = len;
+ int ret;
+
+ if (rowsize != 16 rowsize != 32)
+ rowsize = 16;
+
+ for (i = 0; i len; i += rowsize) {
+ linelen = min(remaining, rowsize);
+ remaining -= rowsize;
+
+ ret = seq_printf(m, %s, prefix_str);
+ if (ret 0)
+ break;
+
+ if (prefix_type == DUMP_PREFIX_ADDRESS)
+ ret = seq_printf(m, %p: , ptr + i);
+ else if (prefix_type == DUMP_PREFIX_OFFSET)
+ ret = seq_printf(m, %.8x: , i);
+ if (ret 0)
+ break;
+
+ if (m-size m-count + groupsize * 2 + 1)
+ break;
+
+ hex_dump_to_buffer(ptr + i, linelen, rowsize, groupsize,
+ m-buf + m-count, m-size - m-count,
+ ascii);
+
+ ret = seq_putc(m, '\n');
+ if (ret 0)
+ break;
+ }
+}
+EXPORT_SYMBOL(seq_hex_dump);
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say programmer or something like
that.
-- Linus Torvalds
--
Andy Shevchenko andriy.shevche...@intel.com
Intel Finland Oy
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/5] seq_file: provide an analogue of print_hex_dump()
Hi Andy,
On Mon, Sep 1, 2014 at 11:09 AM, Andy Shevchenko
andriy.shevche...@linux.intel.com wrote:
On Mon, 2014-09-01 at 10:58 +0200, Geert Uytterhoeven wrote:
On Mon, Sep 1, 2014 at 10:36 AM, Andy Shevchenko
andriy.shevche...@linux.intel.com wrote:
... and extra copying for no good reason. Why not check that we have
enough space in buffer and generate directly into it? See what e.g.
seq_escape() is doing...
What about this variant?
I think it needs a call to seq_set_overflow() in case the buffer is too
small,
so the caller will retry with a bigger buffer.
Yes, in two places it would be useful to do.
Two places? I see only one, just before calling hex_dump_to_buffer.
But what the condition for buffer is too small, the same groupsize * 2
+ 1 or you mean something else?
groupsize * 2 + 1 is not the amount of bytes hex_dump_to_buffer() wants
to write. It's only the size for one word.
You could check if there are at least 32 * 3 + 2 + 32 + 1 bytes (your
old linebuf size) available.
However, to protect against overflows if hex_dump_to_buffer() ever changes,
I think it would be better to let hex_dump_to_buffer() indicate if the
passed buffer was to small (it already checks the passed linebuflen).
Then you can just check for that.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say programmer or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/5] seq_file: provide an analogue of print_hex_dump()
On Mon, Sep 1, 2014 at 12:25 PM, Geert Uytterhoeven ge...@linux-m68k.org wrote: Hi Andy, On Mon, Sep 1, 2014 at 11:09 AM, Andy Shevchenko andriy.shevche...@linux.intel.com wrote: On Mon, 2014-09-01 at 10:58 +0200, Geert Uytterhoeven wrote: On Mon, Sep 1, 2014 at 10:36 AM, Andy Shevchenko andriy.shevche...@linux.intel.com wrote: ... and extra copying for no good reason. Why not check that we have enough space in buffer and generate directly into it? See what e.g. seq_escape() is doing... What about this variant? I think it needs a call to seq_set_overflow() in case the buffer is too small, so the caller will retry with a bigger buffer. Yes, in two places it would be useful to do. Two places? I see only one, just before calling hex_dump_to_buffer. seq_putc doesn't set it as I can see. But what the condition for buffer is too small, the same groupsize * 2 + 1 or you mean something else? groupsize * 2 + 1 is not the amount of bytes hex_dump_to_buffer() wants to write. It's only the size for one word. You could check if there are at least 32 * 3 + 2 + 32 + 1 bytes (your old linebuf size) available. This is a good question why this number? What if we have to print only one byte (as different groupsize)? I think the requirement for one groupsize is quite okay. However, to protect against overflows if hex_dump_to_buffer() ever changes, I think it would be better to let hex_dump_to_buffer() indicate if the passed buffer was to small (it already checks the passed linebuflen). Then you can just check for that. I thought about that. We may introduce either new call and make current one the user of it or change all occurrences. Nevertheless, currently it will print only one groupsize if there is enough room for it but for two or more. Thus, I prefer to keep the behaviour print until we can. -- With Best Regards, Andy Shevchenko -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/5] seq_file: provide an analogue of print_hex_dump()
Hi Andy,
On Mon, Sep 1, 2014 at 12:15 PM, Andy Shevchenko
andy.shevche...@gmail.com wrote:
I think it needs a call to seq_set_overflow() in case the buffer is too
small,
so the caller will retry with a bigger buffer.
Yes, in two places it would be useful to do.
Two places? I see only one, just before calling hex_dump_to_buffer.
seq_putc doesn't set it as I can see.
But what the condition for buffer is too small, the same groupsize * 2
+ 1 or you mean something else?
groupsize * 2 + 1 is not the amount of bytes hex_dump_to_buffer() wants
to write. It's only the size for one word.
You could check if there are at least 32 * 3 + 2 + 32 + 1 bytes (your
old linebuf size) available.
This is a good question why this number? What if we have to print only
one byte (as different groupsize)?
I don't think complaining about a too-small buffer prematurely hurts.
I think the requirement for one groupsize is quite okay.
Then you will loose data if the buffer is too small.
However, to protect against overflows if hex_dump_to_buffer() ever changes,
I think it would be better to let hex_dump_to_buffer() indicate if the
passed buffer was to small (it already checks the passed linebuflen).
Then you can just check for that.
I thought about that. We may introduce either new call and make
current one the user of it or change all occurrences.
Nevertheless, currently it will print only one groupsize if there is
enough room for it but for two or more.
Thus, I prefer to keep the behaviour print until we can.
The idea of seq_*() is that it will retry with a bigger bufsize if there's
not enough space.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say programmer or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/5] seq_file: provide an analogue of print_hex_dump()
On Mon, 2014-09-01 at 12:59 +0200, Geert Uytterhoeven wrote: Hi Andy, On Mon, Sep 1, 2014 at 12:15 PM, Andy Shevchenko andy.shevche...@gmail.com wrote: I think it needs a call to seq_set_overflow() in case the buffer is too small, so the caller will retry with a bigger buffer. Yes, in two places it would be useful to do. Two places? I see only one, just before calling hex_dump_to_buffer. seq_putc doesn't set it as I can see. But what the condition for buffer is too small, the same groupsize * 2 + 1 or you mean something else? groupsize * 2 + 1 is not the amount of bytes hex_dump_to_buffer() wants to write. It's only the size for one word. You could check if there are at least 32 * 3 + 2 + 32 + 1 bytes (your old linebuf size) available. This is a good question why this number? What if we have to print only one byte (as different groupsize)? I don't think complaining about a too-small buffer prematurely hurts. I think the requirement for one groupsize is quite okay. Then you will loose data if the buffer is too small. However, to protect against overflows if hex_dump_to_buffer() ever changes, I think it would be better to let hex_dump_to_buffer() indicate if the passed buffer was to small (it already checks the passed linebuflen). Then you can just check for that. I thought about that. We may introduce either new call and make current one the user of it or change all occurrences. Nevertheless, currently it will print only one groupsize if there is enough room for it but for two or more. Thus, I prefer to keep the behaviour print until we can. The idea of seq_*() is that it will retry with a bigger bufsize if there's not enough space. Fair enough. So, summarize above, the check before hex_dump_to_buffer() should care about maximum possible buffer needed for one line. But we have already rowsize calculated (we actually can't rely on groupsize in this case). Do you agree with formula rowsize * 3 + 2 + rowsize + 1? -- Andy Shevchenko andriy.shevche...@intel.com Intel Finland Oy -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/5] seq_file: provide an analogue of print_hex_dump()
Hi Andy,
On Mon, Sep 1, 2014 at 1:33 PM, Andy Shevchenko
andriy.shevche...@linux.intel.com wrote:
However, to protect against overflows if hex_dump_to_buffer() ever
changes,
I think it would be better to let hex_dump_to_buffer() indicate if the
passed buffer was to small (it already checks the passed linebuflen).
Then you can just check for that.
I thought about that. We may introduce either new call and make
current one the user of it or change all occurrences.
Nevertheless, currently it will print only one groupsize if there is
enough room for it but for two or more.
Thus, I prefer to keep the behaviour print until we can.
The idea of seq_*() is that it will retry with a bigger bufsize if there's
not enough space.
Fair enough.
So, summarize above, the check before hex_dump_to_buffer() should care
about maximum possible buffer needed for one line. But we have already
rowsize calculated (we actually can't rely on groupsize in this case).
Do you agree with formula rowsize * 3 + 2 + rowsize + 1?
That's the _current_ formula. The day hex_dump_to_buffer() is changed,
it will break silently.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say programmer or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/5] seq_file: provide an analogue of print_hex_dump()
On Mon, Sep 1, 2014 at 2:49 PM, Geert Uytterhoeven ge...@linux-m68k.org wrote: Hi Andy, On Mon, Sep 1, 2014 at 1:33 PM, Andy Shevchenko andriy.shevche...@linux.intel.com wrote: However, to protect against overflows if hex_dump_to_buffer() ever changes, I think it would be better to let hex_dump_to_buffer() indicate if the passed buffer was to small (it already checks the passed linebuflen). Then you can just check for that. I thought about that. We may introduce either new call and make current one the user of it or change all occurrences. Nevertheless, currently it will print only one groupsize if there is enough room for it but for two or more. Thus, I prefer to keep the behaviour print until we can. The idea of seq_*() is that it will retry with a bigger bufsize if there's not enough space. Fair enough. So, summarize above, the check before hex_dump_to_buffer() should care about maximum possible buffer needed for one line. But we have already rowsize calculated (we actually can't rely on groupsize in this case). Do you agree with formula rowsize * 3 + 2 + rowsize + 1? That's the _current_ formula. The day hex_dump_to_buffer() is changed, it will break silently. Do we have any other option except modify hex_dump_to_buffer() to return an error when buffer is not enough? In this case we require hex_dump_to_buffer() to follow this agreement even if it's changed once. -- With Best Regards, Andy Shevchenko -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/5] seq_file: provide an analogue of print_hex_dump()
On Mon, Aug 25, 2014 at 12:03:11PM +0300, Andy Shevchenko wrote: > The new seq_hex_dump() is a complete analogue of print_hex_dump(). > > We have few users of this functionality already. It allows to reduce their > codebase. I really don't like the stack footprint. > + unsigned char linebuf[32 * 3 + 2 + 32 + 1]; ... and extra copying for no good reason. Why not check that we have enough space in buffer and generate directly into it? See what e.g. seq_escape() is doing... -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/5] seq_file: provide an analogue of print_hex_dump()
On Mon, Aug 25, 2014 at 12:03:11PM +0300, Andy Shevchenko wrote: The new seq_hex_dump() is a complete analogue of print_hex_dump(). We have few users of this functionality already. It allows to reduce their codebase. I really don't like the stack footprint. + unsigned char linebuf[32 * 3 + 2 + 32 + 1]; ... and extra copying for no good reason. Why not check that we have enough space in buffer and generate directly into it? See what e.g. seq_escape() is doing... -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v3 1/5] seq_file: provide an analogue of print_hex_dump()
The new seq_hex_dump() is a complete analogue of print_hex_dump().
We have few users of this functionality already. It allows to reduce their
codebase.
Signed-off-by: Andy Shevchenko
---
fs/seq_file.c| 35 +++
include/linux/seq_file.h | 4
2 files changed, 39 insertions(+)
diff --git a/fs/seq_file.c b/fs/seq_file.c
index 3857b72..fec4a6b 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -12,6 +12,7 @@
#include
#include
#include
+#include
#include
#include
@@ -794,6 +795,40 @@ void seq_pad(struct seq_file *m, char c)
}
EXPORT_SYMBOL(seq_pad);
+/* Analogue of print_hex_dump() */
+void seq_hex_dump(struct seq_file *m, const char *prefix_str, int prefix_type,
+ int rowsize, int groupsize, const void *buf, size_t len,
+ bool ascii)
+{
+ const u8 *ptr = buf;
+ int i, linelen, remaining = len;
+ unsigned char linebuf[32 * 3 + 2 + 32 + 1];
+
+ if (rowsize != 16 && rowsize != 32)
+ rowsize = 16;
+
+ for (i = 0; i < len; i += rowsize) {
+ linelen = min(remaining, rowsize);
+ remaining -= rowsize;
+
+ hex_dump_to_buffer(ptr + i, linelen, rowsize, groupsize,
+ linebuf, sizeof(linebuf), ascii);
+
+ switch (prefix_type) {
+ case DUMP_PREFIX_ADDRESS:
+ seq_printf(m, "%s%p: %s\n", prefix_str, ptr + i,
linebuf);
+ break;
+ case DUMP_PREFIX_OFFSET:
+ seq_printf(m, "%s%.8x: %s\n", prefix_str, i, linebuf);
+ break;
+ default:
+ seq_printf(m, "%s%s\n", prefix_str, linebuf);
+ break;
+ }
+ }
+}
+EXPORT_SYMBOL(seq_hex_dump);
+
struct list_head *seq_list_start(struct list_head *head, loff_t pos)
{
struct list_head *lh;
diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h
index 52e0097..6a8be4c 100644
--- a/include/linux/seq_file.h
+++ b/include/linux/seq_file.h
@@ -107,6 +107,10 @@ int seq_write(struct seq_file *seq, const void *data,
size_t len);
__printf(2, 3) int seq_printf(struct seq_file *, const char *, ...);
__printf(2, 0) int seq_vprintf(struct seq_file *, const char *, va_list args);
+void seq_hex_dump(struct seq_file *m, const char *prefix_str, int prefix_type,
+ int rowsize, int groupsize, const void *buf, size_t len,
+ bool ascii);
+
int seq_path(struct seq_file *, const struct path *, const char *);
int seq_dentry(struct seq_file *, struct dentry *, const char *);
int seq_path_root(struct seq_file *m, const struct path *path,
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v3 1/5] seq_file: provide an analogue of print_hex_dump()
The new seq_hex_dump() is a complete analogue of print_hex_dump().
We have few users of this functionality already. It allows to reduce their
codebase.
Signed-off-by: Andy Shevchenko andriy.shevche...@linux.intel.com
---
fs/seq_file.c| 35 +++
include/linux/seq_file.h | 4
2 files changed, 39 insertions(+)
diff --git a/fs/seq_file.c b/fs/seq_file.c
index 3857b72..fec4a6b 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -12,6 +12,7 @@
#include linux/slab.h
#include linux/cred.h
#include linux/mm.h
+#include linux/printk.h
#include asm/uaccess.h
#include asm/page.h
@@ -794,6 +795,40 @@ void seq_pad(struct seq_file *m, char c)
}
EXPORT_SYMBOL(seq_pad);
+/* Analogue of print_hex_dump() */
+void seq_hex_dump(struct seq_file *m, const char *prefix_str, int prefix_type,
+ int rowsize, int groupsize, const void *buf, size_t len,
+ bool ascii)
+{
+ const u8 *ptr = buf;
+ int i, linelen, remaining = len;
+ unsigned char linebuf[32 * 3 + 2 + 32 + 1];
+
+ if (rowsize != 16 rowsize != 32)
+ rowsize = 16;
+
+ for (i = 0; i len; i += rowsize) {
+ linelen = min(remaining, rowsize);
+ remaining -= rowsize;
+
+ hex_dump_to_buffer(ptr + i, linelen, rowsize, groupsize,
+ linebuf, sizeof(linebuf), ascii);
+
+ switch (prefix_type) {
+ case DUMP_PREFIX_ADDRESS:
+ seq_printf(m, %s%p: %s\n, prefix_str, ptr + i,
linebuf);
+ break;
+ case DUMP_PREFIX_OFFSET:
+ seq_printf(m, %s%.8x: %s\n, prefix_str, i, linebuf);
+ break;
+ default:
+ seq_printf(m, %s%s\n, prefix_str, linebuf);
+ break;
+ }
+ }
+}
+EXPORT_SYMBOL(seq_hex_dump);
+
struct list_head *seq_list_start(struct list_head *head, loff_t pos)
{
struct list_head *lh;
diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h
index 52e0097..6a8be4c 100644
--- a/include/linux/seq_file.h
+++ b/include/linux/seq_file.h
@@ -107,6 +107,10 @@ int seq_write(struct seq_file *seq, const void *data,
size_t len);
__printf(2, 3) int seq_printf(struct seq_file *, const char *, ...);
__printf(2, 0) int seq_vprintf(struct seq_file *, const char *, va_list args);
+void seq_hex_dump(struct seq_file *m, const char *prefix_str, int prefix_type,
+ int rowsize, int groupsize, const void *buf, size_t len,
+ bool ascii);
+
int seq_path(struct seq_file *, const struct path *, const char *);
int seq_dentry(struct seq_file *, struct dentry *, const char *);
int seq_path_root(struct seq_file *m, const struct path *path,
--
2.1.0
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
