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.

Sign up for GitHub

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

Jump to bottom

boot-qemu.py: fix ctrl+c in gdb #87

Merged
merged 1 commit into from
Dec 16, 2022
Merged

boot-qemu.py: fix ctrl+c in gdb #87

merged 1 commit into from
Dec 16, 2022

Conversation

nickdesaulniers
Copy link
Member

Detach QEMU from the process group so that SIGINT isn't also delivered to it.

Ignore SIGINT in python when running GDB subprocess.

Link: https://stackoverflow.com/a/13737455
Link: https://stackoverflow.com/q/30925198
Fixes: #86

nathanchance
nathanchance approved these changes Dec 16, 2022
View reviewed changes
Copy link
Member

@nathanchance nathanchance left a comment

Choose a reason for hiding this comment

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

Aside from the potential nit, looks good to me! Happy this works :)

boot-qemu.py Outdated Show resolved Hide resolved
@nickdesaulniers
Copy link
Member Author

The linter isn't happy.

boot-qemu.py:768:17: W1509: Using preexec_fn keyword which may be unsafe in the presence of threads (subprocess-popen-preexec-fn)

Looks like I might be able to use process_group=1 in the subprocess.Popen constructor, but seems it's only available in python 3.11+. (I'm on 3.10.8).

Should we disable W1509?

nickdesaulniers added a commit to ClangBuiltLinux/actions-workflows that referenced this pull request Dec 16, 2022
@nickdesaulniers
disable pylint W1509 subprocess-popen-preexec-fn
20ba4b5 
I'm not sure how to work around

  boot-qemu.py:768:17: W1509: Using preexec_fn keyword which may be unsafe in the presence of threads (subprocess-popen-preexec-fn)

For python 3.10.8.  I might be able to use process_group=1 in the
subprocess.Popen constructor, but seems it's only available in python
3.11+.

Link: ClangBuiltLinux/boot-utils#87
@nickdesaulniers nickdesaulniers mentioned this pull request Dec 16, 2022
Merged
@nathanchance
Copy link
Member

Should we disable W1509?

Yes, either in actions-workflow as you did or just adding # pylint: disable-next=subprocess-popen-preexec-fn above subprocess.Popen() here. Does not matter to me, pick your poison; just make sure everything is green before merging.

@nathanchance
Copy link
Member

You can drop the pylint comment now since you merged ClangBuiltLinux/actions-workflows@20ba4b5. It looks like there is just one outstanding yapf warning, then this should be good to go.

@nickdesaulniers
boot-qemu.py: fix ctrl+c in gdb
4beafc6 
Detach QEMU from the process group so that SIGINT isn't also delivered
to it.

Ignore SIGINT in python when running GDB subprocess.

Link: https://stackoverflow.com/a/13737455
Link: https://stackoverflow.com/q/30925198
Fixes: #86
@nathanchance nathanchance merged commit 1b837f3 into main Dec 16, 2022
@nathanchance nathanchance deleted the signals branch December 16, 2022 23:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nathanchance nathanchance nathanchance approved these changes

@msfjarvis msfjarvis Awaiting requested review from msfjarvis msfjarvis is a code owner

@tpimh tpimh Awaiting requested review from tpimh

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

issue with ctrl+c in -g mode
2 participants
@nickdesaulniers @nathanchance