linux-kernel - Re: [PATCH next v2 3/3] printk: remove logbuf_lock, add syslog

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <871rg2oe9g.fsf@jogness.linutronix.de>
Date:   Sun, 06 Dec 2020 21:50:59 +0106
From:   John Ogness <john.ogness@...utronix.de>
To:     Sergey Senozhatsky <sergey.senozhatsky@...il.com>
Cc:     Petr Mladek <pmladek@...e.com>,
        Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH next v2 3/3] printk: remove logbuf_lock, add syslog_lock

On 2020-12-04, Sergey Senozhatsky <sergey.senozhatsky@...il.com> wrote:
>> +static char *get_printk_count(void)
>
> A nit: I think get_foo() has some sort of special meaning and one
> would expect that there should be put_foo() as well, and that those
> have something to do with the ref counting.

OK. How about:

static char *printk_recursion_counter(void)

?

>> +static bool printk_enter(unsigned long *flags)
>
> This better explicitly state that it disables local IRQs
>
> 	printk_enter_irqsave()
>
> I'm not very certain that printk_enter/printk_exit are best names:
>
> 	if (!printk_enter())
> 		return;
>
> Maybe it can spell out why we couldn't enter printk so the function
> name can contain recursion_limit or something.

printk_recurse_irqsave() ?

I would prefer printk_enter_irqsave() and just add comments that say it
is checking the recursion level. It is only used in one place.

>> +static void printk_exit(unsigned long flags)
>
> This enables local IRQs, so
>
> 	printk_exit_irqrestore()

Yes, I like that.

John Ogness