Jleong/change exit behavior

src/threads/thread.c

@@ -112,6 +112,20 @@ struct thread *get_initial_thread(void) {
     return initial_thread;
 }
 
+/*! Returns the child thread with this tid. */
+struct thread *get_child_thread(tid_t child_tid) {
+    struct thread *cur = thread_current();
+    struct list_elem *e;
+    for (e = list_begin(&cur->kids); e != list_end(&cur->kids);
+         e = list_next(e)) {
+        struct thread *kid = list_entry(e, struct thread, kid_elem);
+        if (kid->tid == child_tid) {
+            return kid;
+        }
+    }
+    return NULL;
+}
+
 /*! Initializes the threading system by transforming the code
     that's currently running into a thread.  This can't work in
     general and it is possible in this case only because loader.S
@@ -269,6 +283,8 @@ tid_t thread_create(const char *name, int priority, thread_func *function,
     struct thread *parent = thread_current();
     list_push_back(&parent->kids, &t->kid_elem);
     t->parent = parent;
+    ASSERT(t->done_sema.value == 1);
+    sema_down(&t->done_sema);
 
     /* Add to run queue. */
     int prev_highest_priority = get_highest_priority();
@@ -368,24 +384,22 @@ void thread_exit(void) {
     ASSERT (list_empty(&cur->open_files));
 
     /* Let kids know that parent is dead so that their page is freed without
-       waiting for the parent to free them. Will be freed in
-       thread_schedule_tail() instead of process_wait().*/
-    for (e = list_begin(&cur->kids); e != list_end(&cur->kids);
-         /* increment in loop */) {
+       waiting for the parent. Will be freed in thread_schedule_tail().*/
+    while (!list_empty(&cur->kids)) {
+        e = list_pop_front(&cur->kids);
         struct thread *kid = list_entry(e, struct thread, kid_elem);
-        e = list_next(e);
         kid->parent = NULL;
-
-        if (kid != NULL && kid->status == THREAD_DYING
-            && kid != initial_thread) {
-            palloc_free_page(kid);
-        }
+        sema_up(&kid->done_sema);
     }
 
 #ifdef USERPROG
     process_exit();
 #endif
 
+    if ((cur->parent != NULL) && (cur->parent != initial_thread)) {
+        list_remove(&cur->kid_elem);
+    }
+
     intr_disable();
     list_remove(&cur->allelem);
     cur->status = THREAD_DYING;
@@ -593,11 +607,15 @@ int next_fd(struct thread *cur) {
 int add_open_file(struct thread *cur, struct file *file, int fd) {
     /* Initialize file */
     struct sys_file *new_file = palloc_get_page(PAL_ZERO);
+    /* Not enough memory. */
+    if (new_file == NULL) {
+        return -1;
+    }
     memset(new_file, 0, sizeof *new_file);
     new_file->file = file;
     new_file->fd = fd;
 
-    if (is_valid_fd(fd)) {
+    if (is_valid_fd(fd) && !is_existing_fd(cur, fd)) {
         list_push_back(&cur->open_files, &new_file->file_elem);
         return fd;
     }
@@ -634,6 +652,7 @@ void close_fd(struct thread *cur, int fd) {
         struct sys_file *cur_file = list_entry(e, struct sys_file, file_elem);
         if (cur_file->fd == fd) {
             list_remove(&cur_file->file_elem);
+            file_close(cur_file->file);
             palloc_free_page(cur_file);
             return;
         }
@@ -719,11 +738,12 @@ static void init_thread(struct thread *t, const char *name, int priority) {
     list_init(&t->open_files);
     /* Block process_wait of parent until this process is ready to die. */
     sema_init(&t->wait_sema, 0);
-    sema_init(&t->exec_load, 0);
-    lock_init(&t->filesys_lock);
+    sema_init(&t->done_sema, 1);
+    lock_init(&t->wait_lock);
     t->loaded = false;
     t->waited_on = false;
     t->num_files = 0;
+    t->parent = NULL;
 
     if (list_empty(&all_list)) {
         t->niceness = 0;  /* Set niceness to 0 on initial thread */
@@ -827,12 +847,7 @@ void thread_schedule_tail(struct thread *prev) {
     if (prev != NULL && prev->status == THREAD_DYING &&
         prev != initial_thread) {
         ASSERT(prev != cur);
-        /* Let parent know it is done. */
-        sema_up(&prev->wait_sema);
-        if (prev->parent == NULL) {
-            /* Don't need to wait for parent to kill kid */
-            palloc_free_page(prev);
-        }
+        palloc_free_page(prev);
     }
 }
 

src/threads/thread.h

@@ -139,7 +139,9 @@ struct thread {
     struct list kids;                   /*!< List of children processes. */
     struct list_elem kid_elem;          /*!< List element for parent's kids list. */
     struct semaphore wait_sema;         /*!< Sempahore for process_wait. */
+    struct semaphore done_sema;         /*!< Sempahore for process_exit. */
     bool waited_on;                     /*!< True if process_wait has been called. */
+    struct lock wait_lock;              /*!< Lock for checking waited_on. */
     /**@}*/
 
     /*! Shared between by userprog/process.c and userprog.syscall.c and
@@ -149,7 +151,6 @@ struct thread {
     struct semaphore exec_load;         /*!< Semaphore for checking when executable has loaded. */
     bool loaded;                        /*!< Check if exec loaded successfully. */
     struct file *executable;            /*!< Executable to keep open until done. */
-    struct lock filesys_lock;           /*!< Lock for filesys calls. */
     /**@}*/
 
 #ifdef USERPROG
@@ -220,6 +221,7 @@ void add_sleep_thread(struct thread *);
 void sleep_threads(void);
 
 struct thread *get_initial_thread(void);
+struct thread *get_child_thread(tid_t child_tid);
 
 #endif /* threads/thread.h */
 

src/userprog/DESIGNDOC

@@ -164,7 +164,6 @@ In our thread struct, we included these attributes and their following descripti
     struct semaphore exec_load;         /*!< Semaphore for checking when executable has loaded. */
     bool loaded;                        /*!< Check if exec loaded successfully. */
     struct file *executable;            /*!< Executable to keep open until done. */
-    struct lock filesys_lock;           /*!< Lock for filesys calls. */
     /**@}*/
 
 

src/userprog/process.c

@@ -68,6 +68,9 @@ tid_t process_execute(const char *cmdline) {
 
     /* Create a new thread to execute FILE_NAME. */
     tid = thread_create(file_name, PRI_DEFAULT, start_process, cmdline_copy);
+
+    struct thread *kid = get_child_thread(tid);
+    sema_down(&kid->wait_sema);
     palloc_free_page(cmdline_copy2);
     if (tid == TID_ERROR) {
         palloc_free_page(cmdline_copy);
@@ -120,13 +123,14 @@ static void start_process(void *cmdline_) {
     palloc_free_page(file_name);
 
     /* Let sys_exec know loading is done and if it was successful. */
-    struct thread *parent = thread_current()->parent;
+    struct thread *cur = thread_current();
+    struct thread *parent = cur->parent;
     parent->loaded = success;
-    sema_up(&parent->exec_load);
+    sema_up(&cur->wait_sema);
 
     /* If load failed, quit. */
     if (!success) {
-        sys_exit(-1);
+        thread_exit();
     }
 
     /* Start the user process by simulating a return from an
@@ -146,38 +150,30 @@ static void start_process(void *cmdline_) {
     returns -1 immediately, without waiting. */
 int process_wait(tid_t child_tid UNUSED) {
     struct thread *cur = thread_current();
+    struct thread *kid = get_child_thread(child_tid);
 
-    /* Iterate through children to find child */
-    struct list_elem *e;
-    struct thread *kid = NULL;
-    for (e = list_begin(&cur->kids); e != list_end(&cur->kids);
-         e = list_next(e)) {
-        kid = list_entry(e, struct thread, kid_elem);
-        if (kid->tid == child_tid && !kid->waited_on) {
-            /* Next time we look for this kid, we return -1. */
-            kid->waited_on = true;
-            break;
-        }
+    /* Check if no kid with child_tid. */
+    if (kid == NULL || kid->tid != child_tid || kid->parent != cur) {
+        return -1;
     }
 
-    /* Check if no kid with child_tid. */
-    if (kid == NULL || kid->tid != child_tid) {
+    /* Lock in case of interrupt. */
+    lock_acquire(&kid->wait_lock);
+    if (kid->waited_on) {
+        lock_release(&kid->wait_lock);
         return -1;
     }
+    kid->waited_on = true;
+    lock_release(&kid->wait_lock);
 
     /* Wait for child thread to die. */
     sema_down(&kid->wait_sema);
 
-    /* Thread exit should have already been called. */
-    ASSERT(kid->status == THREAD_DYING);
-
     /* If child thread is done, just get exit status. */
     int child_exit_status = kid->exit_status;
-    kid->parent = NULL;
     list_remove(&kid->kid_elem);
-    if (kid != get_initial_thread() && kid->status == THREAD_DYING) {
-        palloc_free_page(kid);
-    }
+    kid->parent = NULL;
+    sema_up(&kid->done_sema);
 
     return child_exit_status;
 }
@@ -187,6 +183,22 @@ void process_exit(void) {
     struct thread *cur = thread_current();
     uint32_t *pd;
 
+    /* Free executable */
+    if (cur->executable != NULL) {
+        file_close(cur->executable);
+    }
+    cur->executable = NULL;
+
+    /* Let parent know it is done. */
+    sema_up(&cur->wait_sema);
+    while(!list_empty(&cur->wait_sema.waiters)) {
+        sema_up(&cur->wait_sema);
+    }
+    /* Wait for parent to retrieve exit status. */
+    ASSERT(cur->parent != NULL || cur->done_sema.value > 0);
+    sema_down(&cur->done_sema);
+    ASSERT(cur->parent == NULL);
+
     /* Destroy the current process's page directory and switch back
        to the kernel-only page directory. */
     pd = cur->pagedir;

src/userprog/syscall.c

@@ -14,7 +14,7 @@
 #include "userprog/process.h"
 
 /* Protect filesys calls. */
-struct lock filesys_lock;
+static struct lock filesys_lock;
 void acquire_file_lock(void);
 void release_file_lock(void);
 
@@ -158,13 +158,11 @@ void *get_third_arg(struct intr_frame *f) {
     static variables go out of memory. */
 void acquire_file_lock(void) {
     lock_acquire(&filesys_lock);
-    lock_acquire(&thread_current()->filesys_lock);
 }
 
 /*! Release file locks. See comment in acquire_file_lock. */
 void release_file_lock(void) {
     lock_release(&filesys_lock);
-    lock_release(&thread_current()->filesys_lock);
 }
 
 /*! Terminates Pintos. Should be seldom used due to loss of information on
@@ -177,30 +175,18 @@ void sys_halt(void) {
 void sys_exit(int status) {
     struct thread *cur = thread_current();
     printf("%s: exit(%d)\n", cur->name, status);
-
     cur->exit_status = status;
 
     if (lock_held_by_current_thread(&filesys_lock)) {
         release_file_lock();
     }
 
-    /* Free executable */
-    if (cur->executable != NULL) {
-        file_allow_write(cur->executable);
-    }
-
     /* Free all file buffers. */
     struct list_elem *e;
-    for (e = list_begin(&cur->open_files);
-         e != list_end(&cur->open_files);
-         /* increment in loop */) {
+    while (!list_empty(&cur->open_files)) {
+        e = list_begin(&cur->open_files);
         struct sys_file *open_file =
             list_entry(e, struct sys_file, file_elem);
-
-        /* Increment before removing. */
-        e = list_next(e);
-
-        /* File system call */
         sys_close(open_file->fd);
     }
     thread_exit();
@@ -217,7 +203,6 @@ pid_t sys_exec(const char *cmd_line) {
     pid_t new_process_pid = process_execute(cmd_line);
 
     /* Wait for executable to load. */
-    sema_down(&cur->exec_load);
     release_file_lock();
 
     if (!cur->loaded) {
@@ -280,7 +265,11 @@ int sys_open(const char *file) {
     fd = add_open_file(cur, open_file, fd);
     release_file_lock();
 
-    ASSERT(fd > 1); /* Only for stdin and stdout */
+    if (fd == -1) {
+        acquire_file_lock();
+        file_close(open_file);
+        release_file_lock();
+    }
     return fd;
 }
 
@@ -300,7 +289,7 @@ int sys_filesize(int fd) {
 /*! Read *size* bytes from file open as fd into buffer. Return the number of
     bytes actually read, 0 at end of file, or -1 if file could not be read. */
 int sys_read(int fd, void *buffer, unsigned size) {
-    if (!valid_read_addr(buffer)) {
+    if (!valid_read_addr(buffer) || !valid_read_addr(buffer + size)) {
         sys_exit(ERR);
     }
     int bytes_read = 0;
@@ -341,7 +330,7 @@ int sys_read(int fd, void *buffer, unsigned size) {
     number written, or 0 if no bytes could be written at all.
     Fd 1 writes to the console. */
 int sys_write(int fd, const void *buffer, unsigned size) {
-    if (!valid_read_addr(buffer)) {
+    if (!valid_read_addr(buffer) || !valid_read_addr(buffer + size)) {
         sys_exit(ERR);
     }
     int bytes_written = 0;
@@ -425,7 +414,6 @@ void sys_close(int fd) {
     acquire_file_lock();
     /* Delete file from thread */
     close_fd(cur, fd);
-    file_close(open_file);
     release_file_lock();
 }