| Message ID | 2f41f484f574512df6919967f641cf2152049a22.1361142401.git.yann.morin.1998@free.fr |
|---|---|
| State | Changes Requested |
| Headers | show |
On 18/02/13 00:10, Yann E. MORIN wrote: > Some bootloaders have a buggy ext2 support, and require ext2 rev1 > instead of the traditional ext2 rev0 that genext2fs produces. > > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr> > --- > fs/ext/Config.in | 19 ++++++++++++------- > fs/ext/genextfs.sh | 38 +++++++++++++++++++++++++++----------- > 2 files changed, 39 insertions(+), 18 deletions(-) > > diff --git a/fs/ext/Config.in b/fs/ext/Config.in > index 1a7a315..be0ed9c 100644 > --- a/fs/ext/Config.in > +++ b/fs/ext/Config.in > @@ -7,10 +7,14 @@ if BR2_TARGET_ROOTFS_EXT > > choice > bool "ext generation" > - default BR2_TARGET_ROOTFS_EXT_EXT2 > + default BR2_TARGET_ROOTFS_EXT_EXT2r0 > > -config BR2_TARGET_ROOTFS_EXT_EXT2 > - bool "ext2" > +config BR2_TARGET_ROOTFS_EXT_EXT2r0 > + bool "ext2 (rev0)" > + > +config BR2_TARGET_ROOTFS_EXT_EXT2r1 > + bool "ext2 (rev1)" > + select BR2_PACKAGE_HOST_E2FSPROGS > > config BR2_TARGET_ROOTFS_EXT_EXT3 > bool "ext3" > @@ -23,10 +27,11 @@ config BR2_TARGET_ROOTFS_EXT_EXT4 > endchoice > > config BR2_TARGET_ROOTFS_EXT_GEN > - int > - default 2 if BR2_TARGET_ROOTFS_EXT_EXT2 > - default 3 if BR2_TARGET_ROOTFS_EXT_EXT3 > - default 4 if BR2_TARGET_ROOTFS_EXT_EXT4 > + string > + default 2.0 if BR2_TARGET_ROOTFS_EXT_EXT2r0 > + default 2.1 if BR2_TARGET_ROOTFS_EXT_EXT2r1 > + default 3 if BR2_TARGET_ROOTFS_EXT_EXT3 > + default 4 if BR2_TARGET_ROOTFS_EXT_EXT4 I think it makes things simpler if you keep the GEN as it is, and process the rev separately. Or perhaps set config BR2_TARGET_ROOTFS_EXT_REV int default 0 if BR2_TARGET_ROOTFS_EXT_EXT2r0 default 1 > > config BR2_TARGET_ROOTFS_EXT_BLOCKS > int "size in blocks (leave at 0 for auto calculation)" > diff --git a/fs/ext/genextfs.sh b/fs/ext/genextfs.sh > index fcbd43c..ef47a25 100755 > --- a/fs/ext/genextfs.sh > +++ b/fs/ext/genextfs.sh > @@ -6,11 +6,13 @@ export LC_ALL=C > > CALC_BLOCKS=1 > CALC_INODES=1 > +EXT2_REV=0 > > while getopts x:d:D:b:i:N:m:g:e:zfqUPhVv234 f > do > case $f in > - 2|3|4) GEN=$f ;; > + 2.*) GEN=2; EXT2_REV=${f#*.} ;; > + 3|4) GEN=$f ;; > b) CALC_BLOCKS=0 ;; > N) CALC_INODES=0; INODES=$OPTARG ;; > d) TARGET_DIR=$OPTARG ;; > @@ -56,25 +58,39 @@ for o; do > esac > done > > +ext_fsck() { > + gen="${1}" > + img="${2}" > + ret=0 > + fsck.ext${gen} -pDf "${img}" >/dev/null || ret=$? Don't bother with ${gen}, just use e2fsck. > + # Exit codes 1 & 2 are OK, it means fs errors > + # were successfully corrected > + case ${ret} in > + 0|1|2) ;; > + *) exit 1;; > + esac > + # fsck.ext* will force a UUID, which we do not want > + tune2fs -U clear "${img}" >/dev/null > +} > + > # Generate the base ext2 file system > genext2fs "$@" > > +# Upgrade to ext2 rev1 if needed > +if [ ${EXT2_REV} -ge 1 -o ${GEN} -ge 3 ]; then With BR2_TARGET_ROOTFS_EXT_REV this condition becomes simpler. > + tune2fs -O filetype "${IMG}" >/dev/null > + ext_fsck 2 "${IMG}" > +fi > + > # Upgrade to ext3 if needed > if [ ${GEN} -ge 3 ]; then > tune2fs -j -J size=1 "${IMG}" >/dev/null > + ext_fsck 3 "${IMG}" > fi > > # Upgrade to ext4 if needed > if [ ${GEN} -ge 4 ]; then > tune2fs -O extents,uninit_bg,dir_index "${IMG}" >/dev/null > - ret=0 > - fsck.ext4 -pDf "${IMG}" >/dev/null || ret=$? > - # Exit codes 1 & 2 are OK, it means fs errors > - # were successfully corrected > - case ${ret} in > - 0|1|2) ;; > - *) exit 1;; > - esac > - # fsck.ext4 will force a UUID, which we do not want > - tune2fs -U clear "${IMG}" >/dev/null > + NEED_FSCK=1 I guess you originally had just a single fsck and used this variable to decide if it was needed. That's actually a good idea. Regards, Arnout > + ext_fsck 4 "${IMG}" > fi >
Arnout, All, On Tuesday 19 February 2013 Arnout Vandecappelle wrote: > On 18/02/13 00:10, Yann E. MORIN wrote: > > Some bootloaders have a buggy ext2 support, and require ext2 rev1 > > instead of the traditional ext2 rev0 that genext2fs produces. [--SNIP--] > > @@ -23,10 +27,11 @@ config BR2_TARGET_ROOTFS_EXT_EXT4 > > endchoice > > > > config BR2_TARGET_ROOTFS_EXT_GEN > > - int > > - default 2 if BR2_TARGET_ROOTFS_EXT_EXT2 > > - default 3 if BR2_TARGET_ROOTFS_EXT_EXT3 > > - default 4 if BR2_TARGET_ROOTFS_EXT_EXT4 > > + string > > + default 2.0 if BR2_TARGET_ROOTFS_EXT_EXT2r0 > > + default 2.1 if BR2_TARGET_ROOTFS_EXT_EXT2r1 > > + default 3 if BR2_TARGET_ROOTFS_EXT_EXT3 > > + default 4 if BR2_TARGET_ROOTFS_EXT_EXT4 > > I think it makes things simpler if you keep the GEN as it is, and > process the rev separately. Or perhaps set > > config BR2_TARGET_ROOTFS_EXT_REV > int > default 0 if BR2_TARGET_ROOTFS_EXT_EXT2r0 > default 1 Not sure. ext2 rev1 is just that: a revision 1 ext2. ext3 or ext4 rev1 do not mean anything, AFAIK. [--SNIP--] > > +ext_fsck() { > > + gen="${1}" > > + img="${2}" > > + ret=0 > > + fsck.ext${gen} -pDf "${img}" >/dev/null || ret=$? > > Don't bother with ${gen}, just use e2fsck. OK, I've just checked, and e2fsck's behavior does indeed not depend on its argv[0], so I'll use that. [--SNIP--] > > +# Upgrade to ext2 rev1 if needed > > +if [ ${EXT2_REV} -ge 1 -o ${GEN} -ge 3 ]; then > > With BR2_TARGET_ROOTFS_EXT_REV this condition becomes simpler. Yes, but as I said above, rev0/1 is only meaningfull for ext2, not ext3/4. I'd rather keep the semantics clear. The ext filesystem can be: - ext2 rev0 - ext2 rev1 - ext3 - ext4 And the fsck is only needed for ext2 rev1, ext3 or ext4. But it is not stupid to always run the fsck anyway, so I'll simplify the code by always calling e2fsck, even if it is not strictly required. Doing so will also help catching badly generated ext2 rev0 filesystems, so it's a net gain (and does not cost too much, anyway). > > + tune2fs -O filetype "${IMG}" >/dev/null > > + ext_fsck 2 "${IMG}" > > +fi > > + > > # Upgrade to ext3 if needed > > if [ ${GEN} -ge 3 ]; then > > tune2fs -j -J size=1 "${IMG}" >/dev/null > > + ext_fsck 3 "${IMG}" > > fi > > > > # Upgrade to ext4 if needed > > if [ ${GEN} -ge 4 ]; then > > tune2fs -O extents,uninit_bg,dir_index "${IMG}" >/dev/null > > - ret=0 > > - fsck.ext4 -pDf "${IMG}" >/dev/null || ret=$? > > - # Exit codes 1 & 2 are OK, it means fs errors > > - # were successfully corrected > > - case ${ret} in > > - 0|1|2) ;; > > - *) exit 1;; > > - esac > > - # fsck.ext4 will force a UUID, which we do not want > > - tune2fs -U clear "${IMG}" >/dev/null > > + NEED_FSCK=1 > > I guess you originally had just a single fsck and used this variable to > decide if it was needed. That's actually a good idea. Oh, I forgot to remove that variable. We can't run fsck once at the end. It has to be called after each tune2fs call. Also, I'll rework the code as thus: ext_opts="" ext_opts_O="" if ! ext2rev0: ext_opts_O+="filetype" if ext3: ext_opts+="-j -J size=1" if ext4: ext_opts_O+="extents,uninit_bg,dir_index" if ext_opts_O != "": ext_opts+="-O ext_opts_O" tune2fs ext_opts img e2fsck img That is even better, I think. Regards, Yann E. MORIN.
On 19/02/13 19:10, Yann E. MORIN wrote: > Arnout, All, > > On Tuesday 19 February 2013 Arnout Vandecappelle wrote: >> On 18/02/13 00:10, Yann E. MORIN wrote: >>> Some bootloaders have a buggy ext2 support, and require ext2 rev1 >>> instead of the traditional ext2 rev0 that genext2fs produces. > [--SNIP--] >>> @@ -23,10 +27,11 @@ config BR2_TARGET_ROOTFS_EXT_EXT4 >>> endchoice >>> >>> config BR2_TARGET_ROOTFS_EXT_GEN >>> - int >>> - default 2 if BR2_TARGET_ROOTFS_EXT_EXT2 >>> - default 3 if BR2_TARGET_ROOTFS_EXT_EXT3 >>> - default 4 if BR2_TARGET_ROOTFS_EXT_EXT4 >>> + string >>> + default 2.0 if BR2_TARGET_ROOTFS_EXT_EXT2r0 >>> + default 2.1 if BR2_TARGET_ROOTFS_EXT_EXT2r1 >>> + default 3 if BR2_TARGET_ROOTFS_EXT_EXT3 >>> + default 4 if BR2_TARGET_ROOTFS_EXT_EXT4 >> >> I think it makes things simpler if you keep the GEN as it is, and >> process the rev separately. Or perhaps set >> >> config BR2_TARGET_ROOTFS_EXT_REV >> int >> default 0 if BR2_TARGET_ROOTFS_EXT_EXT2r0 >> default 1 > > Not sure. ext2 rev1 is just that: a revision 1 ext2. ext3 or ext4 rev1 > do not mean anything, AFAIK. They are partly independent - although ext4 is not possible with rev0... And in practice you want rev1 for ext3 filesystems as well. Actually, you always want rev1, except when you have a stupid bootloader that doesn't support it. So what I meant is: set revision to 1, except for ext2r0. > > [--SNIP--] >>> +ext_fsck() { >>> + gen="${1}" >>> + img="${2}" >>> + ret=0 >>> + fsck.ext${gen} -pDf "${img}" >/dev/null || ret=$? >> >> Don't bother with ${gen}, just use e2fsck. > > OK, I've just checked, and e2fsck's behavior does indeed not depend > on its argv[0], so I'll use that. > > [--SNIP--] >>> +# Upgrade to ext2 rev1 if needed >>> +if [ ${EXT2_REV} -ge 1 -o ${GEN} -ge 3 ]; then >> >> With BR2_TARGET_ROOTFS_EXT_REV this condition becomes simpler. > > Yes, but as I said above, rev0/1 is only meaningfull for ext2, not ext3/4. > > I'd rather keep the semantics clear. The ext filesystem can be: > - ext2 rev0 > - ext2 rev1 > - ext3 > - ext4 > > And the fsck is only needed for ext2 rev1, ext3 or ext4. > > But it is not stupid to always run the fsck anyway, so I'll simplify the > code by always calling e2fsck, even if it is not strictly required. Doing > so will also help catching badly generated ext2 rev0 filesystems, so it's > a net gain (and does not cost too much, anyway). > >>> + tune2fs -O filetype "${IMG}" >/dev/null >>> + ext_fsck 2 "${IMG}" >>> +fi >>> + >>> # Upgrade to ext3 if needed >>> if [ ${GEN} -ge 3 ]; then >>> tune2fs -j -J size=1 "${IMG}" >/dev/null >>> + ext_fsck 3 "${IMG}" >>> fi >>> >>> # Upgrade to ext4 if needed >>> if [ ${GEN} -ge 4 ]; then >>> tune2fs -O extents,uninit_bg,dir_index "${IMG}" >/dev/null >>> - ret=0 >>> - fsck.ext4 -pDf "${IMG}" >/dev/null || ret=$? >>> - # Exit codes 1 & 2 are OK, it means fs errors >>> - # were successfully corrected >>> - case ${ret} in >>> - 0|1|2) ;; >>> - *) exit 1;; >>> - esac >>> - # fsck.ext4 will force a UUID, which we do not want >>> - tune2fs -U clear "${IMG}" >/dev/null >>> + NEED_FSCK=1 >> >> I guess you originally had just a single fsck and used this variable to >> decide if it was needed. That's actually a good idea. > > Oh, I forgot to remove that variable. We can't run fsck once at the end. > It has to be called after each tune2fs call. > > Also, I'll rework the code as thus: > > ext_opts="" > ext_opts_O="" > if ! ext2rev0: > ext_opts_O+="filetype" > if ext3: > ext_opts+="-j -J size=1" > if ext4: > ext_opts_O+="extents,uninit_bg,dir_index" > if ext_opts_O != "": > ext_opts+="-O ext_opts_O" > tune2fs ext_opts img > e2fsck img > > That is even better, I think. Agreed! Moving to Python is certainly better! :-) BTW doesn't tune2fs support -O x -O y options? Regards, Arnout
Arnout, All, On Wednesday 20 February 2013 Arnout Vandecappelle wrote: > On 19/02/13 19:10, Yann E. MORIN wrote: [--SNIP--] > > Not sure. ext2 rev1 is just that: a revision 1 ext2. ext3 or ext4 rev1 > > do not mean anything, AFAIK. > They are partly independent - although ext4 is not possible with > rev0... And in practice you want rev1 for ext3 filesystems as well. > Actually, you always want rev1, except when you have a stupid bootloader > that doesn't support it. > > So what I meant is: set revision to 1, except for ext2r0. Even ext3 is always rev1, anyway. Anyway, I've reworked the code, and am doing some tests. On track for being pushed by the end of the week... [--SNIP--] > > Also, I'll rework the code as thus: > > > > ext_opts="" > > ext_opts_O="" > > if ! ext2rev0: > > ext_opts_O+="filetype" > > if ext3: > > ext_opts+="-j -J size=1" > > if ext4: > > ext_opts_O+="extents,uninit_bg,dir_index" > > if ext_opts_O != "": > > ext_opts+="-O ext_opts_O" > > tune2fs ext_opts img > > e2fsck img > > > > That is even better, I think. > > Agreed! Moving to Python is certainly better! :-) > > BTW doesn't tune2fs support -O x -O y options? Yes, probably, that was just a quick-and-dirty pseudo code for the sake of illustration. ;-) Regards, Yann E. MORIN, going to sleep in a short while...
diff --git a/fs/ext/Config.in b/fs/ext/Config.in index 1a7a315..be0ed9c 100644 --- a/fs/ext/Config.in +++ b/fs/ext/Config.in @@ -7,10 +7,14 @@ if BR2_TARGET_ROOTFS_EXT choice bool "ext generation" - default BR2_TARGET_ROOTFS_EXT_EXT2 + default BR2_TARGET_ROOTFS_EXT_EXT2r0 -config BR2_TARGET_ROOTFS_EXT_EXT2 - bool "ext2" +config BR2_TARGET_ROOTFS_EXT_EXT2r0 + bool "ext2 (rev0)" + +config BR2_TARGET_ROOTFS_EXT_EXT2r1 + bool "ext2 (rev1)" + select BR2_PACKAGE_HOST_E2FSPROGS config BR2_TARGET_ROOTFS_EXT_EXT3 bool "ext3" @@ -23,10 +27,11 @@ config BR2_TARGET_ROOTFS_EXT_EXT4 endchoice config BR2_TARGET_ROOTFS_EXT_GEN - int - default 2 if BR2_TARGET_ROOTFS_EXT_EXT2 - default 3 if BR2_TARGET_ROOTFS_EXT_EXT3 - default 4 if BR2_TARGET_ROOTFS_EXT_EXT4 + string + default 2.0 if BR2_TARGET_ROOTFS_EXT_EXT2r0 + default 2.1 if BR2_TARGET_ROOTFS_EXT_EXT2r1 + default 3 if BR2_TARGET_ROOTFS_EXT_EXT3 + default 4 if BR2_TARGET_ROOTFS_EXT_EXT4 config BR2_TARGET_ROOTFS_EXT_BLOCKS int "size in blocks (leave at 0 for auto calculation)" diff --git a/fs/ext/genextfs.sh b/fs/ext/genextfs.sh index fcbd43c..ef47a25 100755 --- a/fs/ext/genextfs.sh +++ b/fs/ext/genextfs.sh @@ -6,11 +6,13 @@ export LC_ALL=C CALC_BLOCKS=1 CALC_INODES=1 +EXT2_REV=0 while getopts x:d:D:b:i:N:m:g:e:zfqUPhVv234 f do case $f in - 2|3|4) GEN=$f ;; + 2.*) GEN=2; EXT2_REV=${f#*.} ;; + 3|4) GEN=$f ;; b) CALC_BLOCKS=0 ;; N) CALC_INODES=0; INODES=$OPTARG ;; d) TARGET_DIR=$OPTARG ;; @@ -56,25 +58,39 @@ for o; do esac done +ext_fsck() { + gen="${1}" + img="${2}" + ret=0 + fsck.ext${gen} -pDf "${img}" >/dev/null || ret=$? + # Exit codes 1 & 2 are OK, it means fs errors + # were successfully corrected + case ${ret} in + 0|1|2) ;; + *) exit 1;; + esac + # fsck.ext* will force a UUID, which we do not want + tune2fs -U clear "${img}" >/dev/null +} + # Generate the base ext2 file system genext2fs "$@" +# Upgrade to ext2 rev1 if needed +if [ ${EXT2_REV} -ge 1 -o ${GEN} -ge 3 ]; then + tune2fs -O filetype "${IMG}" >/dev/null + ext_fsck 2 "${IMG}" +fi + # Upgrade to ext3 if needed if [ ${GEN} -ge 3 ]; then tune2fs -j -J size=1 "${IMG}" >/dev/null + ext_fsck 3 "${IMG}" fi # Upgrade to ext4 if needed if [ ${GEN} -ge 4 ]; then tune2fs -O extents,uninit_bg,dir_index "${IMG}" >/dev/null - ret=0 - fsck.ext4 -pDf "${IMG}" >/dev/null || ret=$? - # Exit codes 1 & 2 are OK, it means fs errors - # were successfully corrected - case ${ret} in - 0|1|2) ;; - *) exit 1;; - esac - # fsck.ext4 will force a UUID, which we do not want - tune2fs -U clear "${IMG}" >/dev/null + NEED_FSCK=1 + ext_fsck 4 "${IMG}" fi
Some bootloaders have a buggy ext2 support, and require ext2 rev1 instead of the traditional ext2 rev0 that genext2fs produces. Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr> --- fs/ext/Config.in | 19 ++++++++++++------- fs/ext/genextfs.sh | 38 +++++++++++++++++++++++++++----------- 2 files changed, 39 insertions(+), 18 deletions(-)