Opened 2 years ago

Closed 2 years ago

#13401 closed enhancement (fixed)

Check some more values for ext2_super_block

Reported by: wangxindsb Owned by: nobody
Priority: normal Milestone: R1
Component: File Systems/ext2 Version: R1/Development
Keywords: GSoC 2017 Cc:
Blocked By: Blocking:
Has a Patch: yes Platform: All

Description


Attachments (3)

0002-ext2-Volume-check-some-more-values-for-ext2_super_bl.patch (1.9 KB) - added by wangxindsb 2 years ago.
Complete the TODO: check some more values! in the haiku/src/add-ons/kernel/file_systems/ext2/Volume.cpp
0001-ext2-Volume-check-some-more-value-for-ext2_super_blo.patch (1.3 KB) - added by wangxindsb 2 years ago.
0001-ext2-Volume-check-some-more-value-for-ext2_super_blo.2.patch (799 bytes) - added by wangxindsb 2 years ago.
Remove the macro #define EXT2_BLOCK_GROUP_64BIT_SIZE

Download all attachments as: .zip

Change History (13)

comment:1 Changed 2 years ago by wangxindsb

Has a Patch: set

Changed 2 years ago by wangxindsb

Complete the TODO: check some more values! in the haiku/src/add-ons/kernel/file_systems/ext2/Volume.cpp

comment:2 Changed 2 years ago by pulkomandy

Resolution: fixed
Status: newclosed

Applied in hrev51052. Thanks!

comment:3 Changed 2 years ago by axeld

Is it possible that you forgot to add a file to your patch? It doesn't compile, and has now broken our build. Kudos for managing that even before getting commit rights!

An updated patch would be appreciated, though.

comment:4 Changed 2 years ago by korli

Resolution: fixed
Status: closedreopened

comment:5 Changed 2 years ago by pulkomandy

Has64bitFeature is in the Volume class, but the code you changed is in ext2_super_block, so it can't access it. Did you not test your changes before submitting the patch?

comment:6 in reply to:  5 Changed 2 years ago by wangxindsb

Replying to pulkomandy:

Has64bitFeature is in the Volume class, but the code you changed is in ext2_super_block, so it can't access it. Did you not test your changes before submitting the patch?

Sorry for my mistake.

comment:7 in reply to:  5 Changed 2 years ago by wangxindsb

Replying to pulkomandy:

Has64bitFeature is in the Volume class, but the code you changed is in ext2_super_block, so it can't access it. Did you not test your changes before submitting the patch?

I have add a new patch and remove the Has64bitFeature because it will be checked in Volume::Mount, there I check the GroupDescriptorSize() should between 32 and 64.

comment:8 Changed 2 years ago by pulkomandy

I already removed the Has64bitFeature() call in our master branch to fix the build, so you should work from the latest version. See http://cgit.haiku-os.org/haiku/commit/?id=6bbb8b3022eb6691784e14aee705bd3fb4118989 for my changes.

Also check the coding style. The previous patch had problems (as reviewed on the commits mailing list: http://www.freelists.org/post/haiku-commits/haiku-hrev51052-srcaddonskernelfile-systemsext2,1 ) and I think you still have a line that is above 80 columns.

comment:9 Changed 2 years ago by wangxindsb

I'm wrong, the Volume::Mount() will check the GroupDescriptorSize, so in the ext2_super_block::IsValid(), it is unnecessary to check it. The hrev51053 is fine. Do I still need to have a patch on the hrev51052

Changed 2 years ago by wangxindsb

Remove the macro #define EXT2_BLOCK_GROUP_64BIT_SIZE

comment:10 Changed 2 years ago by pulkomandy

Resolution: fixed
Status: reopenedclosed

I think we can leave it as it is now. Let's keep this constant in as it will be needed when the feature is implemented, right?

Note: See TracTickets for help on using tickets.