4
\$\begingroup\$

It seems like there's no good library function I can use for popen with array of arguments instead of single shell-interpreted shell. So I implemented my own one: c h.

popen_arr.h

#pragma once

struct FILE;

// Implemented by Vitaly _Vi Shukela in 2013, License=MIT

/**
* Fork and exec the program, enabling stdio access to stdin and stdout of the program
* You may close opened streams with fclose.
* Note: the procedure does no signal handling except of signal(SIGPIPE, SIG_IGN);
* You should waitpid for the returned PID to collect the zombie or use signal(SIGCHLD, SIG_IGN);
*
* @arg in stdin of the program, to be written to. If NULL then not redirected
* @arg out stdout of the program, to be read from. If NULL then not redirected
* @arg program full path of the program, without reference to $PATH
* @arg argv NULL terminated array of strings, program arguments (includiong program name)
* @arg envp NULL terminated array of environment variables, NULL => preserve environment
* @return PID of the program or -1 if failed
*/
int popen2_arr (FILE** in, FILE** out, const char* program, const char* const argv[], const char* const envp[]);

/** like popen2_arr, but uses execvp/execvpe instead of execve/execv, so looks up $PATH */
int popen2_arr_p(FILE** in, FILE** out, const char* program, const char* const argv[], const char* const envp[]);

/**
* Simplified interface to popen2_arr.
* You may close the returned stream with fclose.
* Note: the procedure does no signal handling except of signal(SIGPIPE, SIG_IGN);
* You should wait(2) after closing the descriptor to collect zombie process or use signal(SIGCHLD, SIG_IGN)
*
* @arg program program name, can rely on $PATH
* @arg argv program arguments, NULL-terminated const char* array
* @arg pipe_into_program 1 to be like popen(...,"w"), 0 to be like popen(...,"r")
* @return FILE* instance or NULL if error
*/
FILE* popen_arr(const char* program, const char* const argv[], int pipe_into_program);

popen_arr.c

#define _GNU_SOURCE // execvpe 
#include <stdio.h>
#include <unistd.h>
#include <errno.h>
#include <signal.h>
#include "popen_arr.h"


// Implemented by Vitaly _Vi Shukela in 2013, License=MIT

static int popen2_impl(FILE** in, FILE** out,  const char* program, const char* const argv[], const char* const envp[], int lookup_path) {

    int child_stdout = -1;
    int child_stdin = -1;

    int to_be_written = -1;
    int to_be_read = -1;

    if(in) {
        int p[2]={-1,-1}; 
        int ret = pipe(p);
        if(ret!=0) { return -1; }
        to_be_written=p[1];
        child_stdin =p[0];
        *in = fdopen(to_be_written, "w");
        if (*in == NULL) {
            close(to_be_written);
            close(child_stdin);
            return -1;
        }
    }
    if(out) {
        int p[2]={-1,-1};
        int ret = pipe(p);
        if(ret!=0) {
            if (in) {
               close(child_stdin);
               fclose(*in);
               *in = NULL;
            }
            return -1;
        }
        to_be_read   =p[0];
        child_stdout=p[1];
        *out = fdopen(to_be_read, "r");
    }


    int childpid = fork();
    if(!childpid) {
       if(child_stdout!=-1) {
           close(to_be_read);
           dup2(child_stdout, 1);
           close(child_stdout);
       }
       if(child_stdin!=-1) {
           close(to_be_written);
           dup2(child_stdin, 0);
           close(child_stdin);
       }
       if (lookup_path) {
           if (envp) {
               execvpe(program, (char**)argv, (char**)envp);
           } else {
               execvp (program, (char**)argv);
           }
       } else {
           if (envp) {
               execve(program, (char**)argv, (char**)envp);
           } else {
               execv (program, (char**)argv);
           }
       }
       _exit(ENOSYS);
    }

    if(child_stdout!=-1) {
        close(child_stdout);
    }
    if(child_stdin!=-1) {
        close(child_stdin);
    }

    return childpid;
}


int popen2_arr  (FILE** in, FILE** out,  const char* program, const char* const argv[], const char* const envp[])
{
    signal(SIGPIPE, SIG_IGN);
    return popen2_impl(in, out, program, argv, envp, 0);
}
int popen2_arr_p(FILE** in, FILE** out,  const char* program, const char* const argv[], const char* const envp[])
{
    signal(SIGPIPE, SIG_IGN);
    return popen2_impl(in, out, program, argv, envp, 1);
}
FILE* popen_arr(const char* program, const char* const argv[], int pipe_into_program) {
    FILE* f = NULL;
    if (pipe_into_program) {
        popen2_arr_p(&f, NULL, program, argv, NULL);
    } else {
        popen2_arr_p(NULL, &f, program, argv, NULL);
    }
    return f;
}

Do you have any advice on promoting it from just a project's code snippet into a reusable little library? Is API good? Shall it provide more features (at expence of being complexier) or more simpler (but a bit tricker to use)? Are there obviously bad parts of the code?

\$\endgroup\$
  • 1
    \$\begingroup\$ From what I read, you are not asking how to improve it, you are asking how to make it a library. \$\endgroup\$ – Jeff Vanzella Sep 10 '13 at 20:52
  • 1
    \$\begingroup\$ I see library code as one requiring higher quality than usual application code. So "making this into a library" implies "making good API, removing hacks". Shall I reformulate the question a bit? I'm not asking how to make it into *.a or *.so or how to create package with it... \$\endgroup\$ – Vi. Sep 10 '13 at 21:05
2
\$\begingroup\$

The function popen_arr would resemble popen more closely if it took a 'mode' parameter in the same way, instead of your pipe_into_program:

FILE* popen_arr(const char* program,
                const char* const argv[], 
                const char *mode);

where mode is "r", "w" or "r+".

And popen manages bidirectional operation using just one FILE * whereas your popen2_* functions take two.

Personally, I would simplify things by omitting the environment parameters.

The bidirectional link you create is perhaps easier to create with socketpair.

fdopen failure is caught in the if(in) case but not in the if(out) case.

You also need an analogue of pclose that closes the pipes and waits for the child process to exit (using wait etc).

You also omit any handling of SIGCHLD, which is sent of the death of the child process.

On your formatting, your spacing is inconsistent and there is often insufficient spacing, eg around operators (=, != etc) and after keywords (eg after if).

\$\endgroup\$
  • \$\begingroup\$ BTW am I expected on codereview.SE to edit the question according to the recommendations or leave it as is? \$\endgroup\$ – Vi. Dec 10 '13 at 11:34
  • \$\begingroup\$ No don't edit the code in the original question (answers refer to that code). \$\endgroup\$ – William Morris Dec 10 '13 at 16:40
  • \$\begingroup\$ How we should handle SIGCHLD? Store old handler, override, handle then restore old handler? Does usual popen deal with SIGCHLD? \$\endgroup\$ – Vi. Dec 10 '13 at 17:50

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.