Re: [RFC PATCH] xfstests: Add mkfs input validation tests

Jan Tulak
2016-04-29T14:43:16Z

[-- Attachment #1.1: Type: text/plain, Size: 5807 bytes --]

On Fri, Apr 29, 2016 at 3:59 AM, Dave Chinner <david@fromorbit.com> wrote:

> On Thu, Apr 28, 2016 at 10:29:09AM +0200, Jan Tulak wrote:
> > Test inputs for my mkfs-cleaning patchset. This test will fail with the
> old sphageti code mkfs, among others because the old code accepts incorrect
> values.
> >
> >
> >
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > Signed-off-by: Jan Tulak <jtulak@redhat.com>
>
> Please don't strip the commit messages from patches you've picked up
> from other people - it loses valuable information, as well as the
> original author of the code. i.e. The original commit message was:
>
> ​Sorry about that. I removed it mistakenly long time ago, and now, I
didn't realised I should copy yours instead of making my own.

>
> From: Dave Chinner <dchinner@redhat.com>
>
> mkfs.xfs does not do a very good job of input validation. This test
> is designed to exercise the input validation and test good/bad
> combinations of options being set. It will not pass on a current
> mkfs.xfs binary - it is designed to be the test case for a input
> validation cleanup.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
>
> > ---
> >
> > Hi guys,
> >
> > I'm sending this patch although the mentioned patchset is not yet merged.
> > It might help you a bit with checking if there are any issues with
> > the patchset, as here it is clear, what options works and what not.
>
> in which case, a "_require_xfs_mkfs_validation" rule should be
> written to determine the version of mkfs being. e.g. by testing one
> of the failure cases that the unfixed binary says is ok.
> ....
> > +# basic "should fail" options
> > +# logarithm based options are no longer valid
> > +# NOTE: umm, when it got invalid? It seems to be still supported...
> > +#do_mkfs_fail -s log=10 $SCRATCH_DEV
> > +#do_mkfs_fail -b log=10 $SCRATCH_DEV
> > +#do_mkfs_fail -n log=10 $SCRATCH_DEV
> > +#do_mkfs_fail -i log=10 $SCRATCH_DEV
> > +#do_mkfs_fail -d sectlog=10 $SCRATCH_DEV
> > +#do_mkfs_fail -l sectlog=10 $SCRATCH_DEV
>
> They were expected to fail because I was going to remove the log
> options from mkfs as part of the cleanup series because they are
> redundant and nobody uses them. i.e this test was written with what
> I wanted as the end result of the mkfs input validation cleanup, not
> an iteration of the current behaviour.
>
> After all the data section tests, the new tests you've added all
> seem to be pretty ad-hoc. What I was fleshing out in this test was
> a relatively complete set exercising each the different options mkfs
> supports.
>
> I'd only iterated data section options so far in this test. I'd just
> started on the naming section tests, and had not added any but a
> basic test. That needs to be iterated, as do the inode, log (both
> internal and external), metadata and realtime options....
>
> ​I added many of the new lines when I found some issue, to prevent
regressions. But yeah, I will put it into an orderly fashion and iterate
through other things

>
> > +# invalid file section tests
> > +rm -f $fsimg
> > +$XFS_IO_PROG -f -c "truncate $fssize" $fsimg
> > +do_mkfs_fail -d file $fsimg
> > +do_mkfs_fail -d file,name=$fsimg
>
> Why should these fail - size should not be required if the image
> file already exists and is of sufficient size....
>

​Well, they should pass. I'm sending an updated patch to the set as well.

> > +
> > +# naming section tests
> > +do_mkfs_pass -n size=65536 $SCRATCH_DEV
> > +
> > +# boolean options
> > +$XFS_IO_PROG -f -c "truncate $fssize" $fsimg
> > +do_mkfs_pass -d file=1,size=$fssize $fsimg
> > +do_mkfs_pass -d file=0 $SCRATCH_DEV
> > +do_mkfs_fail -d file=1 $SCRATCH_DEV
>
> More image file tests, belong in the data section with the other
> image file tests.
> ​​
>

> > +# Specific flag combinations where some bug appeared during development,
> > +# to catch the same issue if it re-appears. If there are multiple
> similar
> > +# checks, move them to a standalone block.
> > +
> > +
> > +do_mkfs_pass -m crc=1,finobt=1 $SCRATCH_DEV
>
> What about all the other invalid cases?
>
> > +do_mkfs_pass -m crc=1 -n ftype=1 $SCRATCH_DEV
> > +do_mkfs_pass -m crc=0 -n ftype=1 $SCRATCH_DEV
> > +do_mkfs_fail -m crc=1 -n ftype=0 $SCRATCH_DEV
> > +do_mkfs_pass -m crc=0 -n ftype=0 $SCRATCH_DEV
> > +do_mkfs_pass -n ftype=1 -m crc=0 $SCRATCH_DEV
>
> One of the cleanup requirements was that option parsing would not
> be order sensitive, so I don't think you need to iterate parameters
> in different orders. That would just blow out the test matrix
> unnecessarily. Also, if you really need to repeat the same test but
> with different orders, please place those tests sequentially in the
> file so it's clear that they are duplicate/order swapped tests....
> ​
>

​I added it as a test that the order independency really works. This is one
case, where the original code was order-dependent... But maybe such test is
not necessary, as it should be so by design and there is no way to screw it
up for all options at once. ​

​So to sum all the email, I will make an updated version with more test
data. However, it will take me some time, because in few days, exams on my
university are starting, so I need to focus there and start learning.​ :-)

Thanks and cheers,
Jan

​
>
>
> > +# if user states crc=0,finobt=1, fail instead of warning
> > +do_mkfs_fail -m crc=0,finobt=1 $SCRATCH_DEV
>
> Why is this separate to the other crc,finobt test? Please try to
> keep the parameter checks in logical groupings....
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@fromorbit.com
>
​

--
Jan Tulak
jtulak@redhat.com / jan@tulak.me

[-- Attachment #1.2: Type: text/html, Size: 9483 bytes --]

[-- Attachment #2: Type: text/plain, Size: 121 bytes --]