Skip to content

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

Merged
nathanchance merged 1 commit into
mainfrom
signals
Dec 16, 2022
Merged

boot-qemu.py: fix ctrl+c in gdb#87
nathanchance merged 1 commit into
mainfrom
signals

Conversation

@nickdesaulniers

@nickdesaulniers nickdesaulniers commented Dec 16, 2022

Copy link
Copy Markdown
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

@nathanchance nathanchance 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.

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

Comment thread boot-qemu.py Outdated
@nickdesaulniers

nickdesaulniers commented Dec 16, 2022

Copy link
Copy Markdown
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

nathanchance commented Dec 16, 2022

Copy link
Copy Markdown
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

nathanchance commented Dec 16, 2022

Copy link
Copy Markdown
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