Create nspawn console tty in the child by fbuihuu · Pull Request #12758 · systemd/systemd

fbuihuu · 2019-06-07T10:04:09Z

No description provided.

keszybz

Looks very nice. I'll leave it open for a bit though to let other people review if they wish to.

keszybz · 2019-06-13T13:30:35Z

src/basic/fs-util.c


        if (do_chmod)
-                if (chmod(fd_path, mode & 07777) < 0)
+                if (fchmod_opath(fd, mode & 07777) < 0)


Instead of doing the xsprintf once, now we'll do it twice (sometimes). The compiler might be able to optimize it away, because chmod takes a const*. OK, seems reasonable to do this like this.

src/basic/terminal-util.c

fbuihuu · 2019-06-13T15:27:01Z

Thanks for the review @keszybz, I addressed your comment and force pushed a new version.

keszybz · 2019-06-13T15:40:58Z

Hmm, https://www.freedesktop.org/wiki/Software/systemd/ContainerInterface/#executionenvironment says

bind mount some suitable TTY to /dev/console

So those docs would need adjusting.

fbuihuu · 2019-06-14T06:27:55Z

Indeed although bind mounting a tty is still a possible solution for setting up /dev/console. Maybe a more generic wording would be better, something like "make some suitable TTY available through /dev/console" ?

poettering

looks pretty good, some comments though

poettering · 2019-06-17T17:57:04Z

src/basic/terminal-util.c

+        if (unlockpt(fd) < 0)
+                return -errno;
+
+        *master = TAKE_FD(fd);


why not just return this directly? i.e. do return TAKE_FD(fd); here? no need for the additional return param

poettering · 2019-06-17T17:58:15Z

src/basic/terminal-util.c

 }

-int ptsname_namespace(int pty, char **ret) {
+int openpt_allocate(int flags, int *master, char **console) {


why call the return param console here? that name mostly makes sense in the nspawn usecase, but i figure this one should be generic, hence maybe ret_slave?

poettering · 2019-06-17T18:03:24Z

src/basic/terminal-util.c


-int openpt_in_namespace(pid_t pid, int flags) {
-        _cleanup_close_ int pidnsfd = -1, mntnsfd = -1, usernsfd = -1, rootfd = -1;
+int openpt_allocate_in_namespace(pid_t pid, int flags, int *master, char **console) {


why return the fd as parameter instead of as return value like before?

also, as above, console → ret_slave?

poettering · 2019-06-17T18:03:59Z

src/machine/machine.c

 }

-int machine_openpt(Machine *m, int flags) {
+int machine_openpt(Machine *m, int flags, int *master, char **console) {


poettering · 2019-06-17T18:06:11Z

src/nspawn/nspawn.c

-static int setup_dev_console(const char *dest, const char *console) {
-        _cleanup_umask_ mode_t u;
-        const char *to;
+static int setup_stdio(const char *console) {


the parameter is always "/dev/console", no? hence should be good to just drop it and use the literal string in this function

fstat(2) is fine with O_PATH fds. For changing owership of a file opened with O_PATH, there's fchownat(2). Only changing permissions is problematic but we introduced fchmod_opath() for that purpose.

No functional changes.

…nd_one_fd()

The console tty is now allocated from within the container so it's not necessary anymore to allocate it from the host and bind mount the pty slave into the container. The pty master is sent to the host. /dev/console is now a symlink pointing to the pty slave. This might also be less confusing for applications running inside the container and the overall result looks cleaner (we don't need to apply manually the passed selinux context, if any, to the allocated pty for instance).

fbuihuu · 2019-06-18T06:20:35Z

Thanks for the feedback @poettering , I force pushed a new version addressing your comments.

lgtm-com · 2019-06-18T06:50:17Z

This pull request introduces 2 alerts when merging 31532b5 into b337d89 - view on LGTM.com

new alerts:

2 for Comparison result is always the same

Allocating a pty is done in a couple of places so let's introduce a new helper which does the job. Also the new function, as well as openpt_in_namespace(), returns both pty master and slave so the callers don't need to know about the pty slave allocation details. For the same reasons machine_openpt() prototype has also been changed to return both pty master and slave so callers don't need to allocate a pty slave which might be in a different namespace. Finally openpt_in_namespace() has been renamed into openpt_allocate_in_namespace().

poettering · 2019-06-18T07:57:00Z

lgtm

fbuihuu added the nspawn label Jun 7, 2019

This comment has been minimized.

Sign in to view

fbuihuu force-pushed the nspawn-console-tty branch 2 times, most recently from 73fc5ad to 65f5747 Compare June 11, 2019 07:08

keszybz reviewed Jun 13, 2019

View reviewed changes

fbuihuu force-pushed the nspawn-console-tty branch from 65f5747 to a8b088c Compare June 13, 2019 15:25

keszybz requested a review from poettering June 13, 2019 15:41

keszybz changed the title ~~Nspawn console tty~~ Create nspawn console tty in the child Jun 15, 2019

poettering requested changes Jun 17, 2019

View reviewed changes

poettering added the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Jun 17, 2019

fbuihuu added 4 commits June 18, 2019 07:54

fs-util: no need for fchmod_and_chown() to access /proc/self/fd directly

71ec74d

fstat(2) is fine with O_PATH fds. For changing owership of a file opened with O_PATH, there's fchownat(2). Only changing permissions is problematic but we introduced fchmod_opath() for that purpose.

namespace-util: make use of TAKE_FD()

04c84cd

No functional changes.

nspawn: use correct error variable when logging errors returned by se…

ba72801

…nd_one_fd()

fbuihuu force-pushed the nspawn-console-tty branch from a8b088c to 31532b5 Compare June 18, 2019 06:18

fbuihuu removed the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Jun 18, 2019

fbuihuu force-pushed the nspawn-console-tty branch from 31532b5 to 7f10988 Compare June 18, 2019 07:24

fbuihuu added 2 commits June 18, 2019 09:26

nspawn: make use of openpt_allocate()

dc98cae

fbuihuu force-pushed the nspawn-console-tty branch from 7f10988 to dc98cae Compare June 18, 2019 07:27

poettering added the good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed label Jun 18, 2019

poettering merged commit 59da647 into systemd:master Jun 18, 2019

fbuihuu deleted the nspawn-console-tty branch June 18, 2019 12:23

filbranden mentioned this pull request Oct 8, 2019

test: Make TEST-40 less flaky #13719

Closed

Uh oh!

Conversation

fbuihuu commented Jun 7, 2019

Uh oh!

This comment has been minimized.

This comment has been minimized.

keszybz left a comment

Choose a reason for hiding this comment

Uh oh!

keszybz Jun 13, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fbuihuu commented Jun 13, 2019

Uh oh!

keszybz commented Jun 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fbuihuu commented Jun 14, 2019

Uh oh!

poettering left a comment

Choose a reason for hiding this comment

Uh oh!

poettering Jun 17, 2019

Choose a reason for hiding this comment

Uh oh!

poettering Jun 17, 2019

Choose a reason for hiding this comment

Uh oh!

poettering Jun 17, 2019

Choose a reason for hiding this comment

Uh oh!

poettering Jun 17, 2019

Choose a reason for hiding this comment

Uh oh!

poettering Jun 17, 2019

Choose a reason for hiding this comment

Uh oh!

fbuihuu commented Jun 18, 2019

Uh oh!

lgtm-com bot commented Jun 18, 2019

Uh oh!

poettering commented Jun 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

keszybz commented Jun 13, 2019 •

edited

Loading