Merge #376

376: Fix TODO documentation r=Medowhill a=anemoneflower - code에 적힌 TODO와 issue를 매치했습니다. - 이미 해결된 TODO나 간단하게 해결할 수 있는 TODO는 해결하고 제거했습니다. Co-authored-by: anemoneflower <[email protected]>
kaist-cp · Jan 27, 2021 · 2fec36d · 2fec36d
2 parents 2754ee0 + d70120e
commit 2fec36d
Show file tree

Hide file tree

Showing 18 changed files with 100 additions and 94 deletions.
diff --git a/kernel-rs/src/arena.rs b/kernel-rs/src/arena.rs
@@ -95,7 +95,7 @@ impl<T> ArrayEntry<T> {
 }
 
 impl<T, const CAPACITY: usize> ArrayArena<T, CAPACITY> {
-    // TODO(rv6): unsafe...
+    // TODO(https://github.com/kaist-cp/rv6/issues/371): unsafe...
     pub const fn new(entries: [ArrayEntry<T>; CAPACITY]) -> Self {
         Self { entries }
     }
@@ -193,7 +193,8 @@ impl<T: 'static + ArenaObject, const CAPACITY: usize> Arena for Spinlock<ArrayAr
     unsafe fn dup(&self, handle: &Self::Handle) -> Self::Handle {
         let mut _this = self.lock();
 
-        // TODO: Make a ArrayArena trait and move this there.
+        // TODO(https://github.com/kaist-cp/rv6/issues/369)
+        // Make a ArrayArena trait and move this there.
         (*handle.ptr).refcnt += 1;
         Self::Handle {
             ptr: handle.ptr,
@@ -236,7 +237,7 @@ impl<T> MruEntry<T> {
 }
 
 impl<T, const CAPACITY: usize> MruArena<T, CAPACITY> {
-    // TODO(rv6): unsafe...
+    // TODO(https://github.com/kaist-cp/rv6/issues/371): unsafe...
     pub const fn new(entries: [MruEntry<T>; CAPACITY]) -> Self {
         Self {
             entries,
@@ -270,8 +271,9 @@ impl<T> Drop for MruPtr<T> {
 }
 
 impl<T: 'static + ArenaObject, const CAPACITY: usize> Spinlock<MruArena<T, CAPACITY>> {
-    // TODO(rv6): a workarond for https://github.com/Gilnaa/memoffset/issues/49.  Assumes
-    // `list_entry` is located at the beginning of `MruEntry`.
+    // TODO(https://github.com/kaist-cp/rv6/issues/369)
+    // A workarond for https://github.com/Gilnaa/memoffset/issues/49.
+    // Assumes `list_entry` is located at the beginning of `MruEntry`.
     const LIST_ENTRY_OFFSET: usize = 0;
     // const LIST_ENTRY_OFFSET: usize = offset_of!(MruEntry<T>, list_entry);
 }
@@ -374,7 +376,8 @@ impl<T: 'static + ArenaObject, const CAPACITY: usize> Arena for Spinlock<MruAren
     unsafe fn dup(&self, handle: &Self::Handle) -> Self::Handle {
         let mut _this = self.lock();
 
-        // TODO: Make a MruArena trait and move this there.
+        // TODO(https://github.com/kaist-cp/rv6/issues/369)
+        // Make a MruArena trait and move this there.
         (*handle.ptr).refcnt += 1;
         Self::Handle {
             ptr: handle.ptr,

diff --git a/kernel-rs/src/console.rs b/kernel-rs/src/console.rs
@@ -37,7 +37,8 @@ impl Console {
     }
 
     /// putc for Console.
-    /// TODO(@coolofficials): This function should be changed after refactoring Console-Uart-Printer relationship.
+    /// TODO(https://github.com/kaist-cp/rv6/issues/298)
+    /// This function should be changed after refactoring Console-Uart-Printer relationship.
     pub fn putc(&mut self, c: i32) {
         putc(c);
     }
@@ -48,7 +49,7 @@ impl Console {
             if VAddr::copy_in(&mut c, UVAddr::new(src.into_usize() + (i as usize))).is_err() {
                 return i;
             }
-            // TODO(@coolofficials): Temporarily using global function kernel().
+            // TODO(https://github.com/kaist-cp/rv6/issues/298): Temporarily using global function kernel().
             // This implementation should be changed after refactoring Console-Uart-Printer relationship.
             kernel().uart.putc(c[0] as i32);
         }
@@ -156,7 +157,8 @@ impl Printer {
     }
 
     /// putc for Printer.
-    /// TODO(@coolofficials): This function should be changed after refactoring Console-Uart-Printer relationship.
+    /// TODO(https://github.com/kaist-cp/rv6/issues/298)
+    /// This function should be changed after refactoring Console-Uart-Printer relationship.
     pub fn putc(&mut self, c: i32) {
         putc(c);
     }
@@ -172,7 +174,7 @@ impl fmt::Write for Printer {
 }
 
 /// Send one character to the uart.
-/// TODO(@coolofficials): This global function is temporary.
+/// TODO(https://github.com/kaist-cp/rv6/issues/298): This global function is temporary.
 /// After refactoring Console-Uart-Printer relationship, this function need to be removed.
 pub fn putc(c: i32) {
     if c == BACKSPACE {
@@ -211,7 +213,7 @@ pub unsafe fn consoleinit(devsw: &mut [Devsw; NDEV]) {
 
 /// User write()s to the console go here.
 unsafe fn consolewrite(src: UVAddr, n: i32) -> i32 {
-    // TODO(@coolofficials) Remove below comment.
+    // TODO(https://github.com/kaist-cp/rv6/issues/298) Remove below comment.
     // consolewrite() does not need console.lock() -- can lead to sleep() with lock held.
     kernel().console.get_mut_unchecked().write(src, n)
 }

diff --git a/kernel-rs/src/exec.rs b/kernel-rs/src/exec.rs
@@ -93,12 +93,11 @@ impl Kernel {
             return Err(());
         }
 
-        // TODO(rv6)
+        // TODO(https://github.com/kaist-cp/rv6/issues/290)
         // The method namei can drop inodes. If namei succeeds, its return
         // value, ptr, will be dropped when this method returns. Deallocation
         // of an inode may cause disk write operations, so we must begin a
         // transaction here.
-        // https://github.com/kaist-cp/rv6/issues/290
         let tx = self.file_system.begin_transaction();
         let ptr = path.namei()?;
         let mut ip = ptr.lock();

diff --git a/kernel-rs/src/file.rs b/kernel-rs/src/file.rs
@@ -45,7 +45,8 @@ pub struct Devsw {
 
 pub type RcFile<'s> = Rc<FileTable, &'s FileTable>;
 
-// TODO: will be infered as we wrap *mut Pipe and *mut Inode.
+// TODO(https://github.com/kaist-cp/rv6/issues/374)
+// will be infered as we wrap *mut Pipe and *mut Inode.
 unsafe impl Send for File {}
 
 impl Default for FileType {
@@ -132,9 +133,6 @@ impl File {
                 // might be writing a device like the console.
                 let max = (MAXOPBLOCKS - 1 - 1 - 2) / 2 * BSIZE;
 
-                // TODO(@kimjungwow) : To pass copyin() usertest, I reflect the commit on Nov 5, 2020 (below link).
-                // https://github.com/mit-pdos/xv6-riscv/commit/5e392531c07966fd8a6bee50e3e357c553fb2a2f
-                // This comment will be removed as we fetch upstream(mit-pdos)
                 let mut bytes_written: usize = 0;
                 while bytes_written < n as usize {
                     let bytes_to_write = cmp::min(n as usize - bytes_written, max);
@@ -180,11 +178,10 @@ impl ArenaObject for File {
             match typ {
                 FileType::Pipe { mut pipe } => unsafe { pipe.close(self.writable) },
                 FileType::Inode { ip, .. } | FileType::Device { ip, .. } => {
-                    // TODO(rv6)
+                    // TODO(https://github.com/kaist-cp/rv6/issues/290)
                     // The inode ip will be dropped by drop(ip). Deallocation
                     // of an inode may cause disk write operations, so we must
                     // begin a transaction here.
-                    // https://github.com/kaist-cp/rv6/issues/290
                     let _tx = kernel().file_system.begin_transaction();
                     drop(ip);
                 }
@@ -209,7 +206,7 @@ impl FileTable {
         readable: bool,
         writable: bool,
     ) -> Result<RcFile<'_>, ()> {
-        // TODO: idiomatic initialization.
+        // TODO(https://github.com/kaist-cp/rv6/issues/372): idiomatic initialization.
         let inner = self
             .alloc(|p| {
                 *p = File::new(typ, readable, writable);

diff --git a/kernel-rs/src/fs/inode.rs b/kernel-rs/src/fs/inode.rs
@@ -206,7 +206,7 @@ impl Dirent {
         unsafe { FileName::from_bytes(&self.name[..len]) }
     }
 
-    // TODO: Use iterator
+    // TODO(https://github.com/kaist-cp/rv6/issues/360): Use iterator
     fn read_entry(&mut self, ip: &mut InodeGuard<'_>, off: u32, panic_msg: &'static str) {
         let bytes_read = ip.read(
             KVAddr::new(self as *mut Dirent as usize),
@@ -445,9 +445,6 @@ impl InodeGuard<'_> {
             src = src.add(m as usize);
         }
 
-        // TODO(@kimjungwow) : To pass copyin() usertest, I reflect the commit on Nov 5, 2020 (below link).
-        // https://github.com/mit-pdos/xv6-riscv/commit/5e392531c07966fd8a6bee50e3e357c553fb2a2f
-        // This comment will be removed as we fetch upstream(mit-pdos)
         if off > self.deref_inner().size {
             self.deref_inner_mut().size = off;
         }
@@ -542,7 +539,7 @@ impl ArenaObject for Inode {
         if self.inner.get_mut().valid && self.inner.get_mut().nlink == 0 {
             // inode has no links and no other references: truncate and free.
 
-            // TODO(rv6)
+            // TODO(https://github.com/kaist-cp/rv6/issues/290)
             // Disk write operations must happen inside a transaction. However,
             // we cannot begin a new transaction here because beginning of a
             // transaction acquires a sleep lock while the spin lock of this
@@ -554,7 +551,6 @@ impl ArenaObject for Inode {
             // resulting FsTransaction value is never used. Such transactions
             // can be found in finalize in file.rs, sys_chdir in sysfile.rs,
             // close_files in proc.rs, and exec in exec.rs.
-            // https://github.com/kaist-cp/rv6/issues/290
             let tx = mem::ManuallyDrop::new(FsTransaction {
                 fs: &kernel().file_system,
             });

diff --git a/kernel-rs/src/fs/log.rs b/kernel-rs/src/fs/log.rs
@@ -143,7 +143,7 @@ impl Log {
     }
 
     /// Called at the start of each FS system call.
-    pub unsafe fn begin_op(this: &Sleepablelock<Self>) {
+    pub fn begin_op(this: &Sleepablelock<Self>) {
         let mut guard = this.lock();
         loop {
             if guard.committing ||
@@ -237,7 +237,7 @@ impl Log {
     ///   bp = Disk::read(...)
     ///   modify bp->data[]
     ///   write(bp)
-    pub unsafe fn write(&mut self, b: Buf<'static>) {
+    pub fn write(&mut self, b: Buf<'static>) {
         assert!(
             !(self.lh.len() >= LOGSIZE || self.lh.len() as i32 >= self.size - 1),
             "too big a transaction"

diff --git a/kernel-rs/src/fs/mod.rs b/kernel-rs/src/fs/mod.rs
@@ -38,13 +38,14 @@ const NINDIRECT: usize = BSIZE.wrapping_div(mem::size_of::<u32>());
 const MAXFILE: usize = NDIRECT.wrapping_add(NINDIRECT);
 
 pub struct FileSystem {
-    /// TODO(rv6): initializing superblock should be run only once
-    /// because forkret() calls fsinit()
+    /// TODO(https://github.com/kaist-cp/rv6/issues/358)
+    /// Initializing superblock should be run only once because forkret() calls fsinit()
     /// There should be one superblock per disk device, but we run with
     /// only one device.
     superblock: Once<Superblock>,
 
-    /// TODO(rv6): document it / initializing log should be run
+    /// TODO(https://github.com/kaist-cp/rv6/issues/358)
+    /// document it / initializing log should be run
     /// only once because forkret() calls fsinit()
     log: Once<Sleepablelock<Log>>,
 
@@ -80,7 +81,8 @@ impl FileSystem {
         });
     }
 
-    /// TODO(rv6): calling superblock() after initialize is safe
+    /// TODO(https://github.com/kaist-cp/rv6/issues/358)
+    /// Calling superblock() after initialize is safe
     fn superblock(&self) -> &Superblock {
         if let Some(sb) = self.superblock.get() {
             sb
@@ -89,7 +91,8 @@ impl FileSystem {
         }
     }
 
-    /// TODO(rv6): calling log() after initialize is safe
+    /// TODO(https://github.com/kaist-cp/rv6/issues/358)
+    /// Calling log() after initialize is safe
     fn log(&self) -> &Sleepablelock<Log> {
         if let Some(log) = self.log.get() {
             log
@@ -100,10 +103,7 @@ impl FileSystem {
 
     /// Called for each FS system call.
     pub fn begin_transaction(&self) -> FsTransaction<'_> {
-        // TODO(rv6): safety?
-        unsafe {
-            Log::begin_op(self.log());
-        }
+        Log::begin_op(self.log());
         FsTransaction { fs: self }
     }
 }

diff --git a/kernel-rs/src/fs/path.rs b/kernel-rs/src/fs/path.rs
@@ -55,8 +55,6 @@ impl Path {
         &self.inner
     }
 
-    // TODO: Following functions should return a safe type rather than `*mut Inode`.
-
     pub fn root() -> RcInode<'static> {
         kernel().itable.get_inode(ROOTDEV as u32, ROOTINO)
     }
@@ -99,8 +97,7 @@ impl Path {
     /// assert_eq!(Path::from_bytes(b"////").skipelem(), None);
     /// # }
     /// ```
-    // TODO: Make an iterator.
-    // TODO: Fix doctests work.
+    // TODO(https://github.com/kaist-cp/rv6/issues/359): Fix doctests work.
     fn skipelem(&self) -> Option<(&Self, &FileName)> {
         let mut bytes = &self.inner;
 
@@ -139,7 +136,8 @@ impl Path {
         let mut ptr = if self.is_absolute() {
             Self::root()
         } else {
-            // TODO(rv6): accessing proc.data should be safe after refactoring myproc()
+            // TODO(https://github.com/kaist-cp/rv6/issues/354)
+            // Accessing proc.data should be safe after refactoring myproc()
             unsafe { (*(*myproc()).data.get()).cwd.clone().unwrap() }
         };
 

diff --git a/kernel-rs/src/kernel.rs b/kernel-rs/src/kernel.rs
@@ -8,7 +8,7 @@ use crate::{
     file::{Devsw, FileTable},
     fs::{FileSystem, Itable},
     kalloc::Kmem,
-    page::{Page, RawPage},
+    page::Page,
     param::{NCPU, NDEV},
     plic::{plicinit, plicinithart},
     println,
@@ -21,8 +21,7 @@ use crate::{
 };
 
 /// The kernel.
-// TODO(rv6): remove pub from `pub static mut KERNEL`.
-pub static mut KERNEL: Kernel = Kernel::zero();
+static mut KERNEL: Kernel = Kernel::zero();
 
 /// After intialized, the kernel is safe to immutably access.
 #[inline]
@@ -36,7 +35,7 @@ pub struct Kernel {
     /// Sleeps waiting for there are some input in console buffer.
     pub console: Sleepablelock<Console>,
 
-    /// TODO(@coolofficials): Kernel owns uart temporarily.
+    /// TODO(https://github.com/kaist-cp/rv6/issues/298): Kernel owns uart temporarily.
     /// This might be changed after refactoring relationship between Console-Uart-Printer.
     pub uart: Uart,
 
@@ -56,13 +55,6 @@ pub struct Kernel {
 
     pub bcache: Bcache,
 
-    /// Memory for virtio descriptors `&c` for queue 0.
-    ///
-    /// This is a global instead of allocated because it must be multiple contiguous pages, which
-    /// `kernel().alloc()` doesn't support, and page aligned.
-    // TODO(efenniht): I moved out pages from Disk. Did I changed semantics (pointer indirection?)
-    virtqueue: [RawPage; 2],
-
     pub devsw: [Devsw; NDEV],
 
     pub ftable: FileTable,
@@ -85,7 +77,6 @@ impl Kernel {
             procs: ProcessSystem::zero(),
             cpus: [Cpu::new(); NCPU],
             bcache: Bcache::zero(),
-            virtqueue: [RawPage::DEFAULT, RawPage::DEFAULT],
             devsw: [Devsw {
                 read: None,
                 write: None,

diff --git a/kernel-rs/src/lib.rs b/kernel-rs/src/lib.rs
@@ -19,7 +19,8 @@
 #![feature(generic_associated_types)]
 #![feature(unsafe_block_in_unsafe_fn)]
 
-// TODO(rv6): We must apply #[deny(unsafe_op_in_unsafe_fn)] to every module.
+// TODO(https://github.com/kaist-cp/rv6/issues/335)
+// We must apply #[deny(unsafe_op_in_unsafe_fn)] to every module.
 mod arena;
 #[deny(unsafe_op_in_unsafe_fn)]
 mod bio;

diff --git a/kernel-rs/src/pipe.rs b/kernel-rs/src/pipe.rs
@@ -40,7 +40,7 @@ pub struct Pipe {
 impl Pipe {
     /// PipeInner::try_read() tries to read as much as possible.
     /// Pipe::read() executes try_read() until all bytes in pipe are read.
-    //TODO : `n` should be u32
+    //TODO(https://github.com/kaist-cp/rv6/issues/366) : `n` should be u32
     pub unsafe fn read(&self, addr: UVAddr, n: usize) -> Result<usize, ()> {
         let mut inner = self.inner.lock();
         loop {
@@ -100,8 +100,6 @@ impl Pipe {
     }
 }
 
-// TODO: Remove Copy and Clone
-#[derive(Copy, Clone)]
 pub struct AllocatedPipe {
     ptr: *mut Pipe,
 }
@@ -121,7 +119,7 @@ impl AllocatedPipe {
         // `Pipe` must be align with `Page`
         const_assert!(mem::size_of::<Pipe>() <= PGSIZE);
 
-        //TODO(rv6): Since Pipe is a huge struct, need to check whether stack is used to fill `*ptr`
+        //TODO(https://github.com/kaist-cp/rv6/issues/367): Since Pipe is a huge struct, need to check whether stack is used to fill `*ptr`
         // It is safe because unique access to page is guaranteed since page is just allocated,
         // and the pipe size and alignment are compatible with the page.
         unsafe {
@@ -157,8 +155,8 @@ impl AllocatedPipe {
         Ok((f0, f1))
     }
 
-    // TODO: use `Drop` instead of `close`
-    // TODO: use `self` instead of `&mut self`
+    // TODO(https://github.com/kaist-cp/rv6/issues/365): use `Drop` instead of `close`
+    // TODO(https://github.com/kaist-cp/rv6/issues/365): use `self` instead of `&mut self`
     // `&mut self` is used because `Drop` of `File` uses AllocatedPipe inside File.
     // https://github.com/kaist-cp/rv6/pull/211#discussion_r491671723
     pub unsafe fn close(&mut self, writable: bool) {