[U-Boot] [PATCH 1/3] test/fs: Restructure file path specification to allow some flexibility

[U-Boot] [PATCH 1/3] test/fs: Restructure file path specification to allow some flexibility

[Stefan Brüns]

Instead of providing the full path, specify directory and filename
separately. This allows to specify intermediate directories, required
for some additional tests.

Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
---
 test/fs/fs-test.sh | 56 +++++++++++++++++++-----------------------------------
 1 file changed, 20 insertions(+), 36 deletions(-)

diff --git a/test/fs/fs-test.sh b/test/fs/fs-test.sh
index 93679cb..12450ed 100755
--- a/test/fs/fs-test.sh
+++ b/test/fs/fs-test.sh
@@ -135,22 +135,6 @@ function create_image() {
 	fi
 }
 
-# 1st parameter is the FS type: fat/ext4
-# 2nd parameter is the name of small file
-# Returns filename which can be used for fat or ext4 for writing
-function fname_for_write() {
-	case $1 in
-		ext4)
-			# ext4 needs absolute path name of file
-			echo /${2}.w
-			;;
-
-		*)
-			echo ${2}.w
-			;;
-	esac
-}
-
 # 1st parameter is image file
 # 2nd parameter is file system type - fat/ext4
 # 3rd parameter is name of small file
@@ -166,11 +150,14 @@ function test_image() {
 
 	case "$2" in
 		fat)
+		FPATH=""
 		PREFIX="fat"
 		WRITE="write"
 		;;
 
 		ext4)
+		# ext4 needs absolute path
+		FPATH="/"
 		PREFIX="ext4"
 		WRITE="write"
 		;;
@@ -205,15 +192,12 @@ function test_image() {
 
 	esac
 
-	if [ -z "$6" ]; then
-		FILE_WRITE=`fname_for_write $2 $3`
-		FILE_SMALL=$3
-		FILE_BIG=$4
-	else
-		FILE_WRITE=$6/`fname_for_write $2 $3`
-		FILE_SMALL=$6/$3
-		FILE_BIG=$6/$4
+	if [ ! -z "$6" ]; then
+		FPATH=${6}/${FPATH}
 	fi
+	FILE_WRITE=${3}.w
+	FILE_SMALL=$3
+	FILE_BIG=$4
 
 	# In u-boot commands, <interface> stands for host or hostfs
 	# hostfs maps to the host fs.
@@ -230,13 +214,13 @@ ${PREFIX}ls host${SUFFIX} $6
 # sb size hostfs - $3 for hostfs commands.
 # 1MB is 0x0010 0000
 # Test Case 2 - size of small file
-${PREFIX}size host${SUFFIX} $FILE_SMALL
+${PREFIX}size host${SUFFIX} ${FPATH}$FILE_SMALL
 printenv filesize
 setenv filesize
 
 # 2.5GB (1024*1024*2500) is 0x9C40 0000
 # Test Case 3 - size of big file
-${PREFIX}size host${SUFFIX} $FILE_BIG
+${PREFIX}size host${SUFFIX} ${FPATH}$FILE_BIG
 printenv filesize
 setenv filesize
 
@@ -245,14 +229,14 @@ setenv filesize
 # Last two parameters are size and offset.
 
 # Test Case 4a - Read full 1MB of small file
-${PREFIX}load host${SUFFIX} $addr $FILE_SMALL
+${PREFIX}load host${SUFFIX} $addr ${FPATH}$FILE_SMALL
 printenv filesize
 # Test Case 4b - Read full 1MB of small file
 md5sum $addr \$filesize
 setenv filesize
 
 # Test Case 5a - First 1MB of big file
-${PREFIX}load host${SUFFIX} $addr $FILE_BIG $length 0x0
+${PREFIX}load host${SUFFIX} $addr ${FPATH}$FILE_BIG $length 0x0
 printenv filesize
 # Test Case 5b - First 1MB of big file
 md5sum $addr \$filesize
@@ -260,7 +244,7 @@ setenv filesize
 
 # fails for ext as no offset support
 # Test Case 6a - Last 1MB of big file
-${PREFIX}load host${SUFFIX} $addr $FILE_BIG $length 0x9C300000
+${PREFIX}load host${SUFFIX} $addr ${FPATH}$FILE_BIG $length 0x9C300000
 printenv filesize
 # Test Case 6b - Last 1MB of big file
 md5sum $addr \$filesize
@@ -268,7 +252,7 @@ setenv filesize
 
 # fails for ext as no offset support
 # Test Case 7a - One from the last 1MB chunk of 2GB
-${PREFIX}load host${SUFFIX} $addr $FILE_BIG $length 0x7FF00000
+${PREFIX}load host${SUFFIX} $addr ${FPATH}$FILE_BIG $length 0x7FF00000
 printenv filesize
 # Test Case 7b - One from the last 1MB chunk of 2GB
 md5sum $addr \$filesize
@@ -276,7 +260,7 @@ setenv filesize
 
 # fails for ext as no offset support
 # Test Case 8a - One from the start 1MB chunk from 2GB
-${PREFIX}load host${SUFFIX} $addr $FILE_BIG $length 0x80000000
+${PREFIX}load host${SUFFIX} $addr ${FPATH}$FILE_BIG $length 0x80000000
 printenv filesize
 # Test Case 8b - One from the start 1MB chunk from 2GB
 md5sum $addr \$filesize
@@ -284,7 +268,7 @@ setenv filesize
 
 # fails for ext as no offset support
 # Test Case 9a - One 1MB chunk crossing the 2GB boundary
-${PREFIX}load host${SUFFIX} $addr $FILE_BIG $length 0x7FF80000
+${PREFIX}load host${SUFFIX} $addr ${FPATH}$FILE_BIG $length 0x7FF80000
 printenv filesize
 # Test Case 9b - One 1MB chunk crossing the 2GB boundary
 md5sum $addr \$filesize
@@ -292,17 +276,17 @@ setenv filesize
 
 # Generic failure case
 # Test Case 10 - 2MB chunk from the last 1MB of big file
-${PREFIX}load host${SUFFIX} $addr $FILE_BIG 0x00200000 0x9C300000
+${PREFIX}load host${SUFFIX} $addr ${FPATH}$FILE_BIG 0x00200000 0x9C300000
 printenv filesize
 #
 
 # Read 1MB from small file
-${PREFIX}load host${SUFFIX} $addr $FILE_SMALL
+${PREFIX}load host${SUFFIX} $addr ${FPATH}$FILE_SMALL
 # Write it back to test the writes
 # Test Case 11a - Check that the write succeeded
-${PREFIX}${WRITE} host${SUFFIX} $addr $FILE_WRITE \$filesize
+${PREFIX}${WRITE} host${SUFFIX} $addr ${FPATH}$FILE_WRITE \$filesize
 mw.b $addr 00 100
-${PREFIX}load host${SUFFIX} $addr $FILE_WRITE
+${PREFIX}load host${SUFFIX} $addr ${FPATH}$FILE_WRITE
 # Test Case 11b - Check md5 of written to is same as the one read from
 md5sum $addr \$filesize
 setenv filesize
-- 
2.10.0

[U-Boot] [PATCH 2/3] test/fs: Check ext4 behaviour if dirent is first entry in directory block

[Stefan Brüns]

This is a regression test for a crash happening if the first dirent
in the block matches. Code tried to access a predecessor entry which
does not exist.
The crash happened for any block, but "." is always the first entry in
the first directory block and thus easy to check for.

Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
---
 test/fs/fs-test.sh | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/test/fs/fs-test.sh b/test/fs/fs-test.sh
index 12450ed..cb2a765 100755
--- a/test/fs/fs-test.sh
+++ b/test/fs/fs-test.sh
@@ -291,6 +291,12 @@ ${PREFIX}load host${SUFFIX} $addr ${FPATH}$FILE_WRITE
 md5sum $addr \$filesize
 setenv filesize
 #
+
+# Next test case checks writing a file whose dirent
+# is the first in the block, which is always true for "."
+# The write should fail, but the lookup should work
+# Test Case 12 - Check directory traversal
+${PREFIX}${WRITE} host${SUFFIX} $addr ${FPATH}. 0x10
 reset
 
 EOF
@@ -471,6 +477,11 @@ function check_results() {
 	pass_fail "TC11: 1MB write to $5 - write succeeded"
 	check_md5 "Test Case 11b " "$1" "$2" 1 \
 		"TC11: 1MB write to $5 - content verified"
+
+	# Check lookup of 'dot' directory
+	grep -A5 "Test Case 12 " "$1" | \
+		egrep -q 'Unable to write file'
+	pass_fail "TC12: 1MB write to . - write denied"
 	echo "** End $1"
 }
 
-- 
2.10.0

[U-Boot] [PATCH 3/3] test/fs: Check writes using "." (same dir) relative path

[Stefan Brüns]

<path>/<fname> and <path>/./<fname> should reference the same file.

Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
---
 test/fs/fs-test.sh | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/test/fs/fs-test.sh b/test/fs/fs-test.sh
index cb2a765..46975ec 100755
--- a/test/fs/fs-test.sh
+++ b/test/fs/fs-test.sh
@@ -297,6 +297,23 @@ setenv filesize
 # The write should fail, but the lookup should work
 # Test Case 12 - Check directory traversal
 ${PREFIX}${WRITE} host${SUFFIX} $addr ${FPATH}. 0x10
+
+# Read 1MB from small file
+${PREFIX}load host${SUFFIX} $addr ${FPATH}$FILE_SMALL
+# Write it via "same directory", i.e. "." dirent
+# Test Case 13a - Check directory traversal
+${PREFIX}${WRITE} host${SUFFIX} $addr ${FPATH}./${FILE_WRITE}2 \$filesize
+mw.b $addr 00 100
+${PREFIX}load host${SUFFIX} $addr ${FPATH}./${FILE_WRITE}2
+# Test Case 13b - Check md5 of written to is same as the one read from
+md5sum $addr \$filesize
+setenv filesize
+mw.b $addr 00 100
+${PREFIX}load host${SUFFIX} $addr ${FPATH}${FILE_WRITE}2
+# Test Case 13c - Check md5 of written to is same as the one read from
+md5sum $addr \$filesize
+setenv filesize
+#
 reset
 
 EOF
@@ -482,6 +499,16 @@ function check_results() {
 	grep -A5 "Test Case 12 " "$1" | \
 		egrep -q 'Unable to write file'
 	pass_fail "TC12: 1MB write to . - write denied"
+
+	# Check directory traversal
+	grep -A6 "Test Case 13a " "$1" | \
+		egrep -q '1048576 bytes written|update journal'
+	pass_fail "TC13: 1MB write to ./$5 - write succeeded"
+	check_md5 "Test Case 13b " "$1" "$2" 1 \
+		"TC13: 1MB read from ./$5 - content verified"
+	check_md5 "Test Case 13c " "$1" "$2" 1 \
+		"TC13: 1MB read from $5 - content verified"
+
 	echo "** End $1"
 }
 
-- 
2.10.0

[U-Boot] [PATCH 1/3] test/fs: Restructure file path specification to allow some flexibility

[Stephen Warren]

On 09/11/2016 02:46 PM, Stefan Br?ns wrote:
> Instead of providing the full path, specify directory and filename
> separately. This allows to specify intermediate directories, required
> for some additional tests.

> diff --git a/test/fs/fs-test.sh b/test/fs/fs-test.sh

> @@ -166,11 +150,14 @@ function test_image() {
>
>  	case "$2" in
>  		fat)
> +		FPATH=""
>  		PREFIX="fat"
>  		WRITE="write"
>  		;;
>
>  		ext4)
> +		# ext4 needs absolute path
> +		FPATH="/"
>  		PREFIX="ext4"
>  		WRITE="write"
...
> -		FILE_BIG=$6/$4
> +	if [ ! -z "$6" ]; then
> +		FPATH=${6}/${FPATH}
>  	fi

That adds a double slash in the case of ext4. That probably gets parsed 
OK, but it'd be nicer not to do the join with only a single /.

[U-Boot] [PATCH 2/3] test/fs: Check ext4 behaviour if dirent is first entry in directory block

[Stephen Warren]

On 09/11/2016 02:46 PM, Stefan Br?ns wrote:
> This is a regression test for a crash happening if the first dirent
> in the block matches. Code tried to access a predecessor entry which
> does not exist.
> The crash happened for any block, but "." is always the first entry in
> the first directory block and thus easy to check for.

> diff --git a/test/fs/fs-test.sh b/test/fs/fs-test.sh

> +# Next test case checks writing a file whose dirent
> +# is the first in the block, which is always true for "."
> +# The write should fail, but the lookup should work
> +# Test Case 12 - Check directory traversal
> +${PREFIX}${WRITE} host${SUFFIX} $addr ${FPATH}. 0x10

Doesn't that attempt to write a file named ".", which would end up 
over-writing the directory content? Unless I'm misunderstanding, that 
doesn't seem like a good idea.

Also, ${FPATH} might be "", "/", "something", or "something/". Appending 
"." to some of those won't work the same way as some others.

> @@ -471,6 +477,11 @@ function check_results() {

> +	# Check lookup of 'dot' directory
> +	grep -A5 "Test Case 12 " "$1" | \
> +		egrep -q 'Unable to write file'
> +	pass_fail "TC12: 1MB write to . - write denied"

Oh, I see; this expects the write to be denied since the destination is 
a directory. Perhaps that's OK. I'd rather see a read attempt though, to 
guarantee that even if the access does end up being allowed, the FS 
isn't trashed for any future tests that may be added afterwards.

[U-Boot] [PATCH 3/3] test/fs: Check writes using "." (same dir) relative path

[Stephen Warren]

On 09/11/2016 02:46 PM, Stefan Br?ns wrote:
> <path>/<fname> and <path>/./<fname> should reference the same file.

> diff --git a/test/fs/fs-test.sh b/test/fs/fs-test.sh

> +# Read 1MB from small file
> +${PREFIX}load host${SUFFIX} $addr ${FPATH}$FILE_SMALL

I think the same issue with $FPATH ending/not-ending in / applies here 
too, and for all commands in this patch.

> @@ -482,6 +499,16 @@ function check_results() {

> +	# Check directory traversal
> +	grep -A6 "Test Case 13a " "$1" | \
> +		egrep -q '1048576 bytes written|update journal'

Why is "update journal" considered successful? Surely the "n bytes 
written" message is always printed irrespective of whether anything 
journal-related happened?

[U-Boot] [PATCH 2/3] test/fs: Check ext4 behaviour if dirent is first entry in directory block

[Stefan Bruens]

On Montag, 12. September 2016 12:39:35 CEST you wrote:
> On 09/11/2016 02:46 PM, Stefan Br?ns wrote:
> > This is a regression test for a crash happening if the first dirent
> > in the block matches. Code tried to access a predecessor entry which
> > does not exist.
> > The crash happened for any block, but "." is always the first entry in
> > the first directory block and thus easy to check for.
> > 
> > diff --git a/test/fs/fs-test.sh b/test/fs/fs-test.sh
> > 
> > +# Next test case checks writing a file whose dirent
> > +# is the first in the block, which is always true for "."
> > +# The write should fail, but the lookup should work
> > +# Test Case 12 - Check directory traversal
> > +${PREFIX}${WRITE} host${SUFFIX} $addr ${FPATH}. 0x10
> 
> Doesn't that attempt to write a file named ".", which would end up
> over-writing the directory content? Unless I'm misunderstanding, that
> doesn't seem like a good idea.
> 
> Also, ${FPATH} might be "", "/", "something", or "something/". Appending
> "." to some of those won't work the same way as some others.

"something" can not happen. The other three cases are exactly what is wanted 
here, as fat currently needs a relative path, with CWD being "/", and ext 
explicitly wants an absolute path.


> > @@ -471,6 +477,11 @@ function check_results() {
> > 
> > +	# Check lookup of 'dot' directory
> > +	grep -A5 "Test Case 12 " "$1" | \
> > +		egrep -q 'Unable to write file'
> > +	pass_fail "TC12: 1MB write to . - write denied"
> 
> Oh, I see; this expects the write to be denied since the destination is
> a directory. Perhaps that's OK. I'd rather see a read attempt though, to
> guarantee that even if the access does end up being allowed, the FS
> isn't trashed for any future tests that may be added afterwards.

U-Boot master/2016-09 crashes here without the pending ext4 fixes. IMHO after 
a failed test case, everything afterwards is invalid anyway, if the same fs 
image is used. 

As soon as the image is modified, tests are no longer idempotent. Block 
allocation order may change, anything that allocates any resource changes the 
image.

Atempting a write here is done as it actually exercises a different code path. 
"ext4load host 0:0 0 /." reports "File not found", "ext4write host 0:0 0 /." 
crashes.

Kind regards,

Stefan

-- 
Stefan Br?ns  /  Bergstra?e 21  /  52062 Aachen
home: +49 241 53809034     mobile: +49 151 50412019
work: +49 2405 49936-424

[U-Boot] [PATCH 3/3] test/fs: Check writes using "." (same dir) relative path

[Stefan Bruens]

On Montag, 12. September 2016 12:44:08 CEST you wrote:
> On 09/11/2016 02:46 PM, Stefan Br?ns wrote:
> > <path>/<fname> and <path>/./<fname> should reference the same file.
> > 
> > diff --git a/test/fs/fs-test.sh b/test/fs/fs-test.sh
> > 
> > +# Read 1MB from small file
> > +${PREFIX}load host${SUFFIX} $addr ${FPATH}$FILE_SMALL
> 
> I think the same issue with $FPATH ending/not-ending in / applies here
> too, and for all commands in this patch.

FPATH is either "" for native fat, "/" for native ext4, or <somepath>/ for 
hostfs, so this is correct. Specifically, for fat, we dont want any "/" in 
front of $FILE_foo.
 
> > @@ -482,6 +499,16 @@ function check_results() {
> > 
> > +	# Check directory traversal
> > +	grep -A6 "Test Case 13a " "$1" | \
> > +		egrep -q '1048576 bytes written|update journal'
> 
> Why is "update journal" considered successful? Surely the "n bytes
> written" message is always printed irrespective of whether anything
> journal-related happened?

Thats a question left to the author of Test Case 11, where the fragment was 
copied from.

Ext4 unfortunately is quite verbose, it inserts "File system is consistent" 
and "update journal finished" lines in the output. I think these lines where 
better stripped from the log prior to any further parsing.

Kind regards,

Stefan

-- 
Stefan Br?ns  /  Bergstra?e 21  /  52062 Aachen
home: +49 241 53809034     mobile: +49 151 50412019
work: +49 2405 49936-424

[U-Boot] [PATCH 2/3] test/fs: Check ext4 behaviour if dirent is first entry in directory block

[Stephen Warren]

On 09/12/2016 03:48 PM, Stefan Bruens wrote:
> On Montag, 12. September 2016 12:39:35 CEST you wrote:
>> On 09/11/2016 02:46 PM, Stefan Br?ns wrote:
>>> This is a regression test for a crash happening if the first dirent
>>> in the block matches. Code tried to access a predecessor entry which
>>> does not exist.
>>> The crash happened for any block, but "." is always the first entry in
>>> the first directory block and thus easy to check for.
>>>
>>> diff --git a/test/fs/fs-test.sh b/test/fs/fs-test.sh
>>>
>>> +# Next test case checks writing a file whose dirent
>>> +# is the first in the block, which is always true for "."
>>> +# The write should fail, but the lookup should work
>>> +# Test Case 12 - Check directory traversal
>>> +${PREFIX}${WRITE} host${SUFFIX} $addr ${FPATH}. 0x10
>>
>> Doesn't that attempt to write a file named ".", which would end up
>> over-writing the directory content? Unless I'm misunderstanding, that
>> doesn't seem like a good idea.
>>
>> Also, ${FPATH} might be "", "/", "something", or "something/". Appending
>> "." to some of those won't work the same way as some others.
>
> "something" can not happen.

I don't see any code that prevents that. I think it's fairer to say that 
nothing currently happens to call the function with FPATH with a 
trailing /. Someone could easily edit the script later and add such a 
call without knowing about the undocumented restriction.

I note that in patch 1, the following logic:

+	if [ ! -z "$6" ]; then
+		FPATH=${6}/${FPATH}
  	fi

... ends up with FPATH having a leading / for the second invocation of 
test_image by the main script body, since ${6} has the value 
`pwd`/$MOUNT_DIR. That seems to violate FAT's requirement for pathnames 
not to have a leading /.

 > The other three cases are exactly what is wanted
> here, as fat currently needs a relative path, with CWD being "/", and ext
> explicitly wants an absolute path.
>
>>> @@ -471,6 +477,11 @@ function check_results() {
>>>
>>> +	# Check lookup of 'dot' directory
>>> +	grep -A5 "Test Case 12 " "$1" | \
>>> +		egrep -q 'Unable to write file'
>>> +	pass_fail "TC12: 1MB write to . - write denied"
>>
>> Oh, I see; this expects the write to be denied since the destination is
>> a directory. Perhaps that's OK. I'd rather see a read attempt though, to
>> guarantee that even if the access does end up being allowed, the FS
>> isn't trashed for any future tests that may be added afterwards.
>
> U-Boot master/2016-09 crashes here without the pending ext4 fixes.

 > IMHO after
> a failed test case, everything afterwards is invalid anyway, if the same fs
> image is used.

That's fair.

> As soon as the image is modified, tests are no longer idempotent. Block
> allocation order may change, anything that allocates any resource changes the
> image.
>
> Atempting a write here is done as it actually exercises a different code path.
> "ext4load host 0:0 0 /." reports "File not found", "ext4write host 0:0 0 /."
> crashes.
>
> Kind regards,
>
> Stefan
>

[U-Boot] [PATCH 3/3] test/fs: Check writes using "." (same dir) relative path

[Stephen Warren]

On 09/12/2016 04:04 PM, Stefan Bruens wrote:
> On Montag, 12. September 2016 12:44:08 CEST you wrote:
>> On 09/11/2016 02:46 PM, Stefan Br?ns wrote:
>>> <path>/<fname> and <path>/./<fname> should reference the same file.
>>>
>>> diff --git a/test/fs/fs-test.sh b/test/fs/fs-test.sh
>>>
>>> +# Read 1MB from small file
>>> +${PREFIX}load host${SUFFIX} $addr ${FPATH}$FILE_SMALL
>>
>> I think the same issue with $FPATH ending/not-ending in / applies here
>> too, and for all commands in this patch.
>
> FPATH is either "" for native fat, "/" for native ext4, or <somepath>/ for
> hostfs, so this is correct. Specifically, for fat, we dont want any "/" in
> front of $FILE_foo.

I believe FPATH can be either "", "/" or "/foo/bar" here, due to the 
issue I just mentioned in the other email.

>>> @@ -482,6 +499,16 @@ function check_results() {
>>>
>>> +	# Check directory traversal
>>> +	grep -A6 "Test Case 13a " "$1" | \
>>> +		egrep -q '1048576 bytes written|update journal'
>>
>> Why is "update journal" considered successful? Surely the "n bytes
>> written" message is always printed irrespective of whether anything
>> journal-related happened?
>
> Thats a question left to the author of Test Case 11, where the fragment was
> copied from.

I don't quite agree; you're adding the new test, so should make sure the 
validation code makes sense.

> Ext4 unfortunately is quite verbose, it inserts "File system is consistent"
> and "update journal finished" lines in the output. I think these lines where
> better stripped from the log prior to any further parsing.

ext4 may print some extra output, but that doesn't mean that any of it 
should be used in the validation. I think the simplest thing is to just 
ignore it in the validation code. Using egrep -q '1048576 bytes written' 
should do that just fine, and not get a false-positive if ext4 does say 
"update journal" without saying the required "1048576 bytes written".

[U-Boot] [PATCH 3/3] test/fs: Check writes using "." (same dir) relative path

[Brüns, Stefan]

On Dienstag, 13. September 2016 12:36:26 CEST you wrote:
> On 09/12/2016 04:04 PM, Stefan Bruens wrote:
> > On Montag, 12. September 2016 12:44:08 CEST you wrote:
> >> On 09/11/2016 02:46 PM, Stefan Br?ns wrote:
> >>> <path>/<fname> and <path>/./<fname> should reference the same file.
> >>> 
> >>> diff --git a/test/fs/fs-test.sh b/test/fs/fs-test.sh
> >>> 
> >>> +# Read 1MB from small file
> >>> +${PREFIX}load host${SUFFIX} $addr ${FPATH}$FILE_SMALL
> >> 
> >> I think the same issue with $FPATH ending/not-ending in / applies here
> >> too, and for all commands in this patch.
> > 
> > FPATH is either "" for native fat, "/" for native ext4, or <somepath>/ for
> > hostfs, so this is correct. Specifically, for fat, we dont want any "/" in
> > front of $FILE_foo.
> 
> I believe FPATH can be either "", "/" or "/foo/bar" here, due to the
> issue I just mentioned in the other email.

FPATH is either "", "/", "/foo/bar" + "/" or "/foo/bar" + "/" + "/". The last 
one is not to too nice, but still correct.

FPATH is *never* "/foo/bar", i.e. without trailing slash.

 
> >>> @@ -482,6 +499,16 @@ function check_results() {
> >>> 
> >>> +	# Check directory traversal
> >>> +	grep -A6 "Test Case 13a " "$1" | \
> >>> +		egrep -q '1048576 bytes written|update journal'
> >> 
> >> Why is "update journal" considered successful? Surely the "n bytes
> >> written" message is always printed irrespective of whether anything
> >> journal-related happened?
> > 
> > Thats a question left to the author of Test Case 11, where the fragment
> > was
> > copied from.
> 
> I don't quite agree; you're adding the new test, so should make sure the
> validation code makes sense.

I did not claim this is correct. I just stated this problem exists already for 
the older test cases.
 
> > Ext4 unfortunately is quite verbose, it inserts "File system is
> > consistent"
> > and "update journal finished" lines in the output. I think these lines
> > where better stripped from the log prior to any further parsing.
> 
> ext4 may print some extra output, but that doesn't mean that any of it
> should be used in the validation. I think the simplest thing is to just
> ignore it in the validation code. Using egrep -q '1048576 bytes written'
> should do that just fine, and not get a false-positive if ext4 does say
> "update journal" without saying the required "1048576 bytes written".

No, this will not work for ext4 *and* fat.

Every check starts with a "grep -Axx 'Test Case n'" match. If "-Axx" is 
extended to *always* include the "yyy bytes written", then it will work for 
ext4, but it will also match "Test Case n \n failed \n Test Case n+1 \n yyy 
bytes written".

Kind Regards,

Stefan

[U-Boot] [PATCH 2/3] test/fs: Check ext4 behaviour if dirent is first entry in directory block

[Brüns, Stefan]

On Dienstag, 13. September 2016 12:33:15 CEST Stephen Warren wrote:
> On 09/12/2016 03:48 PM, Stefan Bruens wrote:
> > On Montag, 12. September 2016 12:39:35 CEST you wrote:
> >> On 09/11/2016 02:46 PM, Stefan Br?ns wrote:
> >>> This is a regression test for a crash happening if the first dirent
> >>> in the block matches. Code tried to access a predecessor entry which
> >>> does not exist.
> >>> The crash happened for any block, but "." is always the first entry in
> >>> the first directory block and thus easy to check for.
> >>> 
> >>> diff --git a/test/fs/fs-test.sh b/test/fs/fs-test.sh
> >>> 
> >>> +# Next test case checks writing a file whose dirent
> >>> +# is the first in the block, which is always true for "."
> >>> +# The write should fail, but the lookup should work
> >>> +# Test Case 12 - Check directory traversal
> >>> +${PREFIX}${WRITE} host${SUFFIX} $addr ${FPATH}. 0x10
> >> 
> >> Doesn't that attempt to write a file named ".", which would end up
> >> over-writing the directory content? Unless I'm misunderstanding, that
> >> doesn't seem like a good idea.
> >> 
> >> Also, ${FPATH} might be "", "/", "something", or "something/". Appending
> >> "." to some of those won't work the same way as some others.
> > 
> > "something" can not happen.
> 
> I don't see any code that prevents that. I think it's fairer to say that
> nothing currently happens to call the function with FPATH with a
> trailing /. Someone could easily edit the script later and add such a
> call without knowing about the undocumented restriction.
> 
> I note that in patch 1, the following logic:
> 
> +	if [ ! -z "$6" ]; then
> +		FPATH=${6}/${FPATH}
>   	fi
> 
> ... ends up with FPATH having a leading / for the second invocation of
> test_image by the main script body, since ${6} has the value
> `pwd`/$MOUNT_DIR. That seems to violate FAT's requirement for pathnames
> not to have a leading /.

There are 6 different test runs: { sb, fs, nonfs } x { fat, ext4 }

fs and nonfs can be treated the same way, its just e.g. "ext4ls", "ls" or 
"fatls", "ls" resp. Thats what I called "native" in the other mail.

Now in the "sb" cases, it does not matter if the mounted image is fat or ext4 
- it always uses the full absolute path to the mountpoint.

The specific requirements of ext4 (absolute path) and fat (relative path) only 
apply to the nonfs and fs test runs.

Kind regards,

Stefan