linux-kernel - RE: [Openipmi-developer] [PATCH 9/12] IPMI: add pigeonpoint poweroff

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: <FE74AC4E0A23124DA52B99F17F44159701DBC05D@PA-EXCH03.vmware.com>
Date:	Sun, 3 Dec 2006 20:21:31 -0800
From:	"Bela Lubkin" <blubkin@...are.com>
To:	"Randy Dunlap" <randy.dunlap@...cle.com>
Cc:	"Andrew Morton" <akpm@...l.org>, "Corey Minyard" <minyard@....org>,
	"OpenIPMI Developers" <openipmi-developer@...ts.sourceforge.net>,
	"Linux Kernel" <linux-kernel@...r.kernel.org>,
	"Joseph Barnett" <jbarnett@...orola.com>
Subject: RE: [Openipmi-developer] [PATCH 9/12] IPMI: add pigeonpoint poweroff

Randy Dunlap wrote:
> Bela Lubkin wrote:
>> Andrew Morton wrote:
>> 
>>> Sometime, please go through the IPMI code looking for all these
>>> statically-allocated things which are initialised to 0 or NULL and remove
>>> all those intialisations?  They're unneeded, they increase the vmlinux
>>> image size and there are quite a number of them.  Thanks.

>> Randy Dunlop replied:

<>> I was just about to send that patch.  Here it is,
>>> on top of the series-of-12.
>> ...
>>> -static int bt_debug = BT_DEBUG_OFF;
>>> +static int bt_debug;
>>
>> Is it wise to significantly degrade code readability to work around a
minor
>> compiler / linker bug?
>
> Is that the only one that is a problem?

It was the most obvious one at a quick glance.

I would argue that _every single one_ is a small problem because it makes the
code murkier.  Yes we "know" that statics are initialized to zero.  Not every
reader is perfectly versed in that.  The fact that statics and dynamics are
different is an added subtlety.  Everywhere you've removed an initializer is
a potential bug if someone needs to change that variable to a different
storage class.  Yes they "should" know better.

> I don't think it's a problem.  We *know* that static data areas
> are init to 0.  Everything depends on that.  If that didn't work
> it would all break.

Agreed; that's why it's a compiler/linker bug if _explicit_ initialization to
zero results in any difference in the final binary.  So here you're
submitting dozens of source level changes to repair a toolchain bug.

> I could say that it's a nice coincidence that BT_DEBUG_OFF == 0,
> but I think that it's more than coincidence.

I agree, it's more than coincidence.  But the new version tells you less.  It
ccertainly _could_ have been coded such that debug == 1 means no debug, 0
means debug.  Dumb, but possible.  It could also have been coded such that
debug == BT_DEBUG_OFF means debug, but that's egregiously stupid.

In other words, I believe the reader of:

  static int bt_debug = BT_DEBUG_OFF;

_knows_ debug is off.  The reader of:

  static int bt_debug = 0;

has a _strong suspicion_ that debug is off.  The reader of:

  static int bt_debug;

has at best a _strong hint_ that debug is off.

Any of those three statements could be shortly followed by:

  bt_debug = BT_DEBUG_ON;

but this would be a moderately surprising construction after the first two;
not at all surprising after the third.

>Bela<
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/