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

Ext2FS: Replace Linux macros with homegrown Serenity-style definitions #25027

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

logkos
Copy link
Contributor

@logkos logkos commented Sep 16, 2024

This PR replaces ancient definitions for Ext2FS (which dates back to the first commit) with a custom implementation. The original header (which was borrowed from Linux) contained many unused elements, used Linux types and had an incompatible license. This change should resolve those issues and hopefully provide more flexibility when working with Ext2.

Currently a draft intended to gather consensus on the final approach.

And idk why but this makes serenity hang at console for no known reason. Any help here is welcome

@logkos logkos force-pushed the master branch 4 times, most recently from f854dec to 7128297 Compare September 17, 2024 18:51
@timschumi
Copy link
Member

Regardless of the current build failure, there have been attempts to resolve the NIH-ness of the ext2 header in the past, but it was always deemed insufficient for getting rid of the license conflict. The fact that some comments in the reworked version are identical to the file that we are trying to get rid of should be enough of an indicator.

fwiw, I had started work on a from-scratch ext4 driver in the past (just using information from the Linux kernel wiki and documentation), but it's currently on ice (and therefore currently stuck at superblock parsing). I feel like that might be a more reliable way to resolve the issue.

@kleinesfilmroellchen
Copy link
Collaborator

+1, this is not something that seems worth solving right now, even though the C-ness of this header is really ugly.

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.

3 participants