Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1542728 > unrolled thread

[PATCH] printk: Remove no longer used second struct cont

Back to article view | Back to linux.kernel

Contents

  [PATCH] printk: Remove no longer used second struct cont Geert Uytterhoeven <geert@linux-m68k.org> - 2016-12-15 14:30 +0100
    Re: [PATCH] printk: Remove no longer used second struct cont Petr Mladek <pmladek@suse.com> - 2016-12-15 17:30 +0100
      Re: [PATCH] printk: Remove no longer used second struct cont Joe Perches <joe@perches.com> - 2016-12-16 02:50 +0100
        Re: [PATCH] printk: Remove no longer used second struct cont Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2016-12-16 03:00 +0100
      Re: [PATCH] printk: Remove no longer used second struct cont Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2016-12-16 02:50 +0100
        Re: [PATCH] printk: Remove no longer used second struct cont Joe Perches <joe@perches.com> - 2016-12-16 03:00 +0100
          Re: [PATCH] printk: Remove no longer used second struct cont Linus Torvalds <torvalds@linux-foundation.org> - 2016-12-16 03:20 +0100
            Re: [PATCH] printk: Remove no longer used second struct cont Joe Perches <joe@perches.com> - 2016-12-16 03:40 +0100
              Re: [PATCH] printk: Remove no longer used second struct cont Junio C Hamano <gitster@pobox.com> - 2016-12-16 06:10 +0100
                Re: [PATCH] printk: Remove no longer used second struct cont Joe Perches <joe@perches.com> - 2016-12-16 07:10 +0100
        Re: [PATCH] printk: Remove no longer used second struct cont Linus Torvalds <torvalds@linux-foundation.org> - 2016-12-16 03:00 +0100
          Re: [PATCH] printk: Remove no longer used second struct cont Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2016-12-16 03:10 +0100
          Re: [PATCH] printk: Remove no longer used second struct cont Scott Matheina <scott@matheina.com> - 2016-12-18 20:30 +0100

#1542728 — [PATCH] printk: Remove no longer used second struct cont

If CONFIG_PRINTK=n:

    kernel/printk/printk.c:1893: warning: ‘cont’ defined but not used

Note that there are actually two different struct cont definitions and
objects: the first one is used if CONFIG_PRINTK=y, the second one became
unused by removing console_cont_flush().

Fixes: 5c2992ee7fd8a29d ("printk: remove console flushing special cases for partial buffered lines")
Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
 kernel/printk/printk.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index b3c454b733da8163..bc2e220ed2b0bce5 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -1885,12 +1885,6 @@ asmlinkage __visible int printk(const char *fmt, ...)
 static u64 log_first_seq;
 static u32 log_first_idx;
 static u64 log_next_seq;
-static struct cont {
-	size_t len;
-	size_t cons;
-	u8 level;
-	bool flushed:1;
-} cont;
 static char *log_text(const struct printk_log *msg) { return NULL; }
 static char *log_dict(const struct printk_log *msg) { return NULL; }
 static struct printk_log *log_from_idx(u32 idx) { return NULL; }
-- 
1.9.1

[toc] | [next] | [standalone]

#1542864

On Thu 2016-12-15 13:53:58, Geert Uytterhoeven wrote:
> If CONFIG_PRINTK=n:
> 
>     kernel/printk/printk.c:1893: warning: ‘cont’ defined but not used
> 
> Note that there are actually two different struct cont definitions and
> objects: the first one is used if CONFIG_PRINTK=y, the second one became
> unused by removing console_cont_flush().
> 
> Fixes: 5c2992ee7fd8a29d ("printk: remove console flushing special cases for partial buffered lines")

Great catch. It seems that nobody tried the build without CONFIG_PRINTK
at that time.


> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>

Signed-off-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

[toc] | [prev] | [next] | [standalone]

#1543167

On Fri, 2016-12-16 at 10:37 +0900, Sergey Senozhatsky wrote:
> On (12/15/16 17:23), Petr Mladek wrote:
> > On Thu 2016-12-15 13:53:58, Geert Uytterhoeven wrote:
> > > If CONFIG_PRINTK=n:
> > > 
> > >     kernel/printk/printk.c:1893: warning: ‘cont’ defined but not used
> > > 
> > > Note that there are actually two different struct cont definitions and
> > > objects: the first one is used if CONFIG_PRINTK=y, the second one became
> > > unused by removing console_cont_flush().
> > > 
> > > Fixes: 5c2992ee7fd8a29d ("printk: remove console flushing special cases for partial buffered lines")
> > 
> > Great catch. It seems that nobody tried the build without CONFIG_PRINTK
> > at that time.
> 
> ok... since the patch is a cosmetic tweak... can we add several more
> cosmetic changes to it? yes, I know, N things in one patch is "a bad thing",
> but those extra changes don't deserve to be in a separate patch.
> 
> basically I'm talking about a bunch of 80-cols fixups.
> if it's irrelevant then feel free to ignore it.

While it might be nice to do some 80 column wrapping,
the most common wrap style is to the open parenthesis.

I'd also like to split up printk.c into a bunch of
smaller, more logically self-contained files eventually.

[toc] | [prev] | [next] | [standalone]

#1543177

On (12/15/16 17:39), Joe Perches wrote:
> On Fri, 2016-12-16 at 10:37 +0900, Sergey Senozhatsky wrote:
> > On (12/15/16 17:23), Petr Mladek wrote:
> > > On Thu 2016-12-15 13:53:58, Geert Uytterhoeven wrote:
> > > > If CONFIG_PRINTK=n:
> > > > 
> > > >     kernel/printk/printk.c:1893: warning: ‘cont’ defined but not used
> > > > 
> > > > Note that there are actually two different struct cont definitions and
> > > > objects: the first one is used if CONFIG_PRINTK=y, the second one became
> > > > unused by removing console_cont_flush().
> > > > 
> > > > Fixes: 5c2992ee7fd8a29d ("printk: remove console flushing special cases for partial buffered lines")
> > > 
> > > Great catch. It seems that nobody tried the build without CONFIG_PRINTK
> > > at that time.
> > 
> > ok... since the patch is a cosmetic tweak... can we add several more
> > cosmetic changes to it? yes, I know, N things in one patch is "a bad thing",
> > but those extra changes don't deserve to be in a separate patch.
> > 
> > basically I'm talking about a bunch of 80-cols fixups.
> > if it's irrelevant then feel free to ignore it.
> 
> While it might be nice to do some 80 column wrapping,
> the most common wrap style is to the open parenthesis.

indeed. good morning to me. updated version below.


> I'd also like to split up printk.c into a bunch of
> smaller, more logically self-contained files eventually.

yes, I think you mentioned that before. well, I'm not sure I
see why would it make anyting better/simpler.

---

 kernel/printk/printk.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index bc2e220ed2b0..2319adaf2af2 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -1194,7 +1194,8 @@ static size_t print_prefix(const struct printk_log *msg, bool syslog, char *buf)
 	return len;
 }
 
-static size_t msg_print_text(const struct printk_log *msg, bool syslog, char *buf, size_t size)
+static size_t msg_print_text(const struct printk_log *msg, bool syslog,
+			     char *buf, size_t size)
 {
 	const char *text = log_text(msg);
 	size_t text_size = msg->text_len;
@@ -1600,7 +1601,8 @@ static void cont_flush(void)
 	cont.len = 0;
 }
 
-static bool cont_add(int facility, int level, enum log_flags flags, const char *text, size_t len)
+static bool cont_add(int facility, int level, enum log_flags flags,
+			const char *text, size_t len)
 {
 	/*
 	 * If ext consoles are present, flush and skip in-kernel
@@ -1636,7 +1638,9 @@ static bool cont_add(int facility, int level, enum log_flags flags, const char *
 	return true;
 }
 
-static size_t log_output(int facility, int level, enum log_flags lflags, const char *dict, size_t dictlen, char *text, size_t text_len)
+static size_t log_output(int facility, int level, enum log_flags lflags,
+			 const char *dict, size_t dictlen,
+			 char *text, size_t text_len)
 {
 	/*
 	 * If an earlier line was buffered, and we're a continuation
@@ -1651,7 +1655,10 @@ static size_t log_output(int facility, int level, enum log_flags lflags, const c
 		cont_flush();
 	}
 
-	/* Skip empty continuation lines that couldn't be added - they just flush */
+	/*
+	 * Skip empty continuation lines that couldn't be
+	 * added - they just flush
+	 */
 	if (!text_len && (lflags & LOG_CONT))
 		return 0;
 
@@ -1662,7 +1669,8 @@ static size_t log_output(int facility, int level, enum log_flags lflags, const c
 	}
 
 	/* Store it in the record log */
-	return log_store(facility, level, lflags, 0, dict, dictlen, text, text_len);
+	return log_store(facility, level, lflags, 0, dict, dictlen,
+			 text, text_len);
 }
 
 asmlinkage int vprintk_emit(int facility, int level,
@@ -1777,7 +1785,8 @@ asmlinkage int vprintk_emit(int facility, int level,
 	if (dict)
 		lflags |= LOG_PREFIX|LOG_NEWLINE;
 
-	printed_len += log_output(facility, level, lflags, dict, dictlen, text, text_len);
+	printed_len += log_output(facility, level, lflags, dict, dictlen,
+				  text, text_len);
 
 	logbuf_cpu = UINT_MAX;
 	raw_spin_unlock(&logbuf_lock);

[toc] | [prev] | [next] | [standalone]

#1543170

On (12/15/16 17:23), Petr Mladek wrote:
> On Thu 2016-12-15 13:53:58, Geert Uytterhoeven wrote:
> > If CONFIG_PRINTK=n:
> > 
> >     kernel/printk/printk.c:1893: warning: ‘cont’ defined but not used
> > 
> > Note that there are actually two different struct cont definitions and
> > objects: the first one is used if CONFIG_PRINTK=y, the second one became
> > unused by removing console_cont_flush().
> > 
> > Fixes: 5c2992ee7fd8a29d ("printk: remove console flushing special cases for partial buffered lines")
> 
> Great catch. It seems that nobody tried the build without CONFIG_PRINTK
> at that time.

ok... since the patch is a cosmetic tweak... can we add several more
cosmetic changes to it? yes, I know, N things in one patch is "a bad thing",
but those extra changes don't deserve to be in a separate patch.

basically I'm talking about a bunch of 80-cols fixups.
if it's irrelevant then feel free to ignore it.

---

 kernel/printk/printk.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index bc2e220ed2b0..d09b4f0537ee 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -1194,7 +1194,8 @@ static size_t print_prefix(const struct printk_log *msg, bool syslog, char *buf)
 	return len;
 }
 
-static size_t msg_print_text(const struct printk_log *msg, bool syslog, char *buf, size_t size)
+static size_t msg_print_text(const struct printk_log *msg, bool syslog,
+		char *buf, size_t size)
 {
 	const char *text = log_text(msg);
 	size_t text_size = msg->text_len;
@@ -1636,7 +1637,8 @@ static bool cont_add(int facility, int level, enum log_flags flags, const char *
 	return true;
 }
 
-static size_t log_output(int facility, int level, enum log_flags lflags, const char *dict, size_t dictlen, char *text, size_t text_len)
+static size_t log_output(int facility, int level, enum log_flags lflags,
+		const char *dict, size_t dictlen, char *text, size_t text_len)
 {
 	/*
 	 * If an earlier line was buffered, and we're a continuation
@@ -1651,7 +1653,10 @@ static size_t log_output(int facility, int level, enum log_flags lflags, const c
 		cont_flush();
 	}
 
-	/* Skip empty continuation lines that couldn't be added - they just flush */
+	/*
+	 * Skip empty continuation lines that couldn't
+	 * be added - they just flush
+	 */
 	if (!text_len && (lflags & LOG_CONT))
 		return 0;
 
@@ -1662,7 +1667,8 @@ static size_t log_output(int facility, int level, enum log_flags lflags, const c
 	}
 
 	/* Store it in the record log */
-	return log_store(facility, level, lflags, 0, dict, dictlen, text, text_len);
+	return log_store(facility, level, lflags, 0, dict, dictlen,
+			text, text_len);
 }
 
 asmlinkage int vprintk_emit(int facility, int level,
@@ -1777,7 +1783,8 @@ asmlinkage int vprintk_emit(int facility, int level,
 	if (dict)
 		lflags |= LOG_PREFIX|LOG_NEWLINE;
 
-	printed_len += log_output(facility, level, lflags, dict, dictlen, text, text_len);
+	printed_len += log_output(facility, level, lflags, dict, dictlen,
+			text, text_len);
 
 	logbuf_cpu = UINT_MAX;
 	raw_spin_unlock(&logbuf_lock);

[toc] | [prev] | [next] | [standalone]

#1543174

On Thu, 2016-12-15 at 17:50 -0800, Linus Torvalds wrote:
> On Thu, Dec 15, 2016 at 5:37 PM, Sergey Senozhatsky
> <sergey.senozhatsky.work@gmail.com> wrote:
> > 
> > basically I'm talking about a bunch of 80-cols fixups.
> 
> Please don't.
> 
> Nobody uses a vt100 terminal any more. The 80-column wrapping is
> excessive, and makes things like "grep" not work as well.
> 
> No, we still don't want excessively long lines, but that's generally
> mainly because
> 
>  (a) we don't want to have excessively _complicated_ lines
> 
>  (b) we don't want to have excessively deep indentation (so if line
> length is due to 4+ levels of indentation, that's usually the primary
> problem).
> 
>  (c) email quoting gets iffier and uglier, so short lines always are
> preferred if possible
> 
> but in general, aside from those concerns, a long legible line is
> generally preferred over just adding line breaks for the very
> _occasional_ line.
> 
> At the 100-column mark you almost have to break, because at that point
> people may start to be actually limited by their displays, but 80
> columns generally isn't it.
> 
> In fact, I thought we already upped the check-patch limit to 100?

Nope, CodingStyle neither.

Last time I tried was awhile ago.

[toc] | [prev] | [next] | [standalone]

#1543181

On Thu, Dec 15, 2016 at 5:57 PM, Joe Perches <joe@perches.com> wrote:
>>
>> In fact, I thought we already upped the check-patch limit to 100?
>
> Nope, CodingStyle neither.
>
> Last time I tried was awhile ago.

Ok, it must have been just talked about, and with the exceptions for
strings etc I may not have seen as many of the really annoying line
breaks lately.

I don't mind a 80-column "soft limit" per se: if some code
consistently goes over 80 columns, there really is something seriously
wrong there. So 80 columns may well be the right limit for that kind
of check (or even less).

But if we have just a couple of lines that are longer (in a file that
is 3k+ lines), I'd rather not break those.

I tend use "git grep" a lot, and it's much easier to see function
argument use if it's all on one line.

Of course, some function calls really are *so* long that they have to
be broken up, but that's where the "if it's a couple of lines that go
a bit over the 80 column limit..." exception basically comes in.

Put another way: long lines definitely aren't good. But breaking long
lines has some downsides too, so there should be a balance between the
two, rather than some black-and-white limit.

In fact, we've seldom had cases where black-and-white limits work well.

                     Linus

[toc] | [prev] | [next] | [standalone]

#1543185

On Thu, 2016-12-15 at 18:10 -0800, Linus Torvalds wrote:
> On Thu, Dec 15, 2016 at 5:57 PM, Joe Perches <joe@perches.com> wrote:
> > > 
> > > In fact, I thought we already upped the check-patch limit to 100?
> > 
> > Nope, CodingStyle neither.
> > 
> > Last time I tried was awhile ago.
> 
> Ok, it must have been just talked about, and with the exceptions for
> strings etc I may not have seen as many of the really annoying line
> breaks lately.
> 
> I don't mind a 80-column "soft limit" per se: if some code
> consistently goes over 80 columns, there really is something seriously
> wrong there. So 80 columns may well be the right limit for that kind
> of check (or even less).

Newspaper column widths were relatively small for a good reason.

I think most of the uses of simple statements should be on a single
line.  I'd rather see just a few arguments on a single line than a
dozen though.  Especially those with long identifiers, functions
with many arguments are just difficult to visually scan.

> But if we have just a couple of lines that are longer (in a file that
> is 3k+ lines), I'd rather not break those.
> 
> I tend use "git grep" a lot, and it's much easier to see function
> argument use if it's all on one line.
> 
> Of course, some function calls really are *so* long that they have to
> be broken up, but that's where the "if it's a couple of lines that go
> a bit over the 80 column limit..." exception basically comes in.
> 
> Put another way: long lines definitely aren't good. But breaking long
> lines has some downsides too, so there should be a balance between the
> two, rather than some black-and-white limit.
> 
> In fact, we've seldom had cases where black-and-white limits work well.

One thing that _would_ be useful is some enhancement to git grep
that would look for multi-line statements more easily.

The git grep -P option doesn't span lines.

grep 2.5.4 was the last version that supported the -P option to
grep through for multiple lines.

It'd be nice to have something like
	git grep --code_style=c90 --function <foo>

that'd show all multiple line uses/definitions/declarations of a
particular function.

I played with extending git grep a bit once, mostly to get the \s
mechanism to span lines.  It kinda worked.

Still, it seems like real work to implement well.

[toc] | [prev] | [next] | [standalone]

#1543203

Joe Perches <joe@perches.com> writes:

> grep 2.5.4 was the last version that supported the -P option to
> grep through for multiple lines.

Does anybody know why it was dropped?

[toc] | [prev] | [next] | [standalone]

#1543210

On Thu, 2016-12-15 at 21:00 -0800, Junio C Hamano wrote:
> Joe Perches <joe@perches.com> writes:
> 
> > grep 2.5.4 was the last version that supported the -P option to
> > grep through for multiple lines.
> 
> Does anybody know why it was dropped?

perl compatible regexes in grep have always been "experimental"
and never officially supported.

From the grep manual https://www.gnu.org/software/grep/manual/grep.html

    --perl-regexp

        Interpret the pattern as a Perl-compatible regular expression
    (PCRE). This is highly experimental, particularly when combined with
    the -z (--null-data) option, and ‘grep -P’ may warn of unimplemented
    features. See Other Options.


It wasn't dropped so much as "enhanced" away.

Oh well.

[toc] | [prev] | [next] | [standalone]

#1543176

On Thu, Dec 15, 2016 at 5:37 PM, Sergey Senozhatsky
<sergey.senozhatsky.work@gmail.com> wrote:
>
> basically I'm talking about a bunch of 80-cols fixups.

Please don't.

Nobody uses a vt100 terminal any more. The 80-column wrapping is
excessive, and makes things like "grep" not work as well.

No, we still don't want excessively long lines, but that's generally
mainly because

 (a) we don't want to have excessively _complicated_ lines

 (b) we don't want to have excessively deep indentation (so if line
length is due to 4+ levels of indentation, that's usually the primary
problem).

 (c) email quoting gets iffier and uglier, so short lines always are
preferred if possible

but in general, aside from those concerns, a long legible line is
generally preferred over just adding line breaks for the very
_occasional_ line.

At the 100-column mark you almost have to break, because at that point
people may start to be actually limited by their displays, but 80
columns generally isn't it.

In fact, I thought we already upped the check-patch limit to 100?

              Linus

[toc] | [prev] | [next] | [standalone]

#1543179

On (12/15/16 17:50), Linus Torvalds wrote:
> On Thu, Dec 15, 2016 at 5:37 PM, Sergey Senozhatsky
> <sergey.senozhatsky.work@gmail.com> wrote:
> >
> > basically I'm talking about a bunch of 80-cols fixups.
> 
> Please don't.

I was really going to ask "do we still follow the 80 cols rule?" as
the first line in that email, but then I looked into scripts/checkpatch.pl

	my $max_line_length = 80;

and assumed that the rule is still active.

> Nobody uses a vt100 terminal any more. The 80-column wrapping is
> excessive, and makes things like "grep" not work as well.
> 
> No, we still don't want excessively long lines, but that's generally
> mainly because
> 
>  (a) we don't want to have excessively _complicated_ lines
> 
>  (b) we don't want to have excessively deep indentation (so if line
> length is due to 4+ levels of indentation, that's usually the primary
> problem).
> 
>  (c) email quoting gets iffier and uglier, so short lines always are
> preferred if possible
> 
> but in general, aside from those concerns, a long legible line is
> generally preferred over just adding line breaks for the very
> _occasional_ line.

ok. I was 99% sure those 80+ cols lines were not accidental.

> At the 100-column mark you almost have to break, because at that point
> people may start to be actually limited by their displays, but 80
> columns generally isn't it.
> 
> In fact, I thought we already upped the check-patch limit to 100?

I believe someone proposed it at the last kernel summit (or at least
attempted to propose it, but I'm not sure if it was successful).

thanks.

	-ss

[toc] | [prev] | [next] | [standalone]

#1544191

On 12/15/2016 07:50 PM, Linus Torvalds wrote:
> On Thu, Dec 15, 2016 at 5:37 PM, Sergey Senozhatsky
> <sergey.senozhatsky.work@gmail.com> wrote:
>> basically I'm talking about a bunch of 80-cols fixups.
> Please don't.
>
> Nobody uses a vt100 terminal any more. The 80-column wrapping is
> excessive, and makes things like "grep" not work as well.
>
> No, we still don't want excessively long lines, but that's generally
> mainly because
>
>   (a) we don't want to have excessively _complicated_ lines
>
>   (b) we don't want to have excessively deep indentation (so if line
> length is due to 4+ levels of indentation, that's usually the primary
> problem).
>
>   (c) email quoting gets iffier and uglier, so short lines always are
> preferred if possible
>
> but in general, aside from those concerns, a long legible line is
> generally preferred over just adding line breaks for the very
> _occasional_ line.
>
> At the 100-column mark you almost have to break, because at that point
> people may start to be actually limited by their displays, but 80
> columns generally isn't it.
>
> In fact, I thought we already upped the check-patch limit to 100?
checkpatch.pl line 50
my $max_line_length = 80;

Not changed as of yet.
>
>                Linus

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web