Skip to content
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

Rewrite it in Rust #1

Merged
merged 1 commit into from
May 10, 2024
Merged

Rewrite it in Rust #1

merged 1 commit into from
May 10, 2024

Conversation

teohhanhui
Copy link
Collaborator

@teohhanhui teohhanhui commented May 7, 2024

Replaces slp/krun.legacy#4

As discussed in slp/krun.legacy#1 (comment)


Bugs fixed:

  • Current implementation of krun incorrectly passes own arguments to krun-guest, e.g.

    teohhanhui@Han-MacBook-Air:~/Projects/slp/krun$ ./krun --net=PASST box64 --help
    No IPv6 nameserver available for NDP/DHCPv6
    Couldn't execute '--net=PASST' inside the vm: No such file or directory
    
  • Stop using hardcoded /tmp/passt_1.socket

  • When forking krun-guest to run dhclient in child process, stdin/stdout/stderr were illegally closed.

    Usage of close() on file descriptors STDIN_FILENO, STDOUT_FILENO, or
    STDERR_FILENO should immediately be followed by an operation to reopen these
    file descriptors. Unexpected behavior will result if any of these file
    descriptors is left in a closed state

    See https://pubs.opengroup.org/onlinepubs/9699919799/functions/close.html#tag_16_67_07

  • File exists (os error 17) when trying to create the temp directory for XDG_RUNTIME_DIR after the first run.
    Use properly unique temp dir names for each run.


Dependency tree with cargo tree -e normal --no-dedupe (excluding build-dependencies):

krun v0.1.0 (/home/teohhanhui/Projects/slp/krun/crates/krun)
├── anyhow v1.0.83
├── bpaf v0.9.12
├── env_logger v0.11.3
│   ├── anstream v0.6.14
│   │   ├── anstyle v1.0.7
│   │   ├── anstyle-parse v0.2.4
│   │   │   └── utf8parse v0.2.1
│   │   ├── anstyle-query v1.0.3
│   │   ├── colorchoice v1.0.1
│   │   ├── is_terminal_polyfill v1.70.0
│   │   └── utf8parse v0.2.1
│   ├── anstyle v1.0.7
│   ├── env_filter v0.1.0
│   │   └── log v0.4.21
│   ├── humantime v2.1.0
│   └── log v0.4.21
├── libkrun v1.8.1 (/home/teohhanhui/Projects/slp/krun/crates/libkrun)
├── log v0.4.21
├── nix v0.28.0
│   ├── bitflags v2.5.0
│   ├── cfg-if v1.0.0
│   └── libc v0.2.154
├── rustix v0.38.34
│   ├── bitflags v2.5.0
│   └── linux-raw-sys v0.4.13
└── utils v0.0.0 (/home/teohhanhui/Projects/slp/krun/crates/utils)

krun-guest v0.1.0 (/home/teohhanhui/Projects/slp/krun/crates/krun-guest)
├── anyhow v1.0.83
├── bpaf v0.9.12
├── env_logger v0.11.3
│   ├── anstream v0.6.14
│   │   ├── anstyle v1.0.7
│   │   ├── anstyle-parse v0.2.4
│   │   │   └── utf8parse v0.2.1
│   │   ├── anstyle-query v1.0.3
│   │   ├── colorchoice v1.0.1
│   │   ├── is_terminal_polyfill v1.70.0
│   │   └── utf8parse v0.2.1
│   ├── anstyle v1.0.7
│   ├── env_filter v0.1.0
│   │   └── log v0.4.21
│   ├── humantime v2.1.0
│   └── log v0.4.21
├── log v0.4.21
├── nix v0.28.0
│   ├── bitflags v2.5.0
│   ├── cfg-if v1.0.0
│   └── libc v0.2.154
├── rustix v0.38.34
│   ├── bitflags v2.5.0
│   └── linux-raw-sys v0.4.13
├── tempfile v3.10.1
│   ├── cfg-if v1.0.0
│   ├── fastrand v2.1.0
│   └── rustix v0.38.34
│       ├── bitflags v2.5.0
│       └── linux-raw-sys v0.4.13
└── utils v0.0.0 (/home/teohhanhui/Projects/slp/krun/crates/utils)

libkrun v1.8.1 (/home/teohhanhui/Projects/slp/krun/crates/libkrun)

utils v0.0.0 (/home/teohhanhui/Projects/slp/krun/crates/utils)

Requires:

@teohhanhui teohhanhui requested a review from slp as a code owner May 7, 2024 14:27
@teohhanhui teohhanhui mentioned this pull request May 7, 2024
4 tasks
@teohhanhui
Copy link
Collaborator Author

teohhanhui commented May 7, 2024

A note that the tracing feature bloats the resulting binaries quite a fair bit, from < 1 MiB to 3.8 MiB 😿

Let me know if you need me to change the default feature to exclude tracing. (Or turn off the attributes feature on tracing crate to avoid proc macros... Not sure this would help actually...)

@teohhanhui
Copy link
Collaborator Author

teohhanhui commented May 7, 2024

cargo tree --duplicates -e normal:

regex-automata v0.1.10
└── matchers v0.1.0
    └── tracing-subscriber v0.3.18
        ├── krun v0.1.0 (/home/teohhanhui/Projects/slp/krun/crates/krun)
        └── krun-guest v0.1.0 (/home/teohhanhui/Projects/slp/krun/crates/krun-guest)

regex-automata v0.4.6
└── regex v1.10.4
    └── tracing-subscriber v0.3.18 (*)

regex-syntax v0.6.29
└── regex-automata v0.1.10 (*)

regex-syntax v0.8.3
├── regex v1.10.4 (*)
└── regex-automata v0.4.6 (*)

There's an open PR to deduplicate this in matchers: hawkw/matchers#5

It will only benefit us when tracing-subscriber updates to matchers 0.2: tokio-rs/tracing#2923

@teohhanhui
Copy link
Collaborator Author

teohhanhui commented May 7, 2024

cargo depgraph --all-deps | dot -Tpng > graph.png:

graph

https://sr.ht/~jplatte/cargo-depgraph/#output-explanation

@slp
Copy link
Collaborator

slp commented May 7, 2024

A note that the tracing feature bloats the resulting binaries quite a fair bit, from < 1 MiB to 3.8 MiB 😿

Let me know if you need me to change the default feature to exclude tracing. (Or turn off the attributes feature on tracing crate to avoid proc macros... Not sure this would help actually...)

Hm... yes, that's quite significant. For components that are as relatively simple as krun and krun-guest, how much value are we getting from tracing?

@teohhanhui
Copy link
Collaborator Author

teohhanhui commented May 7, 2024

Switching to log (with kv feature) + env_logger cuts down release binary sizes to 2.5 MiB and 2.4 MiB respectively, while preserving the same expressiveness. Also, libkrun uses log too, so there's familiarity for contributors.

@teohhanhui
Copy link
Collaborator Author

Working on cutting this down further...

@teohhanhui
Copy link
Collaborator Author

teohhanhui commented May 7, 2024

Both binaries are down to 1.1 MiB respectively. This was primarily achieved by disabling the regex feature in env_logger: https://docs.rs/env_logger/latest/env_logger/#filtering-results

And we're also being explicit with all feature flags now, i.e. default-features = false

@teohhanhui
Copy link
Collaborator Author

teohhanhui commented May 9, 2024

I've noticed a problem from my testing: passt processes hanging around...

teohhanhui@Han-MacBook-Air:~$ ps aux | grep passt
teohhan+   73572  0.0  0.0  72272  2560 ?        S    05:23   0:01 /usr/bin/passt -q -f --fd 4
teohhan+   73609  0.0  0.0  72288  3072 ?        S    05:23   0:01 /usr/bin/passt -q -f --fd 4
teohhan+   73763  0.0  0.0  72272  2560 ?        S    05:24   0:01 /usr/bin/passt -q -f --fd 4
teohhan+  168592  0.0  0.0  72288  1536 ?        S    06:54   0:17 /usr/bin/passt -q -f --fd 4
teohhan+  170108  0.0  0.0  72272  2048 ?        S    07:03   0:00 /usr/bin/passt -q -f --fd 4
teohhan+  171325  0.0  0.0  72272  2048 ?        S    07:11   0:00 /usr/bin/passt -q -f --fd 4
teohhan+  171861  0.0  0.0  72288  2048 ?        S    07:14   0:00 /usr/bin/passt -q -f --fd 4
teohhan+  174435  0.0  0.0  72272  2560 ?        S    07:27   0:00 /usr/bin/passt -q -f --fd 4
teohhan+  174531  0.0  0.0  72272  2560 ?        S    07:27   0:00 /usr/bin/passt -q -f --fd 4
teohhan+  174563  0.0  0.0  72272  2560 ?        S    07:28   0:00 /usr/bin/passt -q -f --fd 4
teohhan+  174639  0.0  0.0  72288  2048 ?        S    07:28   0:00 /usr/bin/passt -q -f --fd 4
teohhan+  174673  0.0  0.0  72272  2560 ?        S    07:28   0:00 /usr/bin/passt -q -f --fd 4
teohhan+  176527  0.0  0.0  72272  2560 ?        S    07:36   0:00 /usr/bin/passt -q -f --fd 4
teohhan+  178771  0.0  0.0  72272  2560 ?        S    07:49   0:00 /usr/bin/passt -q -f --fd 4
teohhan+  187569  0.0  0.0  72272  2560 ?        S    08:30   0:00 /usr/bin/passt -q -f --fd 4
teohhan+  199298  0.0  0.0  72272  2048 ?        S    09:47   0:00 /usr/bin/passt -q -f --fd 4
teohhan+  200732  0.0  0.0  72288  1536 ?        S    09:57   0:00 /usr/bin/passt -q -f --fd 4
teohhan+  202108  0.0  0.0  72272  2048 ?        S    10:02   0:00 /usr/bin/passt -q -f --fd 4
teohhan+  203001  0.0  0.0  72288  2048 ?        S    10:09   0:00 /usr/bin/passt -q -f --fd 4
teohhan+  204247  0.0  0.0  72272  1536 ?        S    10:16   0:00 /usr/bin/passt -q -f --fd 4
teohhan+  205135  0.0  0.0  72272  1536 ?        S    10:19   0:00 /usr/bin/passt -q -f --fd 4
teohhan+  209267  0.0  0.0  72288  2048 ?        S    10:45   0:00 /usr/bin/passt -q -f --fd 4
teohhan+  210018  0.0  0.0  72272  2048 ?        S    10:49   0:00 /usr/bin/passt -q -f --fd 4
teohhan+  210061  0.0  0.0  72272  2560 ?        S    10:49   0:00 /usr/bin/passt -q -f --fd 4
teohhan+  210117  0.0  0.0  72288  2048 ?        S    10:50   0:00 /usr/bin/passt -q -f --fd 4
teohhan+  210183  0.0  0.0  72272  2560 ?        S    10:50   0:00 /usr/bin/passt -q -f --fd 4
teohhan+  210235  0.0  0.0  72288  2560 ?        S    10:51   0:00 /usr/bin/passt -q -f --fd 4
teohhan+  210358  0.0  0.0  72288  2048 ?        S    10:52   0:00 /usr/bin/passt -q -f --fd 4
teohhan+  210393  0.0  0.0  72272  2048 ?        S    10:52   0:00 /usr/bin/passt -q -f --fd 4
teohhan+  210573  0.0  0.0  72272  2560 ?        S    10:55   0:00 /usr/bin/passt -q -f --fd 4
teohhan+  210662  0.0  0.0  72288  3072 ?        S    10:55   0:00 /usr/bin/passt -q -f --fd 4
teohhan+  211600  0.0  0.0  72288  2560 ?        S    11:04   0:00 /usr/bin/passt -q -f --fd 4
teohhan+  212464  0.0  0.0  72272  2560 ?        S    11:10   0:00 /usr/bin/passt -q -f --fd 4
teohhan+  212682  0.0  0.0  72272  2048 ?        S    11:12   0:00 /usr/bin/passt -q -f --fd 4
teohhan+  213080  0.0  0.0  72272  2048 ?        S    11:14   0:00 /usr/bin/passt -q -f --fd 4
teohhan+  213442  0.0  0.0  72272  1536 ?        S    11:19   0:00 /usr/bin/passt -q -f --fd 4
teohhan+  213565  0.0  0.0  72288  2048 ?        S    11:20   0:00 /usr/bin/passt -q -f --fd 4
teohhan+  213820  0.7  0.0  72288  1536 ?        S    11:23   0:58 /usr/bin/passt -q -f --fd 4
teohhan+  223715  1.1  0.0  72272  1536 ?        S    12:13   1:00 /usr/bin/passt -q -f --fd 4
teohhan+  227106  0.0  0.0  72272  3072 ?        S    12:40   0:00 /usr/bin/passt -q -f --fd 4
teohhan+  227653  0.0  0.0  72272  2560 ?        S    12:47   0:00 /usr/bin/passt -q -f --fd 4
teohhan+  228052  0.0  0.0  72288  1536 ?        S    12:51   0:00 /usr/bin/passt -q -f --fd 4
teohhan+  228434  0.0  0.0  72272  2048 ?        S    12:53   0:00 /usr/bin/passt -q -f --fd 4
teohhan+  228535  0.0  0.0  72272  2048 ?        S    12:54   0:01 /usr/bin/passt -q -f --fd 4
teohhan+  229990  0.0  0.0  72272  2048 ?        S    13:12   0:00 /usr/bin/passt -q -f --fd 4
teohhan+  230903  0.0  0.0  72272  3584 ?        S    13:14   0:00 /usr/bin/passt -q -f --fd 4
teohhan+  236073  0.0  0.0 222704  2048 pts/12   S+   13:41   0:00 grep --color=auto passt


// SAFETY: `child_fd` is an `OwnedFd` and consumed to prevent closing on drop.
// See https://doc.rust-lang.org/std/io/index.html#io-safety
let child = Command::new(passt_path)
Copy link
Collaborator Author

@teohhanhui teohhanhui May 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'd have to explicitly kill the Child: https://doc.rust-lang.org/std/process/struct.Child.html#method.kill (not possible to do)

Also, it seems like krun is exiting with -1 status code, perhaps because of sommelier?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still a problem...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a limitation of libkrun (containers/libkrun#10). We need to implement a mechanism to relay signals to the main process in the VM and the exit code from it to the host. It's been open since a long time, but being a low priority issue we never got to implement it.

@teohhanhui
Copy link
Collaborator Author

containers/libkrun#185

crates/krun/src/net.rs Outdated Show resolved Hide resolved
crates/krun/src/cli_options.rs Outdated Show resolved Hide resolved
@slp
Copy link
Collaborator

slp commented May 10, 2024

Thanks a lot for the work put on this PR. Merging it!

@slp slp merged commit e35c7da into AsahiLinux:main May 10, 2024
@teohhanhui
Copy link
Collaborator Author

Oops! I will send a follow-up PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants