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] Align with upstream and mark ReactOS diff #5245

Merged
merged 2 commits into from Apr 30, 2023

Conversation

Vince1171
Copy link
Contributor

Purpose

add ifdefs to distinguish ReactOS code/fixes from the upstream code.
NOTE: there are no new features in this PR, we stay with upstream version 0.69.

JIRA issue: CORE-18645

Proposed changes

  • surround ReactOS modifications with ifdef
  • if upstream code was override, reintroduce it in #else section
  • re-align .rc version info with upstream (0.69)

@github-actions github-actions bot added the drivers Kernel mode drivers and frameworks label Apr 16, 2023
if (Property2->bHidingPrefix == Vcb->bHidingPrefix) {
#else
if (Property2->bHidingPrefix = Vcb->bHidingPrefix) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that a bug in the original code, that we fixed in ReactOS? Or is the original code correct here, and we just tried to mute some static code check and did it wrong? (The same thing can also be found at a 2nd location within this file)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's a bug in the original code that we fixed indeed. After this PR, I'll try to upstream those fixes. but unfortunately, as stated in the Jira ticket, upstream seems to be abandoned. only https://github.com/bobranten/Ext4Fsd seems to still be around

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be worth looking at whether the ext4fsd of Bo Branten also covers ext2 and 3, and if so, how much does the code change wrt. that ext2/3 driver we have. In case of positive observations, maybe one could think about updating to that ext4fsd ?

Copy link
Contributor Author

@Vince1171 Vince1171 Apr 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'll try to have a look into it later :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be worth looking at whether the ext4fsd of Bo Branten also covers ext2 and 3, and if so, how much does the code change wrt. that ext2/3 driver we have. In case of positive observations, maybe one could think about updating to that ext4fsd ?

According to its readme it also covers ext2/3.
There are 2 pro arguments.

  1. It additionally supports a more modern file system
  2. It is under active development
    We should really think about switching to this never driver.

Copy link
Member

@gonzoMD gonzoMD Apr 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's a bug in the original code that we fixed indeed. After this PR, I'll try to upstream those fixes. but unfortunately, as stated in the Jira ticket, upstream seems to be abandoned. only https://github.com/bobranten/Ext4Fsd seems to still be around

This also has this bug, so it's worth to send a PR. See https://github.com/bobranten/Ext4Fsd/blob/d9da413ad28afa3c0519d730da60f8598bbaea32/Ext4Fsd/devctl.c#L514
And line 523 too.

Copy link
Contributor Author

@Vince1171 Vince1171 Apr 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when this PR is merged, I'll open a PR on ext4fsd with ReactOS fixes. Then I'll see to use Ext4fsd in ROS :)

@HBelusca HBelusca changed the title [EXT2FS] align with upstream and add ifdef for ReactOS specific code [EXT2FS] Align with upstream and mark ReactOS diff Apr 21, 2023
drivers/filesystems/ext2/inc/linux/module.h Show resolved Hide resolved
drivers/filesystems/ext2/inc/linux/module.h Outdated Show resolved Hide resolved
drivers/filesystems/ext2/inc/linux/types.h Show resolved Hide resolved
drivers/filesystems/ext2/src/ext4/ext4_extents.c Outdated Show resolved Hide resolved
drivers/filesystems/ext2/src/fileinfo.c Outdated Show resolved Hide resolved
Co-authored-by: Hermès BÉLUSCA - MAÏTO <hermes.belusca-maito@reactos.org>
@HBelusca
Copy link
Contributor

I suppose that those changes (restoring the proper ifdefs wrt. upstream) doesn't break reading EXT2/3 partitions with ReactOS.

@Vince1171
Copy link
Contributor Author

I suppose that those changes (restoring the proper ifdefs wrt. upstream) doesn't break reading EXT2/3 partitions with ReactOS.

no break indeed ^^

Screenshot from 2023-04-25 16-02-06

@HBelusca HBelusca added the 3rd party sync Updating 3rd party components, such as Wine and others label Apr 30, 2023
@HBelusca HBelusca merged commit aaeb131 into reactos:master Apr 30, 2023
33 of 34 checks passed
@HBelusca
Copy link
Contributor

Thanks for the sync!

@binarymaster binarymaster added this to New PRs in ReactOS PRs via automation Jun 10, 2023
@binarymaster binarymaster moved this from New PRs to Done in ReactOS PRs Jun 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3rd party sync Updating 3rd party components, such as Wine and others drivers Kernel mode drivers and frameworks
Projects
ReactOS PRs
  
Done
4 participants