Some comments. minor changes and a Makefile

.gitignore

@@ -1,4 +1,6 @@
 *Debug/*
+*bin/*
+*doc/*
 *.cproject
 *.project
 *.settings/*

Makefile

@@ -0,0 +1,48 @@
+#
+# Snel makefile
+#
+# 	 Author: Christos Choutouridis AEM:8997
+#	 email:  cchoutou@ece.auth.gr
+#
+
+CXX      := -c++
+CXXFLAGS := -std=c++14 -Wall -Wextra -Werror
+LDFLAGS  := -lstdc++
+BUILD    := ./bin
+OBJ_DIR  := $(BUILD)/obj
+APP_DIR  := $(BUILD)
+TARGET   := snel
+SRC      := $(wildcard src/*.cpp)
+
+OBJECTS := $(SRC:%.cpp=$(OBJ_DIR)/%.o)
+
+$(OBJ_DIR)/%.o: %.cpp
+	@mkdir -p $(@D)
+	$(CXX) $(CXXFLAGS) -o $@ -c $<
+
+$(APP_DIR)/$(TARGET): $(OBJECTS)
+	@mkdir -p $(@D)
+	$(CXX) $(CXXFLAGS) $(LDFLAGS) -o $(APP_DIR)/$(TARGET) $(OBJECTS)
+
+# === Rules ===
+snel: build $(APP_DIR)/$(TARGET)
+
+build:
+	@mkdir -p $(APP_DIR)
+	@mkdir -p $(OBJ_DIR)
+
+debug: CXXFLAGS += -DDEBUG -g3
+debug: snel
+
+release: CXXFLAGS += -O2
+release: snel
+
+all: release
+
+clean:
+	-@rm -rvf $(OBJ_DIR)/*
+	-@rm -rvf $(APP_DIR)/*
+
+.PHONY: snel build debug release all clean
+
+

src/Snel.cpp

@@ -1,32 +1,45 @@
+/*!
+ * \file
+ *   Snel.cpp
+ * \brief   A shell implementation for A.U.TH (Operating systems Lab)
+ *
+ *  Created on: Feb, 2019
+ *      Author: Christos Choutouridis AEM: 8997
+ *      email : cchoutou@ece.auth.gr
+ */
 #include "sequencer.h"
 
 
 int main(int argc, char* argv[]) try {
-   snel::Sequencer s{};
-   std::string line;
+   snel::Sequencer seq{};
+   std::string     line, filtered;
 
    if (argc > 1) {   // batch mode
       std::ifstream file(argv[1]);
 
       while (std::getline (file, line, '\n')) {
-         s.parse(snel::filter(line)).execute();
+         //^ We use \n as delimiter. No further parsing here
+         filtered = snel::filter(line);   // simple filtering
+         if (filtered == "quit")
+            break;
+         seq.parse(filtered).execute();
       }
    }
    else {   // interactive mode
-      std::cout << "Snel. A quick and dirty shell implementation for auth.gr" << std::endl;
+      std::cout << "Snel. A shell implementation for A.U.TH (OS Lab)." << std::endl;
       do {
          std::cout << "Choutouridis_8997>";
-         std::getline (std::cin, line, '\n');
-         if (line == "quit")
+         std::getline (std::cin, line, '\n');   //< We use \n as delimiter. No parsing here
+         filtered = snel::filter(line);         // simple filtering
+         if (filtered == "quit")
             break;
-         s.parse(snel::filter(line)).execute();
+         seq.parse(filtered).execute();
       } while (1);
    }
    return 0;
 
-}
-catch (std::exception& e) {
-   std::cerr << e.what() << '\n';
+} catch (std::exception& e) {
+   // we probably pollute the user's screen. Comment `cerr << ...` if you don't like it.
+   // std::cerr << e.what() << '\n';
    exit(1);
 }
-

src/sequencer.cpp

@@ -1,15 +1,16 @@
-/*
- * child.cpp
+/*!
+ * \file
+ *   Sequencer.cpp
+ * \brief   A basic sequence interpreter for snel
  *
- *  Created on: Feb 11, 2019
- *      Author: hoo2
+ *  Created on: Feb, 2019
+ *      Author: Christos Choutouridis AEM: 8997
+ *      email : cchoutou@ece.auth.gr
  */
-
 #include "sequencer.h"
 
 namespace snel {
 
-
    //! A type-safe memcpy
    template <typename T>
    void memcpy (T* to, const T* from, size_t n) {
@@ -22,9 +23,9 @@ namespace snel {
     * character.
     * \note
     *    Requires: container with push_back functionality
-    * @param str
-    * @param cont
-    * @param delim
+    * \param str     The input string to split
+    * \param cont    The container to push the tokens (MUST have .push_back(T) member)
+    * \param delim   The delimiter of type \p T to use
     */
    template <typename T, typename Container>
    void split(const std::basic_string<T>& str, Container& cont, T delim) {
@@ -35,21 +36,38 @@ namespace snel {
       }
    }
 
+   /*!
+    * A very very simple filtering for leading ' ', comments and adjustments for
+    * the '|' character.
+    * \note
+    *    The current snel implementation requires '|' to separated
+    *    from left and right from the rest of the text, so we adjust the input to fit
+    *    this expectation.
+    * \param in   Input string
+    * \return     Output string
+    */
    std::string filter (const std::string in) {
-      std::string out;
+      auto first = in.find_first_not_of(' ');
+      std::string nonws = (first != std::string::npos) ?
+                          in.substr(first) : "";
+      std::string out{};
       char prev {0}, cur, next {0};
 
-      for (auto i = in.begin(); i != in.end() ; ++i) {
+      // return empty if the first non-whitespace character is '#'
+      if (nonws[0] == '#')
+         return out;
+
+      for (auto i = nonws.begin(); i != nonws.end() ; ++i) {
          cur = *i;
          if (cur == '|') {
-            if (prev != ' ')  out += ' ';
+            if (prev != ' ')  out += ' '; // prev| insert a space before
             next = *++i;
             if (next == '|' || next == ' ') {
-               out += cur; cur = next;  // syntax ok
+               out += cur; cur = next;    // syntax ok
             }
             else {
-               // syntax problem, insert a space
-               out += cur; out += ' '; cur = next;
+               out += cur; out += ' ';    // |next, insert a space after
+               cur = next;
             }
          }
          prev = cur;
@@ -57,39 +75,64 @@ namespace snel {
       }
       return out;
    }
+
+
    /*
     * ========== ArgList ==========
     */
+   /*!
+    * destructor to free up all the allocations
+    * \note
+    *    Using RAII for ArgList we ensure "no memory leak"
+    */
    ArgList::~ArgList() {
-      for (vtype p : args_) {
-         if (p != nullptr)
-            delete[] p;
-      }
+      for (vtype p : args_)
+         if (p != nullptr) delete[] p;
    }
+
+   /*!
+    * A push_back wrapper to our underling vector. This ensures NULL terminating
+    * argument list for exec() sys-call.
+    * \param item    The argument to push
+    * \return        Reference to ArgList for chaining
+    */
    ArgList& ArgList::push_back(const std::string& item) {
       vtype it = new type[item.length()+1];
 
-      // get data
-      memcpy (it, item.c_str(), item.length());
+      memcpy (it, item.c_str(), item.length()); // get data and null terminate them
       it[item.length()] = 0;
 
-      // update the argument vector
-      args_.back() = it;
+      args_.back() = it;   // update the argument vector
       args_.push_back(nullptr);
       return *this;
    }
 
+
+   /*
+    * ========== Pipe ===========
+    */
+   //! Destructor to free up all the resources
+   Pipe::~Pipe() {
+      // close any possibly leaked pipes
+      if (fd[0] != -1)  close(fd[0]);
+      if (fd[1] != -1)  close(fd[1]);
+   }
+
    /*
     * ========== Child ==========
     */
+
+   //! Destructor to free up all the resources
    Child::~Child () {
       for (int i=0 ; i<3 ; ++i)
          restore_std_if(i);
-      // close any leaked pipes
-//      if (pipe_.fd[0] != -1)  close(pipe_.fd[0]);
-//      if (pipe_.fd[1] != -1)  close(pipe_.fd[1]);
    }
 
+   /*!
+    * Redirect a \p std_fd file descriptor to a file
+    * @param fn      The file name to used as std
+    * @param std_fd  The file descriptor to redirect
+    */
    void Child::redirect_std_if(std::string fn, fd_t std_fd) {
       if (fn != "") {
          if ((files[std_fd] = openat(AT_FDCWD, fn.c_str(), O_RDWR | O_CREAT, 0640)) == -1)
@@ -100,6 +143,10 @@ namespace snel {
             throw Error ("Child: Can not redirect file descriptor");
       }
    }
+   /*!
+    * Restore and redirected std file descriptor to its original location
+    * \param std_fd  The file descriptor to restore
+    */
    void Child::restore_std_if(fd_t std_fd) {
       if (sstd[std_fd] != -1) {
          dup2(sstd[std_fd], std_fd);
@@ -112,6 +159,11 @@ namespace snel {
       }
 
    }
+
+   /*!
+    * Parse the command of the current child to produce the actual arguments
+    * \return  Reference to Child for chaining
+    */
    Child& Child::make_arguments () {
       bool in{false}, out{false}, err{false};
       bool CanRedirectIn  = (!pipe_.from) ? true : false;
@@ -154,12 +206,27 @@ namespace snel {
       return *this;
    }
 
+   /*!
+    * \brief   Execute the child in separate process.
+    *
+    * This is the hart of the snel. We use fork() - execvp() pair to execute
+    * each child. In case of std redirection, the fd/dup2 handling is made in the
+    * parent. In case of piping. The fd handling is made in parent and the
+    * dup2() calls in the child process.
+    *
+    * A child is a member of a vector containing the command chain.
+    * For ex: `ls && cat tralala || uname -a` is a chain with three
+    * childs (ls, cat, uname). See also \see Sequencer
+    *
+    * \param it      Iterator to the command chain execution
+    * \param first   flag to indicate the first command in the chain
+    * \return        True if the chain can stop.
+    */
    bool Child::execute(std::vector<Child>::iterator it, bool first) {
-      bool stop = {false};
-      if (!first) --it;
-      Child& previous = *it;
+      bool stop {false};
+      Child& previous = (!first) ? *--it : *it; // get previous child safely
 
-      // Check parent redirection
+      // Check redirection requirements
       redirect_std_if (filenames[STDIN_],  STDIN_);
       redirect_std_if (filenames[STDOUT_], STDOUT_);
       redirect_std_if (filenames[STDERR_], STDERR_);
@@ -173,79 +240,90 @@ namespace snel {
       if (pid < 0) {
          throw Error("Child: Can not create child process");
       }
-      else if (pid == 0) { // child
+      else if (pid == 0) {
+         // =========== child ================
+
          // Some extra pipe checking while we still have our data
          if (pipe_.to) {         // transmitting child
-            close(pipe_.fd[0]);  // close the read end
+            close(pipe_.fd[0]);  // close the read end (of the child copy)
             if ((dup2(pipe_.fd[1], STDOUT_)) == -1) // redirect output
                throw Error("Child pipe[to]: Can not redirect through pipe");
          }
-         else if (!first && pipe_.from) {  // receiveing child
-            close (previous.pipe().fd[1]); // close the write end
+         else if (!first && pipe_.from) {  // receiving child
+            close (previous.pipe().fd[1]); // close the write end (of the child copy)
             if ((dup2(previous.pipe().fd[0], STDIN_)) == -1)  // redirect input
                throw Error("Child pipe[from]: Can not redirect through pipe");
          }
+
          execvp(arguments.front(), arguments.data());
          // error if we got here
          std::cout << "Can not invoke: " << arguments.front() << std::endl;
          throw Error("Child: Can not run child process");
       }
-      else {   // parent
-         if (pipe_.to)        close (pipe_.fd[1]);
+      else {
+         // =========== parent ================
+         if (pipe_.to)        close (pipe_.fd[1]); // Pipe fd control
          else if (!first && pipe_.from)
                               close (previous.pipe().fd[0]);
 
          int exit_status;
-         waitpid(pid, &exit_status, 0);  // wait child process to finish
-         restore_std_if (STDIN_);
+         waitpid(pid, &exit_status, 0);   // wait child process to finish
+         restore_std_if (STDIN_);         // restore all redirections
          restore_std_if (STDOUT_);
          restore_std_if (STDERR_);
 
-         switch (logic) {
-            case LogicOp::AND:
-               if (exit_status)  stop = true;
-               break;
-            case LogicOp::OR:
-               if (!exit_status) stop = true;
-               break;
-            default: break;
-         }
+         // Check if we don't have to execute the rest of the chain
+         if ( exit_status && logic == LogicOp::AND)   stop = true;
+         if (!exit_status && logic == LogicOp::OR)    stop = true;
       }
 
       return stop;
    }
 
+
+
+
    /*
     * ======== Sequencer ===========
     */
+
+   /*!
+    * First parsing of each line. We split input in ';' to create
+    * command chains and split each chain further to create child tokens.
+    * These token will be processed further by the child's `make_arguments()`
+    *
+    * \param input   The input string to parse
+    * \return        Reference to Sequencer for chaining
+    */
    Sequencer& Sequencer::parse (const std::string& input) {
-      std::vector<std::string> commands;
-      std::vector<std::string> tokens;
+      std::vector<std::string> chains; // chains are vector of commands
+      std::vector<std::string> tokens; // token is any ' ' separated string
       Child::command_t command;
 
-      split (input, commands, ';'); // split all commands
-      for (auto& s : commands) {
-         command.clear();      //clear child tokens
-         tokens.clear();            // make a new token vector for command
-         split (s, tokens, ' ');
+      split (input, chains, ';');      // split input to command chains using ';'
+      for (auto& chain : chains) {     // for each chain
+         command.clear();              // clear child tokens
+         tokens.clear();               // make a new token vector for command
+         split (chain, tokens, ' ');   // split chain in to tokens using ' '
 
          seq_.emplace_back(std::vector<Child>{});     // make a new child for command
-         // check tokens inside command
+         // check tokens inside the chain
          bool from = false;
          for (auto& t: tokens) {
-            command.push_back(t);    // get current token
+            command.push_back(t);      // get current token
             if (is_seperator(t)) {
                // construct a child with what we have so far and clear command
                seq_.back().emplace_back(command);
                command.clear();
                if (from) {
+                  // set from flag to link with the previous child
                   seq_.back().back().pipe().from = from;
                   from = false;
                }
 
                if (is_pipe(t)) {
-                  seq_.back().back().pipe().to = true;
-                  from = true;
+                  seq_.back().back().pipe().to = true;   // mark as transmitting child
+                  from = true;   // mark to inform the next child as receiving
                }
             }
          }
@@ -256,18 +334,24 @@ namespace snel {
       return *this;
    }
 
+   /*!
+    * The main sequencer execution. We recurse for each command chain
+    * and each child in the chain and we parse-execute the child
+    * \return  Reference to Sequence for chaining
+    */
    Sequencer& Sequencer::execute() {
-      for (auto& batch : seq_) {
-         bool first;
+      bool first;
+      for (auto& chain : seq_) { // loop in all of the command chains
          first = true;
-         for (auto it = batch.begin() ; it != batch.end(); ++it) {
+         for (auto it = chain.begin() ; it != chain.end(); ++it) {
+            // loop all of the commands in the chain
             Child& command = *it;
             if (command.make_arguments().execute(it, first))
-               break;
+               break;   // break the chain if child's logic operator says so.
             first = false;
          }
       }
-      seq_.clear();
+      seq_.clear();  // clear the sequencer's data for the next line (call all the destructors)
       return *this;
    }
 

src/sequencer.h

@@ -1,10 +1,12 @@
-/*
- * child.h
+/*!
+ * \file
+ *   Sequencer.h
+ * \brief   A basic sequence interpreter for snel
  *
- *  Created on: Feb 11, 2019
- *      Author: hoo2
+ *  Created on: Feb, 2019
+ *      Author: Christos Choutouridis AEM: 8997
+ *      email : cchoutou@ece.auth.gr
  */
-
 #ifndef __sequencer_h__
 #define __sequencer_h__
 
@@ -34,10 +36,23 @@ namespace snel {
    std::string filter (const std::string in);
 
    /*!
+    * A vector based wrapper above execvp()'s `char* argv[]` interface.
+    * We use a vector for convenience and resizing capabilities. We can then
+    * use a pointer to underling data to pass to execvp()
+    * std::vector elements guaranteed to be contiguous. To quote the standard[1]
+    * \code
+    *    The elements of a vector are stored contiguously, meaning that if v is
+    *    a vector<T, Allocator> where T is some type other than bool, then it obeys
+    *    the identity &v[n] == &v[0] + n for all 0 <= n < v.size().
+    * \endcode
+    *
+    * See also cppreference[2].
     *
+    * [1]: www.open-std.org/jtc1/sc22/wg21/docs/papers/2013/n3690.pdf
+    * [2]: https://en.cppreference.com/w/cpp/container/vector
     */
    struct ArgList {
-      using type        = typename std::string::value_type;     // Basic data type
+      using type        = typename std::string::value_type;     // Basic data type (aka char)
       using vtype       = type*;    // Vector type
       using vtype_ptr   = vtype*;   // Pointer to vector type
 
@@ -46,32 +61,48 @@ namespace snel {
       ArgList(const ArgList&) = default;
       ArgList(ArgList&&) = default;
 
-      ArgList& push_back(const std::string& item);
-      vtype front () { return args_.front(); }
-      size_t size () { return args_.size(); }
+      ArgList& push_back(const std::string& item);    //!< A vector::push_back wrapper
+      vtype front () { return args_.front(); }        //!< A vector::front() wrapper
+      size_t size () { return args_.size(); }         //!< A vector::size() wrapper
 
-      vtype_ptr data() noexcept {
-         return args_.data();
-      }
-      vtype_ptr operator*() noexcept {
-         return data();
-      }
+      //! return a pointer to underling data for `execvp()`
+      vtype_ptr data() noexcept { return args_.data(); }
+      //! same as `data()`
+      vtype_ptr operator*() noexcept { return data();  }
    private:
+      // data
       std::vector<vtype> args_ {nullptr};
    };
 
 
+   /*!
+    * A pipe file descriptor container for each child.
+    * \note
+    *    We store the pipe in the first child and use the flags `from`
+    *    and `to` to inform child's executer for where can find the data.
+    */
    struct Pipe {
+      ~Pipe();
       fd_t fd[2] {-1, -1};
       bool from  {false};
       bool to    {false};
    };
 
+   /*!
+    * An object to represent each process
+    *
+    * We create Child objects from command_t passed from Sequence::parse() and we
+    * keep in each child informations about the file descriptors that may the process
+    * will use, pipes that may need and logic flags(&&, ||) the command may have to control
+    * the execution flow.
+    */
    class Child {
    public:
-      enum class LogicOp { NONE=0, OR, AND };
-      using command_t = std::vector<std::string>;
-      using Error = std::runtime_error;
+      enum class LogicOp {
+         NONE=0, OR, AND
+      };
+      using command_t = std::vector<std::string>;  //!< A command type
+      using Error     = std::runtime_error;        //!< An error type
 
    public:
       ~Child ();
@@ -79,37 +110,73 @@ namespace snel {
       Child (const command_t& c) : command{c} { }
       Child (command_t&& c) : command{std::move(c)} { }
 
-      Child& make_arguments ();
+      Child& make_arguments ();                    //!< A per child parser before execution
+      //! the actual execution routine
       bool execute(std::vector<Child>::iterator it, bool first);
-      Pipe& pipe() { return pipe_; }
+      Pipe& pipe() { return pipe_; }               //!< A pipe_ getter
+
+   //! Private api
+   //! @{
    private:
-      void redirect_std_if(std::string fn, fd_t std_fd);
-      void restore_std_if(fd_t std_fd);
+      void redirect_std_if(std::string fn, fd_t std_fd); //!< tool to redirect std
+      void restore_std_if(fd_t std_fd);                  //!< tool to restor std redirection
+   //! @}
+
+   //! Data members
+   //! \note
+   //!   `arguments`, `files`, `sstd` and `pipe_` have linked resources so destructor(s) must be called.
+   //!   We use RAII by having all children to static allocated vectors. '}' will do the trick even
+   //!   if we throw ;)
+   //! @{
    private:
-      command_t command {};
-      ArgList arguments {};
-
-      fd_t  files[3] = {-1, -1, -1};
-      fd_t  sstd [3] = {-1, -1, -1};
-      std::string filenames[3] = {"", "", ""};
-      LogicOp  logic  {LogicOp::NONE};
-      Pipe pipe_;
+      command_t command {};   //!< Each command is a vector of string as passed on from Sequence::parse()
+      ArgList arguments {};   //!< The after parsing, actual null terminated arguments to pass to execvp()
+
+      fd_t  files[3] = {-1, -1, -1};   //!< file descriptors for the user requested redirections
+      fd_t  sstd [3] = {-1, -1, -1};   //!< file descriptors to store std fd's before redirection
+      std::string filenames[3] = {"", "", ""};  //!< filenames of the user requested redirections
+      LogicOp  logic  {LogicOp::NONE}; //!< Logic flags to store && and || command separators
+      Pipe pipe_;                      //!< Pipe object for each child
+   //! @}
    };
 
+   /*!
+    * The main object to represent and handle a command flow.
+    *
+    * We store commands as vector of vectors of commands. The inner vector
+    * represent a command chain. For example the line:
+    * \code ls | more && uname; cat lala \endcode
+    * is an input with 2 command chains and result to a:
+    * \code
+    * vector<
+    *    vector<(ls |), (more &&), (uname)>,
+    *    vector<(cat)>
+    * >
+    * \endcode
+    * By storing them in different vector we make the execution model simpler.
+    * Each chain can stopped and the sequencer will continue with the next chain etc...
+    * So in this example if `more` returns false(child::execute() returned true),
+    * then `uname` will not parsed and executed and the flow will continue to `cat`
+    */
    class Sequencer {
    public:
-      Sequencer& parse (const std::string& input);
-      Sequencer& execute ();
+      Sequencer& parse (const std::string& input); //!< Input line parser
+      Sequencer& execute (); //!< Main sequencer executor
 
+   //! private tools
+   //! @{
    private:
+      //! separator trait predicate
       bool is_seperator (std::string& s) {
          return (s == "&&" || s == "||" || s == "|") ? true : false;
       }
+      //! pipe trait predicate
       bool is_pipe (std::string& s) {
          return (s == "|") ? true : false;
       }
-
+   //! @}
    private:
+      //! The sequencer data representation
       std::vector <
          std::vector <Child>
       > seq_ {};