linux-kernel - Re: [GIT PULL] first round of SCSI updates for the 2.6.36 merge window

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

Message-ID: <1281054768.22157.2.camel@mulgrave.site>
Date:	Thu, 05 Aug 2010 19:32:48 -0500
From:	James Bottomley <James.Bottomley@...e.de>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	Alan Stern <stern@...land.harvard.edu>,
	Andrew Morton <akpm@...ux-foundation.org>,
	linux-scsi@...r.kernel.org,
	linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [GIT PULL] first round of SCSI updates for the 2.6.36 merge 
 window

On Wed, 2010-08-04 at 15:51 -0700, Linus Torvalds wrote:
> On Tue, Aug 3, 2010 at 9:13 PM, James Bottomley <James.Bottomley@...e.de> wrote:
> >
> > Alan Stern (3):
> >      sd: add support for runtime PM
> >      implement runtime Power Management
> >      convert to the new PM framework
> 
> Guys, these kind of crazy games really aren't appropriate:
> 
>   +/* scsi_pm.c */
>   +#ifdef CONFIG_PM_OPS
>   +extern const struct dev_pm_ops scsi_bus_pm_ops;
>   +#else
>   +#define scsi_bus_pm_ops                (*NULL)
>   +#endif
> 
> that's just crazy. Yes, I see how it's then used (address-of operator
> turns it back into NULL), but the compiler warns about it
> ("drivers/scsi/scsi_sysfs.c:384: warning: dereferencing ‘void *’
> pointer") and I think the compiler is 100% correct about warning about
> it.

Sigh ... I did actually check this because I thought the compiler would
warn.  Apparently it only warns on certain versions.

> It's not just the (*NULL) games, btw. The above can cause confusion.
> It's ugly not just because it causes the compiler to warn, but because
> you use a very subtle and non-standard way of using #define's, so that
> when you look at the source code where this is used, it's not at all
> obvious what is going on. The code looks like
> 
>     .pm             = &scsi_bus_pm_ops,
> 
> and dammit, it would be rather understandable if some _human_ that
> reads that is also confused and thinks that the above means that the
> .pm pointer can never be NULL. The address-of would certainly throw
> me, and not necessarily at all make me grep for "could that possibly
> be some crazy way to say NULL".
> 
> And there is absolutely no reason to play games like that. It would
> have been entirely understandable if you just put the #ifdef in the C
> code itself, or if you used a #define that just said
> 
>   #ifdef CONFIG_PM_OPS
>   #define SCSI_BUS_PM_OPS &scsi_bus_pm_ops
>   #else
>   #define SCSI_BUS_PM_OPS NULL
>   #endif
> 
> and I think it would be less confusing, and it wouldn't upset the compiler.
> 
> Yes, yes, I realize that we do these kinds of things for function
> pointers all the time, so I do understand where the pattern comes
> from. At the same time, I rather think that function pointers are a
> bit different, and they don't have the whole address-of problem.
> 
> I guess I should be happy that you didn't use some linker tricks to
> make "&scsi_bus_pm_ops" turn into NULL at link time. That could be
> done too, and would have been even more subtly confusing.

Agreed.  Alan already sent a patch to fix it.  I'll add it for the final
SCSI patch set.

James


--
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/