Add support for some basic system calls and args handling correctly. #29
Reference in New Issue
Block a user
No description provided.
Delete Branch "system-calls"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Hi Guys,
Here is the merged version of both the args handling and the basic system calls handler and some basic system call implementations.
Includes a few commits:
I am thinking it is worth it merging to the user-programs now as the basic system calls will be required in all future tasks.
As it stands, 30 test cases passes! (53 out of 83 still failing)
requested review from @td1223
assigned to @sb3923
I'm not sure I agree with either of the added commits. Passing in a NULL rather than a 0 makes more sense as it makes the purpose of the code clearer (and in some archiectures the NULL pointer isn't even 0). If you don't like the build errors I can add explicit casting rather than implicit (again in this context, casting from a pointer to an char/integer makes sense and is the intention).
Also as for the exit messages, a process doing the syscall of exit is graceful exiting (kernel threads don't use syscalls, so that case is even implicitly handled). In fact, there's already an instance of the codebase that calls process_exit non-gracefully to free acquired resources if there's not enough memory to initialise the process, and I imagine calling process_exit in the future for further freeing in the case of process errors. If we encounter problems in the future with bad threads printing exit messages we can check over this code again, but for now I think this adds more problems.
Honestly I did not think much of the changes when I made them, but I have had a closer look just now:
For the first point, I agree it does make the code look cleaner, but this is just not the proper way to do it anyway. You would not do that with an array of strings for example, (rather use \0). You are trying to set a pointer as a value. You said something about a NULL ptr not being 0 but this is not true: The NULL ptr value must compare to 0 as per C specification. What it could be is in an address that is not 0. Even if it was, it still isn't relevant since PintOS uses exactly the same architecture. Again, I just did this without much thinking as this is how you assign NULL to a char / int. (If either is unsafe, it is definitely the NULL one). BTW, now that I had a look at start_process, the code is very unreadable - I suggest extracting the arguments setup into a separate function.
For the second point, please read the relevant part of the spec - it is obvious they mean in process_exit ("Whenever a user process terminates, because it called exit or for any other reason, print ..."). In the case you mentioned, where a load fails, the message is optional as per the spec. Again, it just point to it being in process_exit. Even if the spec didn't say this, I would still put it in the process_exit just because it would lead to a lot of changes/code unnecessarily. But yeah, the spec specifically says that we must not print it in only two situations 1) halt (And it won't print because the syscall won't gracefully exit by calling process_exit but will just kill the entire OS) and 2) a kernal thread terminates (And this also won't print). For other cases, it is ncecessary or optional.
So, I would suggest keeping both changes. Let me know what you think!
You're right about the second change, I was misremembering the spec text. As for the code in start_process, it's incomplete so don't worry about the beauty of it yet anyway. I was tired after just gettng it working. I still disagree about the NULL thing but we can just wait till the final PR to see the shape.
mentioned in commit
ed09e0b08e