Skip to content

Conversation

R42ccoon
Copy link
Contributor

@R42ccoon R42ccoon commented Apr 17, 2025

  • Fix poll() error : deleting the not because the function returns None if the process is running
  • Clean up maps parsing : replace memory_maps() by maps() and remove useless code

@R42ccoon
Copy link
Contributor Author

R42ccoon commented Jul 8, 2025

Any update ?

@peace-maker
Copy link
Member

Yes, sorry, I always wanted to check if this was broken before adding the poll() call at the beginning, since you removed parsing of the .addr attribute, but always forgot when being back at a computer. It seems like we need better tests for this function regardless :D

Can you confirm this was broken and the addr parsing isn't needed anymore?

@R42ccoon
Copy link
Contributor Author

R42ccoon commented Jul 15, 2025

The addr parsing is already done by maps(). Also, i edited the commit to move the poll() check from libs() to maps(), because maps() is called in others functions. And i have added error handling, i put RuntimeError but there may be better.

raw_maps = self.poll() is None and memory_maps(self.pid)

if not raw_maps:
raise RuntimeError("Could not read maps, process %s has finished" % self.pid)
Copy link
Member

Choose a reason for hiding this comment

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

Hm, changing the behavior here to throw instead of returning the ELF.maps mapping seems like a big change in behavior that could be talked about. Why do you think this is better?

Can we reduce this PR to just the is not None goof up fix instead of doing multiple changes at once please? That is an obvious typo :D

@R42ccoon
Copy link
Contributor Author

R42ccoon commented Aug 7, 2025

Ok, i reverted the new changes for a future PR.

@peace-maker peace-maker enabled auto-merge (squash) August 9, 2025 14:38
@peace-maker peace-maker merged commit ffc46d2 into Gallopsled:stable Aug 9, 2025
12 checks passed
@R42ccoon R42ccoon mentioned this pull request Aug 19, 2025
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.

2 participants