Constructing a simple shell from scratch

Question

I'm actually doing my whole shell in C from scratch from a Linux computer. The thing is that I think we all do our best from creating the simplest things that can be explained easily. And I'm not sure my shell has these qualities.

All command I tried worked but command 1 | command 2 | command 3 or command 1 | command 2 > command 3. I would be glad if you had any ideas but I'm okay with it. My point is that this code it a bit heavy and I'm sure it could have been done easier.

If you have any idea to make it simpler, I would be glad to hear it.

This shell is constructed this way:

The main method calls a command function which calls a parsing function which reads the function written by the user, fills an array with the function and returns a number for each delimiter. The command is constructed by case and does an execvp on the array with the command. The thing that I'm not allowed to modify is the parsing function.

#include <unistd.h>
#include <stdio.h>
#include <sys/wait.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <sys/types.h>
#include <stdlib.h>
#include <string.h>
#include <errno.h>
char *delimiteurs = ";&<>|";
char *respP[10]; // tableau contenant la commande a executer
char *fout[10];// tableau qui affiche le resultat de l'istruction shell lue dans fin 
char *fin[10]; // la méthode commande va exécuter une instruction shell lue dans le tableau fin elements par element grace a parsing
char *tube[10]; // tube qui assusre la communication entre deux processus
char mot[50];
int symboleP, status, c;
void parsing(){
 int i=0;
 int cmot=0;
 while(1){
 c = getchar();
 if (c == '\n') {symboleP = 0;return;}
 else if (c == ';') {symboleP = 1;return;}
 else if (c == '&') {symboleP = 2;return;}
 else if (c == '<') {symboleP = 3;return;}
 else if (c == '>') {symboleP = 4;return;}
 else if (c == '|') {symboleP = 5;return;}
 else if (c == EOF) {symboleP = 7;return;}
 else if (c != ' ') {
 symboleP = 10;
 while(c != '\n' && !strchr(delimiteurs,c)){
 i=0;
 while(c != 32 ){ // 32 correspond a un caractère dans la table ascii
 if((c != '\n') && !strchr(delimiteurs,c)){
 mot[i]=c;i++;
 c=getchar();
 }
 else {break;}
 }
 break;
 }
 while(c == ' ')
 {
 c=getchar();
 }
 ungetc(c,stdin); // 
 mot[i]=0;
 respP[cmot++]=strdup(mot); // fonction strdup renvoie l'adresse de la nouvelle chaine de caractère qui vient dêtre duppliquer ou sinon NULL --> alloue de l'espace necessaire pour stoker au mot
 fflush(stdout);
 if(c == '\n' || strchr(delimiteurs,c)) //
 {
 respP[cmot]=0;
 return;
 }
 }
 }
}
void commande () {
 pid_t pid, fid;
 int background = 0;
 int status;
 char car;
 int i, j, k, l;
 int p, p2;
 int execute=1;
 int output=0;
 int input=0;
 int tube=0;
 int fd[2];
 int fich, fo, screen;
 while(1){
 if(execute==1){
 if(symboleP==0){
 printf("DAUPHINE> ");
 } 
 for (j=0;j<10;j++){
 respP[j]=NULL;
 } 
 execute=0; 
 background=0;
 }
 fflush(stdout);
 parsing();
 switch (symboleP) {
 case 0 : // SYMBOLE : \n
 p=fork();
//Mettre un if pour que le père observe le fils (exec dans le fils
 if(p==0){ //fils
 if(tube==1){//printf("\n\n\n");
 fich = open("fichtmp",O_RDONLY,0640);
 close(0); 
 dup(fich); //fichier devient entrée 0
 execvp(respP[0], respP);
 close(fich); //fermeture fichier
 }
 else if(output==0 && input==0){ //pas de redirection
 execvp(respP[0], respP); 
 }else if(output==1){ //dans le cas d'une redirection
 printf("truc2");
 close(1); // extrémité a ecrire close (1)
 int filout = creat(respP[0], 0644); // fichier de sortie
 execvp(fout[0], fout); // processus père ce dédouble attend la reponse du fils et c'est le fils qui va etre remplacé par la commande a exécuté
 }
 else if(input==1){
 printf("truc3");
 close(0); // on ferme lextrémité que nous allons pas utilisé: close(0) etremité a lire
 int filin = open(respP[0], O_RDONLY); // fichier d'entrée
 execvp(fin[0], fin);
 }
 return 0;
 }else{ //pere
 if(background==0){ //pas de bg on attend le fils
 waitpid(p, NULL, 0);
 } 
 output=0;
 input=0; 
 execute=1;
 tube=0;
 }
 break;
 case 1: // SYMBOLE : ;
 p=fork(); 
 if(p==0){ //fils
 if(output==0 && input==0 && tube==0){ //pas de redirection
 execvp(respP[0], respP); 
 }else if(output==1){ //dans le cas d'une redirection
 close(1);// pour ecrire dna sun fichier on close la lecture
 int filout = creat(respP[0], 0644);// fichier de sortie
 execvp(fout[0], fout);
 }
 else if(input==1){
 close(0); // pour lire un fichier on close l'ecriture
 int filin = open(respP[0], O_RDONLY); // juste lecture comme mode
 execvp(fin[0], fin);
 }
 return 0; 
 }else{ //pere
 waitpid(p, NULL, 0); 
 output=0;
 input=0;
 execute=1; 
 }
 break;
 case 2: // SYMBOLE : &
 background=1;
 break;
 case 3: // SYMBOLE : <
 if(input==0){
 input=1;
 execute=1;
 for (l=0;l<10;l++){
 fin[l]=respP[l];
 }
 }
 break;
 case 4: // SYMBOLE : >
 if(output==0){
 output=1;
 execute=1;
 for (l=0;l<10;l++){
 fout[l]=respP[l];
 }
 }
 break;
 case 5: // SYMBOLE : |
 //if(tube==0){
 screen = dup(1);
 close(1);
 fo = open(".pipe", O_CREAT | O_TRUNC | O_RDWR, 0640);
 dup(fo);
 p2 = fork();
 if(p2 == 0){
 execvp(respP[0],respP);
 }
 else{
 wait(&status);
 close(1);
 close(fo);
 dup(screen);
 //printf("Retour de l'écran \n");
 //printf("w = %s \n", w);
 //printf("com[0] = %s \n",com[0]);
 i = 0;
 respP[i++] = strdup(mot);
 //com[i++] = 0;
 parsing(); 
 respP[i++] = strdup(mot);
 respP[i++] =0; int keyboard;
 keyboard = dup(0);
 close(0);
 fo = open(".pipe", O_RDWR, 0640);
 dup(fo);
 //printf("Avant le 2nd fork \n");
 p2 = fork();
 if(p2 == 0){
 //printf("IN PID, w = %s \n",w);
 //printf("IN PID, com[0] : %s \n",com[0]);
 execvp(respP[0],respP);
 }
 else{
 wait(&status);
 close(0);
 close(fo);
 dup(keyboard);
 i = 0;
 return 0;
 }
 //dup(screen);
 }
 //return 0; 
 break;
 default:
printf(""); 
 }
 }
 return 0 ;
}
int main(int argc, char* argv[]) {
 int i, j, k, l;
 int execute=1;
 while(1){
 if(execute==1){
 if(symboleP==0){
 printf("");
 } 
 for (j=0;j<10;j++){
 respP[j]=NULL;
 } 
 execute=0; 
 }
 commande();
 }
 return 0 ;
}

• Dan Oberlam: Is this exactly how it looks in your text editor? the indentation looks weird and I want to make sure it wasn't a bad copy-paste job before reviewing it.
• Revolucion for Monica: God, I'm pretty sure! Thanks for watching out! I'm looking a bit further, I may have had some difficulties with the stackexchange code tool...
• Dan Oberlam: I've found that such errors are often related to mixing tabs and spaces in your source code, which is worth reviewing on. If I get around to reviewing this I'll mention that.

Answer 1

As a heads up, I write much more C++ than C now so stylistic comments may not meet what is consider correct for C code. I'll get those out of the way now

Style

The absolute first thing you need to do is fix your indentation. As is, I find it almost impossible to read. Pick a tab-width (I like 4), set your IDE/text editor to replace tabs with spaces, and go from there. Indent inside scopes (i.e. curly braces) or wherever there could be curly braces (i.e. one-line for-loops).

After that, add spaces around operators, before the conditions of while loops, before curly braces, etc. This makes it easier to grok what any given line is saying.

This is a pet peeve, and many people feel differently, but I can't stand code like this:

short_name = value;
super_ridiculously_long_name = value;

I don't find it more readable or visually pleasing to have my equals signs (or parentheses, or whatever) line up in this way.

I also almost never like putting multiple statements on a single line, with the possible exception of if (condition) return; Especially don't do something like this

respP[i++] =0; int keyboard;

Just put them on separate lines.

You should also use consistent brace style - don't mix up these two

while (condition) {
 // do stuff
}
while (condition)
{
 // do stuff
}

I find the first one easier to read, and much more common in C code.

I absolutely detest this aspect of C:

void commande () {
 pid_t pid, fid;
 int background = 0;
 int status;
 char car;
 int i, j, k, l;
 int p, p2;
 int execute=1;
 int output=0;
 int input=0;
 int tube=0;
 int fd[2];
 int fich, fo, screen;

But I understand that it is pretty common. If you must do it, please make sure to give every variable a good, easy to understand name. I generally dislike Hungarian notation but I also usually declare my variables right before I use them and can see their type. When declaring a variable far away from its use, I find Hungarian notation can make it more clear what that variable's type is.

Now we can start on actually understanding your code.

Reduce global state

You don't use the variables c or delimiteurs anywhere except in parsing, so you should move them there (delimiteurs is also a constant, so mark it as such).

Both fin and fout are only used in commande, so move them there.

You never use tube (well, you declare int tube in commande) so you can delete it. Same for status.

You can remove symboleP from global scope as well by doing something like this (I removed a ton from this for clarity):

int main() {
 int symboleP;
 commande(&symboleP);
}
...
int parsing() {
 return symboleP;
}

switch vs if/else if/else

You already use switch statements in your code, but you could use them in more places. This may give a speedup to your program, and for long chains I find them more readable.

parsing function (could use a better name)

I'm not totally clear on the specifics of what is happening in this section, but I'd recommend maybe using fewer character and integer literals, and used named constants/enums instead. I'd also recommend putting the final case into its own helper function for readability. All in all, I think I'd rewrite this section to look something like this:

bool handle_default_case(int c, const char* delimiteurs) { 
 int i = 0;
 int cmot = 0;
 while (c != '\n' && !strchr(delimiteurs, c)) {
 i = 0;
 while (c != ' ' && c != '\n' && !strchr(delimiteurs, c)) {
 mot[i++] = c;
 c = getchar();
 }
 break;
 }
 while(c == ' ') {
 c = getchar();
 }
 ungetc(c, stdin);
 mot[i] = 0;
 respP[cmot++] = strdup(mot); // fonction strdup renvoie l'adresse de la nouvelle chaine de caractère qui vient dêtre duppliquer ou sinon NULL --> alloue de l'espace necessaire pour stoker au mot
 fflush(stdout);
 if (c == '\n' || strchr(delimiteurs, c)) {
 respP[cmot] = 0;
 return true;
 }
 return false;
}
typedef enum {
 NEWLINE,
 SEMICOLON,
 AMPERSAND,
 LESS_THAN,
 GREATER_THAN,
 VERTICAL_BAR,
 END_OF_FILE,
 DEFAULT
} parsed_character;
parsed_character parsing() {
 int c = getchar();
 const char *delimiteurs = ";&<>|";
 while (1) {
 switch (c) {
 case '\n':
 return NEWLINE;
 case ';':
 return SEMICOLON;
 case '&':
 return AMPERSAND;
 case '<':
 return LESS_THAN;
 case '>':
 return GREATER_THAN;
 case '|':
 return VERTICAL_BAR;
 case EOF:
 return END_OF_FILE;
 case ' ':
 break;
 default:
 if (handle_default_case(c, delimiteurs)) {
 return DEFAULT;
 }
 }
 c = getchar();
 }
}

commande function (could also use a better name)

There is entirely too much going on in this function. You probably need something distinct for each case. I ran out of time, so I can't dig into those too much, but it would probably look something like this

void commande (parsed_character* symboleP) {
 pid_t pid, fid;
 char *fout[10];// array that displays the result of the shell instruction read in end
 char *fin[10]; // shell instruction read in and parsed
 int background = 0;
 int status;
 char car;
 int i, j, k, l;
 int p, p2;
 int execute = 1;
 int output = 0;
 int input = 0;
 int tube = 0;
 int fd[2];
 int fich, fo, screen;
 while (1) {
 if (execute == 1) {
 if (symboleP == NEWLINE) {
 printf("DAUPHINE> ");
 } 
 for (j = 0; j < 10; j++) {
 respP[j] = NULL;
 } 
 execute = 0; 
 background = 0;
 }
 fflush(stdout);
 *symboleP = parsing();
 switch (*symboleP) {
 case NEWLINE:
 if (newline_command()) return 0;
 break;
 case SEMICOLON:
 if (semicolon_command()) return 0;
 break;
 case AMPERSAND:
 background = 1;
 break;
 case LESS_THAN:
 less_than_function();
 break;
 case GREATER_THAN: 
 greater_than_function();
 break;
 case VERTICAL_BAR:
 if (vertical_bar_funtion()) return 0;
 break;
 default:
 printf(""); 
 }
 }
 return 0;
}

Having the enum gives you better case labels (you don't need the comments now) and you've clearly delegated responsibility to the correct function (all of those should probably have better names too).

main function (this name is fine ;) )

int main(int argc, char* argv[]) {
 int i, j, k, l;
 int execute = 1;
 parsed_character symboleP;
 while (1) {
 if (execute == 1) {
 if (symboleP == NEWLINE) {
 printf("");
 } 
 for (j = 0; j < 10; j++){
 respP[j] = NULL;
 } 
 execute = 0;
 }
 commande(&symboleP);
 }
 return 0 ;
}

You'll notice I didn't change much except for formatting, and symboleP became a local variable and an enum.