4
\$\begingroup\$

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

#include <stdio.h>
#include <fcntl.h>
#include <unistd.h>
#include <stdlib.h>
#include <string.h>
#include <sys/stat.h>

// 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_free_pointer);
  free(output_content);
  return 1;
}

It's solely for fun and practicing Linux system programming, so I've decided not to divide it into smaller functions.

Here's what I find thought-provoking:

  • instead of reopening a file I'd prefer to open it in such a way that it becomes truncated just before writing,
  • this constant error-checking is a madness - can you propose a nicer solution?
  • I find this flow, with almost every single line beginning with if ... != NULL/-1 etc. a bit distracting
  • I'm not 100% sure if the code is devoid of off-by-one errors (ok, now I'm sure).
\$\endgroup\$
0
5
\$\begingroup\$

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:

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

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

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

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

  4. 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;
        }
    }
    
  5. 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.

  6. 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]).

\$\endgroup\$
2
  • \$\begingroup\$ I don't like the idea of modifying file in place. What would happen if we encountered some write error in the middle of file? Moving tempfile seems to be a better solution. When it comes to reading files containing multiple zero bytes (ie. file holes or sparse files), how would you do it? Isn't the easiest way reading from file into temporary char buffer until we meet either newline (it might need reallocing the buffer, if the line is long) or a hole, then writing or changing offset? \$\endgroup\$ – cafe_ Nov 1 '15 at 23:04
  • 1
    \$\begingroup\$ Yes, read a whole line, transform, and output. Just don't depend on any function using zero-terminated strings. \$\endgroup\$ – Deduplicator Nov 1 '15 at 23:57
1
\$\begingroup\$

Here's what I changed:

  1. Space usage - now a size of buffer containing read characters shouldn't be bigger than a doubled length of the longest line in the file.
  2. Time complexity - the average complexity should be linear.
  3. I use perror(argv[1]).
  4. validate_cmdline_args is removed.
  5. try macro is really nice and makes code easier to read.

I start with a buffer of DEFAULT_BUFFER_SIZE size. When I find a new line, it is reversed and written to temporary file. If the end of buffer is reached, the beginning of next line is moved to the beginning of buffer and initial_offset is increased accordingly. If there isn't any newline character in buffer, then the buffer size is also doubled. It might look like reinventing the wheel, but my goal was to not use FILE* operating functions like getline.

Code:

#include <fcntl.h>
#include <unistd.h>
#include <stdio.h>
#include <stdlib.h>
#include <stdbool.h>
#include <string.h>
#include <sys/stat.h>

#define try(cond) if(cond) {} else goto errno_error;
#define DEFAULT_BUFFER_SIZE 1024

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

void reverse_line(char *from, char* to) {
  while(from < to) {
    char tmp = *from;
    *from++ = *to;
    *to-- = tmp;
  }
}

bool end_of_file(int fd) {
  off_t old_position = lseek(fd, 0, SEEK_CUR);
  off_t end_position = lseek(fd, 0, SEEK_END);
  lseek(fd, old_position, SEEK_SET);
  return old_position >= end_position;
}

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

  char* buffer = NULL;
  char tmpfile_filename[13] = "cp_tmpXXXXXX";
  int source_descriptor;
  int tmpfile_descriptor;

  try((source_descriptor = open(argv[1], O_RDONLY)) != -1)
  try((tmpfile_descriptor = mkstemp(tmpfile_filename)) != -1)
  try((buffer = malloc(sizeof(char) * DEFAULT_BUFFER_SIZE)) != NULL)

  bool reading_file = true;
  int size_multiplier = 1;
  int initial_offset = 0;

  while(reading_file) {
    ssize_t bytes_read;
    int i = 0;
    int from = 0;
    bool found_newline = false;

    try((bytes_read = read(source_descriptor, buffer+initial_offset, (DEFAULT_BUFFER_SIZE*size_multiplier)-initial_offset)) != -1)
    if(end_of_file(source_descriptor))
      reading_file = false;

    int all_bytes = initial_offset + bytes_read;
    for(i = 0; i < all_bytes; ++i) {
      if(buffer[i] == '\n') { // found a line
        found_newline = true;
        reverse_line(buffer+from, buffer+i-1);
        try(write(tmpfile_descriptor, buffer+from, i-from+1))
        from = i+1;
        initial_offset = 0;
      }
      else if(i == all_bytes-1 && !reading_file) { // found end of file, lack of newline character
        reverse_line(buffer+from, buffer+i);
        try(write(tmpfile_descriptor, buffer+from, i-from+1))
      }
      else if(i == all_bytes-1) { 
        if(!found_newline) { // buffer is too small
          size_multiplier *= 2;
          try((buffer = realloc(buffer, DEFAULT_BUFFER_SIZE*size_multiplier)) != NULL)
        }
        initial_offset = i-from+1;
        strncpy(buffer, buffer+from, i-from+1); // copy beginning of next line into the buffer start
      }
    }
  }

  try(close(source_descriptor) != -1)
  try(rename(tmpfile_filename, argv[1]) != -1)
  try(close(tmpfile_descriptor) != -1)

  free(buffer);
  return 0;
errno_error:
  perror(argv[1]);
  free(buffer);
  return 1;
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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