Re: [PATCH][MTD-UTILS] Set mkfs.jffs2 page size runtime instead of fixed.

[Josh Boyer <jwboyer@gmail.com>]

On Thu, 6 Dec 2007 12:43:47 +0100 (CET)
Ricard Wanderlof <ricard.wanderlof@axis.com> wrote:

> 
> This patch reads the default PAGE_SIZE from sysconf(), i.e. the system
> mkfs.jffs2 is running on, instead of just setting it to 4096 (which of 
> course is valid for most systems but not all).

This makes sense overall.  I'd like to do it a slightly different way
though.

> This is useful if mkfs.jffs2 is running on the target system, e.g. to 
> create a backup image during firmware upgrade, so that the page size does 
> not have to be set explicitly using a command line parameter.
> 
> The --pagesize option is supported just as before.
> 
> Patch is included both as an attachment for patching and inline for 
> review. (Patch made against git head).
> 
> Signed-off-by Ricard Wanderlöf <ricardw@axis.com> .
> 
> Index: mkfs.jffs2.c
> ===================================================================
> RCS file: /usr/local/cvs/linux/apps/utils/mtdutils/mkfs.jffs2.c,v
> retrieving revision 1.2
> retrieving revision 1.3
> diff -u -r1.2 -r1.3
> --- mkfs.jffs2.c	17 Oct 2006 11:40:40 -0000	1.2
> +++ mkfs.jffs2.c	6 Dec 2007 07:46:47 -0000	1.3
> @@ -672,9 +672,9 @@
>   	0xff, 0xff, 0xff, 0xff, 0xff
>   };
> 
> -/* We default to 4096, per x86.  When building a fs for
> - * 64-bit arches and whatnot, use the --pagesize=SIZE option */
> -int page_size = 4096;
> +/* We set this at start of main() using sysconf(), -1 means we don't know */
> +/* When building an fs for non-native systems, use --pagesize=SIZE option */
> +int page_size = -1;
> 
>   #include "compr.h"
> 
> @@ -787,6 +787,10 @@
>   	struct stat *statbuf;
>   	unsigned int totcomp = 0;
> 
> +	page_size = sysconf(_SC_PAGESIZE);
> +	if (page_size < 0) /* System doesn't know so ... */
> +		page_size = 4096; /* ... we make an educated guess */
> +

Could we add an additional check here to see if page_size != 4096, and
print a loud warning with the page size that is actually being used and
suggest the --pagesize option if that isn't what is desired?

It's not that we have tons of systems using a page size different than
4KiB, but it's a change in behavior for those that do and I'd like to
at least warn users about it.  Otherwise the patch looks fine.

josh