<div dir="ltr"><div dir="ltr"><div class="gmail_default" style="font-size:small">Hi Petr,</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Jan 15, 2021 at 2:59 PM Petr Vorel <<a href="mailto:pvorel@suse.cz">pvorel@suse.cz</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Li,<br>
<br>
...<br>
> > diff --git a/testcases/kernel/device-drivers/zram/zram_lib.sh<br>
> > b/testcases/kernel/device-drivers/zram/zram_lib.sh<br>
> > index 3f4d1d55f..1a611b974 100755<br>
> > --- a/testcases/kernel/device-drivers/zram/zram_lib.sh<br>
> > +++ b/testcases/kernel/device-drivers/zram/zram_lib.sh<br>
> > @@ -178,13 +178,30 @@ zram_swapoff()<br>
> >  zram_makefs()<br>
> >  {<br>
> >         tst_require_cmds mkfs<br>
> > +<br>
> > +       local default_fs fs<br>
> >         local i=0<br>
<br>
> > +       for fs in $zram_filesystems ext2; do<br>
> > +               if tst_supported_fs $fs 2> /dev/null; then<br>
> > +                       default_fs="$fs"<br>
> > +                       break<br>
> > +               fi<br>
> > +       done<br>
<br>
<br>
> This workaround makes some sense but a bit overlap to traverse<br>
>  $zram_filesystems.<br>
Not sure if I understand you.<span class="gmail_default" style="font-size:small"></span></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> Maybe we can remove the unsupported filesystems from $zram_filesystems<br>
> list via tst_supported_fs and tst_cmd_available, to avoid involving that<br>
> additional<br>
> variable 'default_fs', then in following test if $zram_filesystems is a<br>
> null string<br>
> just exit with TCONF directly?<br>
<br>
I understood, that there must be 4 runs, because 4 /dev/zram* has been used<br>
(dev_num=4). Do you mean to check supported systems in the setup (it'd be safer<br>
to move the calculation to the setup) and lower dev_num if needed?<br></blockquote><div><br></div><div><div class="gmail_default" style="font-size:small">My fault, seems I ignored the dev_num in the previous review, I just looked into</div><div class="gmail_default" style="font-size:small">the zram_makefs() and suggest pruning the $zram_filesystems.</div><div class="gmail_default" style="font-size:small"><br></div><div class="gmail_default" style="font-size:small">You are right, we could have two ways now:</div><div class="gmail_default" style="font-size:small">1. check supported filesystems and recalculate relative parameters in the setup</div><div class="gmail_default" style="font-size:small">(I prefer this, but needs more work/time to refactor code)</div><div class="gmail_default" style="font-size:small"><br></div><div class="gmail_default" style="font-size:small">2. add variable default_fs as what you did</div><div class="gmail_default" style="font-size:small">(the cons side might have duplicated test, but I'm not against it because of the coming LTP release date)</div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
And tst_cmd_available is not needed, because tst_supported_fs checks also for<br>
mkfs.foo presence.<br></blockquote><div><span class="gmail_default" style="font-size:small">Great.</span></div><div><span class="gmail_default" style="font-size:small"></span> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Also further cleanup after release: it'd make sense to move zram_makefs and<br>
zram_mount to zram01.sh, which is the only test which use them. And zram_makefs<br>
uses $zram_filesystems.<br>
Or keep them in zram_lib.sh, but pass $zram_filesystems to zram_makefs as a<br>
parameter. Current state is confusing a bit.<br></blockquote><div><br></div><div><div class="gmail_default" style="font-size:small">Yes, or let's do the refactoring after release.</div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Kind regards,<br>
Petr<br>
<br>
> > +<br>
> > +       if [ -z "$default_fs" ]; then<br>
> > +               tst_res TINFO "supported filesystems"<br>
> > +               tst_supported_fs > /dev/null<br>
> > +               tst_brk TCONF "missing kernel support or mkfs for all of<br>
> > these filesystems: $zram_filesystems"<br>
> > +       fi<br>
> > +<br>
> >         for fs in $zram_filesystems; do<br>
> > -               # if requested fs not supported default it to ext2<br>
> > -               tst_supported_fs $fs 2> /dev/null || fs=ext2<br>
> > +               # use default if requested fs not supported or missing mkfs<br>
> > +               tst_supported_fs $fs 2> /dev/null && tst_cmd_available<br>
> > mkfs.$fs \<br>
> > +                       || fs=$default_fs<br>
<br>
> >                 tst_res TINFO "make $fs filesystem on /dev/zram$i"<br>
> > +<br>
> >                 mkfs.$fs /dev/zram$i > err.log 2>&1<br>
> >                 if [ $? -ne 0 ]; then<br>
> >                         cat err.log<br>
> > --<br>
> > 2.29.2<br>
<br>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="gmail_signature"><div dir="ltr"><div>Regards,<br></div><div>Li Wang<br></div></div></div></div>