| Message ID | 1440580477.29614.3.camel@ari-macbook (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Aug 26, 2015 at 12:14:37PM +0300, Ari Sundholm wrote: > From: Ari Sundholm <ari@tuxera.com> > > Signed-off-by: Ari Sundholm <ari@tuxera.com> Why? Cheers, Dave.
Hello Dave! On Thu, 2015-08-27 at 11:47 +1000, Dave Chinner wrote: > On Wed, Aug 26, 2015 at 12:14:37PM +0300, Ari Sundholm wrote: > > From: Ari Sundholm <ari@tuxera.com> > > > > Signed-off-by: Ari Sundholm <ari@tuxera.com> > > Why? > Do you mean why this patch is needed? Because there are filesystems that do not support hard links (at least not yet, but may support symlinks) we would like to run xfstests on. I'd be glad to add this explanation and reroll the patches, but I thought this would be obvious from what the patch does. OTOH, if you meant to ask why I have both a From: line and a Signed-off-by: line, it's just a habit due to a pedantic reading of the kernel patch submission instructions, which I've since applied to other contexts as well. Best regards, Ari Sundholm ari@tuxera.com -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Aug 27, 2015 at 01:58:23PM +0300, Ari Sundholm wrote: > Hello Dave! > > On Thu, 2015-08-27 at 11:47 +1000, Dave Chinner wrote: > > On Wed, Aug 26, 2015 at 12:14:37PM +0300, Ari Sundholm wrote: > > > From: Ari Sundholm <ari@tuxera.com> > > > > > > Signed-off-by: Ari Sundholm <ari@tuxera.com> > > > > Why? > > > > Do you mean why this patch is needed? Of course. Just because the answer migh be obvious to you, it doesn't mean it is obvious to anyone else, nor is an empty commit message useful in 2-3 years time when someone reads the commit and wonders "why did they make that change?" > Because there are filesystems that > do not support hard links (at least not yet, but may support symlinks) > we would like to run xfstests on. > > I'd be glad to add this explanation and reroll the patches, but I > thought this would be obvious from what the patch does. xfstests doesn't support any filesystems that don't have hardlinks. Why we should carry dead code, at minimum, needs explaining and discussing. > OTOH, if you meant to ask why I have both a From: line and a > Signed-off-by: line, it's just a habit due to a pedantic reading of the > kernel patch submission instructions, which I've since applied to other > contexts as well. Then I'm sure that you would have also read the entire section on writing a decent commit message: " 2) Describe your changes. ------------------------- Describe your problem. .... Describe user-visible impact. .... Quantify optimizations and trade-offs. .... Once the problem is established, describe what you are actually doing about it in technical detail. .... " Develop the habits that make you a better software engineer - using a "from:" line in patch submissions does not do that. -Dave.
On Fri, 2015-08-28 at 14:25 +1000, Dave Chinner wrote: > On Thu, Aug 27, 2015 at 01:58:23PM +0300, Ari Sundholm wrote: > > Do you mean why this patch is needed? > > Of course. > > Just because the answer migh be obvious to you, it doesn't mean it > is obvious to anyone else, nor is an empty commit message useful in > 2-3 years time when someone reads the commit and wonders "why did > they make that change?" > I understand. I had simply thought that a longer explanation was not necessary in this case. I was wrong. > > Because there are filesystems that > > do not support hard links (at least not yet, but may support symlinks) > > we would like to run xfstests on. > > > > I'd be glad to add this explanation and reroll the patches, but I > > thought this would be obvious from what the patch does. > > xfstests doesn't support any filesystems that don't have hardlinks. > Why we should carry dead code, at minimum, needs explaining and > discussing. > But it does support filesystems which don't support symlinks. What I did is just an analog of that for hardlinks. Our company develops filesystem implementations, some of which support symlinks or hardlinks, but not necessarily both - at least not yet. We have found xfstests immensely useful in testing both work-in-progress and established FS implementations and are integrating it as part of our regular testing framework. We try to contribute back as much as possible of those things that we find generally useful. I am strongly of the opinion that this patch *is* generally useful, as we are far from the only people developing new filesystems. This is why I do not find this patch to be dead code. The types of filesystems we work on might not always completely overlap with the typical features of the filesystems xfstests is usually run on, such as xfs, ext4 and btrfs, but I think the effort required to support our filesystems remains small enough to be worth it. It will definitely not be the end of the world for us if you don't find this feature generally useful, but we try to upstream as much as possible of what we think might benefit not only us, but others as well. > Develop the habits that make you a better software engineer - using > a "from:" line in patch submissions does not do that. Will do. I try to learn and hope to keep learning. Best regards, Ari Sundholm ari@tuxera.com -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/common/rc b/common/rc index 780b7ed..d3aeddf 100644 --- a/common/rc +++ b/common/rc @@ -2714,6 +2714,18 @@ _require_test_symlinks() rm -f $target $link } +_require_test_hardlinks() +{ + target=`mktemp -p $TEST_DIR` + link=`mktemp -p $TEST_DIR -u` + ln `basename $target` $link + if [ "$?" -ne 0 ]; then + rm -f $target + _notrun "Require hardlinks support" + fi + rm -f $target $link +} + _require_test_fcntl_advisory_locks() { [ "$FSTYP" != "cifs" ] && return 0 diff --git a/tests/generic/002 b/tests/generic/002 index f63b208..0b96709 100755 --- a/tests/generic/002 +++ b/tests/generic/002 @@ -43,6 +43,7 @@ _cleanup() # real QA test starts here _supported_fs generic _supported_os IRIX Linux +_require_test_hardlinks _require_test echo "Silence is goodness ..." diff --git a/tests/generic/039 b/tests/generic/039 index 4bbfc50..367a6d0 100755 --- a/tests/generic/039 +++ b/tests/generic/039 @@ -55,6 +55,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15 _supported_fs generic _supported_os Linux _need_to_be_root +_require_test_hardlinks _require_scratch _require_dm_flakey _require_metadata_journaling $SCRATCH_DEV diff --git a/tests/generic/040 b/tests/generic/040 index c841fbc..1b8287f 100755 --- a/tests/generic/040 +++ b/tests/generic/040 @@ -60,6 +60,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15 _supported_fs generic _supported_os Linux _need_to_be_root +_require_test_hardlinks _require_scratch _require_dm_flakey _require_metadata_journaling $SCRATCH_DEV diff --git a/tests/generic/041 b/tests/generic/041 index f38b662..588d5ff 100755 --- a/tests/generic/041 +++ b/tests/generic/041 @@ -64,6 +64,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15 _supported_fs generic _supported_os Linux _need_to_be_root +_require_test_hardlinks _require_scratch _require_dm_flakey _require_metadata_journaling $SCRATCH_DEV diff --git a/tests/generic/056 b/tests/generic/056 index 8bb1522..e111bec 100755 --- a/tests/generic/056 +++ b/tests/generic/056 @@ -53,6 +53,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15 _supported_fs generic _supported_os Linux _need_to_be_root +_require_test_hardlinks _require_scratch _require_dm_flakey _require_metadata_journaling $SCRATCH_DEV diff --git a/tests/generic/057 b/tests/generic/057 index 3b9f89e..d3b1aa2 100755 --- a/tests/generic/057 +++ b/tests/generic/057 @@ -53,6 +53,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15 _supported_fs generic _supported_os Linux _need_to_be_root +_require_test_hardlinks _require_scratch _require_dm_flakey _require_metadata_journaling $SCRATCH_DEV diff --git a/tests/generic/065 b/tests/generic/065 index 739a4d5..5403c53 100755 --- a/tests/generic/065 +++ b/tests/generic/065 @@ -54,6 +54,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15 _supported_fs generic _supported_os Linux _need_to_be_root +_require_test_hardlinks _require_scratch _require_dm_flakey _require_metadata_journaling $SCRATCH_DEV diff --git a/tests/generic/066 b/tests/generic/066 index cb36506..51aa22a 100755 --- a/tests/generic/066 +++ b/tests/generic/066 @@ -58,6 +58,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15 _supported_fs generic _supported_os Linux _need_to_be_root +_require_test_hardlinks _require_scratch _require_dm_flakey _require_attrs diff --git a/tests/generic/084 b/tests/generic/084 index 3fec6c2..86c38db 100755 --- a/tests/generic/084 +++ b/tests/generic/084 @@ -46,6 +46,7 @@ _cleanup() # real QA test starts here _supported_fs generic _supported_os Linux +_require_test_hardlinks _require_scratch link_unlink_storm() diff --git a/tests/generic/090 b/tests/generic/090 index a1f2b89..c3eeb1a 100755 --- a/tests/generic/090 +++ b/tests/generic/090 @@ -52,6 +52,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15 _supported_fs generic _supported_os Linux _need_to_be_root +_require_test_hardlinks _require_scratch _require_dm_flakey _require_metadata_journaling $SCRATCH_DEV diff --git a/tests/generic/104 b/tests/generic/104 index eeb7363..9fed5f2 100755 --- a/tests/generic/104 +++ b/tests/generic/104 @@ -48,6 +48,7 @@ _cleanup() _need_to_be_root _supported_fs generic _supported_os Linux +_require_test_hardlinks _require_scratch _require_dm_flakey _require_metadata_journaling $SCRATCH_DEV diff --git a/tests/generic/106 b/tests/generic/106 index 0afee41..59efe7c 100755 --- a/tests/generic/106 +++ b/tests/generic/106 @@ -47,6 +47,7 @@ _cleanup() _need_to_be_root _supported_fs generic _supported_os Linux +_require_test_hardlinks _require_scratch _require_dm_flakey _require_metadata_journaling $SCRATCH_DEV