Re: [PATCH] ufs: make solaris fsck happy [LWN.net]

From:		Andrew Morton <akpm-AT-linux-foundation.org>
To:		Evgeniy Dushistov <dushistov-AT-mail.ru>
Subject:		Re: [PATCH] ufs: make solaris fsck happy
Date:		Mon, 1 Mar 2010 16:37:54 -0800
Cc:		linux-kernel-AT-vger.kernel.org, Alex Viskovatoff <viskovatoff-AT-imap.cc>
Archive-link:		Article, Thread

On Thu, 25 Feb 2010 21:47:43 +0300
Evgeniy Dushistov <dushistov@mail.ru> wrote:

> --- a/fs/ufs/ufs_fs.h
> +++ b/fs/ufs/ufs_fs.h
> @@ -227,11 +227,16 @@ typedef __u16 __bitwise __fs16;
>   */
>  #define ufs_cbtocylno(bno) \
>  	((bno) * uspi->s_nspf / uspi->s_spc)
> -#define ufs_cbtorpos(bno) \
> +#define ufs_cbtorpos(bno)				      \
> +	((UFS_SB(sb)->s_flags & UFS_CG_SUN) ?		      \
> +	 (((((bno) * uspi->s_nspf % uspi->s_spc) %	      \
> +	    uspi->s_nsect) *				      \
> +	   uspi->s_nrpos) / uspi->s_nsect)		      \
> +	 :						      \
>  	((((bno) * uspi->s_nspf % uspi->s_spc / uspi->s_nsect \
>  	* uspi->s_trackskew + (bno) * uspi->s_nspf % uspi->s_spc \
>  	% uspi->s_nsect * uspi->s_interleave) % uspi->s_nsect \
> -	* uspi->s_nrpos) / uspi->s_npsect)
> +	  * uspi->s_nrpos) / uspi->s_npsect))

yikes, that macro should be killed with a stick before it becomes
self-aware and starts breeding.

Or converted into a C function which humans have a chance of
understanding and maintaining.

(Log in to post comments)

Re: [PATCH] ufs: make solaris fsck happy

Posted Mar 10, 2010 13:44 UTC (Wed) by incase (subscriber, #37115) [Link]

Uhm, why?
I'm not programming much these days, but I certainly remember having defines like

#define square(x) x*x /* see below */

in some very old programs I had to maintain.

(That was just a simple example, not actually one of the defines I had)

Re: [PATCH] ufs: make solaris fsck happy

Posted Mar 10, 2010 14:28 UTC (Wed) by anselm (subscriber, #2796) [Link]

The problem occurs when you then use something like »square(i++)«, which yields undefined behaviour.

Re: [PATCH] ufs: make solaris fsck happy

Posted Mar 10, 2010 14:29 UTC (Wed) by farnz (guest, #17727) [Link]

Remember that macro expansion is done by substitution. So, using your macro:

#define square(x) x*x

y = square(2); /* OK */
z = square(y++); /* Going to behave in an unpredictable fashion */
w = square(readHardware()); /* Going to poke hardware in a way you *really* don't expect */

When the preprocessor has finished with it, this code becomes:

y = 2*2;
z = y++*y++;
w = readHardware() * readHardware();

A programmer reading the code, and not aware that square() was a macro would expect behaviour more akin to:

y = 2*2;
temp = y++
z = temp * temp;
temp = readHardware();
z = temp * temp;

The resulting confusion is not recommended, as it's a lovely source of unexpected bugs.

Re: [PATCH] ufs: make solaris fsck happy

Posted Mar 11, 2010 15:54 UTC (Thu) by welinder (guest, #4699) [Link]

Also

square(2+2)

will give you 8, not 16.

Re: [PATCH] ufs: make solaris fsck happy

Posted Mar 10, 2010 15:28 UTC (Wed) by paulj (subscriber, #341) [Link]

Because multiple stores to objects between sequence points (e.g. ; is a
sequence point) are undefined, e.g. the square(i++) example people gave.
Additionally, even when that's not an issue, there is the problem of side-
effects, consider:

struct foo {
...
int (*inc) (void);
}

square(foo->inc());

This is now well-defined, because a function call implicitly has a sequence
point before it, however it's probably still not what the programmer
intended. ;)