| Message ID | 20210114183226.794-1-pvorel@suse.cz |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/1] zram: Check properly command dependencies | expand |
Hi Petr, On Fri, Jan 15, 2021 at 2:32 AM Petr Vorel <pvorel@suse.cz> wrote: > Instead of relying that there is mkfs.ext2 as a backup, > search for supported default. > > Always check ext2 (in case there is enough space for btrfs but > no mkfs.btrfs). > > This fixes error when even the default ext2 is not supported: > > zram01 5 TINFO: make ext2 filesystem on /dev/zram0 > /opt/ltp/testcases/bin/zram01.sh: line 188: mkfs.ext2: not found > zram01 5 TFAIL: failed to make ext2 on /dev/zram0 > > Signed-off-by: Petr Vorel <pvorel@suse.cz> > --- > Hi, > > fix to be merged before release. > > NOTE: Bug affecting BusyBox needs to be discussed: > http://lists.linux.it/pipermail/ltp/2021-January/020568.html > > Kind regards, > Petr > > .../kernel/device-drivers/zram/zram_lib.sh | 21 +++++++++++++++++-- > 1 file changed, 19 insertions(+), 2 deletions(-) > > diff --git a/testcases/kernel/device-drivers/zram/zram_lib.sh > b/testcases/kernel/device-drivers/zram/zram_lib.sh > index 3f4d1d55f..1a611b974 100755 > --- a/testcases/kernel/device-drivers/zram/zram_lib.sh > +++ b/testcases/kernel/device-drivers/zram/zram_lib.sh > @@ -178,13 +178,30 @@ zram_swapoff() > zram_makefs() > { > tst_require_cmds mkfs > + > + local default_fs fs > local i=0 > > + for fs in $zram_filesystems ext2; do > + if tst_supported_fs $fs 2> /dev/null; then > + default_fs="$fs" > + break > + fi > + done > This workaround makes some sense but a bit overlap to traverse $zram_filesystems. Maybe we can remove the unsupported filesystems from $zram_filesystems list via tst_supported_fs and tst_cmd_available, to avoid involving that additional variable 'default_fs', then in following test if $zram_filesystems is a null string just exit with TCONF directly? > + > + if [ -z "$default_fs" ]; then > + tst_res TINFO "supported filesystems" > + tst_supported_fs > /dev/null > + tst_brk TCONF "missing kernel support or mkfs for all of > these filesystems: $zram_filesystems" > + fi > + > for fs in $zram_filesystems; do > - # if requested fs not supported default it to ext2 > - tst_supported_fs $fs 2> /dev/null || fs=ext2 > + # use default if requested fs not supported or missing mkfs > + tst_supported_fs $fs 2> /dev/null && tst_cmd_available > mkfs.$fs \ > + || fs=$default_fs > > tst_res TINFO "make $fs filesystem on /dev/zram$i" > + > mkfs.$fs /dev/zram$i > err.log 2>&1 > if [ $? -ne 0 ]; then > cat err.log > -- > 2.29.2 > >
Hi Li, ... > > diff --git a/testcases/kernel/device-drivers/zram/zram_lib.sh > > b/testcases/kernel/device-drivers/zram/zram_lib.sh > > index 3f4d1d55f..1a611b974 100755 > > --- a/testcases/kernel/device-drivers/zram/zram_lib.sh > > +++ b/testcases/kernel/device-drivers/zram/zram_lib.sh > > @@ -178,13 +178,30 @@ zram_swapoff() > > zram_makefs() > > { > > tst_require_cmds mkfs > > + > > + local default_fs fs > > local i=0 > > + for fs in $zram_filesystems ext2; do > > + if tst_supported_fs $fs 2> /dev/null; then > > + default_fs="$fs" > > + break > > + fi > > + done > This workaround makes some sense but a bit overlap to traverse > $zram_filesystems. Not sure if I understand you. > Maybe we can remove the unsupported filesystems from $zram_filesystems > list via tst_supported_fs and tst_cmd_available, to avoid involving that > additional > variable 'default_fs', then in following test if $zram_filesystems is a > null string > just exit with TCONF directly? I understood, that there must be 4 runs, because 4 /dev/zram* has been used (dev_num=4). Do you mean to check supported systems in the setup (it'd be safer to move the calculation to the setup) and lower dev_num if needed? And tst_cmd_available is not needed, because tst_supported_fs checks also for mkfs.foo presence. Also further cleanup after release: it'd make sense to move zram_makefs and zram_mount to zram01.sh, which is the only test which use them. And zram_makefs uses $zram_filesystems. Or keep them in zram_lib.sh, but pass $zram_filesystems to zram_makefs as a parameter. Current state is confusing a bit. Kind regards, Petr > > + > > + if [ -z "$default_fs" ]; then > > + tst_res TINFO "supported filesystems" > > + tst_supported_fs > /dev/null > > + tst_brk TCONF "missing kernel support or mkfs for all of > > these filesystems: $zram_filesystems" > > + fi > > + > > for fs in $zram_filesystems; do > > - # if requested fs not supported default it to ext2 > > - tst_supported_fs $fs 2> /dev/null || fs=ext2 > > + # use default if requested fs not supported or missing mkfs > > + tst_supported_fs $fs 2> /dev/null && tst_cmd_available > > mkfs.$fs \ > > + || fs=$default_fs > > tst_res TINFO "make $fs filesystem on /dev/zram$i" > > + > > mkfs.$fs /dev/zram$i > err.log 2>&1 > > if [ $? -ne 0 ]; then > > cat err.log > > -- > > 2.29.2
Hi Petr, On Fri, Jan 15, 2021 at 2:59 PM Petr Vorel <pvorel@suse.cz> wrote: > Hi Li, > > ... > > > diff --git a/testcases/kernel/device-drivers/zram/zram_lib.sh > > > b/testcases/kernel/device-drivers/zram/zram_lib.sh > > > index 3f4d1d55f..1a611b974 100755 > > > --- a/testcases/kernel/device-drivers/zram/zram_lib.sh > > > +++ b/testcases/kernel/device-drivers/zram/zram_lib.sh > > > @@ -178,13 +178,30 @@ zram_swapoff() > > > zram_makefs() > > > { > > > tst_require_cmds mkfs > > > + > > > + local default_fs fs > > > local i=0 > > > > + for fs in $zram_filesystems ext2; do > > > + if tst_supported_fs $fs 2> /dev/null; then > > > + default_fs="$fs" > > > + break > > > + fi > > > + done > > > > This workaround makes some sense but a bit overlap to traverse > > $zram_filesystems. > Not sure if I understand you. > > Maybe we can remove the unsupported filesystems from $zram_filesystems > > list via tst_supported_fs and tst_cmd_available, to avoid involving that > > additional > > variable 'default_fs', then in following test if $zram_filesystems is a > > null string > > just exit with TCONF directly? > > I understood, that there must be 4 runs, because 4 /dev/zram* has been used > (dev_num=4). Do you mean to check supported systems in the setup (it'd be > safer > to move the calculation to the setup) and lower dev_num if needed? > My fault, seems I ignored the dev_num in the previous review, I just looked into the zram_makefs() and suggest pruning the $zram_filesystems. You are right, we could have two ways now: 1. check supported filesystems and recalculate relative parameters in the setup (I prefer this, but needs more work/time to refactor code) 2. add variable default_fs as what you did (the cons side might have duplicated test, but I'm not against it because of the coming LTP release date) > > And tst_cmd_available is not needed, because tst_supported_fs checks also > for > mkfs.foo presence. > Great. > > Also further cleanup after release: it'd make sense to move zram_makefs and > zram_mount to zram01.sh, which is the only test which use them. And > zram_makefs > uses $zram_filesystems. > Or keep them in zram_lib.sh, but pass $zram_filesystems to zram_makefs as a > parameter. Current state is confusing a bit. > Yes, or let's do the refactoring after release. > > Kind regards, > Petr > > > > + > > > + if [ -z "$default_fs" ]; then > > > + tst_res TINFO "supported filesystems" > > > + tst_supported_fs > /dev/null > > > + tst_brk TCONF "missing kernel support or mkfs for all > of > > > these filesystems: $zram_filesystems" > > > + fi > > > + > > > for fs in $zram_filesystems; do > > > - # if requested fs not supported default it to ext2 > > > - tst_supported_fs $fs 2> /dev/null || fs=ext2 > > > + # use default if requested fs not supported or missing > mkfs > > > + tst_supported_fs $fs 2> /dev/null && tst_cmd_available > > > mkfs.$fs \ > > > + || fs=$default_fs > > > > tst_res TINFO "make $fs filesystem on /dev/zram$i" > > > + > > > mkfs.$fs /dev/zram$i > err.log 2>&1 > > > if [ $? -ne 0 ]; then > > > cat err.log > > > -- > > > 2.29.2 > >
diff --git a/testcases/kernel/device-drivers/zram/zram_lib.sh b/testcases/kernel/device-drivers/zram/zram_lib.sh index 3f4d1d55f..1a611b974 100755 --- a/testcases/kernel/device-drivers/zram/zram_lib.sh +++ b/testcases/kernel/device-drivers/zram/zram_lib.sh @@ -178,13 +178,30 @@ zram_swapoff() zram_makefs() { tst_require_cmds mkfs + + local default_fs fs local i=0 + for fs in $zram_filesystems ext2; do + if tst_supported_fs $fs 2> /dev/null; then + default_fs="$fs" + break + fi + done + + if [ -z "$default_fs" ]; then + tst_res TINFO "supported filesystems" + tst_supported_fs > /dev/null + tst_brk TCONF "missing kernel support or mkfs for all of these filesystems: $zram_filesystems" + fi + for fs in $zram_filesystems; do - # if requested fs not supported default it to ext2 - tst_supported_fs $fs 2> /dev/null || fs=ext2 + # use default if requested fs not supported or missing mkfs + tst_supported_fs $fs 2> /dev/null && tst_cmd_available mkfs.$fs \ + || fs=$default_fs tst_res TINFO "make $fs filesystem on /dev/zram$i" + mkfs.$fs /dev/zram$i > err.log 2>&1 if [ $? -ne 0 ]; then cat err.log
Instead of relying that there is mkfs.ext2 as a backup, search for supported default. Always check ext2 (in case there is enough space for btrfs but no mkfs.btrfs). This fixes error when even the default ext2 is not supported: zram01 5 TINFO: make ext2 filesystem on /dev/zram0 /opt/ltp/testcases/bin/zram01.sh: line 188: mkfs.ext2: not found zram01 5 TFAIL: failed to make ext2 on /dev/zram0 Signed-off-by: Petr Vorel <pvorel@suse.cz> --- Hi, fix to be merged before release. NOTE: Bug affecting BusyBox needs to be discussed: http://lists.linux.it/pipermail/ltp/2021-January/020568.html Kind regards, Petr .../kernel/device-drivers/zram/zram_lib.sh | 21 +++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-)