[ID-Mapped Mount] Impl Channel & Message#3409
[ID-Mapped Mount] Impl Channel & Message#3409YamasouA wants to merge 5 commits intoyouki-dev:mainfrom
Conversation
| UnexpectedMessage { | ||
| expected: Message, | ||
| received: Message, | ||
| expected: Box<Message>, |
There was a problem hiding this comment.
Boxed to keep ChannelError small and avoid clippy::result_large_err.
YamasouA
force-pushed
the
ft/id-mapped-mount-channel-message
branch
from
5719013 to
07afe2a
Compare
|
/retest |
YamasouA
force-pushed
the
ft/id-mapped-mount-channel-message
branch
from
20bdaa6 to
07afe2a
Compare
|
Could you please share the final design plan using a sequence diagram or similar documentation? |
|
@utam0k |
nayuta723
left a comment
nayuta723
left a comment
There was a problem hiding this comment.
Thank you for your contribution! I've left a few comments, so please take a look when you have a chance.
| pub struct MountMsg { | ||
| pub source: String, | ||
| pub idmap: Option<MountIdMap>, | ||
| pub recursive: bool, |
There was a problem hiding this comment.
How is this field intended to be used in relation to recursive in MountIdMap?
There was a problem hiding this comment.
@nayuta723
This is an implementation I initially considered but no longer need.
I will delete it.
| @@ -490,6 +569,53 @@ mod tests { | |||
| Ok(()) | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Could you add a test for wait_for_mount_fd_request?
| expected: Box::new(Message::MountFdPlease(MountMsg { | ||
| source: String::new(), | ||
| idmap: None, | ||
| recursive: false, | ||
| })), |
There was a problem hiding this comment.
I have a couple of questions on UnexpectedMessage:
-
Is the dummy expected value necessary?
Since we don't have the actual "expected" info here, does this dummy value provide any real benefit for debugging? -
Can we simplify it?
Would it be better to just store the received message alone? I'm concerned that dummy values might be confusing for future maintenance.
I know the existing code uses this pattern, but I’d love to make this new part a bit cleaner if possible. What do you think?
There was a problem hiding this comment.
That's right.
I'll correct it.
|
@YamasouA |
YamasouA
force-pushed
the
ft/id-mapped-mount-channel-message
branch
from
87a095a to
1cdfcbd
Compare
|
@nayuta723 |
Signed-off-by: YamasouA <akiakiskyhand@gmail.com>
YamasouA
force-pushed
the
ft/id-mapped-mount-channel-message
branch
from
1cdfcbd to
8be375d
Compare
nayuta723
left a comment
nayuta723
left a comment
There was a problem hiding this comment.
I've left a few comments, so please take a look when you have a chance.
| UnexpectedMessage { | ||
| expected: Message, | ||
| received: Message, | ||
| expected: Option<Box<Message>>, |
There was a problem hiding this comment.
How about this approach? I want to avoid Option as it leaves the meaning of None unclear.
| expected: Option<Box<Message>>, | |
| expected: &'static str, |
3 participants
Description
“I implemented ID-mapped mount support based on runc’s approach. In this PR, I extended the channel between Main and Init to allow passing mount FDs, and added unit tests.”
Type of Change
Testing
Related Issues
Fixes #2307
Additional Context
The following PR has too many changes, so it will be split and submitted for review.
#3384