4
\$\begingroup\$

So I implemented a program that takes an input file, two command strings and an output file to mimick the behaviour of running :

<input cmd1 -option | cmd2 -option > output

that's called like this :

./pipe input "cmd1 -opt" "cmd2 -opt" output

and I did without using the wait system call since while there are open file descriptors on the pipe I opened, the exec'd commands will wait for one another. that is the pipe takes care of coordination IIUC.

But I feel like I am doing it wrong since it seems that people I using wait() out of convention. Is it reasonnable to think it is not necessary in my case since I only need the return value of the second command and the pipe ensures communication or am I missing something ? What else am I doing wrong in terms of code structure or style ?

Here is my code :

#include <unistd.h>
#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>
#include <errno.h>
#include <string.h>
#include "pipex.h"

static const char   g_cmd_not_found[] = {
    "command not found: "
};

static const char   g_empty_string[] = {
    "The name of the input or output file cannot be an empty string\n"
};

static int  open_or_die(char *filename, int flags, mode_t mode)
{
    int fd;

    fd = open(filename, flags, mode);
    if (fd == -1)
    {
        if (*filename == '\0')
            write(STDERR_FILENO, g_empty_string, sizeof(g_empty_string));
        else
        {
            write(STDERR_FILENO, "pipex: ", sizeof("pipex: "));
            perror(filename);
        }
        exit(EXIT_FAILURE);
    }
    return (fd);
}

static void pipe_or_die(int *pipe_fds)
{
    int r;

    r = pipe(pipe_fds);
    if (r == -1)
    {
        write(STDERR_FILENO, "pipex: ", sizeof("pipex: "));
        perror("pipe");
        exit(EXIT_FAILURE);
    }
}

static void file_is_ok_or_die(char **cmdv, char **pathvar_entries)
{
    if (access(cmdv[0], X_OK) == -1)
    {
        write(STDERR_FILENO, "pipex: ", sizeof("pipex: "));
        if (cmdv[0][0] != '/')
        {
            write(STDERR_FILENO, g_cmd_not_found, sizeof(g_cmd_not_found));
            ft_puts_stderr(cmdv[0]);
        }
        else
            perror(cmdv[0]);
        free_null_terminated_array_of_arrays(cmdv);
        free_null_terminated_array_of_arrays(pathvar_entries);
        if (errno == ENOENT)
            exit(127);
        else if (errno == EACCES)
            exit(126);
        else
            exit(EXIT_FAILURE);
    }
}

void    execute_pipeline(char *cmd_str, int read_from, int write_to, char **env)
{
    char    **pathvar_entries;
    char    **cmdv;

    redirect_fd_to_fd(0, read_from);
    redirect_fd_to_fd(1, write_to);
    pathvar_entries = ft_split(get_path_var(env), ':');
    cmdv = ft_split(cmd_str, ' ');
    if (!pathvar_entries || !cmdv)
    {
        write(STDERR_FILENO, "pipex: ", sizeof("pipex: "));
        perror("malloc");
        free_null_terminated_array_of_arrays(cmdv);
        free_null_terminated_array_of_arrays(pathvar_entries);
        exit(EXIT_FAILURE);
    }
    cmdv[0] = get_command_path(cmdv, get_pwd_var(env), pathvar_entries);
    file_is_ok_or_die(cmdv, pathvar_entries);
    execve(cmdv[0], cmdv, env);
    free_null_terminated_array_of_arrays(cmdv);
    free_null_terminated_array_of_arrays(pathvar_entries);
}

int main(int ac, char **av, char **envp)
{
    int     pipefd[2];
    int     child_pid;
    int     infile_fd;
    int     outfile_fd;

    if (ac != 5)
        print_usage_exit();
    pipe_or_die(pipefd);
    child_pid = fork();
    if (child_pid == -1)
        perror("fork");
    else if (child_pid == 0)
    {
        infile_fd = open_or_die(av[1], O_RDONLY, 0000);
        close(pipefd[0]);
        execute_pipeline(av[2], infile_fd, pipefd[1], envp);
    }
    else
    {
        outfile_fd = open_or_die(av[ac - 1], O_WRONLY | O_CREAT | O_TRUNC, 0666);
        close(pipefd[1]);
        execute_pipeline(av[3], pipefd[0], outfile_fd, envp);
    }
    return (EXIT_SUCCESS);
}
```
\$\endgroup\$
4
  • \$\begingroup\$ What's "pipex.h"? It seems to be essential for this program, but I don't know what I need to install to compile this. \$\endgroup\$ Oct 18, 2021 at 7:54
  • \$\begingroup\$ @TobySpeight It's just a global header. I haven't put all the functions I am using because it would be too big and some of them are not relevant to what mattered fr me to show you \$\endgroup\$
    – cassepipe
    Oct 18, 2021 at 15:53
  • \$\begingroup\$ You might like to post it for its own review - I'm wondering what's in redirect_fd_to_fd() that plain old dup2() doesn't do, for instance. And I'd like to know whether ft_split() is any better than passing the command string to sh -c, for a much simpler exec() setup. \$\endgroup\$ Oct 18, 2021 at 19:13
  • \$\begingroup\$ @TobySpeight Thanks for the tip. This is an exercise and I am only allowed to use execve. Program usage is like ./pipex infile "cmd1 -op1 -op2" "cm2 -op1 -op2" outfile which is why I have to use a split, to split the command string. redirect_fd_to_fd doesn't to much than dup2 but for calling close before dup2. \$\endgroup\$
    – cassepipe
    Oct 19, 2021 at 15:03

1 Answer 1

7
\$\begingroup\$

Error handling

The way you handle error is quite unusual. Why use write() instead of fprintf(stderr, ...)? Why have some error messages stored in a variable like g_empty_string, but other errors messages are passes as literals, like "pipex: "? Why try to open() first and only if it fails check if filename is empty?

On Linux, I recommend you use err() to report errors and exit with an error code in one go. For example:

static int open_or_die(const char *filename, int flags, mode_t mode)
{
    if (!filename[0])
        err(EXIT_FAILURE, "Empty filename");

    int fd = open(filename, flags, mode);

    if (fd == -1)
        err(EXIT_FAILURE, "Error trying to open '%s'", filename);

    return fd;
}

Don't use access() to check if you can execve()

Instead of first checking with access() if a file is executable before calling execve(), just call execve() unconditionally, and then just check the return value of execve(). Otherwise, you will have a TOCTTOU bug.

if (execve(cmdv[0], cmdv, env) == -1)
    err(EXIT_FAILURE, "Could not execute '%s'", cmdv[0]);

Note that if execve() succeeds, it will never return, so there's no need to free anything afterwards.

Why you should wait()

If you don't call wait(), your program will terminate when the second command terminates. However, consider that the first command might still be doing something. It will then continue in the background, but you won't have control over it anymore. Suppose you want to call ./pipe twice, and the second call depends on results from the first call, then you would really want to ensure both processes of the first call have finished.

\$\endgroup\$
9
  • \$\begingroup\$ I would perhaps suggest that the OP use pthreads or openMP to create threads and join them instead of waiting and hoping that the work is done? \$\endgroup\$
    – jdt
    Oct 17, 2021 at 12:52
  • 3
    \$\begingroup\$ @upkajdt Unfortunately execve() replaces the whole process, not just one thread. Also see this StackOverflow question. \$\endgroup\$
    – G. Sliepen
    Oct 17, 2021 at 13:31
  • \$\begingroup\$ @G.Sliepen You say "It will then continue in the background, but you won't have control over it anymore." Indeed I can see that ./pipex /dev/stdin cat ls outfile does not wait for input indeed but I am not sure why it is the case. Why can't the zombie cat read from stdin even though the main process is over ? It should still run right ? \$\endgroup\$
    – cassepipe
    Oct 17, 2021 at 13:47
  • \$\begingroup\$ @cassepipe cat will still run but then the question is: what happens with /dev/stdin after ls exits? If I try to run that command in a shell, then after it finishes, the cat process is still running, but as soon as I press any key it exits with the error: cat: -: Input/output error. \$\endgroup\$
    – G. Sliepen
    Oct 17, 2021 at 14:46
  • \$\begingroup\$ @G.Sliepen Not sure I understand but to answer your question I have no idea as to what happens to /dev/stdin after ls exits. I'd say nothing since I opened the input file in the child process. When I run </dev/stdin cat | ls in a terminal it prints the ls output and then waits for input. I can enter one line, press Return and then I get back to the shell prompt, no error message. \$\endgroup\$
    – cassepipe
    Oct 17, 2021 at 16:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.