-
Notifications
You must be signed in to change notification settings - Fork 98
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
CDH | Fix ttrpc memory bug and gRPC lock bug #727
CDH | Fix ttrpc memory bug and gRPC lock bug #727
Conversation
299aa28
to
95521ea
Compare
this PR could also undo/revert e3c3fef, right? |
78d6fd6
to
470afc6
Compare
right. Added the commit |
attestation-agent/attestation-agent/src/bin/ttrpc-aa/ttrpc_protocol/attestation_agent.rs
Show resolved
Hide resolved
Fixes confidential-containers#688 This commit will copy only the pointer of CDH to implement different APIs. In old version, each CDH instance will serve for one API, thus occupies multiple times of memory. Signed-off-by: Xynnn007 <[email protected]>
The pull_image API of CDH is marked as Fn not FnMut thus we do not need a RwLock to protect the synchronization. Also, fixes a bug that CDH does not support image pull gRPC. Signed-off-by: Xynnn007 <[email protected]>
Upstream ttrpc crate has fixed the usage of boxed pointers. Thus we do not need to declare explicitly the lint error ignorance. Signed-off-by: Xynnn007 <[email protected]>
470afc6
to
22f6885
Compare
This commit adds a Makefile option `lint` for attestation-agent to test the lint of all sub projects of AA. Before this patch, some ttrpc codes are not covered by CI. Signed-off-by: Xynnn007 <[email protected]>
22f6885
to
e0b5a56
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Hey @mythi Could you help to get another look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thanks @Xynnn007
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I wonder if rebasing is needed to get a clean merge (OTOH, github does not complain about merge conflicts so I guess it's good)
Please see the commit message for details.
Fixes #688