remoting: Implement single-to-multi host migration on Linux [chromium/src : main]
Yuwei Huang (Gerrit)
Yuwei Huang voted and added 1 comment![]( "Open in Gerrit")
Votes added by Yuwei Huang
| Commit-Queue | +1 |
1 comment
Related details
- Lambros Lambrou
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lambros Lambrou (Gerrit)
Lambros Lambrou added 11 comments![]( "Open in Gerrit")
LOG(ERROR) << "Failed to launch process";Is it possible to get `command_line` as a string for logging? It would be helpful for users to know what command failed.
return exit_code == 0;On POSIX, if the process is signaled, WaitForExit() will still return true, but `exit_code` will be -1. So the logic here is correct, but it might be helpful to log something different for each case. If it's -1, log something like "[command_line] terminated unexpectedly". Otherwise if it's non-zero, log something like "[command_line] exited with status [exit_code]". If it's zero, I don't think we need to log anything.
int exit_code = kInitializationFailed;This line is inconsistent with line 80 which doesn't initialize it at all. I think it's OK to not initialize it
I think `kInitializationFailed` comes from `host/base/host_exit_codes.h`. It makes sense as an exit-code for the host process. Does it make sense to re-use the same codes in a standalone migration tool?
std::cerr << "Failed to disable host service (" << unit_name << ").\n";In this block, `exit_code` doesn't have a coherent meaning. It might, or might not, have been overwritten by `WaitForExit()`. It might hold `kInitializationFailed` or it might hold an exit-code from the executed process. Maybe the exit-code of the process coincidentally matched the value of `kInitializationFailed`?
Can we be more explicit about the returned value of this function? Maybe a boolean return value would be more appropriate, and the caller should translate this into exit-codes from `host_exit_codes.h` if those are the constants you want to use.
int SaveMultiProcessPairingsAsNetworkUser() {Same question as before - would it make sense to return a boolean, and let the caller translate the result into constants?
multi_pairing_delegate.Save(Can `Save()` fail? It makes sense to continue the loop and save as much as possible, but do we want to return success in this case? Maybe it's sufficient to return success to allow other parts of the migration to proceed, but log any errors to stderr so the user knows the pairings might not all be saved?
I guess it's not a critical issue that blocks migration. Missing pairings would mean that the user has to re-enter a PIN.
return exit_code;Same comment as before - `DisableService()` might return one of our codes, or an exit-code from `systemctl...` which might coincidentally match our own codes.
std::cerr << "Usage: migrate_host <host_type>\n\n";Are there any optional switches we should document here? On line 152 you wrote "We need to keep the original program and args." Do the args include any switches, or are they just the host_type?
if (it == host_types.end()) {optional: maybe put this logic in a member such as `HostType::Find()`? It can return a `std::optional` or a value that is null-ish if not found?
const HostType& dst_host_type = *it->second;Instead of an abbreviation, maybe "requested_host_type" or "new_host_type" or "target_host_type"?
return kInvalidCommandLineExitCode;The command-line seems fine in this case. Maybe just return success (0) if we do nothing successfully 😊
Related details
- Yuwei Huang
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 12 comments![]( "Open in Gerrit")
Votes added by Yuwei Huang
| Commit-Queue | +1 |
PTAL thanks!
Is it possible to get `command_line` as a string for logging? It would be helpful for users to know what command failed.
Done
On POSIX, if the process is signaled, WaitForExit() will still return true, but `exit_code` will be -1. So the logic here is correct, but it might be helpful to log something different for each case. If it's -1, log something like "[command_line] terminated unexpectedly". Otherwise if it's non-zero, log something like "[command_line] exited with status [exit_code]". If it's zero, I don't think we need to log anything.
Done. Apparently AI missed the check for `exit_code`, and I missed it too :)
This line is inconsistent with line 80 which doesn't initialize it at all. I think it's OK to not initialize it
I think `kInitializationFailed` comes from `host/base/host_exit_codes.h`. It makes sense as an exit-code for the host process. Does it make sense to re-use the same codes in a standalone migration tool?
Done.
This line is inconsistent with line 80 which doesn't initialize it at all. I think it's OK to not initialize it
I just initialize them with -1. It seems if the wait times out then `exit_code` will be untouched. This will technically never happen, since the timeout is infinity if we haven't passed in a duration, but I'd still initialize it for sanity.
std::cerr << "Failed to disable host service (" << unit_name << ").\n";In this block, `exit_code` doesn't have a coherent meaning. It might, or might not, have been overwritten by `WaitForExit()`. It might hold `kInitializationFailed` or it might hold an exit-code from the executed process. Maybe the exit-code of the process coincidentally matched the value of `kInitializationFailed`?
Can we be more explicit about the returned value of this function? Maybe a boolean return value would be more appropriate, and the caller should translate this into exit-codes from `host_exit_codes.h` if those are the constants you want to use.
Done
Same question as before - would it make sense to return a boolean, and let the caller translate the result into constants?
Done
Can `Save()` fail? It makes sense to continue the loop and save as much as possible, but do we want to return success in this case? Maybe it's sufficient to return success to allow other parts of the migration to proceed, but log any errors to stderr so the user knows the pairings might not all be saved?
I guess it's not a critical issue that blocks migration. Missing pairings would mean that the user has to re-enter a PIN.
Done
Same comment as before - `DisableService()` might return one of our codes, or an exit-code from `systemctl...` which might coincidentally match our own codes.
Done
Are there any optional switches we should document here? On line 152 you wrote "We need to keep the original program and args." Do the args include any switches, or are they just the host_type?
All the other switches are internal to the program and not meant to be set by the user. I've added a comment before the switches and also simplified the logic on line 152.
optional: maybe put this logic in a member such as `HostType::Find()`? It can return a `std::optional` or a value that is null-ish if not found?
Done
Instead of an abbreviation, maybe "requested_host_type" or "new_host_type" or "target_host_type"?
Done
The command-line seems fine in this case. Maybe just return success (0) if we do nothing successfully 😊
- Lambros Lambrou
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)
Yuwei Huang added 1 comment![]( "Open in Gerrit")
AI made a bad change. Let me fix it first..
Yuwei Huang (Gerrit)
Yuwei Huang added 1 comment![]( "Open in Gerrit")
AI made a bad change. Let me fix it first..
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lambros Lambrou (Gerrit)
Lambros Lambrou added 1 comment![]( "Open in Gerrit")
if (!base::DeletePathRecursively(user_pairing_dir)) {This feels dangerous. Can you do this part as the regular user?
Instead of using `execvp()` to re-launch as root, can you run a child process instead? Keep the parent user process running, and do the root stuff in the child process. Wait for that to finish, then do the deletion in the user process.
That might also simplify the code, because you are currently building arguments for the `execvp()` call. If you use `base::LaunchProcess()` instead, you could maybe share some code between "launch a root process" and "launch a network-user process"?
Related details
- Yuwei Huang
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. |
Lambros Lambrou (Gerrit)
Lambros Lambrou voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Yuwei Huang
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 2 comments![]( "Open in Gerrit")
Votes added by Yuwei Huang
| Commit-Queue | +2 |
2 comments
if (!base::DeletePathRecursively(user_pairing_dir)) {This feels dangerous. Can you do this part as the regular user?
Instead of using `execvp()` to re-launch as root, can you run a child process instead? Keep the parent user process running, and do the root stuff in the child process. Wait for that to finish, then do the deletion in the user process.
That might also simplify the code, because you are currently building arguments for the `execvp()` call. If you use `base::LaunchProcess()` instead, you could maybe share some code between "launch a root process" and "launch a network-user process"?
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
12 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: remoting/host/linux/migrate_host_main.cc
Insertions: 77, Deletions: 52.
@@ -15,6 +15,7 @@
#include "base/at_exit.h"
#include "base/command_line.h"
+#include "base/containers/span.h"
#include "base/files/file_path.h"
#include "base/files/file_util.h"
#include "base/files/scoped_file.h"
@@ -31,6 +32,7 @@
#include "base/strings/stringprintf.h"
#include "base/task/single_thread_task_executor.h"
#include "base/values.h"
+#include "remoting/base/branding.h"
#include "remoting/base/file_path_util_linux.h"
#include "remoting/base/logging.h"
#include "remoting/base/passwd_utils.h"
@@ -50,17 +52,37 @@
constexpr char kSaveMultiProcessPairingsSwitch[] =
"save-multi-process-pairings";
-bool LaunchProcessWithStdin(const base::CommandLine& command_line,
- const std::string& input) {
+// Runs the current program with sudo.
+// `args`: The arguments to pass to the program.
+// `user_name`: If not empty, the program will be run as this user (sudo -u).
+// `input`: If not empty, this string will be written to the child's stdin.
+bool RunWithSudo(base::span<const std::string_view> args,
+ std::string_view user_name = {},
+ std::string_view input = {}) {
+ base::CommandLine command_line(base::CommandLine::NO_PROGRAM);
+ std::vector<std::string> full_args{"/usr/bin/sudo"};
+ if (!user_name.empty()) {
+ full_args.push_back("-u");
+ full_args.emplace_back(user_name);
+ }
+ full_args.push_back("--");
+ full_args.push_back(
+ base::CommandLine::ForCurrentProcess()->GetProgram().value());
+ full_args.insert(full_args.end(), args.begin(), args.end());
+ command_line.InitFromArgv(full_args);
+
base::ScopedFD read_fd;
base::ScopedFD write_fd;
- if (!base::CreatePipe(&read_fd, &write_fd)) {
- PLOG(ERROR) << "Failed to create pipe";
- return false;
+ base::LaunchOptions options;
+ options.allow_new_privs = true;
+ if (!input.empty()) {
+ if (!base::CreatePipe(&read_fd, &write_fd)) {
+ PLOG(ERROR) << "Failed to create pipe";
+ return false;
+ }
+ options.fds_to_remap.emplace_back(read_fd.get(), STDIN_FILENO);
}
- base::LaunchOptions options;
- options.fds_to_remap.emplace_back(read_fd.get(), STDIN_FILENO);
base::Process process = base::LaunchProcess(command_line, options);
if (!process.IsValid()) {
LOG(ERROR) << "Failed to launch process: "
@@ -68,18 +90,20 @@
return false;
}
- // Close the read end in the parent process.
- read_fd.reset();
+ if (!input.empty()) {
+ // Close the read end in the parent process.
+ read_fd.reset();
- if (!base::WriteFileDescriptor(write_fd.get(), input)) {
- PLOG(ERROR) << "Failed to write to pipe: "
- << command_line.GetCommandLineString();
- return false;
+ if (!base::WriteFileDescriptor(write_fd.get(), input)) {
+ PLOG(ERROR) << "Failed to write to pipe: "
+ << command_line.GetCommandLineString();
+ return false;
+ }
+
+ // Close the write end to signal EOF.
+ write_fd.reset();
}
- // Close the write end to signal EOF.
- write_fd.reset();
-
int exit_code = -1;
if (!process.WaitForExit(&exit_code)) {
LOG(ERROR) << "Failed to wait for process exit: "
@@ -175,22 +199,44 @@
bool MigrateToMultiProcess(const base::CommandLine& command_line) {
if (getuid() != 0) {
- // Re-run with sudo.
std::string user_name_switch =
base::StringPrintf("--%s=%s", kUserNameSwitch, GetUsername().c_str());
- std::vector<const char*> argv;
- argv.push_back("/usr/bin/sudo");
- // Add the original program and args.
- std::vector<std::string> original_argv = command_line.argv();
- for (const auto& arg : original_argv) {
- argv.push_back(arg.c_str());
- }
- argv.push_back(user_name_switch.c_str());
- argv.push_back(nullptr);
- execvp(argv[0], const_cast<char* const*>(argv.data()));
- PLOG(ERROR) << "execvp failed";
- return false;
+ const base::CommandLine::StringVector& args = command_line.GetArgs();
+ CHECK_GE(args.size(), 1u);
+ std::string_view host_type = args[0];
+
+ // The code below the if clause will be executed in the child process
+ // elevated with sudo.
+ if (!RunWithSudo({host_type, user_name_switch})) {
+ return false;
+ }
+
+ // Deleting the old pairing directory as the user.
+ std::cout << "Deleting old single-process config.\n";
+ base::FilePath user_config_dir = GetConfigDir();
+ base::FilePath config_file =
+ user_config_dir.Append(GetHostHash() + ".json");
+ base::FilePath user_pairing_dir = user_config_dir.Append(
+ PairingRegistryDelegateLinux::kRegistryDirectory);
+
+ if (base::PathExists(user_pairing_dir) &&
+ !base::DeletePathRecursively(user_pairing_dir)) {
+ std::cerr << "Failed to delete old pairing directory.\n";
+ return false;
+ }
+ // Ideally we should delete the old host config file, but an internal
+ // service will re-provision the host if it detects that the config file no
+ // longer exists. For now we just clear its content to prevent the
+ // single-process host from accidentally running.
+ // TODO: b/495898776 - just delete the file once the tooling is fixed.
+ if (base::PathExists(config_file) && !base::WriteFile(config_file, "")) {
+ std::cerr << "Failed to clear old host config file.\n";
+ return false;
+ }
+
+ std::cout << "Successfully migrated to multi-process host.\n";
+ return true;
}
// Code below is run as root.
@@ -243,12 +289,8 @@
std::cout << "Saving pairings to multi-process registry.\n";
// We do it in a child process as the network user to ensure the files and
// directories are created with the correct owner and permissions.
- base::CommandLine save_pairing_command_line(base::CommandLine::NO_PROGRAM);
- save_pairing_command_line.InitFromArgv(
- {"/usr/bin/sudo", "-u", std::string(GetNetworkProcessUsername()), "--",
- command_line.GetProgram().value(),
- std::string("--") + kSaveMultiProcessPairingsSwitch});
- if (!LaunchProcessWithStdin(save_pairing_command_line, pairings_json)) {
+ if (!RunWithSudo({"--" + std::string(kSaveMultiProcessPairingsSwitch)},
+ GetNetworkProcessUsername(), pairings_json)) {
std::cerr << "Failed to save pairings to multi-process registry.\n";
return false;
}
@@ -278,23 +320,6 @@
return false;
}
- // Deleting the old pairing directory as root.
- std::cout << "Deleting old single-process config.\n";
- if (!base::DeletePathRecursively(user_pairing_dir)) {
- std::cerr << "Failed to delete old pairing directory.\n";
- return false;
- }
- // Ideally we should delete the old host config file, but an internal service
- // will re-provision the host if it detects that the config file no longer
- // exists. For now we just clear its content to prevent the single-process
- // host from accidentally running.
- // TODO: b/495898776 - just delete the file once the tooling is fixed.
- if (!base::WriteFile(config_file, "")) {
- std::cerr << "Failed to clear old host config file.\n";
- return false;
- }
-
- std::cout << "Successfully migrated to multi-process host.\n";
return true;
}
```
Change information
remoting: Implement single-to-multi host migration on Linux
This CL implements the migration path from single-process host to
multi-process host on Linux. During migration:
1. Elevation: re-execute itself with sudo and sets the --user-name
param.
2. Config retrieval: read the user's host configuration from
~/.config/chrome-remote-desktop/host#...json
3. Pairing extraction: read all pairing data using
PairingRegistryDelegateLinux.
4. Save pairing: Save pairing by launching a subprocess running as the
network user and passing the pairing data via stdin. This ensures
that files and directories are created with the correct owner and
permissions.
5. Service transition: first disable
chrome-remote-desktop@<user>.service, then start the multi-process
service (chrome-remote-desktop.service) via DaemonController.
6. Cleanup: delete the user's local pairing directory, and clear the
content of the local host config file.
The multi-to-single migration path is left as NOTIMPLEMENTED() for now.
- M remoting/host/linux/host_types.cc
- M remoting/host/linux/host_types.h
- M remoting/host/linux/migrate_host_main.cc
Code-Review: +1 by Lambros Lambrou
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |