9
\$\begingroup\$

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().

\$\endgroup\$
5
  • 3
    \$\begingroup\$ posix_spawn combines fork/exec, and can be more efficient on stuff like Cygwin where you have a POSIX API on top of an OS that has CreateProcess, not fork. man7.org/linux/man-pages/man3/posix_spawn.3.html - and it's just one syscall to check errors from. POSIX.2008 \$\endgroup\$ Jan 15 at 16:22
  • \$\begingroup\$ @PeterCordes That's new to me, thank you. It is unable to recognize relative paths albeit, contrary to what its man page says. It recognizes "/usr/bin/ls", but fails for "ls". Similarly, it recognizes "/usr/bin/dot", but fails for "dot". godbolt.org/z/ncrqdb1c1 \$\endgroup\$
    – Harith
    Jan 15 at 21:39
  • 3
    \$\begingroup\$ The man page says posix_spawnp uses $PATH like execvp. posix_spawn doesn't, like execv. You were using execv so I used the non-p version. It works for me on my Arch Linux desktop. (When you say "relative path", I think you mean using PATH. But relative path actually means relative to the current directory. If I ln -s /bin/ls . so ./ls is a valid path to an executable, your code with posix_spawn runs it. But posix_spawnp doesn't because I don't include the current directory in my $PATH. (I changed it from ls to foobar so it wouldn't find it there either.) \$\endgroup\$ Jan 15 at 22:19
  • 1
    \$\begingroup\$ Shouldn't function sterror be strerror? also missing one parentheses at end of line ` fprintf(stderr, "Error: child exited with an error status: %d\n", WEXITSTATUS(rv);` ? \$\endgroup\$ Jan 16 at 14:17
  • \$\begingroup\$ @Harith, as fork() returns a pid_t, why assign the function result to an int? \$\endgroup\$ Jan 16 at 18:04

3 Answers 3

20
\$\begingroup\$

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;
    }
}
\$\endgroup\$
2
  • 7
    \$\begingroup\$ As long as the arguments are fixed and not built up through %s with shoddy quoting... (Keeping in mind that system() in both cases invokes a shell so it has to account either for /bin/sh's or cmd.exe's special characters depending on OS, so if you take user input it quickly stops being 100% portable.) \$\endgroup\$
    – user1686
    Jan 15 at 7:52
  • \$\begingroup\$ Would this still be suitable if I add some flags to the program to change the name of the output files and/or output file format? (For instance, .svg ---> .pdf/any that's supported by dot) \$\endgroup\$
    – Harith
    Jan 19 at 7:32
6
\$\begingroup\$
  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.

  1. 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.

  1. 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.

\$\endgroup\$
1
  • 2
    \$\begingroup\$ execv is called in the forked child process. Returning would actually be wrong here, _Exit is correct in this case as it does not have additional side-effects a normal exit would have. \$\endgroup\$
    – DarkDust
    Jan 16 at 12:12
1
\$\begingroup\$

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.

\$\endgroup\$
3
  • 5
    \$\begingroup\$ It will work on Apple systems because macOS supports execv(). OP's code even explicitly tests for it. \$\endgroup\$
    – G. Sliepen
    Jan 15 at 23:56
  • 2
    \$\begingroup\$ "Replace your code with Python" is an… interesting way to solve the problem. So is "write your own command line parser". Cracking nuts with sledge hammers. \$\endgroup\$
    – DarkDust
    Jan 16 at 12:22
  • \$\begingroup\$ Thanks for the correction G.Sliepen. I've updated my answer to remove that part. \$\endgroup\$
    – CapnJack
    Jan 17 at 0:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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