-
Notifications
You must be signed in to change notification settings - Fork 344
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.
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
Fixed ENAMETOOLONG error in setup_console_socket #2915
Conversation
Signed-off-by: morganllewellynjones <morganjones.gm@gmail.com>
Hey, thanks for helping us with this! Unfortunately I'm afraid we'll need some time to get to reviewing this PR, as there are already several others in the backlog, and the maintainers are a bit busy at the moment. I'll ping you if any changes are needed when I get to checking this, but as the diff is small, this should be ok in general. Also, is #2914 duplicate of this? Can you close that? Thanks! |
No worries! The duplicate has been closed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your first contribution. Your detailed explanation and concise explanation helped me to review this PR.
LGTM with nits.
Co-authored-by: Toru Komatsu <k0ma@utam0k.jp>
Hey, the DCO is failing probably due to directly committing the code from the suggestion. Given that the original suggestion was by utam0k and you have committed it directly from github, I don't think you have any problem with signing DCO. I will sign the last commit myself and force push in sometime unless you want to do it yourself or have any problems. |
Manually setting DCO to pass and merging. |
In reference to the error found here: #2910
sun_path has a max length of 108 bytes: https://man7.org/linux/man-pages/man7/unix.7.html
And including the absolute path name can make the resulting address of the socket path too long for sun_path.
Quick solution, move to the container_dir first, that way the sun_path is always just as long as the name "console-socket" itself, and doesn't break depending on the length of the runtime filepath.
I also wrapped this update in code that saves the previous directory and moves back to it afterwords. I personally doubt this is necessary and may simply be wasted clock cycles. Since we tend to use absolute path names in general, both to navigate and to access specific links. Nevertheless, I didn't want to make that call. If repositioning the directory afterwards is not necessary that could certainly be cleaned out. Cargo test passes in either case.