[remoting] Implement host migration for Linux [chromium/src : main]
Yuwei Huang (Gerrit)
Yuwei Huang added 17 comments![]( "Open in Gerrit")
class HostType {Can you move these new classes to `remoting/host/linux/host_types.h`, and maybe add some comments?
std::cerr << " " << name << ": " << type->description() << "\n";Please print in this style:
```
Supported host types:
gdm-managed
Blah blah blah blah...
single-process
Blah blah blah blah...
```
std::string_view description() const override {The description should have more details. The single-process host is run as the user, and supports X11.
return "GDM-managed multi-process host";It should have more details. Point out that it only supports GNOME 49+ in Wayland mode, and you will be presented the login screen for the first time you connect. Also, it doesn't support running multiple hosts for different users.
{"single-process", new SingleProcessHostType()},This will possibly trigger ASAN/TSAN errors in the trybots. You may need NoDestructors for these objects as well.
int ControlService(const std::string& unit_name, bool enable) {We never enable a service with this method, right? Can you unify this with `sudo_disable_cmd` below, and maybe introduce `int DisableService(unit_name)`, which decides whether sudo should be used depending on whether `getuid() == 0`.
PairingRegistryDelegateLinux pairing_delegate;Comment that the default constructor uses the multi-process pairing registry path because it is run as root.
int MigrateHostMain(int argc, char** argv) {This is a huge main function. Can you restructure the code a bit. Maybe introduce the following helper functions: `MigrateToMultiProcess`, `MigrateToSingleProcess`.
Do identify other chance for improving code structure.
std::cerr << "Usage: migrate_host <host_type> [options]\n";Print another `\n` to separate from the "Supported host types:" below.
base::CommandLine sudo_command_line(base::FilePath("/usr/bin/sudo"));Maybe it's easier to deal with the raw `argv()` then CommandLine.
base::LaunchProcess(sudo_command_line, base::LaunchOptions());Would it be easier to just run `execvp`?
base::LaunchProcess(sudo_command_line, base::LaunchOptions());You probably need `allow_new_privs` for sudo.
struct passwd* pw = getpwnam(user_name.c_str());Consider using remoting/base/passwd_utils.h.
// Disable single-process service.Comment that SetConfigAndStart() already stops the sing-process host (because systemd unit defines a Conflict) but it doesn't disable it, so we disable here to prevent it from running after reboot.
// Disable multi-process service.Ditto. Commend that the multi-process service was stopped but not disabled.
// staticPlease move these below the anonymous namespace.
base::FilePath DaemonControllerDelegateLinuxSingleProcess::GetConfigPath() {Move this below the anonymous namespace?
Related details
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yuwei Huang (Gerrit)
Yuwei Huang added 22 comments![]( "Open in Gerrit")
class SingleProcessHostType : public HostType {These can probably be in the .cc file.
Can you move these new classes to `remoting/host/linux/host_types.h`, and maybe add some comments?
Done
std::cerr << " " << name << ": " << type->description() << "\n";Please print in this style:
```
Supported host types:
gdm-managed
Blah blah blah blah...
single-process
Blah blah blah blah...
```
Done
The description should have more details. The single-process host is run as the user, and supports X11.
Done
It should have more details. Point out that it only supports GNOME 49+ in Wayland mode, and you will be presented the login screen for the first time you connect. Also, it doesn't support running multiple hosts for different users.
Done
This will possibly trigger ASAN/TSAN errors in the trybots. You may need NoDestructors for these objects as well.
Done
int ControlService(const std::string& unit_name, bool enable) {We never enable a service with this method, right? Can you unify this with `sudo_disable_cmd` below, and maybe introduce `int DisableService(unit_name)`, which decides whether sudo should be used depending on whether `getuid() == 0`.
Done
argv.push_back(sudo_path.data());Maybe just use `std::vector<const char*>`, push `"/usr/bin/sudo"` directly, then do a const_cast when calling `execvp`.
for (size_t i = 0; i < original_argv.size(); ++i) {
argv.push_back(original_argv[i].data());Please fix this WARNING reported by ClangTidy: check: modernize-loop-convert
use range-based for loop instead (https://clang.l...
check: modernize-loop-convert
use range-based for loop instead (https://clang.llvm.org/extra/clang-tidy/checks/modernize/loop-convert.html)
(Note: You can add `Skip-Clang-Tidy-Checks: modernize-loop-convert` footer to the CL description to skip the check)
(Lint observed on `linux-clang-tidy-rel`, but not on `android-clang-tidy-rel`)
Comment that the default constructor uses the multi-process pairing registry path because it is run as root.
Done
This is a huge main function. Can you restructure the code a bit. Maybe introduce the following helper functions: `MigrateToMultiProcess`, `MigrateToSingleProcess`.
Do identify other chance for improving code structure.
Done
std::cerr << "Usage: migrate_host <host_type> [options]\n";Print another `\n` to separate from the "Supported host types:" below.
Done
std::move(*config), /*consent=*/true,Comment that the `consent` parameter is not used in Linux, and the `usage_stats_consent` value in the host config is used instead.
// Run sudo <this program> --get-multi-process-configExplain that the privileged config files are only readable by root.
base::CommandLine sudo_command_line(base::FilePath("/usr/bin/sudo"));Maybe it's easier to deal with the raw `argv()` then CommandLine.
Done
base::LaunchProcess(sudo_command_line, base::LaunchOptions());You probably need `allow_new_privs` for sudo.
Done
base::LaunchProcess(sudo_command_line, base::LaunchOptions());Would it be easier to just run `execvp`?
Done
struct passwd* pw = getpwnam(user_name.c_str());Yuwei HuangConsider using remoting/base/passwd_utils.h.
Done
host_config->Clone(), /*consent=*/true,Comment that the `consent` parameter is not used in Linux, and the `usage_stats_consent` value in the host config is used instead.
Comment that SetConfigAndStart() already stops the sing-process host (because systemd unit defines a Conflict) but it doesn't disable it, so we disable here to prevent it from running after reboot.
Done
Ditto. Commend that the multi-process service was stopped but not disabled.
Done
Related details
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yuwei Huang (Gerrit)
Yuwei Huang voted and added 9 comments![]( "Open in Gerrit")
Votes added by Yuwei Huang
| Commit-Queue | +1 |
9 comments
These can probably be in the .cc file.
Done
argv.push_back(sudo_path.data());Yuwei HuangMaybe just use `std::vector<const char*>`, push `"/usr/bin/sudo"` directly, then do a const_cast when calling `execvp`.
Done
for (size_t i = 0; i < original_argv.size(); ++i) {
argv.push_back(original_argv[i].data());Please fix this WARNING reported by ClangTidy: check: modernize-loop-convert
use range-based for loop instead (https://clang.l...
check: modernize-loop-convert
use range-based for loop instead (https://clang.llvm.org/extra/clang-tidy/checks/modernize/loop-convert.html)
(Note: You can add `Skip-Clang-Tidy-Checks: modernize-loop-convert` footer to the CL description to skip the check)
(Lint observed on `linux-clang-tidy-rel`, but not on `android-clang-tidy-rel`)
Done
Yuwei HuangMaybe comment that code below is run as root.
Done
Comment that the `consent` parameter is not used in Linux, and the `usage_stats_consent` value in the host config is used instead.
Done
Explain that the privileged config files are only readable by root.
Done
Comment that the `consent` parameter is not used in Linux, and the `usage_stats_consent` value in the host config is used instead.
Done
// staticYuwei HuangPlease move these below the anonymous namespace.
Done
base::FilePath DaemonControllerDelegateLinuxSingleProcess::GetConfigPath() {Move this below the anonymous namespace?
Done
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yuwei Huang (Gerrit)
Related details
Code-Coverage
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |