| Message ID | 20170626134017.2741-2-ddiss@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jun 26, 2017 at 03:40:17PM +0200, David Disseldorp wrote: > Without any $tmp suffix, users silly enough to run xfstests without > mktemp present will unintentionally "rm -f *". > > Signed-off-by: David Disseldorp <ddiss@suse.de> I posted a patch back in Jan. to cleanup tmp files and other leftover files after test, I switched from mktemp to a normal "tmp=/tmp/$$._mkfs" assignment so it could avoid this problem you want to fix too. But my patch got no review. I'm going to send new version of my patch and will fix _scratch_do_mkfs too, I really appreciate if you could help review and provide Reviewed-by tag. Thanks, Eryu > --- > common/rc | 2 +- > common/xfs | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/common/rc b/common/rc > index ff1b75c9..a38f4e03 100644 > --- a/common/rc > +++ b/common/rc > @@ -595,7 +595,7 @@ _scratch_do_mkfs() > cat $tmp.mkfsstd > eval "cat $tmp.mkfserr | $mkfs_filter" >&2 > > - rm -f $tmp* > + rm -f $tmp.mkfsstd $tmp.mkfserr > return $mkfs_status > } > > diff --git a/common/xfs b/common/xfs > index 0f0825bc..59776a6f 100644 > --- a/common/xfs > +++ b/common/xfs > @@ -104,7 +104,7 @@ _scratch_mkfs_xfs() > # output mkfs stdout and stderr > cat $tmp.mkfsstd > cat $tmp.mkfserr >&2 > - rm -f $tmp* > + rm -f $tmp.mkfsstd $tmp.mkfserr > > return $mkfs_status > } > -- > 2.12.3 > > -- > 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 -- 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 Tue, 27 Jun 2017 12:33:04 +0800, Eryu Guan wrote: > I posted a patch back in Jan. to cleanup tmp files and other leftover > files after test, I switched from mktemp to a normal "tmp=/tmp/$$._mkfs" > assignment so it could avoid this problem you want to fix too. But my > patch got no review. > > I'm going to send new version of my patch and will fix _scratch_do_mkfs > too, I really appreciate if you could help review and provide > Reviewed-by tag. Thanks for the feedback, Eryu. My preference would be to continue to use mktemp. I know it's test code, but I'd still like to avoid any potential symlink races. Cheers, David -- 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 Tue, Jun 27, 2017 at 11:55:22AM +0200, David Disseldorp wrote: > On Tue, 27 Jun 2017 12:33:04 +0800, Eryu Guan wrote: > > > I posted a patch back in Jan. to cleanup tmp files and other leftover > > files after test, I switched from mktemp to a normal "tmp=/tmp/$$._mkfs" > > assignment so it could avoid this problem you want to fix too. But my > > patch got no review. > > > > I'm going to send new version of my patch and will fix _scratch_do_mkfs > > too, I really appreciate if you could help review and provide > > Reviewed-by tag. > > Thanks for the feedback, Eryu. My preference would be to continue to use > mktemp. I know it's test code, but I'd still like to avoid any potential > symlink races. > > Cheers, David mktemp doesn't really help in this case. What we do in _scratch_do_mkfs is like: local tmp=`mktemp` .... eval "$mkfs_cmd ..." > $tmp.mkfsstd $tmp is not used directly as a tmp file, it just serves as a common prefix, $tmp._any_suffix_ can be a symlink too. (BTW, remove only $tmp.mkfsstd and $tmp.mkfserr leaves $tmp itself in /tmp). And I don't think it's that necessary to avoid potential symlink races in fstests, the "tmp=/tmp/$$ then write to $tmp.file" usage is almost everywhere in fstests. I agree it'd be great to avoid symlink races, but IMHO it's not worth the effort to make whole fstests symlink-race-free. Thanks, Eryu -- 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 ff1b75c9..a38f4e03 100644 --- a/common/rc +++ b/common/rc @@ -595,7 +595,7 @@ _scratch_do_mkfs() cat $tmp.mkfsstd eval "cat $tmp.mkfserr | $mkfs_filter" >&2 - rm -f $tmp* + rm -f $tmp.mkfsstd $tmp.mkfserr return $mkfs_status } diff --git a/common/xfs b/common/xfs index 0f0825bc..59776a6f 100644 --- a/common/xfs +++ b/common/xfs @@ -104,7 +104,7 @@ _scratch_mkfs_xfs() # output mkfs stdout and stderr cat $tmp.mkfsstd cat $tmp.mkfserr >&2 - rm -f $tmp* + rm -f $tmp.mkfsstd $tmp.mkfserr return $mkfs_status }
Without any $tmp suffix, users silly enough to run xfstests without mktemp present will unintentionally "rm -f *". Signed-off-by: David Disseldorp <ddiss@suse.de> --- common/rc | 2 +- common/xfs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)