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.

Sign up for GitHub

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

fix: incorrect manner of terminating the rsync process #1596

Merged
merged 2 commits into from
Jun 7, 2023
Merged

fix: incorrect manner of terminating the rsync process #1596

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c8391db
fix: incorrect manner of terminating the process
machinly Jun 7, 2023
bbbde40
Merge remote-tracking branch 'origin' into fix_kill_rsync
machinly Jun 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/pstd/src/rsync.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ int StopRsync(const std::string& raw_path) {
return 0;
}

std::string rsync_stop_cmd = "kill $(ps -o pgid=" + std::to_string(pid) + ")";
std::string rsync_stop_cmd = "kill -- -$(ps -o pgid -p" + std::to_string(pid) + " | grep -o '[0-9]*')";
int ret = system(rsync_stop_cmd.c_str());
if (ret == 0 || (WIFEXITED(ret) && !WEXITSTATUS(ret))) {
LOG(INFO) << "Stop rsync success!";
Copy link

Choose a reason for hiding this comment

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

The reviewed code is a patch for stopping an rsync process. The changes made in the patch include modifying the command used to stop the process by using the pgid (process group id) instead of the pid (process ID). This allows all child processes spawned by the rsync process to be stopped as well.

Improvement suggestions:

  • It's recommended to add error handling for the system() call, so any failure can be properly reported.
  • Instead of using system(), it's better to use fork()/exec() and waitpid() functions for better control over the execution of the command and avoiding potential security issues.
  • It's also good to perform null checks on the returned pointer value of std::to_string(pid) to avoid runtime errors caused by invalid or uninitialized inputs.

Expand Down