Return to Revisions

Answers to your specific questions:

-

My first question is, have I made any common C errors, like buffer overflows or unreleased memory?

A leak is always connected to a resource. A resource is by definition something that you acquire manually, and that you must release manually. Memory is a prime example, but there are other resources, too (file handles, mutex locks, network connections, etc.).

A leak occurs when you acquire a resource, but subsequently lose the handle to the resource so that nobody can release it. A lesser version of a leak is a "still-reachable" kind of situation where you don't release the resource, but you still have the handle and could release it. That's mostly down to laziness, but a leak by contrast is always a programming error.

I don't see any leaks in your code, but I'm only using my half-human side to analyze that. I would use a more thorough tool such as Valgrind to provide more substantive conformation.

-

I'm compiling the application and dependencies statically, how common is this? My main reason for this is that one of the dependencies (wkhtmltopdf) is a bit tricky to compile, so I want to be able to provide a binary release that doesn't depend on a shared library.

I have added the dependencies to the repository, not what I'm used to from Ruby or Go, but it seems like the most straight forward way. One other solution I tried was to download the dependencies using git in the Makefile, like this. So they did not have to be added, but that did not feel like a good C solution. Feels a bit like a "roll you own" solution. How would you do this?

I had a project that was somewhat similar to yours in this aspect. As long as all those dependencies are available, then there is no problem

The problem for my project to compile was that I couldn't guarantee that. I also didn't want the user scrounging around trying to figure out how to get my application to simply compile. Here's what I did: I used CMake to handle all of that. It took a bit of work, but here's the jist of what it does: it takes all of the dependencies that you application relies on and searches the host computer for them. If it finds them, it stores that location for it's use later when compiling. If it can't find them, then it manually downloads them and performs it's own build of the dependency. A lot more work, but also a whole lot more portable and less stressful for the end user.

-

Is there anything in the Makefile I could do to make it easier to build for various package managers? E.g. apt-get, homebrew, ports, pkg?

Another reason to use CMake or another similar build system, so that it generates a Makefile for you based on the system it's running on.

A few comments on the code posted in the question (hopefully not conflicting with @ferada):

• I notice some suspiciously similar code in a few places:

   if (error_code > 0) {
       fprintf(stderr, "I/O errors found while reading input.\n");
       hoedown_buffer_free(ib);
       fclose(in);
       exit(error_code);
   }
  

  The stuff within the conditional could be extracted to a function so that reuse would be possible, and would help your code follow the DRY principles. This should also shorten your code a bit

• You should be declaring your counter variable (int i in this case, within your for loops), so that it is in the smallest scope possible.^(C99)

• You don't have to memset your char arrays to 0, just initialize them that way.

   char nameBuff[23] = "";
  

• Nice job generating a temporary file safely! I have one big problem with it though, and it has to do with this line

strncpy(nameBuff, "/tmp/mdpdf-XXXXXX.html", 22);

See that /tmp/ part? That isn't guaranteed to be the temporary file storage location for every Unix system, making your code a lot less portable. Here's how I solved the problem, I abstracted it away in a function:

    const char* getTmpDir(void)
    {
        char *tmpdir = NULL;
        if ((tmpdir = getenv("TEMP"))) return tmpdir;
        if ((tmpdir = getenv("TMP"))) return tmpdir;
        if ((tmpdir = getenv("TMPDIR"))) return tmpdir;
        return "/tmp";
    }

To which I created a file like such:

    // Creates temporary file safely
    char flacFile[FILENAME_MAX] = "";
    const char *fileRoot = getTmpDir();
    snprintf(flacFile, FILENAME_MAX, "%sXXXXXXXXXXXXX.flac", fileRoot);
    mkstemps(flacFile, 5); // the 5 is for the length of the suffix ".flac"

Since I can never actually know the length of fileRoot for my static array creation, I just used the maximum length that a filename could be, very helpfully stored in the predefined macro FILENAME_MAX.