Reverse all lines in a file

The script below should open a file, read and reverse every line, then overwrite the source file.

```
#include
#include
#include
#include
#include
#include

// reverses the order of characters in every line of SRC file

void validate_cmdline_args(int argc, char **argv) {
if(argc != 2) {
printf("Usage: %s SRC\n", argv[0]);
exit(1);
}
}

void reverse_string(char *str) {
char *end = strchr(str, (int)'\0') - 1;
char tmp;
while(str < end) {
tmp = *str;
str++ = end;
*end-- = tmp;
}
}

void process_line(char output_content, char line) {
reverse_string(line);
strcat(output_content, line);
strcat(output_content, "\n");
}

int main(int argc, char **argv) {
validate_cmdline_args(argc, argv);

char* file_content = NULL;
char* output_content = NULL;
char *file_content_free_pointer = NULL;

int file_descriptor;
if((file_descriptor = open(argv[1], O_RDONLY)) == -1)
goto errno_error;

struct stat file_stat;
if(fstat(file_descriptor, &file_stat) == -1)
goto errno_error;

if((file_content = malloc((size_t)file_stat.st_size+1)) == NULL)
goto errno_error;
file_content_free_pointer = file_content;

if(read(file_descriptor, file_content, (size_t)file_stat.st_size) == -1)
goto errno_error;
file_content[(int)file_stat.st_size] = '\0';

if(close(file_descriptor) == -1)
goto errno_error;

if((output_content = malloc((size_t)file_stat.st_size+2)) == NULL)
goto errno_error;
output_content[0] = '\0';

char* delim = "\n";
char* line = NULL;

while((line = strsep(&file_content, delim)))
process_line(output_content, line);

if((file_descriptor = open(argv[1], O_WRONLY | O_TRUNC)) == -1)
goto errno_error;

if(write(file_descriptor, output_content, (size_t)file_stat.st_size) == -1)
goto errno_error;
if(close(file_descriptor) == -1)
goto errno_error;

free(file_content_free_pointer);
free(output_content);

return 0;
errno_error:
perror(NULL);
free(file_content

Let's get the broad picture right first.

• Do you know Schlemiel the Painter? No? Well, read about him on Joel's blog.

• Don't follow in his footsteps; they are too deep because he always restarts from the beginning.

• You might want to take that opportunity to make your code tolerate zero-bytes.

Now, having gotten that off the agenda, some other gross wastefulness: you are allocating memory on the order of twice the files size. But you could easily avoid that: Only read in a line, transform it, write it to the output.

There you have two basic good choices:

• Modify the file in-place, as your transformation does not affect how much space each line needs. You don't even need any additional disk-space.

• Write the output to a temporary file, and then atomically update it by moving: rewrite existing file so that it gets replaced by new version atomically, only once fully written

Now, about reducing the tedium of repeating the exact same code. That's one of the (few) places where the C preprocessor shines:

#define TRY(cond) if(cond) {} else goto error;
// Use it
#undef TRY

Now lots of other small things:

-
Curiously, one normally doesn't extract a functions precondition-checking into another function, as then the precondition is no longer there to see, and anyway the boilerplate is normally longer than what was extracted: main → validate_cmdline_args.

-
Why do you use strchr to find the end of a string, instead of the better-known (and probably more efficient) strlen?

-
For completeness, casting the second argument, a character-literal, to int is completely useless because it's already an int.

-
There's no reason to define tmp in reverse_string outside the loop. Actually, wouldn't that be a good place for a for-loop?

void reverse_string(char* str) {
    for(char* end = str + strlen(str) - 1; str > end; ++str, --end) {
        char tmp = *str;
        *str = *end;
        *end = tmp;
    }
}

-
Strictly speaking, both your original and my replacement for reverse_string are UB if strlen(str) == 0, because that would create a pointer one before the string. That can be fixed by replacing the first 1 with !!*str or adding an if-condition, but it doesn't matter in practice.

-
It might make sense to output the filename given to your program on error. That might be useful for anyone trying to understand what went wrong, so perror(argv[1]).

StackExchange Code Review Q#107975, answer score: 5