[#6] Basic syscall handler and skeleton for each syscall #42

Lment · 2017-04-07T16:46:28Z

Basic syscall handler, and skeleton code to start writing each system call.
System call which has return value must put its result to eax, which is accessible by f->eax.
Skeleton with return has return value when fully implemented.

hangpark

Why do your codes always have lots of INVALID coding styles? PLEASE CHECK yourself before (and even after) pushing your commits. 💢

hangpark · 2017-04-07T16:47:19Z

src/userprog/syscall.c

+#include "threads/init.h"
+#include "threads/malloc.h"
+
+#define WORD_SIZE sizeof(uintptr_t)


Parenthesis, Spacing

hangpark · 2017-04-07T16:47:34Z

src/userprog/syscall.c


 static void syscall_handler (struct intr_frame *);

+


hangpark · 2017-04-07T16:47:43Z

src/userprog/syscall.c

 void
 syscall_init (void) 
 {
  intr_register_int (0x30, 3, INTR_ON, syscall_handler, "syscall");
 }

+


hangpark · 2017-04-07T16:48:05Z

src/userprog/syscall.c

+  for (i = 1; i <= 3; i++)
+    args[i] = f->esp + WORD_SIZE * i;
+
+


hangpark · 2017-04-07T16:49:46Z

src/userprog/syscall.c

+
+  switch (syscall_num)
+    {
+      case SYS_HALT:


Indentation

hangpark · 2017-04-07T16:55:06Z

src/userprog/syscall.c

+  power_off();
+}
+
+static void syscall_exit (void ** args)


Spacing (Same for all the cases below)

Also on prototypes.

hangpark · 2017-04-07T16:56:37Z

src/userprog/syscall.c

+static int      syscall_write    (void ** args);
+static void     syscall_seek     (void ** args);
+static unsigned syscall_tell     (void ** args);
+static void     syscall_close    (void ** args);


Don't use vertical aligning.

hangpark · 2017-04-07T17:02:49Z

src/userprog/syscall.c

+static int      syscall_write    (void ** args);
+static void     syscall_seek     (void ** args);
+static unsigned syscall_tell     (void ** args);
+static void     syscall_close    (void ** args);


Above prototypes should move to line 12.

hangpark · 2017-04-07T17:05:16Z

src/userprog/syscall.c

+        syscall_exit(args);
+        break;
+      case SYS_EXEC:
+        f->eax = syscall_exec (void ** args);


Why does each of your f->eax = syscall_xxxx (void ** args); lines has void ** ?

hangpark · 2017-04-07T17:06:20Z

src/userprog/syscall.c

+
+static void syscall_halt (void)
+{
+  power_off();


Why is this implemented? It's not for this issue.

hangpark · 2017-04-07T17:12:23Z

I asked you to make your infrastructure helps other contributor to implement each system call without thinking other things. It means if I'm the assignee for implementing write() system call, your infrastructure should give me an environment that I can start with int fd, const void *buffer, and unsigned size, NOT with void **args.

hangpark · 2017-04-07T17:13:40Z

src/userprog/syscall.c

+static void     syscall_seek     (void ** args);
+static unsigned syscall_tell     (void ** args);
+static void     syscall_close    (void ** args);
+
 static void
 syscall_handler (struct intr_frame *f UNUSED) 


Comment is missing.

hangpark · 2017-04-07T17:15:02Z

Let me review later for other things about your logic, algorithm, and implementation AFTER you fix above requests.

hangpark · 2017-04-07T18:11:24Z

src/userprog/syscall.c

 static void
 syscall_handler (struct intr_frame *f UNUSED) 
 {
-  printf ("system call!\n");
-  thread_exit ();
+  enum old_level = intr_disable();


Is there any reference for why interrupt should be disable? I think syscall_handler invokes in interrupt already.

In 31 pg of pintos manual, it says Your system call implementation must treat the file system code as a critical section. I regarded it as 'Syscall handler must be in mutally excluded state, by using locks, semaphores, monitors, etc... or, disabling interrupt.' Am I totally wrong?

Read carefully.

You can get intr level and check whether exec is by intr or not in handler.

@Lment Check this plz.

@hangpark Now I think interrupt should not be turned off. Instead it should be handled with locks and semaphores. And also, intr_disable() cannot turn off synchronous internal interrupts, as memtioned in pintos manual. 70pg.

hangpark · 2017-04-07T18:14:21Z

src/userprog/syscall.c

-  thread_exit ();
+  enum old_level = intr_disable();
+  int syscall_num = *((int *) f->esp);    
+  void **args = (void **) malloc (3 * WORD_SIZE);


Is there any reason for allocating memory to store arguments? It is already put on the stack. Why don't you just use that?

Then, there would be *(esp + 1), *(esp +2), *(esp + 3), instead of *args[0], *args[1], *args[2]. I thought argv-statement more intuitive to viewers, If you strongly recommends, I would get rid of these.

Kernel programming should use recource minimally.

hangpark · 2017-04-07T18:18:14Z

src/userprog/syscall.c

 static void
 syscall_handler (struct intr_frame *f UNUSED) 
 {
-  printf ("system call!\n");
-  thread_exit ();


I think thread_exit() should be called if none of system call matched. So I request you to move this to default clause of theswitch statement.

Seems to be commented on deleted region. Default case now has thread_exit() in it,

hangpark

Please follow the coding convention.

hangpark · 2017-04-07T18:20:39Z

src/userprog/syscall.c

+  switch (syscall_num)
+    {
+    case SYS_HALT:
+      syscall_halt();


hangpark · 2017-04-07T18:20:46Z

src/userprog/syscall.c

+      syscall_halt();
+      break;
+    case SYS_EXIT:
+      syscall_exit((int) *args[0]);


hangpark · 2017-04-07T18:21:25Z

src/userprog/syscall.c

+      syscall_close ((int) *args[0]);
+      break;
+    }
+  intr_set_level(old_level);


hangpark · 2017-04-07T18:22:04Z

src/userprog/syscall.c

 static void
 syscall_handler (struct intr_frame *f UNUSED) 
 {
-  printf ("system call!\n");
-  thread_exit ();
+  enum old_level = intr_disable();


hangpark · 2017-04-07T19:50:52Z

src/userprog/syscall.c

+      syscall_close ((int) *args[0]);
+      break;
+    default:
+      printf ("system call!\n");


This line seems to be not neccessary.

hangpark · 2017-04-08T06:11:29Z

src/userprog/syscall.c

+#include "threads/init.h"
+#include "threads/malloc.h"
+
+#define WORD_SIZE sizeof (uintptr_t)


Is this in use?

hangpark · 2017-04-08T06:12:47Z

src/userprog/syscall.c

+#include "threads/malloc.h"
+
+#define WORD_SIZE sizeof (uintptr_t)
+#define pid_t int


Why did you define pid_t customly?

According to here, this states that pid_t should be in header files <unistd.h> and <sys/types.h>, but compiler says that it can't find both of them. So, I followed "The pid_t data type is a signed integer type which is capable of representing a process ID. In the GNU C Library, this is an int", and customly defined it, to avoid compiler errors shouting that it could not find type pid_t.

Your link is for GNU C. Our pintos's library is in src/lib/. See here.

Implement the following system calls. The prototypes listed are those seen by a user program that includes ‘lib/user/syscall.h’. (This header, and all others in ‘lib/user’, are for use by user programs only.) - pintos manual 28pg. Linked file is inside dir 'user', which I thought I should not include. I looked for other files that include pid_t definition, but nothing else came up, so I decided I should customly define it.

hangpark · 2017-04-08T06:14:07Z

src/userprog/syscall.c

+
+  /* array saving arguments that could or could not be
+     placed on esp + 4, esp + 8, esp + 12. */
+


hangpark · 2017-04-08T08:36:11Z

src/userprog/syscall.c

 #include "threads/interrupt.h"
 #include "threads/thread.h"
+#include "threads/init.h"
+#include "threads/malloc.h"
+#include <unistd.h>


Can you explain why are these new includes necessary for your implementation? On my env, building without these 5 includes is in success.

hangpark · 2017-04-08T09:51:59Z

src/userprog/syscall.c

 void
 syscall_init (void) 
 {
  intr_register_int (0x30, 3, INTR_ON, syscall_handler, "syscall");
+  lock_init(&sys_lock);


I think this lock is used for file system interactions. I mean, not necessary in general. It would be inserted when it becomes needed in a specific system call. So I recommend you to delete it.

Order of execution becomes important such as read(), write(). Yes, they do not interfere with each other directly, but they could read/write to same memory space, and then order of execution would be significant to results. So I think we should use lock.

Also, according to one of OS TA, she said she applied lock to handlers while taking this course. Testing this synchronization problem might not be in test cases, but it would be better solution, i suppose.

I also used lock last year, and that is for filesystem. Pintos must let at most one thread use the file system at the specific time. Some of system calls should be able to use the file system, and also should process_execute(). But, for other system calls, is this lock still needed? I don't think so.

Oh, I'm so sorry. You just initialized lock, not acquired. Then, if you explain the usage for this lock, then I will follow you.

It means, you should give a specific explanation above the line 10.

+) I suggest you to switch the order of execution for lock_init() and intr_register_int() to each other.

+) If you want to use sys_lock, consider whether it should be static or not. If it is not need to be static, then it should moved to the header file to make other source files can use it.

@hangpark Re-pushed, lock defined in header file as you suggested.

hangpark · 2017-04-09T09:39:18Z

src/userprog/syscall.c

+      break;
+    default:
+      thread_exit ();
+      break;


Don't need this.

hangpark · 2017-04-09T09:40:21Z

src/userprog/syscall.c

+   which acquired a lock is calling a file system code. This lock must be
+   released immediately when a thread is no longer running a code which 
+   must be executed in mutually exclueded state. */
+static struct lock sys_lock;


You said that you move this to the header file in the comment above, but it is still in c file.

hangpark · 2017-04-09T09:41:08Z

src/userprog/syscall.c

+/* Structure lock defined in synch.h, which would ensure only one thread
+   which acquired a lock is calling a file system code. This lock must be
+   released immediately when a thread is no longer running a code which 
+   must be executed in mutually exclueded state. */


Why do you explain lock is defined in synch.h and its functionality? Nobody cares it. Please summary.

hangpark

Where is your detail commit message?

Basic system call infrastructure, which dispatches appropriate handler to specific system call. Detailed handlers that handle system calls are not yet implemented.

hangpark · 2017-04-09T10:12:54Z

👍

Lment requested a review from hangpark April 7, 2017 16:46

Lment added the feature label Apr 7, 2017

Lment added this to the Project #2 milestone Apr 7, 2017

hangpark requested changes Apr 7, 2017

View reviewed changes

Lment force-pushed the iss/6 branch from 2880e5c to a16b13d Compare April 7, 2017 18:06

hangpark assigned Lment Apr 7, 2017

hangpark reviewed Apr 7, 2017

View reviewed changes

hangpark requested changes Apr 7, 2017

View reviewed changes

Lment force-pushed the iss/6 branch 2 times, most recently from 18a87f3 to 29623fd Compare April 7, 2017 19:48

hangpark requested changes Apr 7, 2017

View reviewed changes

Lment force-pushed the iss/6 branch from 29623fd to a449b7f Compare April 7, 2017 21:00

hangpark requested changes Apr 8, 2017

View reviewed changes

Lment force-pushed the iss/6 branch from a449b7f to 45b612e Compare April 8, 2017 07:01

hangpark requested changes Apr 8, 2017

View reviewed changes

Lment force-pushed the iss/6 branch 2 times, most recently from 8d63a06 to c9c9158 Compare April 8, 2017 09:48

hangpark requested changes Apr 8, 2017

View reviewed changes

Lment force-pushed the iss/6 branch 4 times, most recently from 458dd51 to d1c431e Compare April 9, 2017 04:23

hangpark requested changes Apr 9, 2017

View reviewed changes

Lment force-pushed the iss/6 branch from d1c431e to c9f2ea4 Compare April 9, 2017 09:52

[#6] Basic syscall handler and skeleton for each syscall

4bac2e9

Basic system call infrastructure, which dispatches appropriate handler to specific system call. Detailed handlers that handle system calls are not yet implemented.

Lment force-pushed the iss/6 branch from c9f2ea4 to 4bac2e9 Compare April 9, 2017 09:57

hangpark approved these changes Apr 9, 2017

View reviewed changes

hangpark merged commit 3f74056 into develop Apr 9, 2017

hangpark deleted the iss/6 branch April 9, 2017 10:13

hangpark mentioned this pull request Apr 9, 2017

Implement system call infrastructure #6

Closed

hangpark unassigned Lment Apr 10, 2017

hangpark mentioned this pull request Apr 14, 2017

Release v2.0 #89

Merged

hangpark mentioned this pull request May 11, 2017

Release v3.0 #130

Merged


		/* array saving arguments that could or could not be
		placed on esp + 4, esp + 8, esp + 12. */

[#6] Basic syscall handler and skeleton for each syscall #42

[#6] Basic syscall handler and skeleton for each syscall #42

Conversation

Lment commented Apr 7, 2017

hangpark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hangpark commented Apr 7, 2017

Choose a reason for hiding this comment

hangpark commented Apr 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lment Apr 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hangpark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hangpark Apr 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lment Apr 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lment Apr 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hangpark Apr 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hangpark Apr 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hangpark left a comment

Choose a reason for hiding this comment

hangpark commented Apr 9, 2017

hangpark commented Apr 7, 2017 •

edited

Loading

Lment Apr 8, 2017 •

edited

Loading

hangpark Apr 8, 2017 •

edited

Loading

Lment Apr 8, 2017 •

edited

Loading

Lment Apr 8, 2017 •

edited

Loading

hangpark Apr 8, 2017 •

edited

Loading

hangpark Apr 8, 2017 •

edited

Loading