On Sun, 2013-10-13 at 22:41 -0400, Theodore Ts'o wrote:
> On Wed, Aug 28, 2013 at 01:25:51PM +0800, Robert Yang wrote:
> > @@ -2773,7 +2776,6 @@ no_journal:
> > "filesystem accounting information: "));
> > checkinterval = fs->super->s_checkinterval;
> > max_mnt_count = fs->super->s_max_mnt_count;
> > - retval = ext2fs_close(fs);
> > if (retval) {
> > fprintf(stderr,
> > _("\nWarning, had trouble writing out superblocks."));
>
> You can't just move the call to ext2fs_close(). You also need to move
>
> if (!quiet)
> printf(_("Writing superblocks and "
> "filesystem accounting information: "));
>
> before the call to ext2fs_close() since this is used to print the
> message for the progress information that will be emitted by
> ext2fs_close(), and you also have to move the error checking:
>
> if (retval) {
> fprintf(stderr,
> _("\nWarning, had trouble writing out superblocks."));
> ...
>
> after the call to ext2fs_close().
>
>
> This would have been ***painfully*** obvious if you had run the
> regression test suite. ("make -j8 ; make -j8 check"), since the
> inconsistent move of ext2fs_close() without the preceeding printf
> would cause all of the mke2fs tests (the m_* tests) to fail.
>
> This is why regression test suites are so important. :-)
>
> - Ted
Robert,
Can you please take Ted's feedback into account (this and his response
to patch 10/10) and prepare another version of the patches.
As Ted suggests here, please run the regression test suite prior to any
future patch submissions. Looks like we missed some critical bits by not
doing that.
Ted, thank you for the response and taking the time to point out the
test suite. Robert, could you check the README and see if anything needs
to get updated there to make sure other developers are aware of the
regressions suite and how to run it?
Thanks,
--
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
