Opened 8 years ago

Closed 8 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:
Platform: All

Description


Attachments (3)

0002-ext2-Volume-check-some-more-values-for-ext2_super_bl.patch​ (1.9 KB ) - added by wangxindsb 8 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 8 years ago.
0001-ext2-Volume-check-some-more-value-for-ext2_super_blo.2.patch​ (799 bytes ) - added by wangxindsb 8 years ago.
Remove the macro #define EXT2_BLOCK_GROUP_64BIT_SIZE

Download all attachments as: .zip

Change History (13)

comment:1 by wangxindsb, 8 years ago

patch: 0 → 1

by wangxindsb, 8 years ago

Attachment: 0002-ext2-Volume-check-some-more-values-for-ext2_super_bl.patch​ added

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

comment:2 by pulkomandy, 8 years ago

Resolution: → fixed
Status: new → closed

Applied in hrev51052. Thanks!

comment:3 by axeld, 8 years ago

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 by korli, 8 years ago

Resolution: fixed
Status: closed → reopened

comment:5 by pulkomandy, 8 years ago

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?

by wangxindsb, 8 years ago

Attachment: 0001-ext2-Volume-check-some-more-value-for-ext2_super_blo.patch​ added

in reply to:  5 comment:6 by wangxindsb, 8 years ago

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.

in reply to:  5 comment:7 by wangxindsb, 8 years ago

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 by pulkomandy, 8 years ago

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 by wangxindsb, 8 years ago

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

by wangxindsb, 8 years ago

Attachment: 0001-ext2-Volume-check-some-more-value-for-ext2_super_blo.2.patch​ added

Remove the macro #define EXT2_BLOCK_GROUP_64BIT_SIZE

comment:10 by pulkomandy, 8 years ago

Resolution: → fixed
Status: reopened → closed

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.