[Buildroot] [PATCH 4/4] fs/ext: add support for ext2 rev0 and rev1

Yann E. MORIN yann.morin.1998 at free.fr
Tue Feb 19 18:10:33 UTC 2013


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.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'


More information about the buildroot mailing list