Skip to content

Do not empty config.php file if reading failed for any reason#33921

Merged
come-nc merged 1 commit into
masterfrom
fix/fix-config-file-emptied
Sep 12, 2022
Merged

Do not empty config.php file if reading failed for any reason#33921
come-nc merged 1 commit into
masterfrom
fix/fix-config-file-emptied

Conversation

@come-nc

@come-nc come-nc commented Sep 6, 2022

Copy link
Copy Markdown
Contributor

Fixes #18620

Signed-off-by: Côme Chilliet [email protected]

@come-nc come-nc added the 2. developing Work in progress label Sep 6, 2022
@come-nc come-nc added this to the Nextcloud 25 milestone Sep 6, 2022
@come-nc come-nc self-assigned this Sep 6, 2022
@come-nc come-nc force-pushed the fix/fix-config-file-emptied branch from 72ffa0a to c2dafb0 Compare September 6, 2022 15:37
@blizzz blizzz mentioned this pull request Sep 6, 2022
@szaimen

szaimen commented Sep 6, 2022

Copy link
Copy Markdown
Contributor

Seens like the tests work now? :)

@come-nc come-nc added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Sep 8, 2022
@come-nc come-nc requested review from a team, ArtificialOwl, icewind1991, skjnldsv and szaimen and removed request for a team September 8, 2022 07:23
@come-nc

come-nc commented Sep 8, 2022

Copy link
Copy Markdown
Contributor Author

Seens like the tests work now? :)

Yes 😄

@szaimen

szaimen commented Sep 8, 2022

Copy link
Copy Markdown
Contributor

/backport to stable24

@szaimen

szaimen commented Sep 8, 2022

Copy link
Copy Markdown
Contributor

/backport to stable23

@szaimen

szaimen commented Sep 8, 2022

Copy link
Copy Markdown
Contributor

@come-nc can you please link the issues that get fixed by this? Thanks! :)

@come-nc

come-nc commented Sep 8, 2022

Copy link
Copy Markdown
Contributor Author

Fixes #18620

@solracsf solracsf left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🥳

@solracsf

solracsf commented Sep 8, 2022

Copy link
Copy Markdown
Member

@come-nc should also fix #25175 or it doesn't cover these cases?

@come-nc

come-nc commented Sep 8, 2022

Copy link
Copy Markdown
Contributor Author

@come-nc should also fix #25175 or it doesn't cover these cases?

No it does not fix this one. It fixes cases where reading failed, not the ones where writing fails.
Maybe replacing ftruncate by opening the file in write mode would solve those? Not sure.

@andrey-utkin

Copy link
Copy Markdown

Maybe replacing ftruncate by opening the file in write mode would solve those? Not sure.

No. Create new + rename would solve this, as suggested 1, 2

I'm just following the issue, would get my hands on it but you seem to be almost done :)

@andrey-utkin

andrey-utkin commented Sep 9, 2022

Copy link
Copy Markdown

I've created that PR to demonstrate the atomic update approach. To be continued.

@come-nc come-nc merged commit 1f7e769 into master Sep 12, 2022
@come-nc come-nc deleted the fix/fix-config-file-emptied branch September 12, 2022 07:43
@backportbot-nextcloud

Copy link
Copy Markdown

The backport to stable23 failed. Please do this backport manually.

@szaimen

szaimen commented Sep 12, 2022

Copy link
Copy Markdown
Contributor

/backport to stable23

@backportbot-nextcloud

Copy link
Copy Markdown

The backport to stable23 failed. Please do this backport manually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews bug high

Projects

None yet

Development

Successfully merging this pull request may close these issues.

complete config reset with filled disk

4 participants