-
Notifications
You must be signed in to change notification settings - Fork 764
epoll support #1558
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
base: dev
Are you sure you want to change the base?
epoll support #1558
Conversation
|
Hi, Thanks for the PR and welcome to Qiling Framework. Please take a look at the error. it seems lik here is some error |
|
Ok, I disabled the unit tests I had provided, as they won't yet work until my examples are accepted upstream in |
|
let me accept that pr |
|
you can enable the test. i updated the CI test and the merged rootfs |
FWIW, one problem I've pointed out is with a single test case -- and only under unit testing, not the underlying Qiling emulation of multithreaded code. The example binaries are single-threaded, as is the epoll implementation I have so far. |
elicn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi there,
Thanks for your contribution. See my review comments inline.
Also please add comments to explain the tests, what is happening on each stage and what is the expected outcome. Comments are key to make the tests usable; otherwise nobody will know what is actually tested and how to react when they fail.
|
I appreciate the feedback @elicn I'm running into a couple of issues with the tests I have written, both in 1: When trying to feed my 2: When running the test Would you please take a look at these problems? |
|
I've patched some of the code earlier today and posted a PR to your repo. Please review the changes I made to see if they make sense. There were a few snippets in which I could not figure out what should be the expected behavior; look for my inline comments starting with "elicn". |
Refactor and fixes
|
Looking into the permission denied errors I'm getting when running the whole test suite here |
|
The commits you pushed after my PR are introducing wrong 'fixes'. |
|
Rolled back to Ran in PDB, which shows what I think is the issue -- this is what I was trying to deal with in the next commit |
|
Got it. |
|
@elicn Done |
|
I've created a new PR. |
|
The test errors suggest that you relocate the |
|
Commented out the test case, will dig deeper with what's going on in unittest. |
|
@elicn Up to you if you want to merge as is, or I can convert to draft PR. The blocker was/is that the code works outside of unittest, but gets "Operation not permitted" errors when running the ELFTest group. |
|
@elicn Thoughts? |
|
I prefer avoiding incorporating changes that cannot be tested. |
|
@elicn Haven't looked at this in a while, but I finally figured out what was the issue. |
elicn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to hear. Providing a brief explanation could help us look into issues around this in the future.
Please refer to my comments in this review.
| ql = Qiling(["../examples/rootfs/x86_linux/bin/x86_hello"], "../examples/rootfs/x86_linux", verbose=QL_VERBOSE.DEBUG, log_devices=[filename]) | ||
| ql.run() | ||
|
|
||
| ql._log_file_fd.handlers[0].close() # prevent FD leak that causes downstream issues |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very unusual.
Care to explain what this is about?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When running this test case, there was a dangling FD pointing to ql.log on the disk after os.remove got invoked. This FD ended up being added to the epoll args, and not the legitimate server socket as I observed outside the test suite.
So here, I just force a closure to keep the FD list in a consistent state.
|
@elicn Thoughts? |
Checklist
Which kind of PR do you create?
Coding convention?
Extra tests?
Changelog?
Target branch?
One last thing
Comments
This PR is intended to provide initial support for the
epoll_create,epoll_create,epoll_ctl, andepoll_waitsyscalls using theselectlibrary; similar to howpollis done already. The binaries for testing have also been PR'd here; but I have since changed the names slightly after checking out theexamples/srcfolder.