linux-kernel - Re: [PATCH v11 3/4] x86, mce: Add __mcsafe

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]

Date:	Fri, 19 Feb 2016 09:43:47 +0100
From:	Peter Zijlstra <peterz@...radead.org>
To:	Ingo Molnar <mingo@...nel.org>
Cc:	Tony Luck <tony.luck@...el.com>, linux-kernel@...r.kernel.org,
	Thomas Gleixner <tglx@...utronix.de>,
	"H. Peter Anvin" <hpa@...or.com>, Borislav Petkov <bp@...en8.de>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy()

On Fri, Feb 19, 2016 at 08:58:43AM +0100, Ingo Molnar wrote:
> > I prefer to use my modern console width to display multiple columns of text, 
> > instead of wasting it to display mostly whitespace. Therefore I still very much 
> > prefer ~80 char wide code.
> 
> Btw., the main reason I hate the col80 limit is that I see such patches 
> frequently:
> 
>  void pcibios_add_bus(struct pci_bus *bus)
>  {
> +#ifdef CONFIG_DMI
> +       const struct dmi_device *dmi;
> +       struct dmi_dev_onboard *dslot;
> +       char sname[128];
> +
> +       dmi = NULL;
> +       while ((dmi = dmi_find_device(DMI_DEV_TYPE_DEV_SLOT,
> +                                     NULL, dmi)) != NULL) {
> +               dslot = dmi->device_data;
> +               if (dslot->segment == pci_domain_nr(bus) &&
> +                   dslot->bus == bus->number) {
> +                       dev_info(&bus->dev, "Found SMBIOS Slot %s\n",
> +                                dslot->dev.name);
> +                       snprintf(sname, sizeof(sname), "%s-%d",
> +                                dslot->dev.name,
> +                                dslot->instance);
> +                       pci_create_slot(bus, dslot->devfn,
> +                                       sname, NULL);
> +               }
> +       }
> +#endif
>         acpi_pci_add_bus(bus);
> 
> Which gobbledygook has 6 (!) col80 artifacts - and it's a pretty straightforward 
> piece of code with just 2 levels of indentation.
> 
> It is IMHO much more readable in the following form:
> 
>  void pcibios_add_bus(struct pci_bus *bus)
>  {
>  #ifdef CONFIG_DMI
>         const struct dmi_device *dmi;
>         struct dmi_dev_onboard *dslot;
>         char sname[128];
> 
>         dmi = NULL;
>         while ((dmi = dmi_find_device(DMI_DEV_TYPE_DEV_SLOT, NULL, dmi)) != NULL) {
>                 dslot = dmi->device_data;
>                 if (dslot->segment == pci_domain_nr(bus) && dslot->bus == bus->number) {
>                         dev_info(&bus->dev, "Found SMBIOS Slot %s\n", dslot->dev.name);
>                         snprintf(sname, sizeof(sname), "%s-%d", dslot->dev.name, dslot->instance);
>                         pci_create_slot(bus, dslot->devfn, sname, NULL);
>                 }
>         }
>  #endif
>         acpi_pci_add_bus(bus);
> 
> BYMMV.

So I mostly agree, although your example does wrap on my normal display
width. The thing is though, we have to have a limit, otherwise people
will completely let loose and we'll end up with lines >200 chars wide
(I've worked on code like that in the past, and its a right pain).

And 80 has so far mostly worked just fine. Its just that people seem
unable to take guidelines as just than, and instead produce the most
horrible code just to adhere to checkpatch or whatnot.

And I'd much rather have an extra column of code than waste a lot of
screen-estate to display mostly whitespace.

Also, this 'artificial' limit on indentation level does in general
encourage people to write saner code.