Make argfd not return &'static RcFile

[Medowhill]

raw 함수가 굳이 있어야 하나요?

Originally posted by @jeehoonkang in #407 (comment)

---

#385 작업에서 생긴 변경사항입니다. argfd()에서 deref_mut_data를 이용하여 아래 코드를 작성하는 방법을 찾지 못해 deref_data_raw를 사용하였습니다.

rv6/kernel-rs/src/sysfile.rs

Lines 51 to 54 in ad9226f

	let f = some_or!(
	unsafe { &(*proc.deref_data_raw()).open_files[fd as usize] },
	return Err(())
	);

기존 코드는 아래와 같았습니다.

rv6/kernel-rs/src/sysfile.rs

Lines 50 to 54 in e28e1a2

	let p = unsafe { myproc() };
	let f = some_or!(
	unsafe { &(*(*p).data.get()).open_files[fd as usize] },
	return Err(())
	);

혹시 deref_mut_data등을 사용해서 구현할 수 있는 방법이 있을까요?

Originally posted by @anemoneflower in #407 (comment)

---

deref_mut_data를 넣으면 왜 안되나요?

Originally posted by @jeehoonkang in #407 (comment)

---

explicit lifetime required in the type of `proc`
lifetime `'static` required

reference를 사용하면 return되는 RcFile에 static lifetime이 붙어있어서, CurrentProc에도 static lifetime이 붙어야 한다는 에러가 뜹니다.

unsafe fn argfd(
    n: usize,
    proc: &mut CurrentProc<'_>,
) -> Result<(i32, &'static RcFile<'static>), ()> {

Originally posted by @anemoneflower in #407 (comment)

---

argfd가 &'static RcFile을 반환하는 것부터 문제가 있는 것 같습니다. 이 PR에서는 deref_data_raw를 그대로 두고, argfd 리팩토링과 deref_data_raw를 없애는 것은 추후에 별도로 진행하면 어떨까요?

Originally posted by @Medowhill in #407 (comment)

[Medowhill]

좋은 해결책이 쉽게 보이지 않는 것 같습니다.

첫 번째로 시도한 방식은 argfd가 &'a CurrentProc으로부터 &'a RcFile을 만들도록 하는 것입니다.

--- a/kernel-rs/src/proc.rs
+++ b/kernel-rs/src/proc.rs
@@ -385,8 +385,9 @@ impl<'p> CurrentProc<'p> {
         unsafe { guard.sched() };
     }

-    pub fn deref_data_raw(&mut self) -> *mut ProcData {
-        self.data.get()
+    pub fn deref_data(&self) -> &ProcData {
+        // Safety: Only `CurrentProc` can use `ProcData` without lock.
+        unsafe { &*self.data.get() }
     }

     pub fn deref_mut_data(&mut self) -> &mut ProcData {
diff --git a/kernel-rs/src/sysfile.rs b/kernel-rs/src/sysfile.rs
index e46c2d5..1569a25 100644
--- a/kernel-rs/src/sysfile.rs
+++ b/kernel-rs/src/sysfile.rs
@@ -42,19 +42,15 @@ impl RcFile<'static> {

 /// Fetch the nth word-sized system call argument as a file descriptor
 /// and return both the descriptor and the corresponding struct file.
-unsafe fn argfd(
-    n: usize,
-    proc: &mut CurrentProc<'_>,
-) -> Result<(i32, &'static RcFile<'static>), ()> {
+unsafe fn argfd<'a>(n: usize, proc: &'a CurrentProc<'_>) -> Result<(i32, &'a RcFile<'static>), ()> {
     let fd = argint(n, proc)?;
     if fd < 0 || fd >= NOFILE as i32 {
         return Err(());
     }

-    let f = some_or!(
-        unsafe { &(*proc.deref_data_raw()).open_files[fd as usize] },
-        return Err(())
-    );
+    let f = proc.deref_data().open_files[fd as usize]
+        .as_ref()
+        .ok_or(())?;

     Ok((fd, f))
 }

이렇게 하면 sys_read 등에서 RcFile의 메서드를 호출할 때 &mut CurrentProc을 넘겨야 하기 때문에 lifetime이 겹쳐 타임 검사를 통과하지 못합니다. 또한, sys_read 등에서 CurrentProc을 받아 다양한 필드에 접근하기 때문에 넘기기 전에 미리 CurrentProc을 쪼개서 lifetime 문제를 해결하기도 어려워 보입니다.

---

두 번째로 시도한 방식은 argfd에서 RcFile을 아예 move해 CurrentProc에서 꺼내 오는 것입니다. 이렇게 하면 문제 없이 컴파일되며 테스트도 잘 통과합니다. 다만, RcFile을 다 사용한 뒤에 다시 CurrentProc에 넣어 주어야 합니다. 커널의 동작을 생각할 때, RcFile을 굳이 꺼내 왔다가 다시 넣어 주어야 할 이유가 없기 때문에, 좋은 해결책은 아니라고 생각합니다.

--- a/kernel-rs/src/sysfile.rs
+++ b/kernel-rs/src/sysfile.rs
@@ -45,16 +45,13 @@ impl RcFile<'static> {
 unsafe fn argfd(
     n: usize,
     proc: &mut CurrentProc<'_>,
-) -> Result<(i32, &'static RcFile<'static>), ()> {
+) -> Result<(i32, RcFile<'static>), ()> {
     let fd = argint(n, proc)?;
     if fd < 0 || fd >= NOFILE as i32 {
         return Err(());
     }

-    let f = some_or!(
-        unsafe { &(*proc.deref_data_raw()).open_files[fd as usize] },
-        return Err(())
-    );
+    let f = proc.deref_mut_data().open_files[fd as usize].take().ok_or(())?;

     Ok((fd, f))
 }
@@ -320,8 +317,9 @@ impl Kernel {
     /// Return a new file descriptor referring to the same file as given fd.
     /// Returns Ok(new file descriptor) on success, Err(()) on error.
     pub unsafe fn sys_dup(&self, proc: &mut CurrentProc<'_>) -> Result<usize, ()> {
-        let (_, f) = unsafe { argfd(0, proc)? };
+        let (fd, f) = unsafe { argfd(0, proc)? };
         let newfile = f.clone();
+        proc.deref_mut_data().open_files[fd as usize] = Some(f);
         let fd = newfile.fdalloc(proc).map_err(|_| ())?;
         Ok(fd as usize)
     }
@@ -329,19 +327,23 @@ impl Kernel {
     /// Read n bytes into buf.
     /// Returns Ok(number read) on success, Err(()) on error.
     pub unsafe fn sys_read(&self, proc: &mut CurrentProc<'_>) -> Result<usize, ()> {
-        let (_, f) = unsafe { argfd(0, proc)? };
+        let (fd, f) = unsafe { argfd(0, proc)? };
         let n = argint(2, proc)?;
         let p = argaddr(1, proc)?;
-        unsafe { f.read(p.into(), n, proc) }
+        let res = unsafe { f.read(p.into(), n, proc) };
+        proc.deref_mut_data().open_files[fd as usize] = Some(f);
+        res
     }

     /// Write n bytes from buf to given file descriptor fd.
     /// Returns Ok(n) on success, Err(()) on error.
     pub unsafe fn sys_write(&self, proc: &mut CurrentProc<'_>) -> Result<usize, ()> {
-        let (_, f) = unsafe { argfd(0, proc)? };
+        let (fd, f) = unsafe { argfd(0, proc)? };
         let n = argint(2, proc)?;
         let p = argaddr(1, proc)?;
-        unsafe { f.write(p.into(), n, proc) }
+        let res = unsafe { f.write(p.into(), n, proc) };
+        proc.deref_mut_data().open_files[fd as usize] = Some(f);
+        res
     }

     /// Release open file fd.
@@ -355,10 +357,11 @@ impl Kernel {
     /// Place info about an open file into struct stat.
     /// Returns Ok(0) on success, Err(()) on error.
     pub unsafe fn sys_fstat(&self, proc: &mut CurrentProc<'_>) -> Result<usize, ()> {
-        let (_, f) = unsafe { argfd(0, proc)? };
+        let (fd, f) = unsafe { argfd(0, proc)? };
         // user pointer to struct stat
         let st = argaddr(1, proc)?;
         unsafe { f.stat(st.into(), proc)? };
+        proc.deref_mut_data().open_files[fd as usize] = Some(f);
         Ok(0)
     }

오프라인 미팅에서 의논하면 좋을 것 같습니다.