Re: [PATCH 1/2] Moved core dump functionality into its own file

[Ingo Molnar]

* Alex Kelly <eshink@xxxxxxxxx> wrote:

> From: Alex Kelly <alex.page.kelly@xxxxxxxxx>
> 
> This was done in preparation for making core dump functionality optional.

Please use present tense and a sane title, something like:

  fs: Move core dump functionality into its own file
  fs: Make core dump functionality optional

While looking at that code, there's also a few uglies in it, 
like:

> +	return nr;
> +}
> +
> +
> +/*

> +
> +	/* Repeat as long as we have more pattern to process and more output
> +	   space */

> +}
> +
> +
> +void do_coredump(long signr, int exit_code, struct pt_regs *regs)

> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -413,6 +413,7 @@ static inline void arch_pick_mmap_layout(struct mm_struct *mm) {}
>  
>  extern void set_dumpable(struct mm_struct *mm, int value);
>  extern int get_dumpable(struct mm_struct *mm);
> +extern int __get_dumpable(unsigned long mm_flags);

These prototypes should move out of sched.h and into a 
coredump.h header or so.

If we are touching this code lets use the opportunity and do 
this right.

Note, I'd suggest to put any such further changes into separate, 
additional patches at the end: a cleanup patch, a header file 
introduction patch, etc. - and keep the "dumb code movement" 
chnage in the initial patch. This will make it all much easier 
to review.

Please also review the code for more uglies, the above was just 
a very quick stylistic scan.

Thanks,

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