vstinner / hachoir

Hachoir is a Python library to view and edit a binary stream field by field
http://hachoir.readthedocs.io/
GNU General Public License v2.0
604 stars 70 forks source link

ext2: add support to various inode sizes #92

Closed masatake closed 6 months ago

masatake commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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

masatake commented 7 months ago

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 commented 7 months ago

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 commented 6 months ago

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 :-)

masatake commented 6 months ago

I reduced the number of test files to three.

vstinner commented 6 months ago

Merged, thank you!