Executing a shell command OS-independently

Question

The goal of the code is to convert a Graphviz DOT file to an SVG file, and it achieves this by creating a child process and executing the "dot" command.

#include <stdio.h>
#include <stdlib.h>

#if defined(_WIN32) 
#include <windows.h>
#define OS_WINDOWS 1
#elif defined(__unix__) || defined(__unix) || defined(__linux__) || (defined(__APPLE__) && defined(__MACH__))) 
#include <sys/wait.h>
#include <unistd.h>
#define OS_UNIX 1
#endif


int main(void)
{
#if defined(OS_UNIX) 
    const int pid = fork();

    if (pid == -1) {
        fprintf(stderr, "Error: could not fork a child: %s\n", 
                strerror(errno));
        return EXIT_FAILURE;
    } else if (pid == 0) {
        static char *const args[] = {"dot", "-Tsvg", "trie.dot", "-o", "trie.svg", (char *)NULL};

        if (execv("/usr/bin/dot", args) == -1) {
            fprintf(stderr, "Error: could not execute the child: %s\n",
                    sterror(errno));
            _Exit(EXIT_FAILURE);
        }
    } else {
        int rv;
        if (wait(&rv) == -1) {
            fprintf(stderr, "Error: could not wait for the forked child: %s\n",
                    sterror(errno));
            return EXIT_FAILURE;
        } 

        if (!WIFEXITED(rv)) { 
            fprintf(stderr, "Error: child exited with an error status: %d\n", WEXITSTATUS(rv);
            return EXIT_FAILURE;
        }
    }
#else
    STARTUPINFO si;
    PROCESS_INFORMATION pi;

    ZeroMemory(&si, sizeof(si));
    si.cb = sizeof(si);
    ZeroMemory(&pi, sizeof(pi));

    static char *const cmd= "dot -Tsvg trie.dot -o trie.svg";

    if (!CreateProcess(NULL, cmd, NULL, NULL, FALSE, 0, NULL, NULL, &si, &pi)) {
       fprintf(stderr, "CreateProcess() failed (%d).\n", GetLastError());
       return EXIT_FAILURE;
    }

    const DWORD rv = WaitForSingleObject(pi.hProcess, INFINITE);
    
    switch(rv) {
      case WAIT_ABANDONED:
            fprintf(stderr, "Error: mutex object was not released by the thread that"
                    "owned the mutex object before the owning thread terminated.\n");
            return EXIT_FAILURE;
      case WAIT_OBJECT_0:
            fprintf(stderr, "Error: the child thread state was signaled.\n");
            return EXIT_FAILURE;
      case WAIT_TIMEOUT:
            fprintf(stderr, "Error: time-out interval elapsed, and the child thread's state is nonsignaled.\n");
            return EXIT_FAILURE;
      case WAIT_FAILED:
            fprintf(stderr, "Error: WaitForSingleObject() failed, error status: %d.\n", GetLastError());
            return EXIT_FAILURE; 
    }

    CloseHandle(pi.hProcess);
    CloseHandle(pi.hThread);
    return EXIT_SUCCESS;
#endif
}

The code compiles successfully on both Windows 10 and Linux Mint 21, which serves my purpose.

Review request:

1. Are there any simplifications or improvements that can be made to the code?
2. Is the current implementation sufficient for its intended purpose, or are there potential pitfalls that need addressing?
3. Is the error-handling approach used in the code appropriate?

Edit: The code is part of a larger command-line program, which has to be ported over to Windows. The only thing non-portable about the program is fork() and exec().

Answer 1

Just use system()

While system() is often abused, here it is exactly what you want. It's a standard C function, so it's very portable:

#include <stdlib.h>

int main(void)
{
    int result = system("dot -Tsvg trie.dot -o trie.svg");

    if (result != 0) {
        // Something went wrong, handle the error
        …
        return EXIT_FAILURE;
    }
}

Answer 2

1. simplifications?

You didn't describe the constraints this project is working under. If a python interpreter is available, you could readily achieve your goal by using "import graphviz". The simplest thing to do would be to write a Bash script, or Makefile, but for some reason you chose not to.

As far as the C code goes, it is twice as complex as necessary. You have a pair of entirely separate applications here, separated by ifdef's. Consider breaking out a pair of distinct source files, one per target platform.

There is no need to #define OS_WINDOWS 1, given that you never use it.

The review context suggests that you are testing on exactly two machines, yet the #if disjuncts tell me that there's a bunch of operating system / compiler combinations you should try compiling on, to verify the CPP symbols worked out as expected.

2. sufficient?

It would help to include the project's Makefile as part of this submission, so we can see what CFLAGS are being passed to the different compilers.

I found the "trie.dot" & "trie.svg" hardcodes a bit surprising. Consider obtaining the input filename from argv.

3. error handling

For this

        fprintf(stderr, "Error: could not fork a child: %s\n", strerror(errno));

consider using perror(). Similarly for several other error reports. Consider using goto err, so you can DRY up the various "report and exit" snippets of code.

In the event that execv() fails, it's unclear why you chose to prefer _Exit(EXIT_FAILURE). I was expecting to see a return statement instead.

I was a little surprised to see /usr/bin/dot hardcoded, instead of searching $PATH for it.

I am reading the wait(2) man page:

WEXITSTATUS(wstatus)

returns the exit status of the child. This consists of the least significant 8 bits of the status argument that the child specified in a call to exit(3) or _exit(2) or as the argument for a return statement in main(). This macro should be employed only if WIFEXITED returned true.

Yet you only call it after seeing a return value of false. Which makes me wonder if your testing ever managed to actually execute that if clause.

---

This codebase achieves its design goals.

I would be willing to delegate or accept maintenance tasks on it.

Answer 3

This is not an OS Independent solution. This is a 2 OS dependent solution, and will only work in *nix-like systems and (maybe) Windows. Might work in Android. Might not. Also, you have one codebase, but 2 distributables.

For a truly OS independent solution use an OS Independent language like Python. It will also simplify your code and maintenance. That might mean including an interpreter with the installer for Windows solutions.

The solution to use system() is a viable solution, depending on how users will interact with the program and how you parse the command line. The default handling of the command line is probably NOT robust enough to deal with handling the two targeted OSes, so you should consider taking in the entire command line string and writing your own command line parser. You'll have less surprises that way.

Lastly, your current solution might break if Microsoft alters how the commandline works, or even what privileges it takes to launch a commandline, or program from it, from inside another program. Antivirus programs may also take issue with your implementation. There are other possible gotchas for Windows deployment. Hence something like Python is probably preferable.