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

Jump to bottom

Removed dup code from jailer/env/clone() #4677

Merged
merged 3 commits into from
Jul 22, 2024
Merged

Removed dup code from jailer/env/clone() #4677

merged 3 commits into from
Jul 22, 2024

Conversation

seafoodfry
Copy link
Contributor

Changes

Found duplicated code while reviewing the jailer.
There were no differences between the x86_64 and the aarch64 versions of the clone sys call.

Reason

Cleanup

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following Developer
Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.

PR Checklist

  • If a specific issue led to this PR, this PR closes the issue.
  • The description of changes is clear and encompassing.
  • Any required documentation changes (code and docs) are included in this
    PR.
  • API changes follow the Runbook for Firecracker API changes.
  • User-facing changes are mentioned in CHANGELOG.md.
  • All added/changed functionality is tested.
  • New TODOs link to an issue.
  • Commits meet
    contribution quality standards.
  • This functionality cannot be added in rust-vmm.

@seafoodfry seafoodfry force-pushed the jailer-env-clone-dup-cleanup branch from 6017523 to d6fe56f Compare July 12, 2024 00:14
@roypat
Copy link
Contributor

roypat commented Jul 12, 2024

Hi @seafoodfry,
thank you for your PR! You're right that this code is indeed duplicated. There is a reason for this, namely that the order of the arguments to the clone syscall are different on x86_64 vs aarch64 [1]. Particularly, the final two arguments are child_tid and tls, with the x86_64 of the syscall having child_tid first, and the aarch64 version of the syscall having tls first.

Now, Firecracker sets both of those arguments to 0, so really, the order doesnt matter, and using a cfg to indicate this is quite overkill (and really, doesn't make the code any clearer either, imo. I remember being confused by this exactly thing around a year ago too when I stumbled across it!), so overall I agree with the change you propose. Could you add a code comment about the argument order thing? Just in case someone ever does try to use the child_tid and tls arguments, in which case the cfg would be actually needed.

@codecov Codecov
Copy link

codecov bot commented Jul 12, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 0% with 11 lines in your changes missing coverage. Please review.

Project coverage is 82.09%. Comparing base (f91b800) to head (4952075).

Files Patch % Lines
src/jailer/src/env.rs 0.00% 11 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #4677       /-   ##
==========================================
  Coverage   82.08%   82.09%    0.01%     
==========================================
  Files         255      255              
  Lines       31312    31308       -4     
==========================================
  Hits        25701    25701              
  Misses       5611     5607       -4
Flag Coverage Δ
5.10-c5n.metal 82.16% <0.00%> ( 0.01%) ⬆️
5.10-m5n.metal 82.15% <0.00%> ( 0.01%) ⬆️
5.10-m6a.metal 81.46% <0.00%> ( 0.01%) ⬆️
5.10-m6g.metal 79.45% <0.00%> ( 0.01%) ⬆️
5.10-m6i.metal 82.15% <0.00%> ( 0.01%) ⬆️
5.10-m7g.metal 79.45% <0.00%> ( 0.01%) ⬆️
6.1-c5n.metal 82.16% <0.00%> ( 0.01%) ⬆️
6.1-m5n.metal 82.15% <0.00%> ( 0.01%) ⬆️
6.1-m6a.metal 81.46% <0.00%> ( <0.01%) ⬆️
6.1-m6g.metal 79.45% <0.00%> ( 0.01%) ⬆️
6.1-m6i.metal 82.14% <0.00%> ( <0.01%) ⬆️
6.1-m7g.metal 79.45% <0.00%> ( 0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@seafoodfry
Copy link
Contributor Author

Thank you for your review @roypat !
Am new here but the explciit documentation there does sound like a great idea - super easy to forget that clone looks different in diff archs.
I went a bit verbose to leave an easy to find trail of how to make the code work for different platforms in case it was ever needed again.
ptal

roypat
roypat previously approved these changes Jul 15, 2024
View reviewed changes
Copy link
Contributor

@roypat roypat left a comment

Choose a reason for hiding this comment

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

Thanks!

zulinx86
zulinx86 previously approved these changes Jul 15, 2024
View reviewed changes
Copy link
Contributor

@zulinx86 zulinx86 left a comment

Choose a reason for hiding this comment

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

@seafoodfry Thank you for your contribution! LGTM, but could you please fix styling issues to merge this?

@pb8o pb8o assigned zulinx86 and roypat and unassigned zulinx86 Jul 15, 2024
@roypat roypat added the Status: Awaiting author Indicates that an issue or pull request requires author action label Jul 15, 2024
@seafoodfry seafoodfry dismissed stale reviews from zulinx86 and roypat via 3140f44 July 16, 2024 00:33
@seafoodfry seafoodfry force-pushed the jailer-env-clone-dup-cleanup branch from d39a545 to 3140f44 Compare July 16, 2024 00:33
@seafoodfry
Copy link
Contributor Author

Thank you both for the tips!
i updated my PR to fix CI

@roypat
Copy link
Contributor

roypat commented Jul 16, 2024

Thanks! Could you also squash the commit about removing the return into your first commit? :o that should also sidestep the "commit with no body" that the CI is now running into :D

@seafoodfry
Removed dup code from jailer/env/clone()
0af2906 
There were no differences between the x86_64 and the aarch64 versions
of the clone sys call.

Signed-off-by: seafoodfry <99568361 [email protected]>
@seafoodfry
Documented the discrepancy in arguments for the clone sys call
bd099cd 
Used
https://man7.org/linux/man-pages/man2/clone.2.html#VERSIONS
as reference

Signed-off-by: seafoodfry <99568361 [email protected]>
@seafoodfry seafoodfry force-pushed the jailer-env-clone-dup-cleanup branch from f6e527a to bd099cd Compare July 16, 2024 21:03
@seafoodfry
Copy link
Contributor Author

seafoodfry commented Jul 16, 2024 •
edited
Loading

Done @roypat @zulinx86 , and thank you for your patience while I learn the ropes here 🙏
ptal

zulinx86
zulinx86 approved these changes Jul 17, 2024
View reviewed changes
Copy link
Contributor

@zulinx86 zulinx86 left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!! Looks good to me!

roypat
roypat approved these changes Jul 22, 2024
View reviewed changes
Copy link
Contributor

@roypat roypat left a comment

Choose a reason for hiding this comment

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

Apologies for the late review, I was on vacation last week. Everything looks good to me, thanks for this!!

@roypat roypat merged commit d5d67b5 into firecracker-microvm:main Jul 22, 2024
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zulinx86 zulinx86 zulinx86 approved these changes

@roypat roypat roypat approved these changes

Assignees

@roypat roypat

Labels
Status: Awaiting author Indicates that an issue or pull request requires author action
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@seafoodfry @roypat @zulinx86