I am trying to make shell.

This is my struct for storing command:

struct command {
    char *comm;
    char *args[MAX_NO_ARGS 2];
    int no_args;
    int order;
};

In these function I trim the command based on argument and pipe operators:

char *mytrim(char *s);
int get_commands(char *line, struct command mycommands[]);
void print_command(struct command mycommands[], int no_commands);

These are my two function for execute command and pipe handling:

void execute_commands(struct command mycommands[], int no_commands);
int execute_command(char *command, int argc, char *args[], int fd_in, int fd_out);

When I call execute_commands() for pipe handling and execute_command() for executing instead of printing. I got stuck at infinite reading

Also when I tried (ifconfig | grep "dropped") command, it's printing whole ifconfig detail and then infinite reading.

void execute_commands(struct command mycommands[], int no_commands)
{
    if (no_commands == 1) {
        execute_command(mycommands[0].comm, mycommands[0].no_args, mycommands[0].args, -1, -1);
    } else {
        int i = 0, j = 0;
        int fd[no_commands-1][2];
        for (i = 0; i < no_commands; i  ) {
            pipe(fd[i]);

        }
        for (i = 0; i < no_commands; i  ) {
            if (i == 0) {
                execute_command(mycommands[i].comm, mycommands[i].no_args, mycommands[i].args, -1, fd[i][1]);
            } else if (i == no_commands -1 ) {
                execute_command(mycommands[i].comm, mycommands[i].no_args, mycommands[i].args, fd[i-1][0], -1);
            } else {
                execute_command(mycommands[i].comm, mycommands[i].no_args, mycommands[i].args, fd[i-1][0], fd[i][1]);
            }
        }

        for (i = 0; i < no_commands-1; i  ) {
            close(fd[i][0]);
        }
    }
    return;
}

int execute_command(char *command, int argc, char *args[], int fd_in, int fd_out)
{
    int pid;
    pid = fork();
    if (pid == 0) {
        if (fd_in) {
            close(fd_in);
            dup2(fd_in, STDIN_FILENO);
            execvp(command, args);
        }
        if (fd_out) {
            close(fd_out);
            dup2(fd_out, STDOUT_FILENO);
            execvp(command, args);
        }
        if (fd_in == fd_out == -1) {
            execvp(command, args);
        }
    }
    waitpid(pid, NULL, 0);
    return 0;
}

CodePudding user response：

You're closing fds in the wrong order. You write:

    close(fd_in);
    dup2(fd_in, STDIN_FILENO);

but you need to dup first with:

    dup2(fd_in, STDIN_FILENO);
    close(fd_in);

Also, the logic looks suspect. Rather than:

 if (fd_out) {

you probably want

if (fd_out != 1) {

checking if (fd_in == fd_out == -1) almost certainly does not do what you think it does. (By that, I mean you probably think that it is checking (fd_in == -1 && fd_out == -1), but it is not). You need to check the value returned from pipe rather than the values of the file descriptors.

Also, it is incorrect to have exec in multiple places. The way your code is currently structured, assuming no errors, fd_in will always be non-zero and your code will exec after it dups fd_in. It will never even check fd_out and certainly it will never dup it to stdout. You should delete the execs in the if clauses and only call exec once. The only code you should ever write after an exec is perror("exec"); exit(EXIT_FAILURE);

CodePudding user response：

You should also close fd_in and fd_out in parent process