Improving C file reading function

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 ("

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

• 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

StackExchange Code Review Q#11675, answer score: 5