---
# System prepended metadata

title: Final Reflection

---

# Final Reflection

## Design Doc Changes

### Argument Passing

Our original design for this was incomplete, it did not go into detail about how exactly the arguments were to be pushed onto the stack. After a conversation with our TA, the final process was:

- Create a struct in `process_execute` that has `argc` and pointers to the parsed arguments from the original command
- Parse/tokenize arguments using `strtok_r`
- Pass this struct as an argument to `start_process`
- In `start_process`:
    - Align the stack
    - Push these variables in order:
        1. The command chars, tokenized (4-byte aligned)
        1. Pointers to the command tokens (with an null pointer to end the array)
        1. Argv, argc (16-byte aligned)
        1. Fake return address

### System Calls

Validation scheme:
- We validated user arguments to the kernel very similarly to how we described in the design doc (i.e. running `is_user_vaddr` and `pagedir_get_page`), and validating the pointer and the actual content if it is a string.

Our original thread struct design (storing the TCB) called for:

- A semaphore that indicates whether thread has finished running or not
- A load status variable that says whether the program executable is loaded properly
- An "exit code" field of the thread
- TID of the parent

In the end, our thread struct had:
- A list of file pointers of the current open files (covered in the next section)
- A file pointer to the loaded current program (we call `file_deny_write` on this at the start and `file_allow_write` in `process_exit`)—we didn't address this in our first design doc
- The exit code of the thread
- A pointer to the thread struct of the parent
- A semaphore that indicates whether the thread has finished
- The number of children threads
- A linked list of the children threads
- A lock on the children threads list
- A semaphore to indicate whether the children threads are finished

The load status variable was stored along with the other arguments, because it is set in start_process—we used an extra semaphore to wait until the executable is loaded, so we could return -1 if load fails.

Our `exec/wait` scheme was the main driver for these changes. Our original scheme was very underdeveloped, and didn't properly cover how we would implement the wait cases, so here is the final scheme:
- Pass a reference to the parent into `start_process` along with the arguments
- In `start_process`, if `load` succeeds:
    - Add child to parent's child list (+ increment number of children)
    - Set parent pointer in child thread
- In `process_wait`:
    - Try to sema_down the running semaphore (waits till it is sema_up'd in `process_exit`)
    - Remove the child from the parent's list
    - Dealloc the child
- In `process_exit`:
    - Wait for each child to finish: we accomplish this by calling sema_down on the children_threads_finished semaphore (# of children) times
    - Sema_up running semaphore
- In `thread_schedule_tail` (called just before a thread is deallocated, so after `process_exit` has been called)
    - Call sema_up on children_threads_finished
    - Deallocate the children

With this design, we're able to accomplish `exec`, because we have the exit code of the process, and properly and efficiently deallocate threads because the parent deletes children only when it is sure that the child has fully finished (i.e. is about to die in `thread_schedule_tail`)

### File System Calls

We didn't change much from our original scheme for this task. We did change how we allocated new file descriptors, by keeping space to hold file pointers, and returning the first index in the list that is available. We also did not create a `open_file` struct—we only needed the index and the file from the array.


## Group Reflection

We pair-programmed the entire project—no one did any work except for the time we spent together in a room with the code on a projector and everybody calling out what to write. No one was "in charge" of any specific part of the project.

This worked really well in that everyone was always on the same page on the project—it was really good that we didn't split off tasks. Perhaps we could have switched off the task of actually writing the code more often in the pair-programming task, but that requires more time pulling from the repository and getting set-up properly in Pintos.

## Student Testing Report

### Test 1: close-write

The close-write test tests if we can write to a file that has been created / opened and then immediately closed.

We create a file and close it before calling write and check if the file size is 0. Its expected output is 0 as we can’t write to an already closed file. There are existing tests that test writing to a file that doesn’t exist, writing to bad fds, and writing kernel addressed strings to a file, but none that test the status of a file after it should have been closed properly by `close`.

*close-write.output*
```
Copying tests/userprog/close-write to scratch partition...
qemu-system-i386 -device isa-debug-exit -hda /tmp/hBez3x8Zn6.dsk -m 4 -net none -nographic -monitor null
PiLo hda1
Loading...........
Kernel command line: -q -f extract run close-write
Pintos booting with 3,968 kB RAM...
367 pages available in kernel pool.
367 pages available in user pool.
Calibrating timer...  423,526,400 loops/s.
hda: 5,040 sectors (2 MB), model "QM00001", serial "QEMU HARDDISK"
hda1: 181 sectors (90 kB), Pintos OS kernel (20)
hda2: 4,096 sectors (2 MB), Pintos file system (21)
hda3: 104 sectors (52 kB), Pintos scratch (22)
filesys: using hda2
scratch: using hda3
Formatting file system...done.
Boot complete.
Extracting ustar archive from scratch device into file system...
Putting 'close-write' into the file system...
Erasing ustar archive...
Executing 'close-write':
(close-write) begin
(close-write) create "data"
(close-write) open "data"
(close-write) open "data"
(close-write) filesize should be 0
(close-write) end
close-write: exit(0)
Execution of 'close-write' complete.
Timer: 70 ticks
Thread: 8 idle ticks, 62 kernel ticks, 0 user ticks
hda2 (filesys): 89 reads, 216 writes
hda3 (scratch): 103 reads, 2 writes
Console: 1014 characters output
Keyboard: 0 keys pressed
Exception: 0 page faults
Powering off...
```
*close-write.result*
```
PASS
```

#### Ways the kernel could fail if implemented incorrectly
1. If the kernel did not actually close the fd and did nothing instead, it would pass many of the `close` tests in Pintos. We actually test if it is closed by trying to write to it.
2. If the kernel panicked on a write to a closed file (i.e. close did not work correctly, because the fd was not deleted correctly, for example not setting the array element to NULL) instead of removing the file pointer on close, the test case would fail.

### Test 2: file-init-size

The file-init-size test tests if the filesize of a file is consistent with the size specified in `create`.

For file-init-size, we first create a file by parsing in the file name and initial size, open the file and check if initial size is the same as before. Its expected output is to have the same size as the initial_size argument in create function call.

*close-write.output*
```
Copying tests/userprog/file-init-size to scratch partition...
qemu-system-i386 -device isa-debug-exit -hda /tmp/mbJ0g4Ia6a.dsk -m 4 -net none -nographic -monitor null
PiLo hda1
Loading...........
Kernel command line: -q -f extract run file-init-size
Pintos booting with 3,968 kB RAM...
367 pages available in kernel pool.
367 pages available in user pool.
Calibrating timer...  287,948,800 loops/s.
hda: 5,040 sectors (2 MB), model "QM00001", serial "QEMU HARDDISK"
hda1: 181 sectors (90 kB), Pintos OS kernel (20)
hda2: 4,096 sectors (2 MB), Pintos file system (21)
hda3: 104 sectors (52 kB), Pintos scratch (22)
filesys: using hda2
scratch: using hda3
Formatting file system...done.
Boot complete.
Extracting ustar archive from scratch device into file system...
Putting 'file-init-size' into the file system...
Erasing ustar archive...
Executing 'file-init-size':
(file-init-size) begin
(file-init-size) create "data"
(file-init-size) open "data"
(file-init-size) filesize should be 1024
(file-init-size) end
file-init-size: exit(0)
Execution of 'file-init-size' complete.
Timer: 61 ticks
Thread: 6 idle ticks, 56 kernel ticks, 0 user ticks
hda2 (filesys): 85 reads, 218 writes
hda3 (scratch): 103 reads, 2 writes
Console: 1021 characters output
Keyboard: 0 keys pressed
Exception: 0 page faults
Powering off...
```
*file-init-size.result*
```
PASS
```

#### Ways the kernel could fail if implemented incorrectly
1. If the kernel did not actually allocate `initial_size` bytes for the file on creation, it would have the incorrect size when queried with `filesize`.
2. If the kernel did not implement `filesize` correctly (if it read from the file and counted bytes), it would give an incorrect answer to this test (it would return 0 instead of the filesize passed into create).

### Overall Experience Writing Pintos Tests

It was tough, to say the least, to write tests. Having to add variables to Make files, and having to create two different files is a lot of overhead to write one test. In addition, in order to generate the .result and .output files (which is how individual tests are run), `make check` needs to be run. 

Having some sort of code generation would be extremely helpful—seems like the process described above can be easily automated. Some sort of measure of code coverage would also be not very tough to implement but could go a long way.

On the other hand, we learned that the tests given in Pintos are very thorough—most wait/exec cases described in the spec were tested, which was easily the toughest part of the project. The filesystem tests were not as thorough, but since all they do is call the `filesys` library, this isn't that much of an issue. Finally, we learned that writing tests about processes that have children / other synchronization is really tough, especially when trying to hit all possible interleaved calls.




