Exit from docker login on SIGTERM and SIGINT.
#1317
Conversation
|
LGTM |
|
Oh, I see that you use your own term. This needs more work than I anticipated. |
|
@calavera hmmm? What do you mean? |
|
Sorry, sometimes I cannot even understand myself. The input comes from the raw terminal. Those signals are never triggered, although it looks like they should be captured by the channel at: https://github.com/dotcloud/docker/blob/master/term/term.go#L47 |
|
I finally could figure out what the problem was. Allowing to generate signals in the new termios fixes the problem. A little gift with this working as expected: /cc @creack |
|
@unclejack could you take a look as you worked on #1249 |
|
@calavera The login used to be handled 100% server side, that's why we used the raw mode. Now that the login is handle client side, I guess we could remove it. |
|
Oh, that makes sense. The echo still needs to be disabled for the password, but the ECHO flag can be disabled in the right moment for that. |
* master: (54 commits) Return JSONError for HTTPResponse error Fix TestEnv Revert "Bind daemon to 0.0.0.0 in Vagrant. Fixes moby#1304" Add unit tests for build no cache Add no cache for docker build Move note about officially supported kernel Solved the logo being squished in Safari Add hostname to the container environment. fix same issue in api.go improve tests Return registy status code in error update http://get.docker.io/latest Add check that the request is good fix tests about refactor checksums add ufw doc Fixed a couple of minor syntax errors. Updated the description of run -d Make sure the index also receives the checksums Switch json/payload order Remove unused parameter ...
It only disables echo asking for the password and lets the terminal to handle everything else. It fixes moby#1392 since blank spaces are not discarded as they did before. It also cleans the login code a little bit to improve readability.
|
I believe this is ready to merge, but I'd really appreciate some 👀 on it. |
|
@calavera there is a \n issue. The client sends 'creack\n' as login instead of 'creack' |
| @@ -33,7 +33,7 @@ func MakeRaw(fd uintptr) (*State, error) { | |||
|
|
|||
| newState.Iflag &^= (syscall.IGNBRK | syscall.BRKINT | syscall.PARMRK | syscall.ISTRIP | syscall.INLCR | syscall.IGNCR | syscall.ICRNL | syscall.IXON) | |||
| newState.Oflag &^= syscall.OPOST | |||
| newState.Lflag &^= (syscall.ECHO | syscall.ECHONL | syscall.ICANON | syscall.ISIG | syscall.IEXTEN) | |||
There was a problem hiding this comment.
Why do you need to remove the ISIG flag? Now when doing 'ctrl-c' within a shell, it kills the client (docker run -i -t ubuntu bash)
There was a problem hiding this comment.
This was the initial cause of the problem with the login command. I can put this back since the login command doesn't use this code anymore.
There was a problem hiding this comment.
Please do so, it breaks the tty behavior.
|
@creack I've put the ISIG sygnal back and remove the writer. I couldn't find any good solution without complicating the term more than it is now. |
|
@calavera could you please rebase this ? |
It only disables echo asking for the password and lets the terminal to handle everything else. It fixes moby#1392 since blank spaces are not discarded as they did before. It also cleans the login code a little bit to improve readability.
* master: (23 commits) Made calling changelog before run return empty. Fixes moby#1705. fix error in docker build when param is not a directory Document FROM <image>:<tag> Dockerfile instruction. Install Ubuntu raring backported kernel from official archives directly. Updated "Use -> The Basics" to use ubuntu:12.10. hide version when not available added a Dockerfile which installs all deps and builds the docs. Unable to find image error should print to stderr remove message during tests use init function add TEST env var during tests and silenced parserun during tests Update python_web_app.rst Update remaining upstart scripts to wait for lxc-net Fixed a minor syntax error. Add privileged flag in documentation for container creation Fix moby#1685: Notes on production use. General installation cleanup. Fix bash completion, remove have added apt-key finger tip and fingerprint in ubuntu installation page Improve formatting with 'go fmt' as stated in CONTRIBUTING.md Start docker after lxc-net to prevent ip forwarding race ...
…o login_signal * 'login_signal' of https://github.com/calavera/docker: Use flag.StringVar to capture the command line flags. Fix syscall name. Remove unused imports. Simplify term signal handler. Add the ISIG syscall back to not kill the client withing a shell with ctrl+c. Print a new line after getting the password from stdin. Exit if there is any error reading from stdin. Stop making a raw terminal to ask for registry login credentials. Allow to generate signals when termios is in raw mode. Use a more idiomatic syntax to capture the exit. Exit from `docker login` on SIGTERM and SIGINT.
Load the auth config before it's used.
|
@vieux I've synchronized my branch with master. Everything looks to still be working. I can rebase the branch if you want, but I rather not. I think history is important. Can you please take it a look? |
|
LGTM |
1 similar comment
|
LGTM |
Exit from `docker login` on SIGTERM and SIGINT.
This should allow to capture an interruption signal and exit when the login command is waiting for the credentials.
I'm completely new to docker's source code, so this might be incomplete. I'd really appreciate some 👀 on it.
Fixes #1299.