More cleanups for swsusp

More cleanups for swsusp

[Pavel Machek]

Hi!

This fixes codingstyle a bit, converts "can not happen" panic into
BUG_ON (fill_suspend_header() allocates no memory so panic is
meaningless) and adds check for sizeof (struct link) [if that is not
PAGE_SIZE, we have *bad* problem, better check early]. Please apply,

								Pavel

Index: linux/kernel/power/swsusp.c
===================================================================
--- linux.orig/kernel/power/swsusp.c	2004-01-13 22:52:40.000000000 +0100
+++ linux/kernel/power/swsusp.c	2004-01-09 20:33:05.000000000 +0100
@@ -340,14 +344,14 @@
 	printk("H");
 	BUG_ON (sizeof(struct suspend_header) > PAGE_SIZE-sizeof(swp_entry_t));
 	BUG_ON (sizeof(union diskpage) != PAGE_SIZE);
+	BUG_ON (sizeof(struct link) != PAGE_SIZE);
 	if (!(entry = get_swap_page()).val)
 		panic( "\nNot enough swapspace when writing header" );
 	if (swapfile_used[swp_type(entry)] != SWAPFILE_SUSPEND)
 		panic("\nNot enough swapspace for header on suspend device" );
 
 	cur = (void *) buffer;
-	if (fill_suspend_header(&cur->sh))
-		panic("\nOut of memory while writing header");
+	BUG_ON (fill_suspend_header(&cur->sh));
 		
 	cur->link.next = prev;
 
@@ -856,23 +837,23 @@
 
 static int sanity_check_failed(char *reason)
 {
-	printk(KERN_ERR "%s%s\n",name_resume,reason);
+	printk(KERN_ERR "%s%s\n", name_resume, reason);
 	return -EPERM;
 }
 
 static int sanity_check(struct suspend_header *sh)
 {
-	if(sh->version_code != LINUX_VERSION_CODE)
+	if (sh->version_code != LINUX_VERSION_CODE)
 		return sanity_check_failed("Incorrect kernel version");
-	if(sh->num_physpages != num_physpages)
+	if (sh->num_physpages != num_physpages)
 		return sanity_check_failed("Incorrect memory size");
-	if(strncmp(sh->machine, system_utsname.machine, 8))
+	if (strncmp(sh->machine, system_utsname.machine, 8))
 		return sanity_check_failed("Incorrect machine type");
-	if(strncmp(sh->version, system_utsname.version, 20))
+	if (strncmp(sh->version, system_utsname.version, 20))
 		return sanity_check_failed("Incorrect version");
-	if(sh->num_cpus != num_online_cpus())
+	if (sh->num_cpus != num_online_cpus())
 		return sanity_check_failed("Incorrect number of cpus");
-	if(sh->page_size != PAGE_SIZE)
+	if (sh->page_size != PAGE_SIZE)
 		return sanity_check_failed("Incorrect PAGE_SIZE");
 	return 0;
 }

-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]
-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Re: More cleanups for swsusp

[Andrew Morton]

Pavel Machek <pavel@ucw.cz> wrote:
>
> +	BUG_ON (sizeof(struct link) != PAGE_SIZE);

Looking at the code, this hardly seems worth checking.  But the compiler
should just rub this code out anwyay, so whatever.

hmm, one could do:

#define compile_time_assert(expr)					\
	do {								\
		if (!(expr))						\
			compile_time_assert_failed();	/* undefined */	\
	} while (0)

Re: More cleanups for swsusp

[Pavel Machek]

Hi!

> > +	BUG_ON (sizeof(struct link) != PAGE_SIZE);
> 
> Looking at the code, this hardly seems worth checking.  But the compiler
> should just rub this code out anwyay, so whatever.
> 
> hmm, one could do:
> 
> #define compile_time_assert(expr)					\
> 	do {								\
> 		if (!(expr))						\
> 			compile_time_assert_failed();	/* undefined */	\
> 	} while (0)

Well, if you provide such macro, I'll be happy to use it ;-). It
should be something like 

compile_time_assert(expr, "message")

because with more of these are in the code, it would be nightmare to
find out which one is wrong.

Perhaps better name, too?

pavel@elonex:/tmp$ cat delme.c

#define COMPILE_ERR_ON(expr, message) \
        do { if (!(expr)) compile_time_assert_failed_##message(); } while (0)

void main(void)
{
        COMPILE_ERR_ON(0==8, cpu_broke_zeros_neck);
        COMPILE_ERR_ON(0==0, compiler_went_crazy);
}
pavel@elonex:/tmp$ gcc delme.c -o delme
delme.c: In function `main':
delme.c:6: warning: return type of `main' is not `int'
/tmp/ccAx2alr.o(.text+0x11): In function `main':
: undefined reference to `compile_time_assert_failed_cpu_broke_zeros_neck'
collect2: ld returned 1 exit status
pavel@elonex:/tmp$



								Pavel

-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Re: More cleanups for swsusp

[Rusty Russell]

In message <20040120151358.09608fc3.akpm@osdl.org> you write:
> Pavel Machek <pavel@ucw.cz> wrote:
> >
> > +	BUG_ON (sizeof(struct link) != PAGE_SIZE);
> 
> Looking at the code, this hardly seems worth checking.  But the compiler
> should just rub this code out anwyay, so whatever.
> 
> hmm, one could do:
> 
> #define compile_time_assert(expr)					\

Already there: BUILD_BUG & BUILD_BUG_ON.

Rusty.
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Re: More cleanups for swsusp

[Pavel Machek]

Hi!

> > > +	BUG_ON (sizeof(struct link) != PAGE_SIZE);
> > 
> > Looking at the code, this hardly seems worth checking.  But the compiler
> > should just rub this code out anwyay, so whatever.
> > 
> > hmm, one could do:
> > 
> > #define compile_time_assert(expr)					\
> 
> Already there: BUILD_BUG & BUILD_BUG_ON.

Unfortunately that one gives no message to the user (AFAICS), so
if/when people use it extensively, it will get nasty.
								Pavel
-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Re: More cleanups for swsusp

[Rusty Russell]

In message <20040120225219.GA19190@elf.ucw.cz> you write:
> -	if (fill_suspend_header(&cur->sh))
> -		panic("\nOut of memory while writing header");
> +	BUG_ON (fill_suspend_header(&cur->sh));

1) fill_suspend_header never fails, perhaps make it return void.

2) If fill_suspend_header could fail, you should indicate why it won't
   fail here, and

3) BUG_ON(complex condition expression) is much less clear than:

	if (complex condition expression)
		BUG();

Cheers,
Rusty.
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Re: More cleanups for swsusp

[Andrew Morton]

Rusty Russell <rusty@rustcorp.com.au> wrote:
>
> In message <20040120225219.GA19190@elf.ucw.cz> you write:
>  > -	if (fill_suspend_header(&cur->sh))
>  > -		panic("\nOut of memory while writing header");
>  > +	BUG_ON (fill_suspend_header(&cur->sh));
> 
> ...
>  3) BUG_ON(complex condition expression) is much less clear than:
> 
>  	if (complex condition expression)
>  		BUG();

Worse.  If some smarty goes and makes BUG_ON a no-op (for space reasons),
it will break software suspend.  We should ensure that the expression which
is supplied to BUG_ON() never has side-effects for this reason.

I'll drop that chunk.

Re: More cleanups for swsusp

[Matt Mackall]

On Tue, Jan 20, 2004 at 09:30:37PM -0800, Andrew Morton wrote:
> Rusty Russell <rusty@rustcorp.com.au> wrote:
> >
> > In message <20040120225219.GA19190@elf.ucw.cz> you write:
> >  > -	if (fill_suspend_header(&cur->sh))
> >  > -		panic("\nOut of memory while writing header");
> >  > +	BUG_ON (fill_suspend_header(&cur->sh));
> > 
> > ...
> >  3) BUG_ON(complex condition expression) is much less clear than:
> > 
> >  	if (complex condition expression)
> >  		BUG();

Disagree. All BUG_ON() stuff should read like:

/* check that impossible stuff didn't happen, move along, nothing to see */
BUG_ON(...);

Which is fine and good until the condition is actually doing more than
just sanity checking.

> Worse.  If some smarty goes and makes BUG_ON a no-op (for space reasons),
> it will break software suspend.  We should ensure that the expression which
> is supplied to BUG_ON() never has side-effects for this reason.

While I generally agree that "assertions" shouldn't have side-effects,
a slightly smarter smarty would make sure that BUG_ON evaluated its
condition. I have this in -tiny:

+#ifndef CONFIG_BUG
+#define BUG()
+#define WARN_ON(condition) do { if (condition) ; } while(0)
+#define BUG_ON(condition) do { if (condition) ; } while(0)
+#define PAGE_BUG(page)
+#else

-- 
Matt Mackall : http://www.selenic.com : Linux development and consulting

Re: More cleanups for swsusp

[Rusty Russell]

On Wed, 21 Jan 2004 12:39:38 -0600
Matt Mackall <mpm@selenic.com> wrote:

> > >  3) BUG_ON(complex condition expression) is much less clear than:
> > > 
> > >  	if (complex condition expression)
> > >  		BUG();
> 
> Disagree. All BUG_ON() stuff should read like:
> 
> /* check that impossible stuff didn't happen, move along, nothing to see */
> BUG_ON(...);

You can disagree all you like.

But Linus only allowed BUG_ON() because of the branch-prediction problem,
preferring explicit "if (x) BUG()".  I happen to agree with him, especially
if x is a complex expression.

Rusty.
-- 
   there are those who do and those who hang on and you don't see too
   many doers quoting their contemporaries.  -- Larry McVoy