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

feat(namespace) feat some namespace bugs #1033

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

codeironman
Copy link
Contributor

  1. remove contains_of ! Macro
  2. fix some bugs

@dragonosbot
Copy link

@codeironman: no appropriate reviewer found, use r? to override

@github-actions github-actions bot added the ambiguous The title of PR/issue doesn't match the format label Nov 7, 2024
Copy link
Member

@fslongjin fslongjin left a comment

Choose a reason for hiding this comment

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

总结一下,这个pr存在几个问题:

  1. 实例管理混乱:user namespace、pid namespace、 mnt namespace、ucounts里面存储的对象,关联性很差。
  2. 未经过测试:有些代码一眼就是有问题的。
  3. 对象的父子关系存储有问题:父级找不到子级。并且关系非常混乱,估计绕几圈之后,指向的都不是同一个实例。

还需要大改哦

Comment on lines 28 to 41
pub struct MntNamespace {
/// namespace 共有的部分
ns_common: Arc<NsCommon>,
/// 关联的用户名字空间
user_ns: Arc<UserNamespace>,
/// 资源计数器
ucounts: Arc<UCounts>,
/// 根文件系统
root: Option<Arc<MountFS>>,
/// 红黑树用于挂载所有挂载点
mounts: RBTree<InodeId, MountFSInode>,
mounts: Arc<RBTree<InodeId, MountFSInode>>,
/// 等待队列
poll: WaitQueue,
poll: Arc<WaitQueue>,
/// 挂载序列号
seq: AtomicU64,
seq: Arc<AtomicU64>,
/// 挂载点的数量
Copy link
Member

Choose a reason for hiding this comment

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

为什么每个成员变量都是Arc<>?这个访存效率低并且浪费内存。直接一个Arc<MntNamespace>会不会好些。
并且,mounts字段还有poll字段,你确定你测试过能用????这个一眼看过去就知道跑不了。

fn name(&self) -> String;
fn clone_flags(&self) -> CloneFlags;
fn get(&self, pid: Pid) -> Option<Arc<dyn Namespace>>;
fn put(&self);
Copy link
Member

Choose a reason for hiding this comment

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

不需要put方法吧

}
fn get(&self, pid: crate::process::Pid) -> Option<Arc<dyn Namespace>> {
ProcessManager::find(pid)
.map(|pcb| pcb.get_nsproxy().read().mnt_namespace.clone() as Arc<dyn Namespace>)
Copy link
Member

Choose a reason for hiding this comment

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

get_nsproxy方法改名为nsproxy。这样比较符合rust的命名规范。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot from 2024-11-18 09-45-30

我是这两个一起设计的 get set

return Err(SystemError::EINVAL);
}
nsproxy.mnt_namespace = mnt_ns;
nsproxy.mnt_namespace = Arc::new(self.clone());
Copy link
Member

Choose a reason for hiding this comment

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

这里指向的根本就不是当前的namespace了啊。
你可能需要搜下我们代码里面的self_ref. 并且你还需要了解Arc指针跟Weak指针。因为类似这种写法就是错误的:
q9MHRqX0XQ

怎么能child存储指向父级的Arc呢?这显然是不正确的。

@@ -1,4 +1,5 @@
#![allow(dead_code, unused_variables, unused_imports)]
Copy link
Member

Choose a reason for hiding this comment

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

删掉全局的这种lint

pub ucount_max: Vec<u32>, //vec![u32; UCOUNT_COUNTS as usize],
pub rlimit_max: Vec<u32>, // vec![u32; UCOUNT_RLIMIT_COUNTS as usize],
pub ucount_max: Vec<u32>,
pub rlimit_max: Vec<u32>,
Copy link
Member

Choose a reason for hiding this comment

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

我认为UserNamespace结构体本身就应该是个Arc的。并且这个创建Arc的过程应该在它的new方法里面体现出来。而不是依赖default方法,然后在外面才创建Arc.
image

Comment on lines +45 to +53
Self {
id_alloctor: Arc::new(RwLock::new(IdAllocator::new(1, PID_MAX).unwrap())),
pid_allocated: 1,
level: 0,
child_reaper: Arc::new(RwLock::new(Pid::from(1))),
parent: None,
ucounts: Arc::new(UCounts::default()),
user_ns: Arc::new(UserNamespace::default()),
}
Copy link
Member

Choose a reason for hiding this comment

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

不应该依赖default方法。并且
Ucounts的Usernamespace和这里的user_ns都不是同一个userNamespace
这个很显然是有问题的。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这个函数不是用于创建的,是类似于linux中的init_pid_ns的角色,默认的

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ambiguous The title of PR/issue doesn't match the format
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants