refactor syscall code

6 files changed, 193 insertions, 243 deletions

diff --git a/defs.h b/defs.h
index fb48700..9babf92 100644
--- a/defs.h
+++ b/defs.h
@@ -16,7 +16,7 @@ struct jmpbuf;
 void setupsegs(struct proc*);
 struct proc* copyproc(struct proc*);
 struct spinlock;
-uint growproc(int);
+int growproc(int);
 void sleep(void*, struct spinlock*);
 void wakeup(void*);
 void scheduler(void);
@@ -43,10 +43,10 @@ int strncmp(const char*, const char*, uint);
 // syscall.c
 void syscall(void);
 int fetchint(struct proc*, uint, int*);
-int fetchbyte(struct proc*, uint, char*);
-int fetcharg(int, void*);
-int checkstring(uint);
-int putint(struct proc*, uint, int);
+int fetchstr(struct proc*, uint, char**);
+int argint(int, int*);
+int argptr(int, char**, int);
+int argstr(int, char**);
 
 // picirq.c
 extern ushort irq_mask_8259A;
@@ -99,7 +99,6 @@ int pipe_read(struct pipe*, char*, int);
 // file.c
 struct stat;
 void fileinit(void);
-int fdalloc(void);
 struct file* filealloc(void);
 void fileclose(struct file*);
 int fileread(struct file*, char*, int n);
diff --git a/file.c b/file.c
index 29f07cc..4c314ec 100644
--- a/file.c
+++ b/file.c
@@ -22,19 +22,7 @@ fileinit(void)
   initlock(&fd_table_lock, "fd_table");
 }
 
-// Allocate a file descriptor number for curproc.
-int
-fdalloc(void)
-{
-  int fd;
-  struct proc *p = curproc[cpu()];
-  for(fd = 0; fd < NOFILE; fd++)
-    if(p->ofile[fd] == 0)
-      return fd;
-  return -1;
-}
-
-// Allocate a file descriptor structure
+// Allocate a file structure
 struct file*
 filealloc(void)
 {
diff --git a/syscall.c b/syscall.c
index ee22d09..8f7350f 100644
--- a/syscall.c
+++ b/syscall.c
@@ -21,64 +21,71 @@
 // library system call function. The saved user %esp points
 // to a saved program counter, and then the first argument.
 
-// Fetch 32 bits from a user-supplied pointer.
-// Returns 0 if addr was OK, -1 if illegal.
+// Fetch the int at addr from process p.
 int
 fetchint(struct proc *p, uint addr, int *ip)
 {
-  *ip = 0;
-
-  if(addr > p->sz - 4)
+  if(addr >= p->sz || addr+4 > p->sz)
     return -1;
   *ip = *(int*)(p->mem + addr);
   return 0;
 }
 
-// Fetch byte from a user-supplied pointer.
-// Returns 0 on success, -1 if pointer is illegal.
+// Fetch the nul-terminated string at addr from process p.
+// Doesn't actually copy the string - just sets *pp to point at it.
+// Returns length of string, not including nul.
 int
-fetchbyte(struct proc *p, uint addr, char *c)
+fetchstr(struct proc *p, uint addr, char **pp)
 {
+  char *cp, *ep;
+
   if(addr >= p->sz)
     return -1;
-  *c = *(p->mem + addr);
-  return 0;
+  *pp = p->mem + addr;
+  ep = p->mem + p->sz;
+  for(cp = *pp; cp < ep; cp++)
+    if(*cp == 0)
+      return cp - *pp;
+  return -1;
 }
 
+// Fetch the argno'th word-sized system call argument as an integer.
 int
-fetcharg(int argno, void *ip)
+argint(int argno, int *ip)
 {
-  uint esp;
+  struct proc *p = curproc[cpu()];
 
-  esp = (uint) curproc[cpu()]->tf->esp;
-  return fetchint(curproc[cpu()], esp + 4 + 4*argno, ip);
+  return fetchint(p, p->tf->esp + 4 + 4*argno, ip);
 }
 
-// Check that an entire string is valid in user space.
-// Returns the length, not including null, or -1.
+// Fetch the nth word-sized system call argument as a pointer
+// to a block of memory of size n bytes.  Check that the pointer
+// lies within the process address space.
 int
-checkstring(uint s)
+argptr(int argno, char **pp, int size)
 {
-  char c;
-  int len = 0;
-
-  for(;;){
-    if(fetchbyte(curproc[cpu()], s, &c) < 0)
-      return -1;
-    if(c == '\0')
-      return len;
-    len++;
-    s++;
-  }
+  int i;
+  struct proc *p = curproc[cpu()];
+  
+  if(argint(argno, &i) < 0)
+    return -1;
+  if((uint)i >= p->sz || (uint)i+size >= p->sz)
+    return -1;
+  *pp = p->mem + i;
+  return 0;
 }
 
+// Fetch the nth word-sized system call argument as a string pointer.
+// Check that the pointer is valid and the string is nul-terminated.
+// (There is no shared writable memory, so the string can't change
+// between this check and being used by the kernel.)
 int
-putint(struct proc *p, uint addr, int x)
+argstr(int argno, char **pp)
 {
-  if(addr > p->sz - 4)
+  int addr;
+  if(argint(argno, &addr) < 0)
     return -1;
-  memmove(p->mem + addr, &x, 4);
-  return 0;
+  return fetchstr(curproc[cpu()], addr, pp);
 }
 
 extern int sys_chdir(void);
diff --git a/syscall.h b/syscall.h
index a0279ae..c4e90ab 100644
--- a/syscall.h
+++ b/syscall.h
@@ -1,3 +1,4 @@
+// System call numbers
 #define SYS_fork    1
 #define SYS_exit    2
 #define SYS_wait    3
diff --git a/sysfile.c b/sysfile.c
index be09c43..912ca7f 100644
--- a/sysfile.c
+++ b/sysfile.c
@@ -15,120 +15,117 @@
 #include "file.h"
 #include "fcntl.h"
 
-int
-sys_pipe(void)
+// Fetch the nth word-sized system call argument as a file descriptor
+// and return both the descriptor and the corresponding struct file.
+static int
+argfd(int argno, int *pfd, struct file **pf)
 {
-  struct file *rfd = 0, *wfd = 0;
-  int f1 = -1, f2 = -1;
+  int fd;
+  struct file *f;
   struct proc *p = curproc[cpu()];
-  uint fdp;
-
-  if(pipe_alloc(&rfd, &wfd) < 0)
-    goto oops;
-  if((f1 = fdalloc()) < 0)
-    goto oops;
-  p->ofile[f1] = rfd;
-  if((f2 = fdalloc()) < 0)
-    goto oops;
-  p->ofile[f2] = wfd;
-  if(fetcharg(0, &fdp) < 0)
-    goto oops;
-  if(putint(p, fdp, f1) < 0)
-    goto oops;
-  if(putint(p, fdp+4, f2) < 0)
-    goto oops;
+
+  if(argint(argno, &fd) < 0)
+    return -1;
+  if(fd < 0 || fd >= NOFILE || (f=p->ofile[fd]) == 0)
+    return -1;
+  if(pfd)
+    *pfd = fd;
+  if(pf)
+    *pf = f;
   return 0;
+}
 
- oops:
-  if(rfd)
-    fileclose(rfd);
-  if(wfd)
-    fileclose(wfd);
-  if(f1 >= 0)
-    p->ofile[f1] = 0;
-  if(f2 >= 0)
-    p->ofile[f2] = 0;
+// Allocate a file descriptor for the given file.
+// Takes over file reference from caller on success.
+static int
+fdalloc(struct file *f)
+{
+  int fd;
+  struct proc *p = curproc[cpu()];
+  for(fd = 0; fd < NOFILE; fd++){
+    if(p->ofile[fd] == 0){
+      p->ofile[fd] = f;
+      return fd;
+    }
+  }
   return -1;
 }
 
 int
-sys_write(void)
+sys_pipe(void)
 {
-  int fd, n, ret;
-  uint addr;
+  int *fd;
+  struct file *rf = 0, *wf = 0;
+  int fd0, fd1;
   struct proc *p = curproc[cpu()];
 
-  if(fetcharg(0, &fd) < 0 ||
-     fetcharg(1, &addr) < 0 ||
-     fetcharg(2, &n) < 0)
-    return -1;
-  if(fd < 0 || fd >= NOFILE)
+  if(argptr(0, (void*)&fd, 2*sizeof fd[0]) < 0)
     return -1;
-  if(p->ofile[fd] == 0)
+  if(pipe_alloc(&rf, &wf) < 0)
     return -1;
-  if(addr + n > p->sz)
+  fd0 = -1;
+  if((fd0 = fdalloc(rf)) < 0 || (fd1 = fdalloc(wf)) < 0){
+    if(fd0 >= 0)
+      p->ofile[fd0] = 0;
+    fileclose(rf);
+    fileclose(wf);
     return -1;
+  }
+  return 0;
+}
+
+int
+sys_write(void)
+{
+  struct file *f;
+  int n;
+  char *cp;
 
-  ret = filewrite(p->ofile[fd], p->mem + addr, n);
-  return ret;
+  if(argfd(0, 0, &f) < 0 || argint(2, &n) < 0 || argptr(1, &cp, n) < 0)
+    return -1;
+  return filewrite(f, cp, n);
 }
 
 int
 sys_read(void)
 {
-  int fd, n, ret;
-  uint addr;
-  struct proc *p = curproc[cpu()];
+  struct file *f;
+  int n;
+  char *cp;
 
-  if(fetcharg(0, &fd) < 0 ||
-     fetcharg(1, &addr) < 0 ||
-     fetcharg(2, &n) < 0)
-    return -1;
-  if(fd < 0 || fd >= NOFILE)
-    return -1;
-  if(p->ofile[fd] == 0)
+  if(argfd(0, 0, &f) < 0 || argint(2, &n) < 0 || argptr(1, &cp, n) < 0)
     return -1;
-  if(addr + n > p->sz)
-    return -1;
-  ret = fileread(p->ofile[fd], p->mem + addr, n);
-  return ret;
+  return fileread(f, cp, n);
 }
 
 int
 sys_close(void)
 {
   int fd;
-  struct proc *p = curproc[cpu()];
-
-  if(fetcharg(0, &fd) < 0)
-    return -1;
-  if(fd < 0 || fd >= NOFILE)
+  struct file *f;
+  
+  if(argfd(0, &fd, &f) < 0)
     return -1;
-  if(p->ofile[fd] == 0)
-    return -1;
-  fileclose(p->ofile[fd]);
-  p->ofile[fd] = 0;
+  curproc[cpu()]->ofile[fd] = 0;
+  fileclose(f);
   return 0;
 }
 
 int
 sys_open(void)
 {
-  struct proc *cp = curproc[cpu()];
   struct inode *ip, *dp;
-  uint arg0, arg1;
-  int ufd;
-  struct file *fd;
-  int l;
+  char *path;
+  int omode;
+  int fd;
+  struct file *f;
   char *last;
 
-  if(fetcharg(0, &arg0) < 0 || fetcharg(1, &arg1) < 0)
-    return -1;
-  if((l = checkstring(arg0)) < 0)
+  if(argstr(0, &path) < 0 || argint(1, &omode) < 0)
     return -1;
 
-  if(arg1 & O_CREATE){
-    dp = namei(cp->mem + arg0, NAMEI_CREATE, 0, &last, &ip);
+  if(omode & O_CREATE){
+    dp = namei(path, NAMEI_CREATE, 0, &last, &ip);
     if(dp){
       ip = mknod1(dp, last, T_FILE, 0, 0);
       iput(dp);
@@ -141,86 +138,77 @@ sys_open(void)
       return -1;
     }
   } else {
-    ip = namei(cp->mem + arg0, NAMEI_LOOKUP, 0, 0, 0);
+    ip = namei(path, NAMEI_LOOKUP, 0, 0, 0);
     if(ip == 0)
       return -1;
   }
-  if(ip->type == T_DIR && ((arg1 & O_RDWR) || (arg1 & O_WRONLY))){
+  if(ip->type == T_DIR && ((omode & O_RDWR) || (omode & O_WRONLY))){
     iput(ip);
     return -1;
   }
 
-  if((fd = filealloc()) == 0){
+  if((f = filealloc()) == 0){
     iput(ip);
     return -1;
   }
-  if((ufd = fdalloc()) < 0){
+  if((fd = fdalloc(f)) < 0){
     iput(ip);
-    fileclose(fd);
+    fileclose(f);
     return -1;
   }
 
   iunlock(ip);
-  fd->type = FD_FILE;
-  if(arg1 & O_RDWR) {
-    fd->readable = 1;
-    fd->writable = 1;
-  } else if(arg1 & O_WRONLY) {
-    fd->readable = 0;
-    fd->writable = 1;
+  f->type = FD_FILE;
+  if(omode & O_RDWR) {
+    f->readable = 1;
+    f->writable = 1;
+  } else if(omode & O_WRONLY) {
+    f->readable = 0;
+    f->writable = 1;
   } else {
-    fd->readable = 1;
-    fd->writable = 0;
+    f->readable = 1;
+    f->writable = 0;
   }
-  fd->ip = ip;
-  fd->off = 0;
-  cp->ofile[ufd] = fd;
+  f->ip = ip;
+  f->off = 0;
 
-  return ufd;
+  return fd;
 }
 
 int
 sys_mknod(void)
 {
-  struct proc *cp = curproc[cpu()];
   struct inode *nip;
-  uint arg0, arg1, arg2, arg3;
-  int l;
-
-  if(fetcharg(0, &arg0) < 0 || fetcharg(1, &arg1) < 0 ||
-     fetcharg(2, &arg2) < 0 || fetcharg(3, &arg3) < 0)
+  char *path;
+  int len;
+  int type, major, minor;
+  
+  if((len=argstr(0, &path)) < 0 || argint(1, &type) < 0 ||
+     argint(2, &major) < 0 || argint(3, &minor) < 0)
     return -1;
 
-  if((l = checkstring(arg0)) < 0)
+  if(len >= DIRSIZ)
     return -1;
 
-  if(l >= DIRSIZ)
+  if((nip = mknod(path, type, major, minor)) == 0)
     return -1;
-
-  nip = mknod(cp->mem + arg0, (short) arg1, (short) arg2, (short) arg3);
-  if(nip)
-    iput(nip);
-  return (nip == 0) ? -1 : 0;
+  iput(nip);
+  return 0;
 }
 
 int
 sys_mkdir(void)
 {
-  struct proc *cp = curproc[cpu()];
   struct inode *nip;
   struct inode *dp;
-  uint arg0;
-  int l;
+  char *path;
   struct dirent de;
   char *last;
 
-  if(fetcharg(0, &arg0) < 0)
-    return -1;
-
-  if((l = checkstring(arg0)) < 0)
+  if(argstr(0, &path) < 0)
     return -1;
 
-  dp = namei(cp->mem + arg0, NAMEI_CREATE, 0, &last, 0);
+  dp = namei(path, NAMEI_CREATE, 0, &last, 0);
   if(dp == 0)
     return -1;
 
@@ -248,25 +236,20 @@ sys_mkdir(void)
   return 0;
 }
 
-
 int
 sys_chdir(void)
 {
-  struct proc *cp = curproc[cpu()];
+  struct proc *p = curproc[cpu()];
   struct inode *ip;
-    uint arg0;
-  int l;
-
-  if(fetcharg(0, &arg0) < 0)
-    return -1;
+  char *path;
 
-  if((l = checkstring(arg0)) < 0)
+  if(argstr(0, &path) < 0)
     return -1;
 
-  if((ip = namei(cp->mem + arg0, NAMEI_LOOKUP, 0, 0, 0)) == 0)
+  if((ip = namei(path, NAMEI_LOOKUP, 0, 0, 0)) == 0)
     return -1;
 
-  if(ip == cp->cwd) {
+  if(ip == p->cwd) {
     iput(ip);
     return 0;
   }
@@ -276,100 +259,74 @@ sys_chdir(void)
     return -1;
   }
 
-  idecref(cp->cwd);
-  cp->cwd = ip;
-  iunlock(cp->cwd);
+  idecref(p->cwd);
+  p->cwd = ip;
+  iunlock(p->cwd);
   return 0;
 }
 
 int
 sys_unlink(void)
 {
-  struct proc *cp = curproc[cpu()];
-  uint arg0;
-  int r;
-
-  if(fetcharg(0, &arg0) < 0)
+  char *path;
+  
+  if(argstr(0, &path) < 0)
     return -1;
-  if(checkstring(arg0) < 0)
-    return -1;
-  r = unlink(cp->mem + arg0);
-  return r;
+  return unlink(path);
 }
 
 int
 sys_fstat(void)
 {
-  struct proc *cp = curproc[cpu()];
-  uint fd, addr;
-  int r;
-
-  if(fetcharg(0, &fd) < 0)
-    return -1;
-  if(fetcharg(1, &addr) < 0)
-    return -1;
-  if(fd < 0 || fd >= NOFILE)
-    return -1;
-  if(cp->ofile[fd] == 0)
-    return -1;
-  if(addr + sizeof(struct stat) > cp->sz)
+  struct file *f;
+  struct stat *st;
+  
+  if(argfd(0, 0, &f) < 0 || argptr(1, (void*)&st, sizeof *st) < 0)
     return -1;
-  r = filestat(cp->ofile[fd], (struct stat*)(cp->mem + addr));
-  return r;
+  return filestat(f, st);
 }
 
 int
 sys_dup(void)
 {
-  struct proc *cp = curproc[cpu()];
-  uint fd, ufd1;
-
-  if(fetcharg(0, &fd) < 0)
-    return -1;
-  if(fd < 0 || fd >= NOFILE)
-    return -1;
-  if(cp->ofile[fd] == 0)
+  struct file *f;
+  int fd;
+  
+  if(argfd(0, 0, &f) < 0)
     return -1;
-  if((ufd1 = fdalloc()) < 0)
+  if((fd=fdalloc(f)) < 0)
     return -1;
-  cp->ofile[ufd1] = cp->ofile[fd];
-  fileincref(cp->ofile[ufd1]);
-  return ufd1;
+  fileincref(f);
+  return fd;
 }
 
 int
 sys_link(void)
 {
-  struct proc *cp = curproc[cpu()];
-  uint name1, name2;
-  int r;
-
-  if(fetcharg(0, &name1) < 0 || checkstring(name1) < 0)
-    return -1;
-  if(fetcharg(1, &name2) < 0 || checkstring(name2) < 0)
+  char *old, *new;
+  
+  if(argstr(0, &old) < 0 || argstr(1, &new) < 0)
     return -1;
-  r = link(cp->mem + name1, cp->mem + name2);
-  return r;
+  return link(old, new);
 }
 
 int
 sys_exec(void)
 {
   struct proc *cp = curproc[cpu()];
-  uint arg0, arg1, sz=0, ap, sp, p1, p2;
+  uint sz=0, ap, sp, p1, p2;
   int i, nargs, argbytes, len;
   struct inode *ip;
   struct elfhdr elf;
   struct proghdr ph;
   char *mem = 0;
-
-  if(fetcharg(0, &arg0) < 0)
-    return -1;
-  if(fetcharg(1, &arg1) < 0)
+  char *path, *s;
+  uint argv;
+  
+  if(argstr(0, &path) < 0 || argint(1, (int*)&argv) < 0)
     return -1;
-  if(checkstring(arg0) < 0)
-    return -1;
-  ip = namei(cp->mem + arg0, NAMEI_LOOKUP, 0, 0, 0);
+
+  ip = namei(path, NAMEI_LOOKUP, 0, 0, 0);
   if(ip == 0)
     return -1;
 
@@ -399,15 +356,14 @@ sys_exec(void)
     goto bad;
   memset(mem, 0, sz);
 
-  // arg1 is a pointer to an array of pointers to string.
   nargs = 0;
   argbytes = 0;
-  for(i = 0; ; i++){
-    if(fetchint(cp, arg1 + 4*i, &ap) < 0)
+  for(i = 0;; i++){
+    if(fetchint(cp, argv + 4*i, (int*)&ap) < 0)
       goto bad;
     if(ap == 0)
       break;
-    len = checkstring(ap);
+    len = fetchstr(cp, ap, &s);
     if(len < 0)
       goto bad;
     nargs++;
@@ -432,9 +388,9 @@ sys_exec(void)
   p1 = sp + 12;
   p2 = sp + 12 + (nargs + 1) * 4;
   for(i = 0; i < nargs; i++){
-    fetchint(cp, arg1 + 4*i, &ap);
-    len = checkstring(ap);
-    memmove(mem + p2, cp->mem + ap, len + 1);
+    fetchint(cp, argv + 4*i, (int*)&ap);
+    len = fetchstr(cp, ap, &s);
+    memmove(mem + p2, s, len + 1);
     *(uint*)(mem + p1) = p2;
     p1 += 4;
     p2 += len + 1;
diff --git a/sysproc.c b/sysproc.c
index 43ee973..14b85c5 100644
--- a/sysproc.c
+++ b/sysproc.c
@@ -44,7 +44,7 @@ sys_kill(void)
 {
   int pid;
 
-  if(fetcharg(0, &pid) < 0)
+  if(argint(0, &pid) < 0)
     return -1;
   return proc_kill(pid);
 }
@@ -52,20 +52,19 @@ sys_kill(void)
 int
 sys_getpid(void)
 {
-  struct proc *cp = curproc[cpu()];
-  return cp->pid;
+  return curproc[cpu()]->pid;
 }
 
 int
 sys_sbrk(void)
 {
-  uint addr;
+  int addr;
   int n;
   struct proc *cp = curproc[cpu()];
 
-  if(fetcharg(0, &n) < 0)
+  if(argint(0, &n) < 0)
     return -1;
-  if((addr = growproc(n)) == 0xffffffff)
+  if((addr = growproc(n)) < 0)
     return -1;
   setupsegs(cp);
   return addr;