Message ID | 1a995bdbc65141cf4e8a540c41cb56f4a43fba5f.1361142401.git.yann.morin.1998@free.fr |
---|---|
State | Changes Requested |
Headers | show |
Great feature addition! On 18/02/13 00:10, Yann E. MORIN wrote: > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr> > --- > fs/ext2/Config.in | 37 ++++++++++++++++++++++++++++++------- > fs/ext2/ext2.mk | 4 ++-- > fs/ext2/genext2fs.sh | 48 ++++++++++++++++++++++++++++++++++++++++++++++-- > 3 files changed, 78 insertions(+), 11 deletions(-) > > diff --git a/fs/ext2/Config.in b/fs/ext2/Config.in > index cb4beed..00f11a2 100644 > --- a/fs/ext2/Config.in > +++ b/fs/ext2/Config.in > @@ -1,10 +1,33 @@ > config BR2_TARGET_ROOTFS_EXT2 > - bool "ext2 root filesystem" > + bool "ext2/3/4 root filesystem" > help > - Build an ext2 root filesystem > + Build an ext2/3/4 root filesystem > > if BR2_TARGET_ROOTFS_EXT2 > > +choice > + bool "ext generation" Given the way it appears in menuconfig, I think this will be hard to understand for many users. Perhaps "ext generation (ext2, ext3 or ext4)". > + default BR2_TARGET_ROOTFS_EXT2_2 Although this matches the current default, doesn't it make more sense to "bump" to ext4? > + > +config BR2_TARGET_ROOTFS_EXT2_2 > + bool "ext2" > + > +config BR2_TARGET_ROOTFS_EXT2_3 > + bool "ext3" > + select BR2_PACKAGE_HOST_E2FSPROGS We don't usually select the host package. On the other hand, the support for user-selectable host packages is pretty recent, so we don't have a real tradition for this. Peter? > + > +config BR2_TARGET_ROOTFS_EXT2_4 > + bool "ext4" > + select BR2_PACKAGE_HOST_E2FSPROGS > + > +endchoice > + > +config BR2_TARGET_ROOTFS_EXT2_GEN > + int > + default 2 if BR2_TARGET_ROOTFS_EXT2_2 > + default 3 if BR2_TARGET_ROOTFS_EXT2_3 > + default 4 if BR2_TARGET_ROOTFS_EXT2_4 > + > config BR2_TARGET_ROOTFS_EXT2_BLOCKS > int "size in blocks (leave at 0 for auto calculation)" > default 0 [snip] > diff --git a/fs/ext2/ext2.mk b/fs/ext2/ext2.mk > index 7b71592..80ad93f 100644 > --- a/fs/ext2/ext2.mk > +++ b/fs/ext2/ext2.mk > @@ -18,10 +18,10 @@ ifneq ($(strip $(BR2_TARGET_ROOTFS_EXT2_RESBLKS)),0) > EXT2_OPTS += -m $(BR2_TARGET_ROOTFS_EXT2_RESBLKS) > endif > > -ROOTFS_EXT2_DEPENDENCIES = host-genext2fs > +ROOTFS_EXT2_DEPENDENCIES = host-genext2fs $(if $(BR2_PACKAGE_HOST_E2FSPROGS),host-e2fsprogs) Although this is correct, I think it looks confusing. I prefer a more explicit ifeq ($(BR2_TARGET_ROOTFS_EXT2_3)$(BR2_TARGET_ROOTFS_EXT2_4),y) ROOTFS_EXT2_DEPENDENCIES += host-e2fsprogs endif > > define ROOTFS_EXT2_CMD > - PATH=$(TARGET_PATH) fs/ext2/genext2fs.sh -d $(TARGET_DIR) $(EXT2_OPTS) $@ > + PATH=$(TARGET_PATH) fs/ext2/genext2fs.sh -d $(TARGET_DIR) $(EXT2_OPTS) -$(BR2_TARGET_ROOTFS_EXT2_GEN) $@ Minor nit: I would prefer EXT2_OPTS += -$(BR2_TARGET_ROOTFS_EXT2_GEN) > endef > > $(eval $(call ROOTFS_TARGET,ext2)) > diff --git a/fs/ext2/genext2fs.sh b/fs/ext2/genext2fs.sh > index 7a518ae..fcbd43c 100755 > --- a/fs/ext2/genext2fs.sh > +++ b/fs/ext2/genext2fs.sh > @@ -1,19 +1,22 @@ > #!/bin/sh > # genext2fs wrapper calculating needed blocks/inodes values if not specified > +set -e > > export LC_ALL=C > > CALC_BLOCKS=1 > CALC_INODES=1 > > -while getopts x:d:D:b:i:N:m:g:e:zfqUPhVv f > +while getopts x:d:D:b:i:N:m:g:e:zfqUPhVv234 f > do > case $f in > + 2|3|4) GEN=$f ;; > b) CALC_BLOCKS=0 ;; > N) CALC_INODES=0; INODES=$OPTARG ;; > d) TARGET_DIR=$OPTARG ;; > esac > done > +eval IMG="\"\${${OPTIND}}\"" > > # calculate needed inodes > if [ $CALC_INODES -eq 1 ]; > @@ -30,7 +33,48 @@ then > # we scale inodes / blocks with 10% to compensate for bitmaps size + slack > BLOCKS=$(du -s -c -k $TARGET_DIR | grep total | sed -e "s/total//") > BLOCKS=$(expr 500 + \( $BLOCKS + $INODES / 8 \) \* 11 / 10) > + # we add 1081 blocks (a bit more than 1 MiB, assuming 1KiB blocks) for > + # the journal if ext3/4 Is this based on anything? Could you add something like "This allows filesystems up to 4GiB"? > + if [ ${GEN} -ge 3 ]; then > + BLOCKS=$(expr 1081 + $BLOCKS ) > + fi > set -- $@ -b $BLOCKS > fi > > -exec genext2fs $@ > +# Remove -{2,3,4} from the arguments, they are not recognised > +# by genext2fs and we handle them manually later Wouldn't it be a lot simpler to pass the generation through the environment instead? > +first=1 > +for o; do > + case "${o}" in > + -2|-3|-4) ;; > + *) if [ ${first} -eq 1 ]; then > + set -- > + first=0 > + fi > + set -- "$@" "${o}" > + ;; > + esac > +done > + > +# Generate the base ext2 file system > +genext2fs "$@" > + > +# Upgrade to ext3 if needed > +if [ ${GEN} -ge 3 ]; then > + tune2fs -j -J size=1 "${IMG}" >/dev/null Ah, this is where the 1081 blocks come from. There should be a comment pointing to that 1081 so it's easier to find this back if it is ever changed to a different value. Why does it have to be >/dev/null? We don't usually do that... In the script I used, I also added -c 0 (max mount count) and -i 0 (interval between checks). That's not for this patch of course, but I think it's something useful to have in general. > +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=$? This fsck is needed for ext3 as well, just to set rev0 -> rev1. Of course, patch 4/4 does that already. You should add a comment why fsck is needed. I would use e2fsck rather than fsck.ext4, but that's a minor thing. > + # 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 Why don't we want a UUID? We have it for other filesystems, e.g. ubifs... Regards, Arnout > +fi >
>>>>> "Arnout" == Arnout Vandecappelle <arnout@mind.be> writes: Hi, >> if BR2_TARGET_ROOTFS_EXT2 >> >> +choice >> + bool "ext generation" Arnout> Given the way it appears in menuconfig, I think this will be hard to Arnout> understand for many users. Perhaps "ext generation (ext2, ext3 or Arnout> ext4)". Or simply ext variant? >> + default BR2_TARGET_ROOTFS_EXT2_2 Arnout> Although this matches the current default, doesn't it make Arnout> more sense to "bump" to ext4? Possibly. It would break existing configs though. >> +config BR2_TARGET_ROOTFS_EXT2_2 >> + bool "ext2" >> + >> +config BR2_TARGET_ROOTFS_EXT2_3 >> + bool "ext3" >> + select BR2_PACKAGE_HOST_E2FSPROGS Arnout> We don't usually select the host package. On the other hand, the Arnout> support for user-selectable host packages is pretty recent, so we Arnout> don't have a real tradition for this. Arnout> Peter? I would argue that we should keep kconfig and makefiles consistent when possible, so when we have a user selectable host package (kconfig symbol), we should select it.
On 19/02/13 13:03, Peter Korsgaard wrote: >>>>>> "Arnout" == Arnout Vandecappelle <arnout@mind.be> writes: > > Hi, > > >> if BR2_TARGET_ROOTFS_EXT2 > >> > >> +choice > >> + bool "ext generation" > > Arnout> Given the way it appears in menuconfig, I think this will be hard to > Arnout> understand for many users. Perhaps "ext generation (ext2, ext3 or > Arnout> ext4)". > > Or simply ext variant? > > >> + default BR2_TARGET_ROOTFS_EXT2_2 > > Arnout> Although this matches the current default, doesn't it make > Arnout> more sense to "bump" to ext4? > > Possibly. It would break existing configs though. No it doesn't. It just _changes_ existing _defconfigs_. It is similar to (though a bit more invasive than) bumping the default kernel version. > > >> +config BR2_TARGET_ROOTFS_EXT2_2 > >> + bool "ext2" > >> + > >> +config BR2_TARGET_ROOTFS_EXT2_3 > >> + bool "ext3" > >> + select BR2_PACKAGE_HOST_E2FSPROGS > > Arnout> We don't usually select the host package. On the other hand, the > Arnout> support for user-selectable host packages is pretty recent, so we > Arnout> don't have a real tradition for this. > > Arnout> Peter? > > I would argue that we should keep kconfig and makefiles consistent when > possible, so when we have a user selectable host package (kconfig > symbol), we should select it. OK. We probably have quite a few violations of this convention, but we can look out for it when changes are made. Regards, Arnout
Arnout, All, On Tuesday 19 February 2013 Arnout Vandecappelle wrote: > Great feature addition! :-) > On 18/02/13 00:10, Yann E. MORIN wrote: [--SNIP--] > > config BR2_TARGET_ROOTFS_EXT2 > > - bool "ext2 root filesystem" > > + bool "ext2/3/4 root filesystem" > > help > > - Build an ext2 root filesystem > > + Build an ext2/3/4 root filesystem > > > > if BR2_TARGET_ROOTFS_EXT2 > > > > +choice > > + bool "ext generation" > > Given the way it appears in menuconfig, I think this will be hard to > understand for many users. Perhaps "ext generation (ext2, ext3 or ext4)". OK, will look at it. > > + default BR2_TARGET_ROOTFS_EXT2_2 > > Although this matches the current default, doesn't it make more sense > to "bump" to ext4? No. There are stuff that does have to be ext2 (mostly because of dumb bootloaders that can't read ext3/4). > > + > > +config BR2_TARGET_ROOTFS_EXT2_2 > > + bool "ext2" > > + > > +config BR2_TARGET_ROOTFS_EXT2_3 > > + bool "ext3" > > + select BR2_PACKAGE_HOST_E2FSPROGS > > We don't usually select the host package. On the other hand, the > support for user-selectable host packages is pretty recent, so we don't > have a real tradition for this. OK, so only depend on e2fsprogs from the .mk, then. [--SNIP--] > > -ROOTFS_EXT2_DEPENDENCIES = host-genext2fs > > +ROOTFS_EXT2_DEPENDENCIES = host-genext2fs $(if $(BR2_PACKAGE_HOST_E2FSPROGS),host-e2fsprogs) > > Although this is correct, I think it looks confusing. I prefer a more > explicit > > ifeq ($(BR2_TARGET_ROOTFS_EXT2_3)$(BR2_TARGET_ROOTFS_EXT2_4),y) > ROOTFS_EXT2_DEPENDENCIES += host-e2fsprogs > endif OK. > > define ROOTFS_EXT2_CMD > > - PATH=$(TARGET_PATH) fs/ext2/genext2fs.sh -d $(TARGET_DIR) $(EXT2_OPTS) $@ > > + PATH=$(TARGET_PATH) fs/ext2/genext2fs.sh -d $(TARGET_DIR) $(EXT2_OPTS) -$(BR2_TARGET_ROOTFS_EXT2_GEN) $@ > > Minor nit: I would prefer > EXT2_OPTS += -$(BR2_TARGET_ROOTFS_EXT2_GEN) Right. > > diff --git a/fs/ext2/genext2fs.sh b/fs/ext2/genext2fs.sh > > index 7a518ae..fcbd43c 100755 > > --- a/fs/ext2/genext2fs.sh > > +++ b/fs/ext2/genext2fs.sh [--SNIP--] > > @@ -30,7 +33,48 @@ then > > # we scale inodes / blocks with 10% to compensate for bitmaps size + slack > > BLOCKS=$(du -s -c -k $TARGET_DIR | grep total | sed -e "s/total//") > > BLOCKS=$(expr 500 + \( $BLOCKS + $INODES / 8 \) \* 11 / 10) > > + # we add 1081 blocks (a bit more than 1 MiB, assuming 1KiB blocks) for > > + # the journal if ext3/4 > > Is this based on anything? Could you add something like "This allows > filesystems up to 4GiB"? I've "bisected" the number of blocks required to add a journal, and 1081 was the strict minimum. 1080 blocks, and tune2fs would yell. This is a purely test-and-check process with two different ext2 filesystems, one ~10 MiBs, another 128MiB. YMMV, as they use to say! ;-) > > + if [ ${GEN} -ge 3 ]; then > > + BLOCKS=$(expr 1081 + $BLOCKS ) > > + fi > > set -- $@ -b $BLOCKS > > fi > > > > -exec genext2fs $@ > > +# Remove -{2,3,4} from the arguments, they are not recognised > > +# by genext2fs and we handle them manually later > > Wouldn't it be a lot simpler to pass the generation through the > environment instead? I'd like to avoid 'scanning' .config if possible. But I'll see what to do about it. > > +first=1 > > +for o; do > > + case "${o}" in > > + -2|-3|-4) ;; > > + *) if [ ${first} -eq 1 ]; then > > + set -- > > + first=0 > > + fi > > + set -- "$@" "${o}" > > + ;; > > + esac > > +done > > + > > +# Generate the base ext2 file system > > +genext2fs "$@" > > + > > +# Upgrade to ext3 if needed > > +if [ ${GEN} -ge 3 ]; then > > + tune2fs -j -J size=1 "${IMG}" >/dev/null > > Ah, this is where the 1081 blocks come from. There should be a comment > pointing to that 1081 so it's easier to find this back if it is ever > changed to a different value. OK. > Why does it have to be >/dev/null? We don't usually do that... I usually >/dev/null to only see errors, as I don't give a dime about the output when all goes well. I'll strip it off before I resend. > In the script I used, I also added -c 0 (max mount count) and -i 0 > (interval between checks). That's not for this patch of course, but I > think it's something useful to have in general. Well, I'd rather leave that for a post-image script. I think it is highly context-specific whether you want time- or count-based checks, or not. But that's trivial enough to add, so I'll give it a whirl. > > +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=$? > > This fsck is needed for ext3 as well, just to set rev0 -> rev1. Of > course, patch 4/4 does that already. No, because *this* patch does not set rev1, and fsck is not needed after adding a journal. It's needed only when setting rev1, or upgrading to ext4. > You should add a comment why fsck is needed. I'll do. > I would use e2fsck rather than fsck.ext4, but that's a minor thing. Well, does e2fsck behave properly when called e2fsck, and not through its symlinks mkfs.extX ? I'll check that. > > + # 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 > > Why don't we want a UUID? We have it for other filesystems, e.g. ubifs... Reproducible builds. The UUID added by e2fsck is random, so one can not reproduce the same images. Explicitly adding a UUID can then be done in a post-image hook, but is context-specific. Thanks for the review. :-) Regards, Yann E. MORIN.
Arnout, All, On Tuesday 19 February 2013 Yann E. MORIN wrote: > On Tuesday 19 February 2013 Arnout Vandecappelle wrote: [--SNIP--] > > > +# Remove -{2,3,4} from the arguments, they are not recognised > > > +# by genext2fs and we handle them manually later > > > > Wouldn't it be a lot simpler to pass the generation through the > > environment instead? > > I'd like to avoid 'scanning' .config if possible. > But I'll see what to do about it. Forget it, I've seen The Light! ;-) Regards, Yann E. MORIN.
diff --git a/fs/ext2/Config.in b/fs/ext2/Config.in index cb4beed..00f11a2 100644 --- a/fs/ext2/Config.in +++ b/fs/ext2/Config.in @@ -1,10 +1,33 @@ config BR2_TARGET_ROOTFS_EXT2 - bool "ext2 root filesystem" + bool "ext2/3/4 root filesystem" help - Build an ext2 root filesystem + Build an ext2/3/4 root filesystem if BR2_TARGET_ROOTFS_EXT2 +choice + bool "ext generation" + default BR2_TARGET_ROOTFS_EXT2_2 + +config BR2_TARGET_ROOTFS_EXT2_2 + bool "ext2" + +config BR2_TARGET_ROOTFS_EXT2_3 + bool "ext3" + select BR2_PACKAGE_HOST_E2FSPROGS + +config BR2_TARGET_ROOTFS_EXT2_4 + bool "ext4" + select BR2_PACKAGE_HOST_E2FSPROGS + +endchoice + +config BR2_TARGET_ROOTFS_EXT2_GEN + int + default 2 if BR2_TARGET_ROOTFS_EXT2_2 + default 3 if BR2_TARGET_ROOTFS_EXT2_3 + default 4 if BR2_TARGET_ROOTFS_EXT2_4 + config BR2_TARGET_ROOTFS_EXT2_BLOCKS int "size in blocks (leave at 0 for auto calculation)" default 0 @@ -21,27 +44,27 @@ choice prompt "Compression method" default BR2_TARGET_ROOTFS_EXT2_NONE help - Select compressor for ext2 filesystem of the root filesystem + Select compressor for ext2/3/4 filesystem of the root filesystem config BR2_TARGET_ROOTFS_EXT2_NONE bool "no compression" help - Do not compress the ext2 filesystem. + Do not compress the ext2/3/4 filesystem. config BR2_TARGET_ROOTFS_EXT2_GZIP bool "gzip" help - Do compress the ext2 filesystem with gzip. + Do compress the ext2/3/4 filesystem with gzip. config BR2_TARGET_ROOTFS_EXT2_BZIP2 bool "bzip2" help - Do compress the ext2 filesystem with bzip2. + Do compress the ext2/3/4 filesystem with bzip2. config BR2_TARGET_ROOTFS_EXT2_LZMA bool "lzma" help - Do compress the ext2 filesystem with lzma. + Do compress the ext2/3/4 filesystem with lzma. endchoice diff --git a/fs/ext2/ext2.mk b/fs/ext2/ext2.mk index 7b71592..80ad93f 100644 --- a/fs/ext2/ext2.mk +++ b/fs/ext2/ext2.mk @@ -18,10 +18,10 @@ ifneq ($(strip $(BR2_TARGET_ROOTFS_EXT2_RESBLKS)),0) EXT2_OPTS += -m $(BR2_TARGET_ROOTFS_EXT2_RESBLKS) endif -ROOTFS_EXT2_DEPENDENCIES = host-genext2fs +ROOTFS_EXT2_DEPENDENCIES = host-genext2fs $(if $(BR2_PACKAGE_HOST_E2FSPROGS),host-e2fsprogs) define ROOTFS_EXT2_CMD - PATH=$(TARGET_PATH) fs/ext2/genext2fs.sh -d $(TARGET_DIR) $(EXT2_OPTS) $@ + PATH=$(TARGET_PATH) fs/ext2/genext2fs.sh -d $(TARGET_DIR) $(EXT2_OPTS) -$(BR2_TARGET_ROOTFS_EXT2_GEN) $@ endef $(eval $(call ROOTFS_TARGET,ext2)) diff --git a/fs/ext2/genext2fs.sh b/fs/ext2/genext2fs.sh index 7a518ae..fcbd43c 100755 --- a/fs/ext2/genext2fs.sh +++ b/fs/ext2/genext2fs.sh @@ -1,19 +1,22 @@ #!/bin/sh # genext2fs wrapper calculating needed blocks/inodes values if not specified +set -e export LC_ALL=C CALC_BLOCKS=1 CALC_INODES=1 -while getopts x:d:D:b:i:N:m:g:e:zfqUPhVv f +while getopts x:d:D:b:i:N:m:g:e:zfqUPhVv234 f do case $f in + 2|3|4) GEN=$f ;; b) CALC_BLOCKS=0 ;; N) CALC_INODES=0; INODES=$OPTARG ;; d) TARGET_DIR=$OPTARG ;; esac done +eval IMG="\"\${${OPTIND}}\"" # calculate needed inodes if [ $CALC_INODES -eq 1 ]; @@ -30,7 +33,48 @@ then # we scale inodes / blocks with 10% to compensate for bitmaps size + slack BLOCKS=$(du -s -c -k $TARGET_DIR | grep total | sed -e "s/total//") BLOCKS=$(expr 500 + \( $BLOCKS + $INODES / 8 \) \* 11 / 10) + # we add 1081 blocks (a bit more than 1 MiB, assuming 1KiB blocks) for + # the journal if ext3/4 + if [ ${GEN} -ge 3 ]; then + BLOCKS=$(expr 1081 + $BLOCKS ) + fi set -- $@ -b $BLOCKS fi -exec genext2fs $@ +# Remove -{2,3,4} from the arguments, they are not recognised +# by genext2fs and we handle them manually later +first=1 +for o; do + case "${o}" in + -2|-3|-4) ;; + *) if [ ${first} -eq 1 ]; then + set -- + first=0 + fi + set -- "$@" "${o}" + ;; + esac +done + +# Generate the base ext2 file system +genext2fs "$@" + +# Upgrade to ext3 if needed +if [ ${GEN} -ge 3 ]; then + tune2fs -j -J size=1 "${IMG}" >/dev/null +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 +fi
Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr> --- fs/ext2/Config.in | 37 ++++++++++++++++++++++++++++++------- fs/ext2/ext2.mk | 4 ++-- fs/ext2/genext2fs.sh | 48 ++++++++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 78 insertions(+), 11 deletions(-)