<div dir="ltr"><div class="gmail_default" style="font-family:arial,helvetica,sans-serif"><span style="font-family:arial,sans-serif">On Fri, Apr 29, 2016 at 3:59 AM, Dave Chinner </span><span dir="ltr" style="font-family:arial,sans-serif"><<a href="mailto:david@fromorbit.com" target="_blank">david@fromorbit.com</a>></span><span style="font-family:arial,sans-serif"> wrote:</span><br></div><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span>On Thu, Apr 28, 2016 at 10:29:09AM +0200, Jan Tulak wrote:<br>
> 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.<br>
><br>
><br>
><br>
> Signed-off-by: Dave Chinner <<a href="mailto:dchinner@redhat.com" target="_blank">dchinner@redhat.com</a>><br>
> Signed-off-by: Jan Tulak <<a href="mailto:jtulak@redhat.com" target="_blank">jtulak@redhat.com</a>><br>
<br>
</span>Please don't strip the commit messages from patches you've picked up<br>
from other people - it loses valuable information, as well as the<br>
original author of the code. i.e. The original commit message was:<br>
<br></blockquote><div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">​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.</div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
From: Dave Chinner <<a href="mailto:dchinner@redhat.com" target="_blank">dchinner@redhat.com</a>><br>
<br>
mkfs.xfs does not do a very good job of input validation. This test<br>
is designed to exercise the input validation and test good/bad<br>
combinations of options being set. It will not pass on a current<br>
mkfs.xfs binary - it is designed to be the test case for a input<br>
validation cleanup.<br>
<br>
Signed-off-by: Dave Chinner <<a href="mailto:dchinner@redhat.com" target="_blank">dchinner@redhat.com</a>><br>
<span><br>
> ---<br>
><br>
> Hi guys,<br>
><br>
> I'm sending this patch although the mentioned patchset is not yet merged.<br>
> It might help you a bit with checking if there are any issues with<br>
> the patchset, as here it is clear, what options works and what not.<br>
<br>
</span>in which case, a "_require_xfs_mkfs_validation" rule should be<br>
written to determine the version of mkfs being. e.g. by testing one<br>
of the failure cases that the unfixed binary says is ok.<br>
....<br>
<span>> +# basic "should fail" options<br>
> +# logarithm based options are no longer valid<br>
> +# NOTE: umm, when it got invalid? It seems to be still supported...<br>
> +#do_mkfs_fail -s log=10 $SCRATCH_DEV<br>
> +#do_mkfs_fail -b log=10 $SCRATCH_DEV<br>
> +#do_mkfs_fail -n log=10 $SCRATCH_DEV<br>
> +#do_mkfs_fail -i log=10 $SCRATCH_DEV<br>
> +#do_mkfs_fail -d sectlog=10 $SCRATCH_DEV<br>
> +#do_mkfs_fail -l sectlog=10 $SCRATCH_DEV<br>
<br>
</span>They were expected to fail because I was going to remove the log<br>
options from mkfs as part of the cleanup series because they are<br>
redundant and nobody uses them. i.e this test was written with what<br>
I wanted as the end result of the mkfs input validation cleanup, not<br>
an iteration of the current behaviour.<br>
<br>
After all the data section tests, the new tests you've added all<br>
seem to be pretty ad-hoc.  What I was fleshing out in this test was<br>
a relatively complete set exercising each the different options mkfs<br>
supports.<br>
<br>
I'd only iterated data section options so far in this test. I'd just<br>
started on the naming section tests, and had not added any but a<br>
basic test. That needs to be iterated, as do the inode, log (both<br>
internal and external), metadata and realtime options....<br>
<span><br></span></blockquote><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">​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</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span>
<br>
> +# invalid file section tests<br>
> +rm -f $fsimg<br>
> +$XFS_IO_PROG -f -c "truncate $fssize" $fsimg<br>
> +do_mkfs_fail -d file $fsimg<br>
> +do_mkfs_fail -d file,name=$fsimg<br>
<br>
</span>Why should these fail - size should not be required if the image<br>
file already exists and is of sufficient size....<br></blockquote><div><br></div><div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">​Well, they should pass. I'm sending an updated patch to the set as well.</div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span>
> +<br>
> +# naming section tests<br>
> +do_mkfs_pass -n size=65536 $SCRATCH_DEV<br>
> +<br>
> +# boolean options<br>
> +$XFS_IO_PROG -f -c "truncate $fssize" $fsimg<br>
> +do_mkfs_pass -d file=1,size=$fssize $fsimg<br>
> +do_mkfs_pass -d file=0 $SCRATCH_DEV<br>
> +do_mkfs_fail -d file=1 $SCRATCH_DEV<br>
<br>
</span>More image file tests, belong in the data section with the other<br>
image file tests.<div class="gmail_default" style="font-family:arial,helvetica,sans-serif;display:inline">​​</div></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<span><br>
> +# Specific flag combinations where some bug appeared during development,<br>
> +# to catch the same issue if it re-appears. If there are multiple similar<br>
> +# checks, move them to a standalone block.<br>
> +<br>
> +<br>
> +do_mkfs_pass -m crc=1,finobt=1 $SCRATCH_DEV<br>
<br>
</span>What about all the other invalid cases?<br>
<span><br>
> +do_mkfs_pass -m crc=1 -n ftype=1 $SCRATCH_DEV<br>
> +do_mkfs_pass -m crc=0 -n ftype=1 $SCRATCH_DEV<br>
> +do_mkfs_fail -m crc=1 -n ftype=0 $SCRATCH_DEV<br>
> +do_mkfs_pass -m crc=0 -n ftype=0 $SCRATCH_DEV<br>
> +do_mkfs_pass -n ftype=1 -m crc=0 $SCRATCH_DEV<br>
<br>
</span>One of the cleanup requirements was that option parsing would not<br>
be order sensitive, so I don't think you need to iterate parameters<br>
in different orders. That would just blow out the test matrix<br>
unnecessarily. Also, if you really need to repeat the same test but<br>
with different orders, please place those tests sequentially in the<br>
file so it's clear that they are duplicate/order swapped tests....<div class="gmail_default" style="font-family:arial,helvetica,sans-serif;display:inline">​</div></blockquote><div><br></div><div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">​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. ​</div><br></div><div><br></div><div><br></div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">​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.​ :-)</div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif"><br></div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">Thanks and cheers,</div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">Jan</div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="gmail_default" style="font-family:arial,helvetica,sans-serif;display:inline">​</div><br>
<span><br>
> +# if user states crc=0,finobt=1, fail instead of warning<br>
> +do_mkfs_fail -m crc=0,finobt=1 $SCRATCH_DEV<br>
<br>
</span>Why is this separate to the other crc,finobt test? Please try to<br>
keep the parameter checks in logical groupings....<br>
<br>
Cheers,<br>
<br>
Dave.<br>
<span><font color="#888888">--<br>
Dave Chinner<br>
<a href="mailto:david@fromorbit.com" target="_blank">david@fromorbit.com</a><br>
</font></span></blockquote></div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">​</div><div><br></div>-- <br><div><div dir="ltr"><div><div dir="ltr"><div>Jan Tulak<br></div><a href="mailto:jtulak@redhat.com" target="_blank">jtulak@redhat.com</a> / <a href="mailto:jan@tulak.me" target="_blank">jan@tulak.me</a></div></div></div></div>
</div></div>