fix: return tty size in container_inspect.go#28579
fix: return tty size in container_inspect.go#28579priyanshsao wants to merge 1 commit intocontainers:mainfrom
Conversation
In the output of podman inspect, there is a field for the container's TTY size (ConsoleSize), which Podman presently only returns [0,0]. This PR updates the tty size for containers which have terminal enabled and are in running state. For cases where there is an issue with getting the size or the container is not running, suitable warning is given, no update happens and default [0,0] is returned. Fixes: containers#28368 Signed-off-by: Priyansh Sao <saopriyansh06@gmail.com>
|
A simple video to see the change quickly podman_tty.mp4 |
danishprakash
left a comment
There was a problem hiding this comment.
Can you please also add a test case for this?
| } else { | ||
| defer f.Close() | ||
|
|
||
| height, width, err := xterm.GetSize(int(f.Fd())) |
There was a problem hiding this comment.
GetSize returns (width, height), so the size is flipped.
| procPath := fmt.Sprintf("/proc/%d/fd/0", c.state.PID) | ||
|
|
||
| f, err := os.Open(procPath) |
There was a problem hiding this comment.
That path doesn't exist on FreeBSD, so this would always fail. We could either guard this for Linux or even split this into *_freebsd.go. Bottom line, the behavior on FreeBSD won't change (podman will still return [0,0]), but there's going to be an additional warning, which is superfluous considering we know this won't work.
| f, err := os.Open(procPath) | ||
| if err != nil { | ||
| if c.state.State != define.ContainerStateRunning { | ||
| logrus.Warnf("container %q is not running, unable to get console size", c.ID()) |
There was a problem hiding this comment.
To me, this seems to be noise.
There was a problem hiding this comment.
Maybe debug or exit with an error?
| "go.podman.io/podman/v6/libpod/driver" | ||
| "go.podman.io/podman/v6/pkg/signal" | ||
| "go.podman.io/podman/v6/pkg/util" | ||
| xterm "golang.org/x/term" |
There was a problem hiding this comment.
| xterm "golang.org/x/term" | |
| "golang.org/x/term" |
| if c.state.State != define.ContainerStateRunning { | ||
| logrus.Warnf("container %q is not running, unable to get console size", c.ID()) | ||
| } else { | ||
| logrus.Warnf("unable to open %s", procPath) |
There was a problem hiding this comment.
The lack of context here might confuse more than it would help, consider:
| logrus.Warnf("unable to open %s", procPath) | |
| logrus.Debugf("unable to get console size for container %s: %v", c.ID(), err) |
| if c.Terminal() { | ||
| procPath := fmt.Sprintf("/proc/%d/fd/0", c.state.PID) | ||
|
|
||
| f, err := os.Open(procPath) |
There was a problem hiding this comment.
What if the container doesn't run, and some other process gets that PID?
| f, err := os.Open(procPath) | ||
| if err != nil { | ||
| if c.state.State != define.ContainerStateRunning { | ||
| logrus.Warnf("container %q is not running, unable to get console size", c.ID()) |
There was a problem hiding this comment.
Maybe debug or exit with an error?
In the output of podman inspect, there is a field for the container's TTY size (ConsoleSize), which Podman presently only returns [0,0].
This PR updates the tty size for containers which have terminal enabled and are in running state.
For cases where there is an issue with getting the size or the container is not running, suitable warning is given, no update happens and default [0,0] is returned.
Fixes: #28368
Checklist
Ensure you have completed the following checklist for your pull request to be reviewed:
commits. (
git commit -s). (If needed, usegit commit -s --amend). The author email must matchthe sign-off email address. See CONTRIBUTING.md
for more information.
Fixes: #00000in commit message (if applicable)make validatepr(format/lint checks)Noneif no user-facing changes)Does this PR introduce a user-facing change?