patterncModerate
Application for converting markdown to PDF
Viewed 0 times
applicationmarkdownforconvertingpdf
Problem
First time writing C in a long time and would like some code review to try and improve my code. I have some specific questions, but first an introduction to the code.
The application is a simple command line application for converting markdown to pdf.
The source code can be found in the repository on Github.
The main logic is in main.c, and is hopefully pretty straight forward.
My first question is, have I made any common C errors, like buffer overflows or unreleased memory?
I don't think so, I don't use any malloc calls in my code, but would like someone more experienced to take a look.
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?
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?
Are there anything else I should do to be a good *nix citizen? E.g. the man page.
This block in main.c feels a bit clunky, and I would like to extract it to a couple of methods instead, to get a better flow when reading the main application logic. But I could not find a good way to do so. The logical parts are
```
/ Read SOURCE(s) /
hoedown_buffer *ib = hoedown_buffer_new(HOEDOWN_IUNIT);
if (n_files == 1) {
int error_code = read
The application is a simple command line application for converting markdown to pdf.
The source code can be found in the repository on Github.
The main logic is in main.c, and is hopefully pretty straight forward.
My first question is, have I made any common C errors, like buffer overflows or unreleased memory?
I don't think so, I don't use any malloc calls in my code, but would like someone more experienced to take a look.
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?
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?
Are there anything else I should do to be a good *nix citizen? E.g. the man page.
This block in main.c feels a bit clunky, and I would like to extract it to a couple of methods instead, to get a better flow when reading the main application logic. But I could not find a good way to do so. The logical parts are
read_markdown, create_renderer, generate_filename and write_html. But the code seems to be doing to much for a simple extract method. Do you have any suggestions?```
/ Read SOURCE(s) /
hoedown_buffer *ib = hoedown_buffer_new(HOEDOWN_IUNIT);
if (n_files == 1) {
int error_code = read
Solution
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 confirmation.
-
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 your 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 which it will then use later for compiling. The real "fun" part comes in where you dependency has sub-dependencies, which I also had to deal with...
It's a lot more work, but also a whole lot more portable and less stressful for the end user (which should be the ultimate goal really for all developers, making the usage of your application as hassle-free as possible for your consumers).
-
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.
-
Are there anything else I should do to be a good *nix citizen? E.g.
the man page.
man pages are nice. Besides, how else are you supposed to tell your users to RTFM?
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:
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 (
-
You don't have to
Or the more "universal" way to zero out an array (since it works with every type):
-
Nice job generating a temporary file safely! I have one big problem with it though, and it has to do with this line
See that
To which I created a file like such:
```
// Creates temporary file safely
char flacFile[FILENAME_MAX] = "";
const cha
-
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 confirmation.
-
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 your 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 which it will then use later for compiling. The real "fun" part comes in where you dependency has sub-dependencies, which I also had to deal with...
It's a lot more work, but also a whole lot more portable and less stressful for the end user (which should be the ultimate goal really for all developers, making the usage of your application as hassle-free as possible for your consumers).
-
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.
-
Are there anything else I should do to be a good *nix citizen? E.g.
the man page.
man pages are nice. Besides, how else are you supposed to tell your users to RTFM?
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] = "";Or the more "universal" way to zero out an array (since it works with every type):
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. For example, the location my system prefers to point to is /var/folders/xp/j1l6q2z12n54k9y4r7574l0w0000gn/T/. Here's how I solved the problem, abstracted away in a function:const char* getTmpDir(void)
{
char *tmpdir = NULL;
if ((tmpdir = getenv("TEMP"))) return tmpdir;
else if ((tmpdir = getenv("TMP"))) return tmpdir;
else if ((tmpdir = getenv("TMPDIR"))) return tmpdir;
else return "/tmp/";
}To which I created a file like such:
```
// Creates temporary file safely
char flacFile[FILENAME_MAX] = "";
const cha
Code Snippets
if (error_code > 0) {
fprintf(stderr, "I/O errors found while reading input.\n");
hoedown_buffer_free(ib);
fclose(in);
exit(error_code);
}char nameBuff[23] = "";char nameBuff[23] = {};strncpy(nameBuff, "/tmp/mdpdf-XXXXXX.html", 22);const char* getTmpDir(void)
{
char *tmpdir = NULL;
if ((tmpdir = getenv("TEMP"))) return tmpdir;
else if ((tmpdir = getenv("TMP"))) return tmpdir;
else if ((tmpdir = getenv("TMPDIR"))) return tmpdir;
else return "/tmp/";
}Context
StackExchange Code Review Q#71191, answer score: 16
Revisions (0)
No revisions yet.