bubblewrap
bubblewrap copied to clipboard
containers
Add CLI option to pass common signals to sandboxed process
Adds an command line switch to optionally pass SIGINT and SIGTERM to the sandboxed process to handle.
Builds on the work done in #402 .
Fixes #369 Fixes #573
Thanks for the review, and sorry for the shoddy work. I've addressed the parts of review I could in code.
This PR relied on the work from #402 , and I had just added a command line option to switch the behaviour on or off.
What does "here" denote? Is the intention here that we are blocking signals so that they will be blocked in the parent process outside the sandbox (the one that calls monitor_child()), or so that they will be blocked in the child process inside the sandbox (the one that eventually executes a user-specified COMMAND), or what?
I don't really know what the intention was, I only tested that the work from the previous author solves the symptom I was facing.
I have renamed the function (and reworked other things based on your review) , but haven't edited the comment. I will be happy to rework more things based on another review.
Also, do you think PR #588 is a better fix for issue #369 ? Is it worth spending time on this PR to make it up to shape ?
Also, do you think PR #588 is a better fix for issue #369 ?
It's not clear to me either way - I am not as expert in Unix minutiae as the primary authors of bubblewrap, but I'm trying to get some reviews done in the other maintainers' absence.
I think #588 is probably a better fix for #369 as originally stated, which can be summarized as "I want Ctrl+C to do the normal thing in an interactive terminal". But, explicitly forwarding signals as done here might well be a better approach when bubblewrap is used as an implementation detail of GUI or background processes (most prominently Flatpak, but also Steam, WebKitGTK, various build environments in NixOS, and whatever other situations people put it into), so that a pkill bwrap will pass down the SIGTERM to the top-level sandboxed process, to do whatever it wants to do with that signal.
I think the fact that there are two implementations, apparently equally viable but rather different, confirms my belief that neither of these two approaches should be the default.
Is it worth spending time on this PR to make it up to shape ?
If this is something that you're interested in making happen, then yes I think so.
I'd prefer to review this (and almost any other contribution, really) as a series rebased onto the current bubblewrap main branch, where each commit is fully correct individually, rather than having things that are wrong and are fixed in a subsequent commit - that's the form that is going to be most useful when a future maintainer needs to do the research to find out why the code is the way it is, for example when tracking down a regression.
the work from the previous author
Please amend any commits that are not all your own work to set the original author or add an appropriate Co-authored-by. Losing other contributors' attribution is not really OK.
I'd prefer to review this (and almost any other contribution, really) as a series rebased onto the current bubblewrap main branch, where each commit is fully correct individually, rather than having things that are wrong and are fixed in a subsequent commit - that's the form that is going to be most useful when a future maintainer needs to do the research to find out why the code is the way it is, for example when tracking down a regression.
This, please? In this case I think it would probably be a single commit, Co-authored-by you and the author of #402.
I'd really like to see some automated test coverage for this - perhaps running a shell with a trap on SIGUSR1 or some such as a child of a background bwrap, killing the bwrap with that signal, asserting that the process is still running and has received the signal (maybe the trap could write to a temporary file or similar), and then killing the bwrap with SIGTERM to clean up.
I'd prefer to review this (and almost any other contribution, really) as a series rebased onto the current bubblewrap main branch, where each commit is fully correct individually, rather than having things that are wrong and are fixed in a subsequent commit - that's the form that is going to be most useful when a future maintainer needs to do the research to find out why the code is the way it is, for example when tracking down a regression.
This, please? In this case I think it would probably be a single commit, Co-authored-by you and the author of #402.
I thought I already added the original author of #402 as a co author in commit https://github.com/containers/bubblewrap/pull/586/commits/225ca094b1b1acae6521702f5edc4a703163aba2 ?
I thought I already added the original author of https://github.com/containers/bubblewrap/pull/402 as a co author in commit https://github.com/containers/bubblewrap/commit/225ca094b1b1acae6521702f5edc4a703163aba2 ?
You did. What I'm now asking for is combining the initial "not quite right" version with your later fixes, into a single commit that is as correct as we can get it - so that when a future maintainer digs back through the history to find out why something is the way it is, they don't have to look at the initial commit, think "but surely this is wrong?", and then mentally combine it with the commits that followed it to get a better picture of the change that was actually applied. The intermediate stages that your change went through as a result of reviews and revisions are part of the history of this PR, but they don't need to be part of the history of bubblewrap.
(It's often easier to review a complete/correct commit that makes sense on its own, too.)
Sometimes it does make sense to contribute a PR with more than one commit, but ideally each prefix of the series of commits should be something that would have been valid to apply on its own - like for example #488 adds two related features, but if I had stopped after the first one, that would have been valid (although less useful).
For simpler changes like this one, one commit is often enough.
I have addressed the parts of the code review that I could, and left a question for the one I didn't understand. I will squash all my commits into a single co-authored commit after I have included the one remaining point as well.
I have tested this patchset (backported to 0.8.0), and it unfortunately does not work for Ctrl-C. :(
Can you share exactly what you tried? My branch reports version 0.8.0 as well , but there may be other changes incorporated into master since then till the point I branched off.
The last commit shared between my branch and master is ad76c2d6, if you want to try from there (or just try cloning my work?).
Took the v0.8.0 tag, applied https://github.com/containers/bubblewrap/pull/586.patch, resolved the trivial conflict, built and tested with gdb. Ctrl-C kills the entire gdb program rather than merely printing "Quit" and giving me back the gdb prompt as normally gdb does.