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

Prospective fix for strange issue with invalid UTF-8 with test #494

Merged

Conversation

troglodyne
Copy link
Contributor

For now this PR is only for purposes of showing the problem and one potential fix to it.

Related bug:
https://bugzilla.redhat.com/show_bug.cgi?id=2212953

I can fixup the commit later for formatting, etc. -- in the meantime I am going to try to gather more data from cPanel customers experiencing this to see just what the heck is in the smaps files for the failing cases.

Regardless, this appeared to me to be a real bug in the upstream code, so I figured I'd approach it in that manner.

@troglodyne
Copy link
Contributor Author

troglodyne commented Jun 6, 2023

Test looking like it failed to write the thing I was writing to tmp over on CI. Figured something like that might occur. Will clean this up later presuming the general approach even seems sane here to fix the problem.

@jan-kolarik jan-kolarik self-assigned this Jun 12, 2023
@jan-kolarik
Copy link
Member

jan-kolarik commented Jun 21, 2023

I find your approach OK as it just allows listing opened files in additional (corner) use cases that you've provided. As we currently focus our efforts into the development of DNF5, I wouldn't develop the current solution further as it seems fixing your problematic cases.

I guess that the CI failure could be related to passing the uid = 0 in the test case. Could you please try passing there None, like: ofiles = list(needs_restarting.list_opened_files(None))? Also if it is working, please eventually drop the reviewer / TODO notes from the PR, do the final polishing and it should be good to go. Thanks for that!

= changelog =
msg: Avoid issue with garbage smaps chars in needs-restarting.py
type: bugfix
resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2212953
related: None
@troglodyne
Copy link
Contributor Author

Reworked commit, thanks for the input. Sorry to have missed the notification earlier.

@troglodyne
Copy link
Contributor Author

troglodyne commented Aug 9, 2023

Hmm. The CI failures in the integration tests here don't really seem related to needs-restarting as far as I can tell. A reposync test failed, line 41563 of log, but nothing else that appears to be failing in the set.

@m-blaha
Copy link
Member

m-blaha commented Aug 9, 2023

No, the failures of reposync are unrelated. I guess it's caused by some changes in createrepo_c which caused different set of metadata to be generated. Anyway, we need to fix the test suite.

@troglodyne
Copy link
Contributor Author

Everything looking good now, it seems :)

Copy link
Member

@jan-kolarik jan-kolarik left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@jan-kolarik jan-kolarik merged commit 8867aa8 into rpm-software-management:master Aug 14, 2023
3 checks passed
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.

None yet

4 participants