From: "Steven Rostedt (Red Hat)" <[email protected]>
Add a seq_buf_can_fit() helper function that removes the possible mistakes
of comparing the seq_buf length plus added data compared to the size of
the buffer.
Signed-off-by: Steven Rostedt <[email protected]>
---
kernel/trace/seq_buf.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/kernel/trace/seq_buf.c b/kernel/trace/seq_buf.c
index ce17f65268ed..89d1bd5c27fe 100644
--- a/kernel/trace/seq_buf.c
+++ b/kernel/trace/seq_buf.c
@@ -16,6 +16,11 @@
#include <linux/seq_file.h>
#include <linux/seq_buf.h>
+static bool seq_buf_can_fit(struct seq_buf *s, size_t len)
+{
+ return s->len + len < s->size;
+}
+
/**
* seq_buf_print_seq - move the contents of seq_buf into a seq_file
* @m: the seq_file descriptor that is the destination
@@ -48,7 +53,7 @@ int seq_buf_vprintf(struct seq_buf *s, const char *fmt, va_list args)
if (s->len < s->size) {
len = vsnprintf(s->buffer + s->len, s->size - s->len, fmt, args);
- if (s->len + len < s->size) {
+ if (seq_buf_can_fit(s, len)) {
s->len += len;
return 0;
}
@@ -137,7 +142,7 @@ int seq_buf_bprintf(struct seq_buf *s, const char *fmt, const u32 *binary)
if (s->len < s->size) {
ret = bstr_printf(s->buffer + s->len, len, fmt, binary);
- if (s->len + ret < s->size) {
+ if (seq_buf_can_fit(s, ret)) {
s->len += ret;
return 0;
}
@@ -161,7 +166,7 @@ int seq_buf_puts(struct seq_buf *s, const char *str)
WARN_ON(s->size == 0);
- if (s->len + len < s->size) {
+ if (seq_buf_can_fit(s, len)) {
memcpy(s->buffer + s->len, str, len);
s->len += len;
return 0;
@@ -183,7 +188,7 @@ int seq_buf_putc(struct seq_buf *s, unsigned char c)
{
WARN_ON(s->size == 0);
- if (s->len + 1 < s->size) {
+ if (seq_buf_can_fit(s, 1)) {
s->buffer[s->len++] = c;
return 0;
}
@@ -207,7 +212,7 @@ int seq_buf_putmem(struct seq_buf *s, const void *mem, unsigned int len)
{
WARN_ON(s->size == 0);
- if (s->len + len < s->size) {
+ if (seq_buf_can_fit(s, len)) {
memcpy(s->buffer + s->len, mem, len);
s->len += len;
return 0;
--
2.1.1
On Fri, 14 Nov 2014 23:59:07 -0500
Steven Rostedt <[email protected]> wrote:
> From: "Steven Rostedt (Red Hat)" <[email protected]>
>
> Add a seq_buf_can_fit() helper function that removes the possible mistakes
> of comparing the seq_buf length plus added data compared to the size of
> the buffer.
>
> Signed-off-by: Steven Rostedt <[email protected]>
> ---
> kernel/trace/seq_buf.c | 15 ++++++++++-----
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/trace/seq_buf.c b/kernel/trace/seq_buf.c
> index ce17f65268ed..89d1bd5c27fe 100644
> --- a/kernel/trace/seq_buf.c
> +++ b/kernel/trace/seq_buf.c
> @@ -16,6 +16,11 @@
> #include <linux/seq_file.h>
> #include <linux/seq_buf.h>
>
I'm adding a comment here:
/**
* seq_buf_can_fit - can the new data fit in the current buffer?
* @s: the seq_buf descriptor
* @len: The length to see if it can fit in the current buffer
*
* Returns true if there's enough unused space in the seq_buf buffer
* to fit the amount of new data according to @len.
*/
-- Steve
> +static bool seq_buf_can_fit(struct seq_buf *s, size_t len)
> +{
> + return s->len + len < s->size;
> +}
> +
On Mon, 2014-11-17 at 12:36 -0500, Steven Rostedt wrote:
> On Fri, 14 Nov 2014 23:59:07 -0500
> Steven Rostedt <[email protected]> wrote:
>
> > From: "Steven Rostedt (Red Hat)" <[email protected]>
> >
> > Add a seq_buf_can_fit() helper function that removes the possible mistakes
> > of comparing the seq_buf length plus added data compared to the size of
> > the buffer.
[]
> > +static bool seq_buf_can_fit(struct seq_buf *s, size_t len)
> > +{
> > + return s->len + len < s->size;
> > +}
> > +
Why is this useful?
On Mon, 17 Nov 2014 16:07:33 -0800
Joe Perches <[email protected]> wrote:
> On Mon, 2014-11-17 at 12:36 -0500, Steven Rostedt wrote:
> > On Fri, 14 Nov 2014 23:59:07 -0500
> > Steven Rostedt <[email protected]> wrote:
> >
> > > From: "Steven Rostedt (Red Hat)" <[email protected]>
> > >
> > > Add a seq_buf_can_fit() helper function that removes the possible mistakes
> > > of comparing the seq_buf length plus added data compared to the size of
> > > the buffer.
> []
> > > +static bool seq_buf_can_fit(struct seq_buf *s, size_t len)
> > > +{
> > > + return s->len + len < s->size;
> > > +}
> > > +
>
> Why is this useful?
Places the logic in one place and makes the next patch much shorter.
As the change log states, makes mistakes much less likely to happen
(note, I made one doing the next change and this makes me more
confident not to do another one)
-- Steve
On Mon, 2014-11-17 at 19:27 -0500, Steven Rostedt wrote:
> On Mon, 17 Nov 2014 16:07:33 -0800
> Joe Perches <[email protected]> wrote:
>
> > On Mon, 2014-11-17 at 12:36 -0500, Steven Rostedt wrote:
> > > On Fri, 14 Nov 2014 23:59:07 -0500
> > > Steven Rostedt <[email protected]> wrote:
> > >
> > > > From: "Steven Rostedt (Red Hat)" <[email protected]>
> > > >
> > > > Add a seq_buf_can_fit() helper function that removes the possible mistakes
> > > > of comparing the seq_buf length plus added data compared to the size of
> > > > the buffer.
> > []
> > > > +static bool seq_buf_can_fit(struct seq_buf *s, size_t len)
> > > > +{
> > > > + return s->len + len < s->size;
> > > > +}
> > > > +
> >
> > Why is this useful?
>
> Places the logic in one place and makes the next patch much shorter.
What "logic" does it place in one place and
how does it matter?
I don't see it making mistakes more or less
likely, I just see it being used to avoid
setting the overflow state which seems like
more of an error than anything else.
Why avoid setting overflow at all?
On Mon, 17 Nov 2014 16:35:32 -0800
Joe Perches <[email protected]> wrote:
> What "logic" does it place in one place and
> how does it matter?
Look at the next patch.
>
> I don't see it making mistakes more or less
> likely, I just see it being used to avoid
> setting the overflow state which seems like
> more of an error than anything else.
>
> Why avoid setting overflow at all?
>
It has nothing to do with overflow. Where did you get that from?
It has to do with updating the seq->len value or not.
-- Steve
On Mon, 2014-11-17 at 19:56 -0500, Steven Rostedt wrote:
> On Mon, 17 Nov 2014 16:35:32 -0800
> Joe Perches <[email protected]> wrote:
>
>
> > What "logic" does it place in one place and
> > how does it matter?
>
> Look at the next patch.
I don't have it and you are not cc'ing me.
I think you are getting carried away with the helpers.
> > I don't see it making mistakes more or less
> > likely, I just see it being used to avoid
> > setting the overflow state which seems like
> > more of an error than anything else.
> >
> > Why avoid setting overflow at all?
[]
> It has nothing to do with overflow. Where did you get that from?
writing to seq_buf really only cares about overflow.
seq_buf -> write to buffer -> overflowed?
expand buffer, redo everything else when finished,
dump buffer
On Mon, 17 Nov 2014 17:07:58 -0800
Joe Perches <[email protected]> wrote:
> > Look at the next patch.
>
> I don't have it and you are not cc'ing me.
It's on LKML.
> I think you are getting carried away with the helpers.
That's nice.
>
> > > I don't see it making mistakes more or less
> > > likely, I just see it being used to avoid
> > > setting the overflow state which seems like
> > > more of an error than anything else.
> > >
> > > Why avoid setting overflow at all?
> []
> > It has nothing to do with overflow. Where did you get that from?
>
> writing to seq_buf really only cares about overflow.
>
> seq_buf -> write to buffer -> overflowed?
> expand buffer, redo everything else when finished,
> dump buffer
Um, that may be the case for seq_file, but it is not the case for
trace_seq. seq_buf is influenced by seq_file because I have a patch to
make seq_file use it, but it's also the guts of trace_seq that has some
different requirements. And it's also not the case with the users of
seq_buf in the last patch.
-- Steve
On Mon, 2014-11-17 at 20:24 -0500, Steven Rostedt wrote:
> On Mon, 17 Nov 2014 17:07:58 -0800
> Joe Perches <[email protected]> wrote:
> > > Look at the next patch.
> > I don't have it and you are not cc'ing me.
> It's on LKML.
And? There's no convenient way to retrieve it.
> > I think you are getting carried away with the helpers.
> That's nice.
And possibly true.
> > > > I don't see it making mistakes more or less
> > > > likely, I just see it being used to avoid
> > > > setting the overflow state which seems like
> > > > more of an error than anything else.
> > > >
> > > > Why avoid setting overflow at all?
> > []
> > > It has nothing to do with overflow. Where did you get that from?
> >
> > writing to seq_buf really only cares about overflow.
> >
> > seq_buf -> write to buffer -> overflowed?
> > expand buffer, redo everything else when finished,
> > dump buffer
>
> Um, that may be the case for seq_file, but it is not the case for
> trace_seq. seq_buf is influenced by seq_file because I have a patch to
> make seq_file use it, but it's also the guts of trace_seq that has some
> different requirements. And it's also not the case with the users of
> seq_buf in the last patch.
I think your patch subject description needs expanding.
It says seq_buf, nothing about trace.
Perhaps making trace use seq internals is not the right
thing to do.
I also think you should break up this perhaps overlarge
patch set into multiple independent sets that can be applied
in separate chucks rather than all at once.
On Mon, 17 Nov 2014 17:48:37 -0800
Joe Perches <[email protected]> wrote:
> On Mon, 2014-11-17 at 20:24 -0500, Steven Rostedt wrote:
> > On Mon, 17 Nov 2014 17:07:58 -0800
> > Joe Perches <[email protected]> wrote:
> > > > Look at the next patch.
> > > I don't have it and you are not cc'ing me.
> > It's on LKML.
>
> And? There's no convenient way to retrieve it.
You're not subscribed?
> > Um, that may be the case for seq_file, but it is not the case for
> > trace_seq. seq_buf is influenced by seq_file because I have a patch to
> > make seq_file use it, but it's also the guts of trace_seq that has some
> > different requirements. And it's also not the case with the users of
> > seq_buf in the last patch.
>
> I think your patch subject description needs expanding.
> It says seq_buf, nothing about trace.
It doesn't need to. This helps out the code. seq_buf has nothing to do
with seq_file (yet).
>
> Perhaps making trace use seq internals is not the right
> thing to do.
But this code comes from the trace_seq internals. It's a way to combine
the code between seq_file and trace_seq. It is influenced by seq_file,
but the code is trace_seq. Actually, this patch set has nothing to do
with seq_file and finishes up solving a problem with printks dump
stacks from NMIs.
Look at the path of the seq_buf.c code.
>
> I also think you should break up this perhaps overlarge
> patch set into multiple independent sets that can be applied
> in separate chucks rather than all at once.
>
Why? I'm the one that is maintaining it. It's going to go through my
tree and it has almost all the acks I need.
I will say the first 13 patches have already been acked and reviewed,
and are going to be going into my for-next queue soon. I'll be posting
a smaller set with the patches that changed.
Well, all but the last 3 patches. Those are the ones that I'm going to
work with Linus and Andrew on before I pull them into my tree.
-- Steve
On Mon, 2014-11-17 at 21:37 -0500, Steven Rostedt wrote:
> On Mon, 17 Nov 2014 17:48:37 -0800
> Joe Perches <[email protected]> wrote:
> > On Mon, 2014-11-17 at 20:24 -0500, Steven Rostedt wrote:
> > > On Mon, 17 Nov 2014 17:07:58 -0800
> > > Joe Perches <[email protected]> wrote:
> > > > > Look at the next patch.
> > > > I don't have it and you are not cc'ing me.
> > > It's on LKML.
> >
> > And? There's no convenient way to retrieve it.
>
> You're not subscribed?
I am subscribed and I delete stuff, don't you?
> > > Um, that may be the case for seq_file, but it is not the case for
> > > trace_seq. seq_buf is influenced by seq_file because I have a patch to
> > > make seq_file use it, but it's also the guts of trace_seq that has some
> > > different requirements. And it's also not the case with the users of
> > > seq_buf in the last patch.
> >
> > I think your patch subject description needs expanding.
> > It says seq_buf, nothing about trace.
>
> It doesn't need to. This helps out the code. seq_buf has nothing to do
> with seq_file (yet).
No, I don't think it does help seq_buf. It only helps trace.
btw:
I think you should cc Al Viro on the entire patch set.
On Mon, 17 Nov 2014 18:50:52 -0800
Joe Perches <[email protected]> wrote:
> On Mon, 2014-11-17 at 21:37 -0500, Steven Rostedt wrote:
> > On Mon, 17 Nov 2014 17:48:37 -0800
> > Joe Perches <[email protected]> wrote:
> > > On Mon, 2014-11-17 at 20:24 -0500, Steven Rostedt wrote:
> > > > On Mon, 17 Nov 2014 17:07:58 -0800
> > > > Joe Perches <[email protected]> wrote:
> > > > > > Look at the next patch.
> > > > > I don't have it and you are not cc'ing me.
> > > > It's on LKML.
> > >
> > > And? There's no convenient way to retrieve it.
> >
> > You're not subscribed?
>
> I am subscribed and I delete stuff, don't you?
Nope, I keep all LKML messages. I have archives since 2005.
>
> > > > Um, that may be the case for seq_file, but it is not the case for
> > > > trace_seq. seq_buf is influenced by seq_file because I have a patch to
> > > > make seq_file use it, but it's also the guts of trace_seq that has some
> > > > different requirements. And it's also not the case with the users of
> > > > seq_buf in the last patch.
> > >
> > > I think your patch subject description needs expanding.
> > > It says seq_buf, nothing about trace.
> >
> > It doesn't need to. This helps out the code. seq_buf has nothing to do
> > with seq_file (yet).
>
> No, I don't think it does help seq_buf. It only helps trace.
What are you talking about? seq_buf did not exist until this patch set.
>
> btw:
>
> I think you should cc Al Viro on the entire patch set.
>
Why? Currently this is only tracing code. There's nothing in here that
has anything to do with VFS.
When I post patches to convert seq_file to use seq_buf, then I'll Cc Al.
-- Steve
On Fri 2014-11-14 23:59:07, Steven Rostedt wrote:
> From: "Steven Rostedt (Red Hat)" <[email protected]>
>
> Add a seq_buf_can_fit() helper function that removes the possible mistakes
> of comparing the seq_buf length plus added data compared to the size of
> the buffer.
>
> Signed-off-by: Steven Rostedt <[email protected]>
Reviewed-by: Petr Mladek <[email protected]>
Best Regards,
Petr