Enigma execpvp

Hello,

I'm writing a mini shell in C and have problems with my code that I don't understand.

Code:

---


#include <stdio.h>
#include <stdlib.h>
#include <sys/types.h>
#include <signal.h>
#include <stdbool.h>
#include <string.h>
#include <unistd.h>
#define maxLine 80

void execute(char**args){
    pid_t pid;
    if ((pid = fork()) < 0) { perror("fork"); exit(1);}
    if (pid == 0){  if (execvp(*args, args) == -1){ perror("executing command\n"); exit(1);}}
    else wait(&pid);
}
void parse(char* input, char** args){
        int argc = 0; char* delm = " \t\n"; char* token = strtok(input,delm);
        while (token != NULL){
                args[argc] = token; argc++;
                token = strtok(NULL, delm);
        }  execute(args);
}
int main(){
    char buf[maxLine];
    char *args[64];

    while(1){
        printf("Prompt: ");
        if (fgets(buf,maxLine, stdin) == NULL) {
            printf("\n");
            exit(0);
        } parse(buf, args);
    }
}

---

This code would work if:
- I remove the the if condition around the excevp call, and move the execute call from the parse to the main. Why, I'd like to know.

For starters execvp() never returns; it returns only if there is an error with an exit status of -1 to the environment so it should really be coded as

Code:

---

if (pid == 0){if (execvp(*args, args) == -1) {perror("executing command\n"); exit(1);}}

---

It would help if it wasn't so badly formatted.

I did that modification before seeing your post but still no difference.

Salem, clearly there are different programming prospectives. Formatted like this I see it focus more on the 'important' lines of code, and reduce scroll-blindness. But anyway this is not the topic of the thread, so u are welcome to 'properly' format it for your best understanding of it.

The problem remains as of why when I include execute inside the parse, as shown in the code I get the following error when I launch a process that would otherwise work:

executing command
: Bad address

Well it certainly would help if it were formatted as previously noted. Post the errors you're getting and the code that works.

Code:

---


void execute(char**args){
    pid_t pid;
    if ((pid = fork()) < 0) {
          perror("fork");
        exit(1);
    }
    if (pid == 0){ 
      if (execvp(*args, args) == -1){
          perror("executing command\n");
          exit(1);
        }
    }
    else wait(&pid);
}
void parse(char* input, char** args){
        int argc = 0;
        char* delm = " \t\n";
        char* token = strtok(input,delm);
        while (token != NULL){
                args[argc] = token; argc++;
                token = strtok(NULL, delm);
        }
}
int main(){
    char buf[maxLine];
    char *args[64];

    while(1){
        printf("Prompt: ");
        if (fgets(buf,maxLine, stdin) == NULL) {
            printf("\n");
            exit(0);
        }
        parse(buf, args);
        execute(args);
    }
}

---

This code works. Compare with the previous posting where the execute statement is moved, and that simple movement gives the error msg pasted above:

executing command
: Bad address

> Formatted like this I see it focus more on the 'important' lines of code
Yeah, at the expense of being able to figure out the program logic.
Which is exactly the hole you've dug yourself into.

Code is read a hell of a lot more than it is written (once if you're really good).
Writing code so that it's easy to parse for the reader with minimal effort is what counts.

With your better code, this took about 5 seconds to figure out.

args[argc] = NULL;
is what you need when you've finished parsing the arguments.

Note that the second argument to execvp() is an array of pointers to char strings with the last one ie args[argc] being a NULL pointer, as in

Code:

---

while (token != NULL) {
    args[argc] = token;
    argc++;
    token = strtok(NULL, delm);
    args[argc] = NULL;
}

---

Just my 2c, but I tried both versions of the code and had no problems whether execute() was called from main() or parse().

thank u very much Salem! May you complete my info pointing out why this extra line of code is required? Is it to stop the exec function from taking args from values that i actually didn't fill in (not initialized)?

Without that last bit execvp() doesn't know where the list of arguments ends and moves onto the next pointer which could contain a bad/garbage value(address).

execv - Man Pages at IceWalkers.com

Quote:

---

The execv and execvp functions provide an array of pointers to null-
terminated strings that represent the argument list available to the
new program. The first argument, by convention, should point to the
file name associated with the file being executed. The array of point-
ers must be terminated by a NULL pointer.

---

It's like mentioned 4 times, did you read it even once?
Or just guess from the prototype?