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

Make Process's strace file shareable across Processes #2971

Closed
wants to merge 2 commits into from

Conversation

sporksmith
Copy link
Contributor

When we fork a Process, the child will use the same strace file (if any). This is analogous to strace -f.

Progress on #1987.

@sporksmith sporksmith added this to the Support missing syscalls milestone May 24, 2023
@sporksmith sporksmith self-assigned this May 24, 2023
@github-actions github-actions bot added Component: Libraries Support functions like LD_PRELOAD and logging Component: Main Composing the core Shadow executable labels May 24, 2023
When we fork a Process, the child will use the same strace file (if
any). This is analogous to `strace -f`.

Progress on shadow#1987.
Comment on lines -194 to +195
strace_logging: Option<StraceLogging>,
// strace logging file, if any. Shared with forked Processes.
strace_logging: Option<RootedRc<RootedRefCell<StraceLogging>>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would be simpler by just cloning/duping (File::try_clone) the file instead of sharing it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, that sounds much simpler :)

Comment on lines -194 to +195
strace_logging: Option<StraceLogging>,
// strace logging file, if any. Shared with forked Processes.
strace_logging: Option<RootedRc<RootedRefCell<StraceLogging>>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the RootedRefCell needed? I don't think we need to mutate the format options at runtime, and the file is already inside a RefCell. If it's needed for Send/Sync or something, maybe replace the RefCell in StraceLogging with RootedRefCell instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, otherwise the inner RefCell can't go inside the RootedRc, since it's not Sync. I'll take a look at replacing the inner RefCell instead if File::try_clone doesn't work out.

@sporksmith
Copy link
Contributor Author

Obsoleted by adding StraceLogging::try_clone in 54f2e68

@sporksmith sporksmith closed this Aug 22, 2023
@sporksmith sporksmith deleted the fork-strace branch August 22, 2023 00:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Libraries Support functions like LD_PRELOAD and logging Component: Main Composing the core Shadow executable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants