| Message ID | 20220614050843.11802-2-lan@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] common/rc: add _parse_size_from_string | expand |
On Tue, Jun 14, 2022 at 01:08:43PM +0800, An Long wrote: > _scratch_mkfs_sized only receive integer number of bytes as a valid > input. But if the MKFS_OPTIONS variable exists, it will use the value of > block size in MKFS_OPTIONS to override input. In case of > MKFS_OPTIONS="-b 4k", would result in blocksize=4 but not 4096. This Hi An, Good catch. Can you provide an example that a case fails on this issue, and then it tests passed after having this patch? I'm wondering why no one notice that before. > will give errors to ext2/3/4 etc, and brings potential bugs to xfs or > btrfs. > > In addition, since we can receive various strings, so remove integer > number check. > > Signed-off-by: An Long <lan@suse.com> > --- > common/rc | 18 ++++++------------ > 1 file changed, 6 insertions(+), 12 deletions(-) > > diff --git a/common/rc b/common/rc > index 22050bc2..026007d3 100644 > --- a/common/rc > +++ b/common/rc > @@ -1077,7 +1077,7 @@ _parse_size_from_string() > } > > # Create fs of certain size on scratch device > -# _scratch_mkfs_sized <size in bytes> [optional blocksize] > +# _scratch_mkfs_sized <size> [optional blocksize] > _scratch_mkfs_sized() > { > local fssize=$1 > @@ -1086,13 +1086,13 @@ _scratch_mkfs_sized() > > case $FSTYP in > xfs) > - def_blksz=`echo $MKFS_OPTIONS | sed -rn 's/.*-b ?size= ?+([0-9]+).*/\1/p'` > + def_blksz=`echo $MKFS_OPTIONS | sed -rn 's/.*-b ?size= ?+([0-9]+[a-zA-Z]?).*/\1/p'` > ;; > btrfs) > - def_blksz=`echo $MKFS_OPTIONS | sed -rn 's/.*-s ?+([0-9]+).*/\1/p'` > + def_blksz=`echo $MKFS_OPTIONS | sed -rn 's/.*-s ?+([0-9]+[a-zA-Z]?).*/\1/p'` > ;; > ext2|ext3|ext4|ext4dev|udf|reiser4|ocfs2|reiserfs) > - def_blksz=`echo $MKFS_OPTIONS | sed -rn 's/.*-b ?+([0-9]+).*/\1/p'` > + def_blksz=`echo $MKFS_OPTIONS | sed -rn 's/.*-b ?+([0-9]+[a-zA-Z]?).*/\1/p'` > ;; > jfs) > def_blksz=4096 > @@ -1101,14 +1101,8 @@ _scratch_mkfs_sized() > > [ -n "$def_blksz" ] && blocksize=$def_blksz > [ -z "$blocksize" ] && blocksize=4096 > - > - local re='^[0-9]+$' > - if ! [[ $fssize =~ $re ]] ; then > - _notrun "error: _scratch_mkfs_sized: fs size \"$fssize\" not an integer." > - fi > - if ! [[ $blocksize =~ $re ]] ; then > - _notrun "error: _scratch_mkfs_sized: block size \"$blocksize\" not an integer." > - fi > + blocksize=$(_parse_size_from_string $blocksize) > + fssize=$(_parse_size_from_string $fssize) For now, the _parse_size_from_string is only used for this patch, I think these two patches can be merged into one patch, as it trys to fix one single problem. Thanks, Zorro > > local blocks=`expr $fssize / $blocksize` > > -- > 2.35.3 >
On Wed, 2022-06-15 at 12:15 +0800, Zorro Lang wrote: > On Tue, Jun 14, 2022 at 01:08:43PM +0800, An Long wrote: > > _scratch_mkfs_sized only receive integer number of bytes as a valid > > input. But if the MKFS_OPTIONS variable exists, it will use the > > value of > > block size in MKFS_OPTIONS to override input. In case of > > MKFS_OPTIONS="-b 4k", would result in blocksize=4 but not 4096. > > This > > Hi An, > > Good catch. Can you provide an example that a case fails on this > issue, and > then it tests passed after having this patch? I'm wondering why no > one notice > that before. > For example, I set MKFS_OPTIONS="-b 4k" and then run generic/416 on ext4 filesystem. # ./check tests/generic/416 FSTYP -- ext4 PLATFORM -- Linux/x86_64 bogon 5.3.18-57-default #1 SMP Wed Apr 28 10:54:41 UTC 2021 (ba3c2e9) MKFS_OPTIONS -- -b 4k /dev/loop1 MOUNT_OPTIONS -- -o acl,user_xattr /dev/loop1 /mnt/scratch generic/416 90s ... [failed, exit status 1]- output mismatch (see /home/lan/work/xfstests/results//generic/416.out.bad) --- tests/generic/416.out 2021-06-25 23:18:09.595254118 +0800 +++ /home/lan/work/xfstests/results//generic/416.out.bad 2022- 06-15 16:33:19.584206316 +0800 @@ -1,3 +1,4 @@ QA output created by 416 -wrote 16777216/16777216 bytes at offset 0 -XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +mount: /mnt/scratch: wrong fs type, bad option, bad superblock on /dev/loop1, missing codepage or helper program, or other error. +mount -o acl,user_xattr /dev/loop1 /mnt/scratch failed +(see /home/lan/work/xfstests/results//generic/416.full for details) ... (Run 'diff -u /home/lan/work/xfstests/tests/generic/416.out /home/lan/work/xfstests/results//generic/416.out.bad' to see the entire diff) Ran: generic/416 Failures: generic/416 Failed 1 of 1 tests But the more common result as below: # ./check tests/generic/416 FSTYP -- ext4 PLATFORM -- Linux/x86_64 bogon 5.3.18-57-default #1 SMP Wed Apr 28 10:54:41 UTC 2021 (ba3c2e9) MKFS_OPTIONS -- -b 4k /dev/loop1 MOUNT_OPTIONS -- -o acl,user_xattr /dev/loop1 /mnt/scratch umount: /mnt/scratch: target is busy. generic/416 77s ... 90s Ran: generic/416 Passed all 1 tests Test passed due to _require_scratch_nocheck() ignore the umount error. However, "mkfs.ext4: invalid block size - 4" was found in the 416.full Apart from this, the error only occurs when MKFS_OPTIONS is set, and doesn't affect xfs or btrfs. Since ext4 only supports 4k blocksize on most platforms, we usually don't set blocksize in MKFS_OPTIONS. I think that's why this problem hasn't been found before. In fact, I found this issue when I tested on ppc64 with 64k kernel and 4k subblocksize fs. > > will give errors to ext2/3/4 etc, and brings potential bugs to xfs > > or > > btrfs. > > > > In addition, since we can receive various strings, so remove > > integer > > number check. > > > > Signed-off-by: An Long <lan@suse.com> > > --- > > common/rc | 18 ++++++------------ > > 1 file changed, 6 insertions(+), 12 deletions(-) > > > > diff --git a/common/rc b/common/rc > > index 22050bc2..026007d3 100644 > > --- a/common/rc > > +++ b/common/rc > > @@ -1077,7 +1077,7 @@ _parse_size_from_string() > > } > > > > # Create fs of certain size on scratch device > > -# _scratch_mkfs_sized <size in bytes> [optional blocksize] > > +# _scratch_mkfs_sized <size> [optional blocksize] > > _scratch_mkfs_sized() > > { > > local fssize=$1 > > @@ -1086,13 +1086,13 @@ _scratch_mkfs_sized() > > > > case $FSTYP in > > xfs) > > - def_blksz=`echo $MKFS_OPTIONS | sed -rn 's/.*-b ?size= > > ?+([0-9]+).*/\1/p'` > > + def_blksz=`echo $MKFS_OPTIONS | sed -rn 's/.*-b ?size= > > ?+([0-9]+[a-zA-Z]?).*/\1/p'` > > ;; > > btrfs) > > - def_blksz=`echo $MKFS_OPTIONS | sed -rn 's/.*-s ?+([0- > > 9]+).*/\1/p'` > > + def_blksz=`echo $MKFS_OPTIONS | sed -rn 's/.*-s ?+([0- > > 9]+[a-zA-Z]?).*/\1/p'` > > ;; > > ext2|ext3|ext4|ext4dev|udf|reiser4|ocfs2|reiserfs) > > - def_blksz=`echo $MKFS_OPTIONS | sed -rn 's/.*-b ?+([0- > > 9]+).*/\1/p'` > > + def_blksz=`echo $MKFS_OPTIONS | sed -rn 's/.*-b ?+([0- > > 9]+[a-zA-Z]?).*/\1/p'` > > ;; > > jfs) > > def_blksz=4096 > > @@ -1101,14 +1101,8 @@ _scratch_mkfs_sized() > > > > [ -n "$def_blksz" ] && blocksize=$def_blksz > > [ -z "$blocksize" ] && blocksize=4096 > > - > > - local re='^[0-9]+$' > > - if ! [[ $fssize =~ $re ]] ; then > > - _notrun "error: _scratch_mkfs_sized: fs size > > \"$fssize\" not an integer." > > - fi > > - if ! [[ $blocksize =~ $re ]] ; then > > - _notrun "error: _scratch_mkfs_sized: block size > > \"$blocksize\" not an integer." > > - fi > > + blocksize=$(_parse_size_from_string $blocksize) > > + fssize=$(_parse_size_from_string $fssize) > > For now, the _parse_size_from_string is only used for this patch, I > think these > two patches can be merged into one patch, as it trys to fix one > single problem. > There are a total of two logical changes. In particular it also include a Function update. So it's better to keep two separate patch. But I'll add patch depends on info to commit message. Thank you for your time! > Thanks, > Zorro > > > > > local blocks=`expr $fssize / $blocksize` > > > > -- > > 2.35.3 > >
diff --git a/common/rc b/common/rc index 22050bc2..026007d3 100644 --- a/common/rc +++ b/common/rc @@ -1077,7 +1077,7 @@ _parse_size_from_string() } # Create fs of certain size on scratch device -# _scratch_mkfs_sized <size in bytes> [optional blocksize] +# _scratch_mkfs_sized <size> [optional blocksize] _scratch_mkfs_sized() { local fssize=$1 @@ -1086,13 +1086,13 @@ _scratch_mkfs_sized() case $FSTYP in xfs) - def_blksz=`echo $MKFS_OPTIONS | sed -rn 's/.*-b ?size= ?+([0-9]+).*/\1/p'` + def_blksz=`echo $MKFS_OPTIONS | sed -rn 's/.*-b ?size= ?+([0-9]+[a-zA-Z]?).*/\1/p'` ;; btrfs) - def_blksz=`echo $MKFS_OPTIONS | sed -rn 's/.*-s ?+([0-9]+).*/\1/p'` + def_blksz=`echo $MKFS_OPTIONS | sed -rn 's/.*-s ?+([0-9]+[a-zA-Z]?).*/\1/p'` ;; ext2|ext3|ext4|ext4dev|udf|reiser4|ocfs2|reiserfs) - def_blksz=`echo $MKFS_OPTIONS | sed -rn 's/.*-b ?+([0-9]+).*/\1/p'` + def_blksz=`echo $MKFS_OPTIONS | sed -rn 's/.*-b ?+([0-9]+[a-zA-Z]?).*/\1/p'` ;; jfs) def_blksz=4096 @@ -1101,14 +1101,8 @@ _scratch_mkfs_sized() [ -n "$def_blksz" ] && blocksize=$def_blksz [ -z "$blocksize" ] && blocksize=4096 - - local re='^[0-9]+$' - if ! [[ $fssize =~ $re ]] ; then - _notrun "error: _scratch_mkfs_sized: fs size \"$fssize\" not an integer." - fi - if ! [[ $blocksize =~ $re ]] ; then - _notrun "error: _scratch_mkfs_sized: block size \"$blocksize\" not an integer." - fi + blocksize=$(_parse_size_from_string $blocksize) + fssize=$(_parse_size_from_string $fssize) local blocks=`expr $fssize / $blocksize`
_scratch_mkfs_sized only receive integer number of bytes as a valid input. But if the MKFS_OPTIONS variable exists, it will use the value of block size in MKFS_OPTIONS to override input. In case of MKFS_OPTIONS="-b 4k", would result in blocksize=4 but not 4096. This will give errors to ext2/3/4 etc, and brings potential bugs to xfs or btrfs. In addition, since we can receive various strings, so remove integer number check. Signed-off-by: An Long <lan@suse.com> --- common/rc | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-)