linux-kernel - Re: [PATCH v3] kdb: kdb_support: Fix debugging information problem

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>] [day] [month] [year] [list]

Date:   Tue, 2 Feb 2021 09:39:51 -0800
From:   Doug Anderson <dianders@...omium.org>
To:     Stephen Zhang <stephenzhangzsd@...il.com>
Cc:     Jason Wessel <jason.wessel@...driver.com>,
        Daniel Thompson <daniel.thompson@...aro.org>,
        "Gustavo A. R. Silva" <gustavoars@...nel.org>,
        kgdb-bugreport@...ts.sourceforge.net,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3] kdb: kdb_support: Fix debugging information problem

Hi,

On Tue, Feb 2, 2021 at 3:15 AM Stephen Zhang <stephenzhangzsd@...il.com> wrote:
>>
>>
>> > @@ -147,11 +141,10 @@ int kdbnearsym(unsigned long addr, kdb_symtab_t *symtab)
>> >
>> >         if (symtab->mod_name == NULL)
>> >                 symtab->mod_name = "kernel";
>> > -       if (KDB_DEBUG(AR))
>> > -               kdb_printf("kdbnearsym: returns %d symtab->sym_start=0x%lx, "
>> > -                  "symtab->mod_name=%px, symtab->sym_name=%px (%s)\n", ret,
>> > -                  symtab->sym_start, symtab->mod_name, symtab->sym_name,
>> > -                  symtab->sym_name);
>> > +       kdb_dbg_printf(AR, "returns %d symtab->sym_start=0x%lx, "
>> > +               "symtab->mod_name=%px, symtab->sym_name=%px (%s)\n", ret,
>> > +               symtab->sym_start, symtab->mod_name, symtab->sym_name,
>> > +               symtab->sym_name);
>>
>> This indention doesn't line up, but it didn't before.  I guess I'd let
>> Daniel say if he likes this or would prefer different wrapping /
>> indentation.
>
>
> Thanks for your patience. You told me that  the alignment for continued arguments is to
> match up with the first argument, so I was confused that the first line and the second line
> here don't follow the rule.  There are many  cases like this in  this file.

There's a saying: all rules are made to be broken.

I think the general rule is that the 2nd line of arguments should line
up with the first.  However, sometimes that forces way too much
wrapping.  If it's "too ugly" to use the general rule, then you can
fall back to some alternate rule.  Usually this alternate rule is
something like indending all subsequent lines by one tab.  The
definition of "too ugly" is left to the judgement of the person
writing / reviewing the code.  In this case maybe the general rule
would make your call need to take up 3 lines?

If I had to make a judgement call on this code, I'd say:

1. It seems to have been written before the "don't split strings" rule
was in full force.  See
<https://www.kernel.org/doc/html/v5.10/process/coding-style.html#breaking-long-lines-and-strings>
where it says "never break user-visible strings such as printk
messages because that breaks the ability to grep for them".

2. If we fix #1, we're already blowing over the 80 columns limit for
this string anyway.

3. Once we blow over the 80 columns, might as well do it for the
second line.  So then you'd end up with:

<tab>kdb_dbg_printf(AR, "returns [...] (%s)\n",
<tab><tab>       ret, symtab->sym_start, [...], symtab->sym_name);