Improving C file reading function

Question

I am programing in C language and have created the below functions to read files. There are two functions that I can use to read files. I wanted to know how I can further improve these two in terms of efficiency and out of these two which one would be better to use for reading files.

First is:

        char *read_data_from_file(char *data, char *path, int *fsz1)
        {
              int imgFD=0;
              struct stat fst;
              int fsz = *fsz1;
              fsz = 0;

              imgFD = open(path, O_RDONLY);
              if (imgFD <= 0){
                      *fsz1 = fsz;
                      return NULL;
              }

              fstat(imgFD, &fst);
              fsz = fst.st_size;
              printf("the size %s = %d inmfd = %d \n ", path, fsz, imgFD);
              if (fsz <= 0){
                      *fsz1 = fsz;
                      return NULL;
              }
              data = (char *)malloc(sizeof(char)*(fsz+1));
              read(imgFD, data, fsz);
              close(imgFD);

              *fsz1 = fsz;
              return (data);
      }

Another is:

      int read_data_from_file(int fd, void *buffer, int count)
      {
              void *pts = buffer;
              int status = 0, n;

              if(count < 0) return -1;
              while(status != count) {
                      n = read(fd, pts+status, count-status);
                      if(n < 1) return n;
                      status += n;    
              }   
              return (status);
      }       

The calling portion of the above mentioned function is:

      int main(int argc, char *argv[])
      {       
              struct stat sb;
              off_t len;
              char *p, *buffer = NULL;
              int fd; 

              fd = open("test.txt" , O_RDONLY);
              if (fd == -1) {
                      perror ("open");
                      return 1;
              }
              if (fstat (fd, &sb) == -1) {
                      perror ("fstat");
                      return 1;
              }
              if (!S_ISREG (sb.st_mode)) {
                      fprintf (stderr, "%s is not a file\n", argv[1]);
                      return 1;
              }
              debug_print("printing start time and size : %d\n", sb.st_size);
              buffer = (char *)malloc(sizeof(char) * (sb.st_size + 1));

              read_data_from_file(fd, buffer, sb.st_size);

Answer 1

The two functions do not do the same thing so comparing their efficiency is not sensible. A few comments on the code though (adding to those of @mjfgates)

first read_data_from_file:

• path should be const
• imgFD should be comapred <0 not <=0
• check the return value of fstat instead of checking whether fsz is less than 0.
• use perror to print failure reason after detecting open failure
• fsz == 0 is valid and should return an empty buffer (ie return data and set *fsz1 to zero)
• don't cast the return value of malloc
• sizeof(char) is 1 by definition
• check whether malloc succeeded.
• you allocated fsz+1 bytes, presumably allowing for a terminator but didn't add one (\0)
• check whether read succeeded.
• check whether close succeeded (if pedantic).
• why does data get brackets in return (data) and NULL not in return NULL? Prefer the latter (no brackets).

second read_data_from_file:

• pts should be char * or else you will get a compiler warning. Are warnings enabled for you?
• keywords (if, while, etc) preferred with a space - ie. if (...) instead of if(...)
• if you are reading a normal file, the loop is not required. Just one read, as in the first version.
• if you are reading something other than a file, then a loop is needed, but your loop is incorrect. If the read call gave some data, you will get n = number read, if it got to end of file, you will get 0, if there was an error or if reading was interrupted before any daat was received you will get an error (-1) and errno will be set. In the latter case you must work out whether there was an error or whether the call was interrupted in which case you can loop.
• if count is bigger than the actual file size, your loop never exits.
• status is misnamed - it holds the number of bytes read so far.

main:

• return EXIT_FAILURE or EXIT_SUCCESS
• ok, it is a file you are reading so the loop in the second read_data_from_file is not necessary.
• as before, don't cast malloc return and sizeof(char) is 1 by definition

Answer 2

In your first read_data_from_file(), you're filling in the out parameter fsz1 with zero, or with -1, depending, when things go wrong. However, your caller isn't going to be able to DO anything with that number. Don't fill it in, just return NULL.

In your second read_data_from_file(), you carefully loop on read() to handle the possibility that read() will only return part of the data you ask it for. In the first, you don't. Are you worried about this or not? If read() will actually return partial data put the loop in your first function; if it won't, take the loop out of the second.

Answer 3

          if (imgFD <= 0){
                  *fsz1 = fsz;
                  return NULL;
          }

In this particular case the assignment to *fsz1 is a no-op, since fsz's value came from *fsz1.

However, I would argue that doing something inside every return block (like your assignments to *fsz1) get tedious to maintain. One style would be to not have early return statements, and do the assignments you want at the end. You can still check for errors, just don't heave repeated returns, example:

char *data = NULL;

if (imgFd >= 0)
{
   // Do the rest of the work of the function

   data = malloc(/* ... */);
}

if (data)
{
   // more work ...
}

// Single block which assigns to fsz1 in all cases.
// This would also be a good place to free any buffers, etc.
//
*fsz1 = fsz;
return data;

Moving on to:

          fstat(imgFD, &fst);

You're not checking for a failure from fstat.

          fsz = fst.st_size;

What if int is not big enough to hold file size? For example if int were 32-bits, and the file were greater than 2GB. The type of st_size is off_t which is likely to be at least 64 bits in sane platform.

          data = (char *)malloc(sizeof(char)*(fsz+1));

malloc can return NULL on failure. In C you don't need to cast from (void*) to another pointer type. sizeof(char) is also always 1. Further it seems that you've clobbered the data parameter entirely, so there is no sense in having that as a parameter. Lastly, it seems like a really big file would make this allocation unlikely to succeed.

          read(imgFD, data, fsz);

You need to observe the return value here too. It returns the number of bytes read, or 0 on end of file, or -1 on error.

Answer 4

Another pretty popular way of reading files into memory is memory mapping (mmap). That's usually the way programs and shared libraries loaded in linux.

Aside of that what that char *data parameter is for? You just override value of local variable which receives that parameter with pointer to newly allocated memory
data = (char *)malloc(sizeof(char)*(fsz+1));

Why do you use that fsz+1 since you already pass back to caller the size of your file? Just for simplifying converting it to ASCIIz string?
Note that C language standart clearly states that sizeof(char) always equals to 1 and nothing else. So multiplying to it will just disappear during compilation.

Note that the most awful issues you can introduce into the code is scrambling it in that way that no one can understand you. Consider to choose some guide lines in naming your variables fsz and fsz1 is might be understand as "file size" and "file size" (with tick or alternative). But types of those variables are totally different (int and int*).