Code refactor

3 files changed, 72 insertions, 121 deletions

diff --git a/command.c b/command.c
index 041a9c1..444a800 100644
--- a/command.c
+++ b/command.c
@@ -113,23 +113,21 @@ noreturn void external_command(char **argv) {
   if (!index(argv[0], '/') && path) {
     /* TODO: For all paths in PATH construct an absolute path and execve it. */
 #ifdef STUDENT
-    const char *dir = path;
-    const char *dir_end;
-    char *absolute_path = alloca(strlen(path) + strlen(argv[0]));
-    /* MY-TODO: this is ugly. UPDATE: BUT IT WORKS! */
+    char *exec_path;
+    const int command_len = strlen(argv[0]);
     do {
-      dir_end = index(dir, ':');
-      int pnt = 0;
-      while (*dir && dir != dir_end) {
-        absolute_path[pnt++] = *dir++;
+      int dir_len = strcspn(path, ":");
+      /* <dir> + "/" + <command> + "\0" */
+      exec_path = malloc(dir_len + 1 + command_len + 1);
+      sprintf(exec_path, "%.*s/%s", dir_len, path, argv[0]);
+      exec_path[dir_len + 1 + command_len] = 0;
+      execve(exec_path, argv, environ);
+      free(exec_path);
+      path += dir_len;
+      if (*path) { /* *path == ':' */
+        path++;
       }
-      if (dir_end)
-        dir_end++;
-      absolute_path[pnt] = 0;
-      strcat(absolute_path, "/");
-      strcat(absolute_path, argv[0]);
-      (void)execve(absolute_path, argv, environ);
-    } while ((dir = dir_end));
+    } while (*path);
 #endif /* !STUDENT */
   } else {
     (void)execve(argv[0], argv, environ);
diff --git a/jobs.c b/jobs.c
index da81500..99a990d 100644
--- a/jobs.c
+++ b/jobs.c
@@ -27,17 +27,11 @@ static void sigchld_handler(int sig) {
   /* TODO: Change state (FINISHED, RUNNING, STOPPED) of processes and jobs.
    * Bury all children that finished saving their status in jobs. */
 #ifdef STUDENT
-  /* MY-TODO: we waitpid all processes, but what if there is nothing to wait
-   * for and some child dies in some code after the while? Will we reap it? */
-
-  job_t *cur_job;
   proc_t *cur_proc;
-  // msg("sig> Hello from signal!\n");
+  job_t *cur_job;
   /* Change state of each process */
   while (true) {
     pid = waitpid(-1, &status, WCONTINUED | WUNTRACED | WNOHANG);
-    /* MY-TODO: Maybe some other error can occur? */
-    // msg("sig> XD %d\n", pid);
     if (pid == 0 || (pid < 0 && errno == ECHILD))
       break;
     for (int j = 0; j < njobmax; j++) {
@@ -47,12 +41,9 @@ static void sigchld_handler(int sig) {
       for (int p = 0; p < cur_job->nproc; p++) {
         cur_proc = &cur_job->proc[p];
         if (cur_proc->pid == pid) {
-          // msg("What happened to the child?\n");
           if (WIFSTOPPED(status)) {
             cur_proc->state = STOPPED;
-            // msg("Stopped\n");
           } else if (WIFCONTINUED(status)) {
-            // msg("Continued\n");
             cur_proc->state = RUNNING;
           } else {
             cur_proc->state = FINISHED;
@@ -66,17 +57,10 @@ static void sigchld_handler(int sig) {
     }
   }
 
-  /* Go through jobs list and change jobs' states only if
-   * every process in given job has the same state
-   * OR if all states other than finished are the same!
-   */
-
-  /* MY-TODO: this is not correct: what if two processes are finished,
-   * and one is suspended in a job? Then the job should change state
-   * to suspended for sure
-   * Maybe move this logic to jobstate?
-   * Nope
-   * Should be correct now!
+  /* I changed a little bit what Cahir wanted
+   * - If any process is running, then job is considered running
+   * - Else if any process is suspended, then job is suspended
+   * - Else job is finished
    */
   for (int j = 0; j < njobmax; j++) {
     cur_job = jobs + j;
@@ -89,17 +73,14 @@ static void sigchld_handler(int sig) {
       if (cur_proc->state == RUNNING)
         is_running = true;
       if (cur_proc->state == STOPPED) {
-        // msg("Henlo :)\n");
         is_stopped = true;
       }
     }
     if (is_running) {
       cur_job->state = RUNNING;
     } else if (is_stopped) {
-      // msg("Henlo\n");
       cur_job->state = STOPPED;
     } else {
-      // msg("Henlo????\n");
       cur_job->state = FINISHED;
     }
   }
@@ -191,7 +172,6 @@ static int jobstate(int j, int *statusp) {
 
   /* TODO: Handle case where job has finished. */
 #ifdef STUDENT
-  /* MY-TODO: shouldn't we go through the processes list and check the state? */
   if (state == FINISHED) {
     *statusp = exitcode(job);
     deljob(job);
@@ -225,12 +205,11 @@ bool resumejob(int j, int bg, sigset_t *mask) {
     return false;
 
   printf("[%d] continue \'%s\'\n", j, job->command);
+  /* This is awkward, but it has to be called in this order */
   if (bg == FG) {
     setfgpgrp(job->pgid);
   }
   Kill(-job->pgid, SIGCONT);
-  /* MY-TODO: what if there is some job in the foreground?
-   * I think it shouldn't happen, but what if? */
   if (bg == FG) {
     movejob(j, FG);
     monitorjob(mask);
@@ -248,10 +227,6 @@ bool killjob(int j) {
 
   /* TODO: I love the smell of napalm in the morning. */
 #ifdef STUDENT
-  /* MY-TODO: shouldn't we check if jobs[j].pgid != 0?
-   * answer: yeah, we should!
-   * kontr-answer: or do we?
-   */
   if (jobs[j].pgid == 0)
     return false;
   /* If job is stopped, then it won't handle the SIGTERM */
@@ -270,7 +245,6 @@ void watchjobs(int which) {
 
       /* TODO: Report job number, state, command and exit code or signal. */
 #ifdef STUDENT
-/* MY-TODO: Maybe this can be done cleaner? */
 #define KILLED 3
     const char *state_to_name[] = {"exited", "running", "suspended", "killed"};
     sigset_t mask;
@@ -278,29 +252,25 @@ void watchjobs(int which) {
     /* MY-TODO: shouldn't we do this outside of the loop? Ask Cahir */
     Sigprocmask(SIG_BLOCK, &sigchld_mask, &mask);
 
-    job_t *job = &jobs[j];
-    int status = 0;
-    int state = job->state;
+    /* Jobstate potentially deletes the job, so we have to copy its command */
+    char *command = strdup(jobs[j].command);
+    int status;
+    int state = jobstate(j, &status);
     if (which == ALL || state == which) {
-      /* Potentially deleting this job, so we have to copy the command */
-      if (state == FINISHED) {
-        status = exitcode(job);
-        if (status > 128) {
-          state = KILLED;
-        }
+      if (state == FINISHED && status > 128) {
+        state = KILLED;
       }
 
-      printf("[%d] %s \'%s\'", j, state_to_name[state], job->command);
+      printf("[%d] %s \'%s\'", j, state_to_name[state], command);
       if (state == KILLED) {
         printf(" by signal %d", status - 128);
-        deljob(job);
       }
       if (state == FINISHED) {
         printf(", status=%d", status);
-        deljob(job);
       }
       printf("\n");
     }
+    free(command);
     Sigprocmask(SIG_SETMASK, &mask, NULL);
 #endif /* !STUDENT */
   }
@@ -313,21 +283,16 @@ int monitorjob(sigset_t *mask) {
 
   /* TODO: Following code requires use of Tcsetpgrp of tty_fd. */
 #ifdef STUDENT
-  /* MY-TODO: don't we have to check for jobs[FG] existance first? */
-  /* MY-TODO: use state somehow */
-  /* MY-TODO: we probably want to block sigchld, that's what mask is for */
-
+  /* Don't need to change fg group of terminal because we do it
+   * in the child process in fork.
+   * Tcsetpgrp is required only when taking back the control over terminal */
   job_t *j = &jobs[FG];
-  Tcsetpgrp(tty_fd, j->pgid);
 
   do {
-    // msg("> Suspedning\n");
     Sigsuspend(mask);
-    // msg("> Resumed\n");
     state = jobstate(FG, &exitcode);
   } while (state == RUNNING);
 
-  // msg("> State: %d\n", state);
   if (state == STOPPED) {
     int new_j = addjob(0, BG);
     j = &jobs[new_j];
@@ -337,7 +302,6 @@ int monitorjob(sigset_t *mask) {
   Tcgetattr(tty_fd, &j->tmodes);
   Tcsetattr(tty_fd, TCSADRAIN, &shell_tmodes);
   Tcsetpgrp(tty_fd, getpid());
-
 #endif /* !STUDENT */
 
   return exitcode;
@@ -378,9 +342,7 @@ void shutdownjobs(void) {
 
   /* TODO: Kill remaining jobs and wait for them to finish. */
 #ifdef STUDENT
-  /* MY-TODO: if job is stopped, then we cannot kill it :P */
   for (int j = 0; j < njobmax; j++) {
-
     if (!jobs[j].pgid) {
       continue;
     }
@@ -388,17 +350,16 @@ void shutdownjobs(void) {
     /* This FOR SURE shouldn't be here
      * What if some job ignores sigterm?
      * This is here only because of test_kill_at_quit
+     *
+     * Another unexpected behaviour due to this:
+     * # cat
+     * ^Z
+     * # kill %1
+     * # ^D
+     * <shell freezes>
      */
     sigsuspend(&mask);
   }
-
-  /* MY-TODO: what if some job get the signal after the wait?
-   * We shouldn't wait here i think :/
-   * Nope, we should. Just wait for every specific job.
-   */
-  // int ret;
-  // while ((ret = wait(NULL)) > 0)
-  //   ;
 #endif /* !STUDENT */
 
   watchjobs(FINISHED);
diff --git a/shell.c b/shell.c
index 38f6691..5330782 100644
--- a/shell.c
+++ b/shell.c
@@ -31,30 +31,20 @@ static int do_redir(token_t *token, int ntokens, int *inputp, int *outputp) {
   for (int i = 0; i < ntokens; i++) {
     /* TODO: Handle tokens and open files as requested. */
 #ifdef STUDENT
-    /* MY-TODO: I assume that tokens are only WORD, T_INPUT, T_OUTPUT. */
-    /* MY-TODO: MaybeClose? What for? */
-    // msg("do_redir: ntokens: %d ", ntokens);
-    // for (int i = 0; i < ntokens; i++) {
-    //   msg("%s ", token[i]);
-    // }
-    // msg("\n");
+    /* I assume that tokens are only WORD, T_INPUT, T_OUTPUT and T_NULL. */
     mode = token[i];
     if (mode == T_INPUT) {
-      // MaybeClose(STDIN_FILENO);
+      MaybeClose(inputp);
       *inputp = Open(token[i + 1], O_RDONLY, 0);
       token[i] = T_NULL;
       token[i + 1] = T_NULL;
     } else if (mode == T_OUTPUT) {
-      // MaybeClose(STDOUT_FILENO);
+      MaybeClose(outputp);
       *outputp = Open(token[i + 1], O_WRONLY | O_TRUNC | O_CREAT, 0644);
       token[i] = T_NULL;
       token[i + 1] = T_NULL;
-    } else if (mode != T_NULL) {
-      token[n] = token[i];
-      if (i != n)
-        token[i] = T_NULL;
+    } else if (mode != T_NULL)
       n++;
-    }
 #endif /* !STUDENT */
   }
 
@@ -80,37 +70,34 @@ static int do_job(token_t *token, int ntokens, bool bg) {
 
   /* TODO: Start a subprocess, create a job and monitor it. */
 #ifdef STUDENT
-  /* MY-TODO: we have to copy the tokens to some other array */
-  // msg("do_job after do_redir: %d ", ntokens);
-  // for (int i = 0; i < ntokens; i++) {
-  //   msg("%s ", token[i]);
-  // }
-  // msg("\n");
-  // msg("Whadupp, sigchld is blocked!\n");
   pid_t pid = Fork();
   if (pid) { /* shell */
-    /* This can result with an error: child can not yet execute
-     * and therefore setpgid will error with errno set to EPERM
-     */
+    /* This can result with an EACCESS */
     setpgid(pid, pid);
+
     MaybeClose(&input);
     MaybeClose(&output);
     int j = addjob(pid, bg);
     addproc(j, pid, token);
+
     if (bg == FG)
       exitcode = monitorjob(&mask);
     else {
       watchjobs(RUNNING);
     }
   } else { /* job */
-    /* Reset signal mask */
+    Setpgid(0, 0);
     if (bg == FG) {
       setfgpgrp(getpid());
     }
+
+    /* Reset signal mask */
     Signal(SIGINT, SIG_DFL);
     Signal(SIGTSTP, SIG_DFL);
     Signal(SIGTTIN, SIG_DFL);
     Signal(SIGTTOU, SIG_DFL);
+    Signal(SIGCHLD, SIG_DFL);
+
     if (input != -1) {
       Dup2(input, STDIN_FILENO);
       MaybeClose(&input);
@@ -119,7 +106,6 @@ static int do_job(token_t *token, int ntokens, bool bg) {
       Dup2(output, STDOUT_FILENO);
       MaybeClose(&output);
     }
-    Setpgid(0, 0);
     external_command(token);
   }
 #endif /* !STUDENT */
@@ -156,10 +142,13 @@ static pid_t do_stage(pid_t pgid, sigset_t *mask, int input, int output,
       }
     }
     Setpgid(pid, pgid);
+
     Signal(SIGINT, SIG_DFL);
     Signal(SIGTSTP, SIG_DFL);
     Signal(SIGTTIN, SIG_DFL);
     Signal(SIGTTOU, SIG_DFL);
+    Signal(SIGCHLD, SIG_DFL);
+
     if (input != -1) {
       Dup2(input, STDIN_FILENO);
       MaybeClose(&input);
@@ -201,43 +190,46 @@ static int do_pipeline(token_t *token, int ntokens, bool bg) {
   /* TODO: Start pipeline subprocesses, create a job and monitor it.
    * Remember to close unused pipe ends! */
 #ifdef STUDENT
-  // (void)input;
-  // (void)job;
-  // (void)pid;
-  // (void)pgid;
-  // (void)do_stage;
   int stage_begin = 0;
-  int pipe_where = 0;
+  int t_pipe_idx = 0;
   while (stage_begin < ntokens) {
-    while (pipe_where < ntokens && token[pipe_where] != T_PIPE)
-      pipe_where++;
-    if (pipe_where < ntokens) {
-      token[pipe_where] = T_NULL;
+    while (t_pipe_idx < ntokens && token[t_pipe_idx] != T_PIPE)
+      t_pipe_idx++;
+    if (t_pipe_idx < ntokens) {
+      token[t_pipe_idx] = T_NULL;
     }
+
     /* If this is the last job in the pipeline, then don't redir the output */
-    if (pipe_where >= ntokens) {
+    if (t_pipe_idx >= ntokens) {
       MaybeClose(&output);
       output = -1;
     }
+
     pid = do_stage(pgid, &mask, input, output, token + stage_begin,
-                   pipe_where - stage_begin, bg);
-    /* MY-TODO: close those fds somehow, but how?? */
-    // MaybeClose(&input);
-    // MaybeClose(&output);
+                   t_pipe_idx - stage_begin, bg);
+
+    /* If pgid == 0, then this is the first process in the pipeline
+     * so set pgid to its pid and create a job */
     if (!pgid) {
       pgid = pid;
       job = addjob(pgid, bg);
     }
+
     addproc(job, pid, token + stage_begin);
-    stage_begin = ++pipe_where;
+    stage_begin = ++t_pipe_idx;
     input = next_input;
     mkpipe(&next_input, &output);
   }
+
+  /* All those are open now */
   MaybeClose(&next_input);
   MaybeClose(&output);
   MaybeClose(&input);
+
   if (bg == FG) {
-    monitorjob(&mask);
+    exitcode = monitorjob(&mask);
+  } else {
+    watchjobs(RUNNING);
   }
 #endif /* !STUDENT */