[LibOS,PAL] Refactor LibOS's /dev/tty and PAL's corresponding dev:tty
#1563
Conversation
There was a problem hiding this comment.
Reviewable status: 0 of 32 files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel)
a discussion (no related file):
@llly Since you touched this subsystem, can you please review this PR?
a discussion (no related file):
Note: I chose to call the new PAL handle console for two reasons:
- This term seems to be more universal (UNIX/Linux prefers
terminalortty, Windows prefersconsole, VMMs preferconsole) - When we'll have VM-based PALs, they will use the
consoledevice (that's how hypervisors typically call it). E.g.virtio-console.
I also kept the /dev/tty device and the initialization of stdin/stdout/stderr from it, because that's what UNIXes do. So I follow their logic in LibOS. (At the PAL level, we actually don't tie to host /dev/tty, but instead re-use 0,1,2 FDs.)
libos/src/sys/libos_ioctl.c line 64 at r1 (raw file):
switch (cmd) { case TIOCGPGRP: if (!hdl->uri || strcmp(hdl->uri, URI_PREFIX_CONSOLE)) {
I couldn't figure out a better way to have this check (TIOCGPGRP must work only on the handles referring to the current TTY, i.e. stdin/stdout/stderr).
libos/src/sys/libos_poll.c line 123 at r1 (raw file):
* a callback, because such handles have two layers of poll indirection (first the * "pseudo" poll callback, then the actual handle callback). In such case we let PAL * do the actual polling.
Yep, this is still ugly, but that's because we have two levels of indirection in our FS code...
pal/src/pal_streams.c line 41 at r1 (raw file):
/* parse_stream_uri scan the uri, seperate prefix and search for stream handler which will open or access the stream */ static int parse_stream_uri(const char** uri, char** prefix, struct handle_ops** ops) {
By the way, this is a ridiculous implementation. If anyone wants to, please refactor it to a more sane and short code.
There was a problem hiding this comment.
Reviewed 32 of 32 files at r1, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv)
libos/src/sys/libos_poll.c line 140 at r1 (raw file):
continue; use_pal_polling:;
Label at the end of code block looks wired. Is it intended in order to get rid of some warning?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 6 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @llly)
libos/src/sys/libos_poll.c line 140 at r1 (raw file):
Previously, llly (Li Xun) wrote…
Label at the end of code block looks wired. Is it intended in order to get rid of some warning?
This is just to skip the middle part of this code block, nothing else.
The logic in this code is like this:
for (each handle) {
if (handle has a poll callback) {
handle->poll();
if (poll failed) {
jump out of this if code block; // this is the `goto` line
} else {
do some handle-specific stuff;
jump to next handle; // this is the `continue` line
}
}
}
I didn't want to modify already-existing control flow with the goto and the label. So I just kept them as-is, but only changed the names.
If you think there's a more readable way to describe this logic, I can change it.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 6 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv)
libos/src/sys/libos_poll.c line 140 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This is just to skip the middle part of this code block, nothing else.
The logic in this code is like this:
for (each handle) { if (handle has a poll callback) { handle->poll(); if (poll failed) { jump out of this if code block; // this is the `goto` line } else { do some handle-specific stuff; jump to next handle; // this is the `continue` line } } }I didn't want to modify already-existing control flow with the goto and the label. So I just kept them as-is, but only changed the names.
If you think there's a more readable way to describe this logic, I can change it.
}
use_pal_polling:;
When I jump to the label, I need roll up to check the following } is the end of if or for to determine next sentence.
Code snippet:
use_pal_polling:;
}
There was a problem hiding this comment.
Reviewable status: 31 of 32 files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @llly)
libos/src/sys/libos_poll.c line 140 at r1 (raw file):
Previously, llly (Li Xun) wrote…
}
use_pal_polling:;When I jump to the label, I need roll up to check the following
}is the end ofiforforto determine next sentence.
Done, what about now?
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners
libos/src/sys/libos_poll.c line 140 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Done, what about now?
LGTM.
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
-- commits line 4 at r2:
Hmm, no? They are whatever the shell passes there, which can be also e.g. a pipe or a regular file.
-- commits line 7 at r2:
On Unixes libc doesn't open /dev/tty, just inherits FDs 0,1,2 from the parent.
There was a problem hiding this comment.
Reviewed 12 of 32 files at r1.
Reviewable status: all files reviewed, 11 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
pal/include/pal/pal.h line 348 at r2 (raw file):
* #PAL_CREATE_ALWAYS is given in `create` flags, the file/directory will be created. * * `dev:...`: Open a device as a stream. * * `console:`: Open a console (stdin/stdout/stderr) as a stream.
Suggestion:
(host process stdin/stdout/stderr) as a streampal/src/host/linux/pal_console.c line 5 at r2 (raw file):
/* * Operations to handle the console (stdin/stdout/stderr). Note that some operations (like stat and
I don't like this merging of these terms. std{in,out,err} may be unrelated to any console.
Code quote:
the console (stdin/stdout/stderr)pal/src/host/linux/pal_console.c line 38 at r2 (raw file):
hdl->hdr.type = PAL_TYPE_CONSOLE; hdl->flags = access == PAL_ACCESS_RDONLY ? PAL_HANDLE_FD_READABLE : PAL_HANDLE_FD_WRITABLE; hdl->console.fd = access == PAL_ACCESS_RDONLY ? /*host stdin*/0 : /*host stdout*/1;
We should really finally split these handle opening functions into specialized ones, we keep abusing this interface for opening regular files over and over...
pal/src/host/linux/pal_devices.c line 27 at r2 (raw file):
int ret; if (strcmp(type, URI_TYPE_DEV))
Maybe an assert then?
pal/src/host/linux/pal_streams.c line 57 at r2 (raw file):
break; case PAL_TYPE_CONSOLE: /* console (stdin/stdout/stderr) has no fields to serialize */
What about the state whether the handle was already closed? Shouldn't be writable afterwards, seems that now migration re-opens the handle?
There was a problem hiding this comment.
I'm very confused by the naming and the explanations here (see my review comments), but in terms of what it does, I think this is a great change :)
Reviewed 19 of 32 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 12 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
libos/src/sys/libos_poll.c line 123 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Yep, this is still ugly, but that's because we have two levels of indirection in our FS code...
What's different in console: handles from others which also exist on the host (e.g. pipes or normal files) that they need this special treatment?
libos/test/regression/console.c line 31 at r2 (raw file):
int main(void) { /* initialization -- write some messages and save stdout/stderr (for further restore) */ ssize_t x = CHECK(write(STDOUT_FILENO, FIRST_HELLO_STDOUT, sizeof(FIRST_HELLO_STDOUT)));
Not strlen()? We rather don't want to print the null byte to the terminal.
ditto below
Code quote:
sizeof(FIRST_HELLO_STDOUT)pal/src/host/linux/pal_devices.c line 27 at r2 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Maybe an assert then?
(ditto for all these comments to SGX PAL)
pal/src/host/linux-sgx/pal_devices.c line 201 at r2 (raw file):
} /* this dummy function is implemented to support opening TTY devices with O_TRUNC flag */
I already managed to forget how bad this whole thing was...
There was a problem hiding this comment.
Reviewable status: all files reviewed, 11 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @mkow)
Previously, mkow (Michał Kowalczyk) wrote…
Hmm, no? They are whatever the shell passes there, which can be also e.g. a pipe or a regular file.
Let me expand on my reasoning.
From the point of view of the applications running inside Gramine:
- Gramine environment can be considered a VM.
- Gramine itself can be considered a Linux kernel.
- Gramine's first process (aka main process aka master process) performs initialization steps that can be considered similar to what
init(and its helper utilities) do in Linux.
From this perspective, Gramine initialization must start with finding devices (like the teletypewriter aka TTY), building abstractions on top of these devices, and passing control to the user programs.
What does init do with respect to initializing stdin/stdout/stderr (aka FDs 0, 1, 2), which then will be inherited by the init child processes? The following:
- Find all TTY devices and create
/dev/ttyXXXpseudo-files corresponding to these devices - Call
gettyutility - The
gettyutility opens each/dev/ttyXXXpseudo-file and connects its read end to stdin (FD 0), and its write ends to stdout + stderr (FDs 1 and 2). - This abstraction/association will be used by all children of
init(I'm fuzzy on howgettysends back toinitthis association).
So Gramine's initialization logic must do a similar thing. In Gramine, we don't have special init and getty processes, so Gramine's LibOS code does this itself. Also, we don't have multiple TTYs, and we don't add the complexity of pseudo TTYs (PTYs), so we just set up a single /dev/tty device and associate FDs 0,1,2 with it.
A bunch of references:
- https://stackoverflow.com/questions/63394927/what-does-the-tty-do-when-a-process-asks-for-input-or-outputs-to-the-screen
- https://unix.stackexchange.com/questions/4126/what-is-the-exact-difference-between-a-terminal-a-shell-a-tty-and-a-con
- https://www.linusakesson.net/programming/tty/ (a great article, recommend)
- https://tldp.org/LDP/intro-linux/html/sect_04_02.html
- https://tldp.org/HOWTO/Text-Terminal-HOWTO-15.html
I hope I didn't screw up too much in my descriptions :)
P.S. If I understand correctly, the above description works both all UNIX-based operating systems. (That's why I say UNIX in the commit message instead of Linux.) I may be wrong here, but I read the same docs about OpenBSD, so I assume that it's the same for all popular UNIX-based OSes.
Previously, mkow (Michał Kowalczyk) wrote…
On Unixes libc doesn't open
/dev/tty, just inherits FDs 0,1,2 from the parent.
But Gramine is not libc? Gramine's initialization code is akin to the init process in Linux. Gramine prepares FDs 0,1,2 for the use by the app's first process (which contains libc). Gramine also prepares the /dev/tty pseudo-file, because some applications may want to use it directly.
libos/src/sys/libos_poll.c line 123 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
What's different in
console:handles from others which also exist on the host (e.g. pipes or normal files) that they need this special treatment?
The console: handle:
- is backed by the pseudo-file
/dev/tty, which is located in thepseudoFS - thus,
handle->fspoints topseudo_builtin_fsstruct - this struct has a generic
poll()callback - This generic
poll()callback can call a pseudo-file-type-specific callback, e.g. forPSEUDO_MEMfiles it looks like this. - In this PR, I add a similar logic with a "pseudo-file-type-specific callback" for
PSEUDO_DEVfiles, to accommodate for my/dev/ttylogic.
So you can see that there are two levels of indirection, and a simple check if (handle->fs->fs_ops->poll) is not enough. We need to actually call the first level of a poll callback to learn whether the second level of a poll callback exists.
Compare with the pipe: handle:
- is backed by the
pipeFS directly - This
pipeFS struct doesn't have apollcallback - Thus, a simple check
if (handle->fs->fs_ops->poll)is enough.
Could we modify console: to be its own FS type? Then we'd have just one level of indirection, and the if (...->poll) check would be good enough. Yes, we could do this, but I'm not sure it will be cleaner...
There was a problem hiding this comment.
Reviewable status: all files reviewed, 11 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Let me expand on my reasoning.
From the point of view of the applications running inside Gramine:
- Gramine environment can be considered a VM.
- Gramine itself can be considered a Linux kernel.
- Gramine's first process (aka main process aka master process) performs initialization steps that can be considered similar to what
init(and its helper utilities) do in Linux.From this perspective, Gramine initialization must start with finding devices (like the teletypewriter aka TTY), building abstractions on top of these devices, and passing control to the user programs.
What does
initdo with respect to initializing stdin/stdout/stderr (aka FDs 0, 1, 2), which then will be inherited by theinitchild processes? The following:
- Find all TTY devices and create
/dev/ttyXXXpseudo-files corresponding to these devices- Call
gettyutility- The
gettyutility opens each/dev/ttyXXXpseudo-file and connects its read end to stdin (FD 0), and its write ends to stdout + stderr (FDs 1 and 2).- This abstraction/association will be used by all children of
init(I'm fuzzy on howgettysends back toinitthis association).So Gramine's initialization logic must do a similar thing. In Gramine, we don't have special
initandgettyprocesses, so Gramine's LibOS code does this itself. Also, we don't have multiple TTYs, and we don't add the complexity of pseudo TTYs (PTYs), so we just set up a single/dev/ttydevice and associate FDs 0,1,2 with it.A bunch of references:
- https://stackoverflow.com/questions/63394927/what-does-the-tty-do-when-a-process-asks-for-input-or-outputs-to-the-screen
- https://unix.stackexchange.com/questions/4126/what-is-the-exact-difference-between-a-terminal-a-shell-a-tty-and-a-con
- https://www.linusakesson.net/programming/tty/ (a great article, recommend)
- https://tldp.org/LDP/intro-linux/html/sect_04_02.html
- https://tldp.org/HOWTO/Text-Terminal-HOWTO-15.html
I hope I didn't screw up too much in my descriptions :)
P.S. If I understand correctly, the above description works both all UNIX-based operating systems. (That's why I say
UNIXin the commit message instead ofLinux.) I may be wrong here, but I read the same docs about OpenBSD, so I assume that it's the same for all popular UNIX-based OSes.
Yes, that I agree with, but this is true only for init and this sentence seem to speak in general. Also, regarding the rest of this PR - these statements are not true in children processes.
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
But Gramine is not libc? Gramine's initialization code is akin to the
initprocess in Linux. Gramine prepares FDs 0,1,2 for the use by the app's first process (which contains libc). Gramine also prepares the/dev/ttypseudo-file, because some applications may want to use it directly.
I thought this sentence describes how this works in each process, while it's only valid for init. Please make it clear that this describes only how the initial process in Unix behaves and how this relates to Gramine.
libos/src/sys/libos_poll.c line 123 at r1 (raw file):
We need to actually call the first level of a
pollcallback to learn whether the second level of apollcallback exists.
Ahhh, that's how it works. The control flow here is quite obfuscated, I didn't notice that it ends up in the same code path.
I'm fine with how we check whether that nested poll() callback exists, I'm just confused by the control flow here. Any idea how to make it more readable?
There was a problem hiding this comment.
Reviewable status: 25 of 32 files reviewed, 11 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "squash! " found in commit messages' one-liners (waiting on @llly and @mkow)
these statements are not true in children processes
Nothing happens in the children processes, because of this check:
gramine/libos/src/bookkeep/libos_handle.c
Line 155 in 2ad54dd
I.e., we don't modify/init anything if the handles array was already inited (which is true for children processes).
Previously, mkow (Michał Kowalczyk) wrote…
I thought this sentence describes how this works in each process, while it's only valid for
init. Please make it clear that this describes only how the initial process in Unix behaves and how this relates to Gramine.
Done. Check the new commit message.
libos/src/sys/libos_poll.c line 123 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
We need to actually call the first level of a
pollcallback to learn whether the second level of apollcallback exists.Ahhh, that's how it works. The control flow here is quite obfuscated, I didn't notice that it ends up in the same code path.
I'm fine with how we check whether that nested
poll()callback exists, I'm just confused by the control flow here. Any idea how to make it more readable?
Done. What about this refactoring?
libos/test/regression/console.c line 31 at r2 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Not
strlen()? We rather don't want to print the null byte to the terminal.
ditto below
Done.
pal/include/pal/pal.h line 348 at r2 (raw file):
* #PAL_CREATE_ALWAYS is given in `create` flags, the file/directory will be created. * * `dev:...`: Open a device as a stream. * * `console:`: Open a console (stdin/stdout/stderr) as a stream.
Done.
By the way, I just realized that this description applies only to process-based PAL of Gramine. For example, a VM-based PAL won't have stdin/stdout/stderr streams; instead such PAL would have e.g. a virtio-console driver. Do we want to generalize the description, or it will only complicate matters, and we should stick to the current process-based PALs?
pal/src/host/linux/pal_console.c line 5 at r2 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
I don't like this merging of these terms. std{in,out,err} may be unrelated to any console.
Done. I rewrote the description, does it make more sense now?
pal/src/host/linux/pal_console.c line 38 at r2 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
We should really finally split these handle opening functions into specialized ones, we keep abusing this interface for opening regular files over and over...
Yep.
pal/src/host/linux/pal_devices.c line 27 at r2 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
(ditto for all these comments to SGX PAL)
Ok, though I find this particular check (on the type of the PAL handle) redundant these days, as I can't imagine how Gramine developers could abuse/mix it.
UPDATE: Actually, in dev_open(), I re-instated the IF check, as it makes some sense to verify during the opening of the object. In other functions, I used the assert.
pal/src/host/linux/pal_streams.c line 57 at r2 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
What about the state whether the handle was already closed? Shouldn't be writable afterwards, seems that now migration re-opens the handle?
Sorry, I'm not sure about which exact case you talk?
Note that PAL objects of type PAL_TYPE_CONSOLE have the fd field, and this field is marked as PAL_IDX_POISON during PAL-object close. Thus, any write() afterwards will return -PAL_ERROR_DENIED because the host fd is not real now.
Also note that PAL object checkpoint-and-restore logic serializes all the natively-typed fields of the object. In particular, pal_handle::console.fd is serialized and sent as an integer value to the child process (enclave), and the child deserializes this field. So in the end, the child will also have fd = PAL_IDX_POISON, and write() in the child will also return -PAL_ERROR_DENIED.
To see this logic, scroll a bit down in this file, and you'll see /* copy into buffer all handle fields and then serialized fields */ code snippet.
There was a problem hiding this comment.
Reviewed 7 of 7 files at r3, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "squash! " found in commit messages' one-liners (waiting on @dimakuv)
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
these statements are not true in children processes
Nothing happens in the children processes, because of this check:
gramine/libos/src/bookkeep/libos_handle.c
Line 155 in 2ad54dd
I.e., we don't modify/init anything if the handles array was already inited (which is true for children processes).
Looks good now.
-- commits line 51 at r3:
They may not inherit all of 0,1,2, some may be closed.
Suggestion:
inherit all the FDs
Suggestion:
parent process.
libos/src/sys/libos_poll.c line 123 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Done. What about this refactoring?
Looks good now, thanks!
libos/src/sys/libos_poll.c line 149 at r3 (raw file):
} PAL_HANDLE pal_handle;
Now that we don't have that "use_pal_polling" label here maybe we should add a comment that this code path is for PAL polling?
pal/include/pal/pal.h line 348 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Done.
By the way, I just realized that this description applies only to process-based PAL of Gramine. For example, a VM-based PAL won't have stdin/stdout/stderr streams; instead such PAL would have e.g. a
virtio-consoledriver. Do we want to generalize the description, or it will only complicate matters, and we should stick to the current process-based PALs?
Yes, that's also a part of my disliking of the current descriptions in this PR :) I'd generalize it more. Gramine just maps first process' 0/1/2 descriptors to some "log output" of Gramine, which may depend on the PAL.
pal/src/host/linux/pal_console.c line 5 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Done. I rewrote the description, does it make more sense now?
Yup, it's better now.
pal/src/host/linux/pal_streams.c line 57 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Sorry, I'm not sure about which exact case you talk?
Note that PAL objects of type
PAL_TYPE_CONSOLEhave thefdfield, and this field is marked asPAL_IDX_POISONduring PAL-object close. Thus, anywrite()afterwards will return-PAL_ERROR_DENIEDbecause the host fd is not real now.Also note that PAL object checkpoint-and-restore logic serializes all the natively-typed fields of the object. In particular,
pal_handle::console.fdis serialized and sent as an integer value to the child process (enclave), and the child deserializes this field. So in the end, the child will also havefd = PAL_IDX_POISON, andwrite()in the child will also return-PAL_ERROR_DENIED.To see this logic, scroll a bit down in this file, and you'll see
/* copy into buffer all handle fields and then serialized fields */code snippet.
Yup, I missed that the native fields are copied normally, didn't look at this code in a while. Looks good then.
There was a problem hiding this comment.
Reviewable status: 29 of 32 files reviewed, 7 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @llly, and @mkow)
Previously, mkow (Michał Kowalczyk) wrote…
They may not inherit all of 0,1,2, some may be closed.
I'll fix during final rebase
-- commits line 52 at r3:
I'll fix during final rebase
libos/src/sys/libos_poll.c line 149 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Now that we don't have that "use_pal_polling" label here maybe we should add a comment that this code path is for PAL polling?
Done.
The path is not exactly for PAL polling (this sounds misleading), but to add the handle to the array of handles to poll in PAL.
The actual polling happens outside of this large for-loop, see PalStreamsWaitEvents() invocation about 30 lines below.
pal/include/pal/pal.h line 348 at r2 (raw file):
Done. Hopefully this reads better.
to some "log output" of Gramine
I don't like your log output, this is misleading. It's not only output, it's only the input channel. Also, it's not about the log (the Gramine log), but about whatever output the application is sending.
There was a problem hiding this comment.
Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
libos/src/sys/libos_poll.c line 149 at r4 (raw file):
} /* below code path adds the handle for PAL polling */
Suggestion:
/* add the handle for PAL polling */pal/include/pal/pal.h line 348 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Done. Hopefully this reads better.
to some "log output" of Gramine
I don't like your
log output, this is misleading. It's not only output, it's only the input channel. Also, it's not about the log (the Gramine log), but about whatever output the application is sending.
Yeah, now it's clear :)
There was a problem hiding this comment.
Reviewed 24 of 32 files at r1, 5 of 7 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
libos/src/fs/dev/fs.c line 105 at r4 (raw file):
} /* this dummy function is implemented to support opening TTY (console) with O_TRUNC flag */
But shouldn't we ignore O_TRUNC flag for such case?
Code quote:
/* this dummy function is implemented to support opening TTY (console) with O_TRUNC flag */libos/test/regression/console.c line 2 at r4 (raw file):
/* SPDX-License-Identifier: LGPL-3.0-or-later */ /* Copyright (C) 2023 Intel Corporation */
pls add an empty line between the copyright and the includes
There was a problem hiding this comment.
Reviewable status: 30 of 32 files reviewed, 7 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @kailun-qin and @mkow)
libos/src/fs/dev/fs.c line 105 at r4 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
But shouldn't we ignore
O_TRUNCflag for such case?
@kailun-qin Sorry, I don't understand your comment. Let me give some context:
LibOS generic FS code, when it sees open(..., O_TRUNC), will call the file-specific truncate() callback. This is described in our headers, and this is implemented in our C code:
gramine/libos/include/libos_fs.h
Line 687 in 2ad54dd
gramine/libos/src/fs/libos_namei.c
Lines 408 to 419 in 2ad54dd
And because /dev/tty is represented as a pseudo-file, the above logic applies to this file as well. So the generic FS code will try to find the truncate() callback, and if won't find this callback, the generic FS code fails with -EINVAL (see C code above). We want to prevent this failure, but we also don't need to do anything in the truncate callback. That's the whole explanation.
libos/src/sys/libos_poll.c line 149 at r4 (raw file):
} /* below code path adds the handle for PAL polling */
Done.
libos/test/regression/console.c line 2 at r4 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
pls add an empty line between the copyright and the includes
Done.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r5.
Reviewable status: all files reviewed, 6 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)
libos/src/fs/dev/fs.c line 105 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
@kailun-qin Sorry, I don't understand your comment. Let me give some context:
LibOS generic FS code, when it sees
open(..., O_TRUNC), will call the file-specifictruncate()callback. This is described in our headers, and this is implemented in our C code:
gramine/libos/include/libos_fs.h
Line 687 in 2ad54dd
gramine/libos/src/fs/libos_namei.c
Lines 408 to 419 in 2ad54dd
And because
/dev/ttyis represented as a pseudo-file, the above logic applies to this file as well. So the generic FS code will try to find thetruncate()callback, and if won't find this callback, the generic FS code fails with-EINVAL(see C code above). We want to prevent this failure, but we also don't need to do anything in the truncate callback. That's the whole explanation.
Yeah, I understand the flow. But from the manpage: https://man7.org/linux/man-pages/man2/open.2.html -- O_TRUNC: ...If the file is a FIFO or terminal device file, the O_TRUNC flag is ignored.?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 6 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @kailun-qin and @mkow)
libos/src/fs/dev/fs.c line 105 at r4 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
Yeah, I understand the flow. But from the manpage: https://man7.org/linux/man-pages/man2/open.2.html --
O_TRUNC: ...If the file is a FIFO or terminal device file, the O_TRUNC flag is ignored.?
Yep, but that's what we do in this dev_tty_truncate() callback? We just ignore the truncation request, which is the same as ignoring the O_TRUNC flag.
Or do you think that this ignore-O_TRUNC logic should be in libos_namei.c file? It can't be there because at that point we don't know which file we're working on -- there's no convenient way to determine that the current file is a "terminal device file".
So we move the logic into this callback, where we simply ignore.
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)
libos/src/fs/dev/fs.c line 105 at r4 (raw file):
Yep, but that's what we do in this
dev_tty_truncate()callback? We just ignore the truncation request, which is the same as ignoring theO_TRUNCflag.
Yes, it's functionally equivalent for this open(terminal device file, O_TRUNC) case. But what if we directly call truncate(, 0)/ftruncate(, 0) on the terminal device file (descriptor)? Then Gramine would return success whereas Linux would return failure I guess.
Or do you think that this ignore-O_TRUNC logic should be in
libos_namei.cfile?
Yes this was what I thought.
It can't be there because at that point we don't know which file we're working on -- there's no convenient way to determine that the current file is a "terminal device file".
Ok, then. Maybe we can at least note somewhere if the above stated behavior is indeed inconsistent w/ Linux?
|
Compile this file and run natively and under Gramine: Native run correctly fails
Let me fix this... |
There was a problem hiding this comment.
Reviewable status: 30 of 33 files reviewed, 4 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @kailun-qin, @llly, and @mkow)
libos/src/fs/libos_fs_pseudo.c line 439 at r6 (raw file):
case PSEUDO_DEV: if (!node->dev.dev_ops.truncate) return -EINVAL;
FYI: Linux returns EINVAL on ftruncate on non regular files, see man 2 ftruncate
libos/src/fs/libos_namei.c line 410 at r6 (raw file):
/* truncate regular writable file if O_TRUNC is given */ if ((flags & O_TRUNC) && ((flags & O_RDWR) | (flags & O_WRONLY)) && (dent->inode->type != S_IFCHR)
According to man 2 open, O_TRUNC is only applicable to regular files. So I ignore it for devices too now.
libos/src/fs/dev/fs.c line 105 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
@kailun-qin is right. I just did a quick test:
#include <errno.h> #include <fcntl.h> #include <stdio.h> #include <sys/stat.h> #include <sys/types.h> #include <unistd.h> int main(void) { int tty = open("/dev/tty", O_WRONLY | O_CLOEXEC); write(tty, "Hi to /dev/tty!\n", sizeof("Hi to /dev/tty!\n")); int ret = ftruncate(tty, /*length=*/0); printf("ftruncate returns %d (errno = %d)\n", ret, errno); close(tty); return 0; }Compile this file and run natively and under Gramine:
$ ./helloworld Hello, world Hi to /dev/tty! ftruncate returns -1 (errno = 22) $ gramine-direct helloworld Hi to /dev/tty! Hello, world ftruncate returns 0 (errno = 0)Native run correctly fails
ftruncate()with -22 which isEINVAL. As the man page says:
EINVAL fd does not reference a regular file or a POSIX shared memory object.Let me fix this...
Done.
Now the correct behavior is restored (same test I mention in the above comment):
$ ./helloworld
Hi to /dev/tty!
ftruncate returns -1 (errno = 22)
$ gramine-direct helloworld
Hi to /dev/tty!
ftruncate returns -1 (errno = 22)
$ gramine-sgx helloworld
Hi to /dev/tty!
ftruncate returns -1 (errno = 22)
Thanks @kailun-qin for noticing this subtle difference.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r5, 3 of 3 files at r6, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @kailun-qin)
libos/src/fs/libos_namei.c line 410 at r6 (raw file):
only applicable to regular files
Then why not do an "if type == regular" check instead of excluding all other types?
There was a problem hiding this comment.
Reviewable status: 32 of 33 files reviewed, 4 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @kailun-qin and @mkow)
libos/src/fs/libos_namei.c line 410 at r6 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
only applicable to regular files
Then why not do an "if type == regular" check instead of excluding all other types?
Done. Heh, true. I don't know why Pawel wrote it like that at the time (~2 years ago).
There was a problem hiding this comment.
Reviewable status: 32 of 33 files reviewed, 4 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @kailun-qin and @mkow)
libos/src/fs/libos_namei.c line 410 at r6 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Done. Heh, true. I don't know why Pawel wrote it like that at the time (~2 years ago).
There was a similar PR for FIFOs: #1401
There was a problem hiding this comment.
Reviewed 2 of 3 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)
There was a problem hiding this comment.
Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
…tty`
Gramine's LibOS initialization routine can be compared to the Linux's
`init` process. Therefore, similar to what's done by the `init` process
(or more specifically, by one of its utilities `getty`), Gramine
initializes the stdin, stdout and stderr streams to file descriptors
0, 1 and 2, opened on the `/dev/tty` device (controlling terminal):
- `stdin = open("/dev/tty", O_RDONLY)`
- `stdout = stderr = open("/dev/tty", O_WRONLY | O_APPEND)`
Note that this initialization routine happens only in the first (aka
main, aka master) Gramine process. Child processes will not open this
`/dev/tty` device; instead they will inherit all the FDs from the parent
process.
On the LibOS side, the previous implementation was confusing: it mounted
`/dev/tty` device file in a special way: not via Gramine's devfs, but
instead via chrootfs. On the PAL side, the `/dev/tty` file was
confusingly mapped to PAL's `dev:tty`, which was implemented via
`PAL_TYPE_DEV` but in a special way: all device APIs in `pal_devices.c`
had a special code path for `dev:tty` (which was also erroneously
checked by comparing the FD to 0 or 1, even though these host FDs could
be different in the child).
This commit refactors this mess:
- In LibOS, `/dev/tty` becomes a pseudo-file mounted via devfs.
- In PAL, `/dev/tty` maps to `console:`, which is a new type of PAL
handle `PAL_TYPE_CONSOLE`. This new type is removed from
`pal_devices.c` and instead implemented in a new `pal_console.c`.
New LibOS test `console` is added to test the tty/console.
Signed-off-by: Dmitrii Kuvaiskii <[email protected]>
dimakuv
force-pushed
the
dimakuv/rewrite-dev-tty
branch
from
5b6c9e9 to
c11f4c6
Compare
There was a problem hiding this comment.
Reviewable status: 32 of 33 files reviewed, 2 unresolved discussions (waiting on @kailun-qin, @llly, and @mkow)
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I'll fix during final rebase
Done.
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I'll fix during final rebase
Done.
There was a problem hiding this comment.
Reviewable status: 32 of 33 files reviewed, 2 unresolved discussions (waiting on @kailun-qin, @llly, and @mkow)
libos/src/fs/libos_namei.c line 410 at r6 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
There was a similar PR for FIFOs: #1401
I verified that this PR (the final rebased version) fixes the issue in #1401. So I will close that PR.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mkow)
There was a problem hiding this comment.
Reviewed 1 of 1 files at r8, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved
Description of the changes
In Gramine, similar to UNIX, the stdin, stdout and stderr streams are file descriptors on the
/dev/ttydevice (controlling terminal):stdin = open("/dev/tty", O_RDONLY)stdout = stderr = open("/dev/tty", O_WRONLY | O_APPEND)On the LibOS side, the previous implementation was confusing: it mounted
/dev/ttydevice file in a special way: not via Gramine's devfs, but instead via chrootfs.On the PAL side, the
/dev/ttyfile was confusingly mapped to PAL'sdev:tty, which was implemented viaPAL_TYPE_DEVbut in a special way: all device APIs inpal_devices.chad a special path fordev:tty(which was also erroneously checked by comparing the FD to 0 or 1, even though these host FDs could be different in the child).This commit refactors this mess:
/dev/ttybecomes a pseudo-file mounted via devfs./dev/ttymaps toconsole:, which is a new type of PAL handlePAL_TYPE_CONSOLE. This new type is removed frompal_devices.cand instead implemented in a newpal_console.c.Closes #1096.
Helpers:
poll/selecton fd0("dev:tty") graphene#2553 (this was fixed by Borys in his Poll Rework but in a hacky way, I modified this to be less hacky in this PR)FILE_DEVfiles graphene#2413 (just for info)How to test this PR?
All CI tests should work. Plus a new LibOS test
consoleis added to test the tty/console.This change is