Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ext2: add support to various inode sizes #92

Merged
merged 1 commit into from Feb 12, 2024

Conversation

masatake
Copy link
Collaborator

@masatake masatake commented Feb 4, 2024

the script used for generating test inputs:

for bsize in 1024 2048 4096; do
    for isize in 128 256 512 1024; do
        f=tests/files/bsize-${bsize}-isize-${isize}.ext2
        dd if=/dev/zero of=$f bs=${bsize} count=128
        mkfs.ext2 -I ${isize} -b ${bsize} $f
        mkdir -p __tmp__
        sudo mount $f __tmp__
        echo hello | sudo dd if=/dev/stdin of=__tmp__/source
        (cd __tmp__/; sudo ln -s source target)
        sudo umount __tmp__
    done
done

@vstinner
Copy link
Owner

vstinner commented Feb 4, 2024

Hi, nice change. Would it be possible to add a single test file and truncate it, to not make the Git repo too big?

@vstinner
Copy link
Owner

vstinner commented Feb 4, 2024

It would also be more interesting if the test file contained actual data, rather than being a just newlt created FS.

@masatake
Copy link
Collaborator Author

masatake commented Feb 7, 2024

It would also be more interesting if the test file contained actual data, rather than being a just newlt created FS.

I could not find a way to do it.

To verify a content of a file, I added the following check to my test case:

                self.checkValue(parser, "/group[0]/inode[11]block[0]", b"hello\n" + b'\0' * (bsize - 6))

However, what I got is

ERROR: test_ext2_various_inode_sizes (test_parser.TestParsers.test_ext2_various_inode_sizes)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/test_parser.py", line 360, in test_ext2_various_inode_sizes
    self.checkValue(parser, "/group[0]/inode[11]block[0]", b"hello\n" + b'\0' * (bsize - 6))
  File "tests/test_parser.py", line 44, in checkValue
    read = parser[path].value
           ~~~~~~^^^^^^
  File "/home/yamato/var/hachoir/hachoir/field/field.py", line 261, in __getitem__
    return self.getField(key, False)
           ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/yamato/var/hachoir/hachoir/field/generic_field_set.py", line 233, in getField
    return Field.getField(self, key, const)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/yamato/var/hachoir/hachoir/field/field.py", line 254, in getField
    raise MissingField(current, part)
hachoir.field.field.MissingField: Can't get field "inode[11]block[0]" from /group[0]

Though I can see the field:

image

Is "/group[0]/inode[11]block[0]" is an acceptable field specifier?
I wonder whether this is related to #20.

@masatake
Copy link
Collaborator Author

masatake commented Feb 7, 2024

Hi, nice change. Would it be possible to add a single test file and truncate it, to not make the Git repo too big?

I truncated the input files as small as possible.
Should I reduce the number of input files?

It would also be more interesting if the test file contained actual data, rather than being a just newlt created FS.

I found I made a mistake when generating the input files.
After fixing the input files, I added checks verifying a block that is part of a regular file.

@vstinner
Copy link
Owner

Should I reduce the number of input files?

Yes please. If the code works with 2-3 inode sizes, it should be enough to validate the implementation in tests. We don't write a full kernel driver, it's ok :-)

the script used for generating test inputs:

    declare -A sizes=(
	[1024,1024]=$((130048 + 1024))
	[2048,512]=$((  45056 + 2048))
	[4096,128]=$((  53248 + 4096))
    )

    adjust_size()
    {
	local f=$1
	local bsize=$2
	local isize=$3

	truncate --size=${sizes[$bsize,$isize]} $f
    }

    makeimg()
    {
	local bsize=$1
	local isize=$2

	local f=tests/files/bsize-${bsize}-isize-${isize}.ext2
	dd if=/dev/zero of=$f bs=${bsize} count=128
	mkfs.ext2 -I ${isize} -b ${bsize} $f
	sudo mount $f __tmp__
	echo hello | sudo dd if=/dev/stdin of=__tmp__/source
	(cd __tmp__/; sudo ln -s source target)
	sudo umount __tmp__
	adjust_size $f $bsize $isize
    }

    mkdir -p __tmp__
    makeimg 1024 1024
    makeimg 2048 512
    makeimg 4096 128
    rmdir __tmp__

Signed-off-by: Masatake YAMATO <yamato@redhat.com>
@masatake
Copy link
Collaborator Author

I reduced the number of test files to three.

@vstinner vstinner merged commit 65f8c20 into vstinner:main Feb 12, 2024
2 checks passed
@vstinner
Copy link
Owner

Merged, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants