[PATCH] xfstests: test EROFS vs. EEXIST when creating on an RO filesystem

[PATCH] xfstests: test EROFS vs. EEXIST when creating on an RO filesystem

[Eric Sandeen]

TBH, I don't know if this is posix-specified, but I found out the
hard way that when trying to re-create existing files on a readonly 
filesystem, some apps expect/handle EEXIST, but fail on EROFS.

This will test mkdir, mknod, and symlinks for that behavior.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

Note:  Think the command output is fixed enough, or should I
be grepping for keywords?

diff --git a/292 b/292
new file mode 100755
index 0000000..a14bb7f
--- /dev/null
+++ b/292
@@ -0,0 +1,73 @@
+#! /bin/bash
+# FS QA Test No. 292
+#
+# Tests for EEXIST (not EROFS) for inode creations, if
+# we ask to create an already-existing entity on an RO filesystem
+#
+#-----------------------------------------------------------------------
+# Copyright (c) 2012 Red Hat, Inc.  All Rights Reserved.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it would be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write the Free Software Foundation,
+# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+#-----------------------------------------------------------------------
+#
+# creator
+owner=sandeen@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup()
+{
+    cd /
+    rm -f $tmp.*
+}
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+# real QA test starts here
+
+# Modify as appropriate.
+_supported_fs generic
+_supported_os Linux
+_require_scratch
+
+THIS_TEST_DIR=$SCRATCH_MNT/$seq.test
+
+_create_files()
+{
+	mknod $THIS_TEST_DIR/testnode c 1 3
+	mkdir $THIS_TEST_DIR/testdir
+	touch $THIS_TEST_DIR/testtarget
+	ln -s $THIS_TEST_DIR/testtarget $THIS_TEST_DIR/testlink
+}
+
+_scratch_mount
+
+rm -rf $THIS_TEST_DIR
+mkdir $THIS_TEST_DIR || _fail "Could not create dir for test"
+
+_create_files 2>&1 | _filter_scratch
+_scratch_mount -o remount,ro || _fail "Could not remount scratch readonly"
+_create_files 2>&1 | _filter_scratch
+
+# success, all done
+status=0
+exit
diff --git a/292.out b/292.out
new file mode 100644
index 0000000..485c567
--- /dev/null
+++ b/292.out
@@ -0,0 +1,5 @@
+QA output created by 292
+mknod: `SCRATCH_MNT/292.test/testnode': File exists
+mkdir: cannot create directory `SCRATCH_MNT/292.test/testdir': File exists
+touch: cannot touch `SCRATCH_MNT/292.test/testtarget': Read-only file system
+ln: creating symbolic link `SCRATCH_MNT/292.test/testlink': File exists
diff --git a/group b/group
index dc8db65..030082b 100644
--- a/group
+++ b/group
@@ -410,3 +410,4 @@ deprecated
 289 auto quick
 290 auto rw prealloc quick ioctl
 291 repair
+292 auto quick

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfstests: test EROFS vs. EEXIST when creating on an RO filesystem

[Dave Chinner]

On Tue, Nov 27, 2012 at 03:16:39PM -0600, Eric Sandeen wrote:
> TBH, I don't know if this is posix-specified, but I found out the
> hard way that when trying to re-create existing files on a readonly 
> filesystem, some apps expect/handle EEXIST, but fail on EROFS.
> 
> This will test mkdir, mknod, and symlinks for that behavior.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> Note:  Think the command output is fixed enough, or should I
> be grepping for keywords?

Command output is good enough at this point.

> 
> diff --git a/292 b/292
> new file mode 100755
> index 0000000..a14bb7f
> --- /dev/null
> +++ b/292
> @@ -0,0 +1,73 @@
> +#! /bin/bash
> +# FS QA Test No. 292
> +#
> +# Tests for EEXIST (not EROFS) for inode creations, if
> +# we ask to create an already-existing entity on an RO filesystem
> +#
> +#-----------------------------------------------------------------------
> +# Copyright (c) 2012 Red Hat, Inc.  All Rights Reserved.
> +#
> +# This program is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU General Public License as
> +# published by the Free Software Foundation.
> +#
> +# This program is distributed in the hope that it would be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write the Free Software Foundation,
> +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> +#-----------------------------------------------------------------------
> +#
> +# creator
> +owner=sandeen@redhat.com
> +
> +seq=`basename $0`
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +status=1	# failure is the default!
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +_cleanup()
> +{
> +    cd /
> +    rm -f $tmp.*
> +}
> +
> +# get standard environment, filters and checks
> +. ./common.rc
> +. ./common.filter
> +
> +# real QA test starts here
> +
> +# Modify as appropriate.
> +_supported_fs generic
> +_supported_os Linux
> +_require_scratch
> +
> +THIS_TEST_DIR=$SCRATCH_MNT/$seq.test

Confusing. $TEST_DIR is test filesystem mount point. This points
something very similarly names at the scratch filesystem....

> +_create_files()
> +{
> +	mknod $THIS_TEST_DIR/testnode c 1 3
> +	mkdir $THIS_TEST_DIR/testdir
> +	touch $THIS_TEST_DIR/testtarget
> +	ln -s $THIS_TEST_DIR/testtarget $THIS_TEST_DIR/testlink
> +}
> +
> +_scratch_mount

You need to mkfs the scratch device before using it. There is no
guarantee that it is inmountable shape, or even contains the right
filesytsem type when the test starts.

> +
> +rm -rf $THIS_TEST_DIR
> +mkdir $THIS_TEST_DIR || _fail "Could not create dir for test"

The error will dump into the output file and fail the golden output
match. Using _fail means there's no .out.bad file for analysis of
the failure.

> +
> +_create_files 2>&1 | _filter_scratch
> +_scratch_mount -o remount,ro || _fail "Could not remount scratch readonly"

Ditto.

Otherwise looks OK.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfstests: test EROFS vs. EEXIST when creating on an RO filesystem

[Eric Sandeen]

On 11/27/12 4:08 PM, Dave Chinner wrote:
> On Tue, Nov 27, 2012 at 03:16:39PM -0600, Eric Sandeen wrote:
>> TBH, I don't know if this is posix-specified, but I found out the
>> hard way that when trying to re-create existing files on a readonly 
>> filesystem, some apps expect/handle EEXIST, but fail on EROFS.
>>
>> This will test mkdir, mknod, and symlinks for that behavior.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>
>> Note:  Think the command output is fixed enough, or should I
>> be grepping for keywords?
> 
> Command output is good enough at this point.
> 
>>
>> diff --git a/292 b/292
>> new file mode 100755
>> index 0000000..a14bb7f
>> --- /dev/null
>> +++ b/292
>> @@ -0,0 +1,73 @@
>> +#! /bin/bash
>> +# FS QA Test No. 292
>> +#
>> +# Tests for EEXIST (not EROFS) for inode creations, if
>> +# we ask to create an already-existing entity on an RO filesystem
>> +#
>> +#-----------------------------------------------------------------------
>> +# Copyright (c) 2012 Red Hat, Inc.  All Rights Reserved.
>> +#
>> +# This program is free software; you can redistribute it and/or
>> +# modify it under the terms of the GNU General Public License as
>> +# published by the Free Software Foundation.
>> +#
>> +# This program is distributed in the hope that it would be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program; if not, write the Free Software Foundation,
>> +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
>> +#-----------------------------------------------------------------------
>> +#
>> +# creator
>> +owner=sandeen@redhat.com
>> +
>> +seq=`basename $0`
>> +echo "QA output created by $seq"
>> +
>> +here=`pwd`
>> +tmp=/tmp/$$
>> +status=1	# failure is the default!
>> +trap "_cleanup; exit \$status" 0 1 2 3 15
>> +
>> +_cleanup()
>> +{
>> +    cd /
>> +    rm -f $tmp.*
>> +}
>> +
>> +# get standard environment, filters and checks
>> +. ./common.rc
>> +. ./common.filter
>> +
>> +# real QA test starts here
>> +
>> +# Modify as appropriate.
>> +_supported_fs generic
>> +_supported_os Linux
>> +_require_scratch
>> +
>> +THIS_TEST_DIR=$SCRATCH_MNT/$seq.test
> 
> Confusing. $TEST_DIR is test filesystem mount point. This points
> something very similarly names at the scratch filesystem....

Meh, ok.  $FOZZIE_BEAR_DIR or something; I'll change it.

>> +_create_files()
>> +{
>> +	mknod $THIS_TEST_DIR/testnode c 1 3
>> +	mkdir $THIS_TEST_DIR/testdir
>> +	touch $THIS_TEST_DIR/testtarget
>> +	ln -s $THIS_TEST_DIR/testtarget $THIS_TEST_DIR/testlink
>> +}
>> +
>> +_scratch_mount

Sigh, right.  I started w/ the test dev but I'm not sure there's
a simple helper to remount it ro, so I was lazy.  Then forgot to
add this.

> You need to mkfs the scratch device before using it. There is no
> guarantee that it is inmountable shape, or even contains the right
> filesytsem type when the test starts.
> 
>> +
>> +rm -rf $THIS_TEST_DIR
>> +mkdir $THIS_TEST_DIR || _fail "Could not create dir for test"
> 
> The error will dump into the output file and fail the golden output
> match. Using _fail means there's no .out.bad file for analysis of
> the failure.

>> +
>> +_create_files 2>&1 | _filter_scratch
>> +_scratch_mount -o remount,ro || _fail "Could not remount scratch readonly"
> 
> Ditto.

OK.

Thanks,
-Eric
 
> Otherwise looks OK.
> 
> Cheers,
> 
> Dave.
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfstests: test EROFS vs. EEXIST when creating on an RO filesystem

[Christoph Hellwig]

On Tue, Nov 27, 2012 at 03:16:39PM -0600, Eric Sandeen wrote:
> TBH, I don't know if this is posix-specified, but I found out the
> hard way that when trying to re-create existing files on a readonly 
> filesystem, some apps expect/handle EEXIST, but fail on EROFS.
> 
> This will test mkdir, mknod, and symlinks for that behavior.

Just curious, which filesystem would fail this currently or did in the
past?

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfstests: test EROFS vs. EEXIST when creating on an RO filesystem

[Eric Sandeen]

On 11/27/12 4:11 PM, Christoph Hellwig wrote:
> On Tue, Nov 27, 2012 at 03:16:39PM -0600, Eric Sandeen wrote:
>> TBH, I don't know if this is posix-specified, but I found out the
>> hard way that when trying to re-create existing files on a readonly 
>> filesystem, some apps expect/handle EEXIST, but fail on EROFS.
>>
>> This will test mkdir, mknod, and symlinks for that behavior.
> 
> Just curious, which filesystem would fail this currently or did in the
> past?

No single filesystem, really - 

I temporarily broke the VFS in a rhel backport.  ;)  But it seems like
the kind of thing that could be missed in the future, so figured it was
worth a quick test.

(basically this was from moving mnt_want_write outside i_mutex for freeze
work, and returning an error directly from mnt_want_write() would
give us EROFS instead of maybe EEXIST)

-Eric

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfstests: test EROFS vs. EEXIST when creating on an RO filesystem

[Christoph Hellwig]

On Tue, Nov 27, 2012 at 04:12:58PM -0600, Eric Sandeen wrote:
> No single filesystem, really - 
> 
> I temporarily broke the VFS in a rhel backport.  ;)  But it seems like
> the kind of thing that could be missed in the future, so figured it was
> worth a quick test.

That's what I wonder about, I was pretty sure we'd normmally do these
checks in the VFS, and I haven't seen any reason changes from you in
the area.

I'm fine with adding this test, btw.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH V2] xfstests: test EROFS vs. EEXIST when creating on an RO filesystem

[Eric Sandeen]

TBH, I don't know if this is posix-specified, but I found out the
hard way that when trying to re-create existing files on a readonly 
filesystem, some apps expect/handle EEXIST, but fail on EROFS.

This will test mkdir, mknod, and symlinks for that behavior.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

V2:  Change test dir var name, do scratch mkfs, drop _fail

diff --git a/292 b/292
new file mode 100755
index 0000000..4f0cc87
--- /dev/null
+++ b/292
@@ -0,0 +1,74 @@
+#! /bin/bash
+# FS QA Test No. 292
+#
+# Tests for EEXIST (not EROFS) for inode creations, if
+# we ask to create an already-existing entity on an RO filesystem
+#
+#-----------------------------------------------------------------------
+# Copyright (c) 2012 Red Hat, Inc.  All Rights Reserved.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it would be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write the Free Software Foundation,
+# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+#-----------------------------------------------------------------------
+#
+# creator
+owner=sandeen@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup()
+{
+    cd /
+    rm -f $tmp.*
+}
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+# real QA test starts here
+
+# Modify as appropriate.
+_supported_fs generic
+_supported_os Linux
+_require_scratch
+
+MYDIR=$SCRATCH_MNT/$seq.test
+
+_create_files()
+{
+	mknod $MYDIR/testnode c 1 3
+	mkdir $MYDIR/testdir
+	touch $MYDIR/testtarget
+	ln -s $MYDIR/testtarget $MYDIR/testlink
+}
+
+_scratch_mkfs >/dev/null 2>&1
+_scratch_mount
+
+rm -rf $MYDIR
+mkdir $MYDIR
+
+_create_files 2>&1 | _filter_scratch
+_scratch_mount -o remount,ro
+_create_files 2>&1 | _filter_scratch
+
+# success, all done
+status=0
+exit
diff --git a/292.out b/292.out
new file mode 100644
index 0000000..485c567
--- /dev/null
+++ b/292.out
@@ -0,0 +1,5 @@
+QA output created by 292
+mknod: `SCRATCH_MNT/292.test/testnode': File exists
+mkdir: cannot create directory `SCRATCH_MNT/292.test/testdir': File exists
+touch: cannot touch `SCRATCH_MNT/292.test/testtarget': Read-only file system
+ln: creating symbolic link `SCRATCH_MNT/292.test/testlink': File exists
diff --git a/group b/group
index dc8db65..030082b 100644
--- a/group
+++ b/group
@@ -410,3 +410,4 @@ deprecated
 289 auto quick
 290 auto rw prealloc quick ioctl
 291 repair
+292 auto quick


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs