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]

Message-ID: <20160219075843.GA7854@gmail.com>
Date:	Fri, 19 Feb 2016 08:58:43 +0100
From:	Ingo Molnar <mingo@...nel.org>
To:	Peter Zijlstra <peterz@...radead.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()


* Peter Zijlstra <peterz@...radead.org> wrote:

> > Yeah, so please change this to something like:
> > 
> >   struct mcsafe_ret {
> >           u64 trap_nr;
> >           u64 bytes_left;
> >   };
> > 
> > this makes it crystal clear what the fields are about and what their unit is. 
> > Readability is king and modern consoles are wide enough, no need to abbreviate 
> > excessively.
> 
> 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.

Thanks,

	Ingo