Fix a few bugs and code smells, but mostly convert to autotools #123
Conversation
Several calls to fread were missing checks to ensure that the expected amount of data was read.
There's a small block of code that calls strnlen on a string, saves the esult in a variable, conditionally decrements the variable, and then does nothing with it, making the entire block of code a no-op. I don't want to just remove it entirely since it's possible that there was intended to be some sort of check here that was inadvertently omitted. So to make the compiler stop complaining I've commented out the code, but I've left a comment above it explaining why it was commented out and pointing out that maybe something different needs to be done with it.
The check for the HTTP response code from the curl library was written incorrectly and guaranteed to always fail. I've fixed the logic to reflect what I believe was intended.
Don't assume that the reason why we didn't find enough slashes in a URL is because the user didn't specify the slash at the end of the host name, unless we did find the first two slashes. Add some curly braces around an if block to make it clear to people and the compiler which statement an `else` applies to. The logic was correct before but the indentation was wrong, making it especially confusing.
This commit converts the build process from a hand-written Makefile that works on Linux, FreeBSD, and macOS, to an automatically generated Makefile managed by the autotools toolset. This incldues: * Add the compile, config.guess, config.sub, depcomp, install-sh, and missing helper scripts that autotools requires to be shipped with the package in order for configure to work. * Rename Makefile to Makefile.am and restructure it for compatibility with autotools and specifically with the stuff in our configure script. * Create the configure.ac source file which is turned into the configure script. * Rename Doxyfile to Doxyfile.in so that the source directories can be substituted into it at configure time. * Tweak .gitignore to ignore temporary and output files related to autotools. * Generate Makefile.in, aclocal.m4, and configure using `autoreconf` and include them as checked-in source files. While I can't fully document how autotools works here the basic workflow is that when you need to make changes to the build, you update Makefile.am and/or configure.ac as needed, run `autoreconf`, and commit the changes you made as well as any resulting changes to Makefile.in, aclocal.m4, and configure. Makefile should _not_ be committed into the source tree; it should always be generated using configure on the system where the build is being run.
|
Thanks, I can see you put in a lot of efforts, and your code quality is really good. I have added you as a collaborator. You can now push into this project directly. :) |
|
@fangfufu I appreciate your confidence but I will still probably ask you to review any future changes that I want to commit, unless they are quite minor. Two pairs of eyes are better than one! |
|
@jikamens , I don't give collaborator access out to everyone - the only other person I have given it to is my Debian package maintainer. |
| @@ -724,16 +739,28 @@ LinkTable *LinkTable_disk_open(const char *dirn) | |||
|
|
|||
| LinkTable *linktbl = CALLOC(1, sizeof(LinkTable)); | |||
|
|
|||
| fread(&linktbl->num, sizeof(int), 1, fp); | |||
| if (sizeof(int) != fread(&linktbl->num, sizeof(int), 1, fp)) { | |||
There was a problem hiding this comment.
Yikes,
On success, fread() and fwrite() return the number of items read or written. This number equals the number of bytes transferred only when size is 1. If an error occurs, or the end of the file is reached, the return value is a short item count (or zero).
There was a problem hiding this comment.
This makes me wonder if the cache system has been broken on the master branch since last September...
There was a problem hiding this comment.
The point is that sizeof(int) is 4, but we are only expecting 1 to be returned here, as we are reading 1 int.
| if (sizeof(long) != fread(&cf->time, sizeof(long), 1, fp) || | ||
| sizeof(off_t) != fread(&cf->content_length, sizeof(off_t), 1, fp) || | ||
| sizeof(int) != fread(&cf->blksz, sizeof(int), 1, fp) || | ||
| sizeof(long) != fread(&cf->segbc, sizeof(long), 1, fp) || |
There was a problem hiding this comment.
These are all wrong too. I have fixed them here:
28293b5
|
Thanks for catching those errors! Sorry about that. |
The best way to review this PR is probably to review the commits one by one rather than reviewing them all at once, since each commit stands alone.
I have confirmed that the code in this PR builds and installs successfully on Debian Linux, FreeBSD, and macOS.