Making a program that lists files in a directory and sorts the output using stat()

Question

I wrote a program that uses stat() to find some details of all the files in a current directory. I then store all that information into a string using sprintf() and keep adding to the size to prevent my string from being overwritten and then I copy the string into a char**. This code seems to work but I was wondering if there is a way to make this code more efficient.

The output has to be as follows

-rw-rw-r-- tom users 1562 Sep 29 12:00 hello.txt
-rw-rw-r-- dick users 1024 Sep 29 12:05 igloo.txt
-rw-rw-r-- harry users 176 Sep 28 11:27 Makefile
-rw-rw-r-- chaz users 2044 Sep 27 18:23 zoo.txt 

The number after users is the file size in bytes

void ls_long(){
char **list = NULL;
int count = 0;
DIR *dir_ptr;
struct dirent *direntp;
char dirname[] =".";
 if(!(dir_ptr = opendir( dirname ))) {
 fprintf(stderr,"ls1: cannot open %s\n", dirname);
 return;
 }
 while((direntp = readdir(dir_ptr))){
 
 if( direntp->d_name[0] == '.' ){// check to see if name starts with a . we skip over it
 continue;
 }
 
 struct stat buf;
 char path[266];
 
 snprintf(path, 266, "%s/%s", dirname, direntp->d_name);
 if(stat(path, &buf)) {
 fprintf(stderr,"stat failed\n");
 break;
 }
 char string[600] = "";
 if(S_ISDIR(buf.st_mode)){
 strcat(string, "d");//printf("d");
 
 }else{
 strcat(string, "-");//printf("-");
 
 }if( buf.st_mode & S_IRUSR )
 strcat(string, "r");
 else
 strcat(string, "-");
 if( buf.st_mode & S_IWUSR )
 strcat(string, "w");
 else
 strcat(string, "-");
 if( buf.st_mode & S_IXUSR )
 strcat(string, "x");
 else
 strcat(string, "-");
 if( buf.st_mode & S_IRGRP )
 strcat(string, "r");
 else
 strcat(string, "-");
 if( buf.st_mode & S_IWGRP )
 strcat(string, "w");
 else
 strcat(string, "-");
 if( buf.st_mode & S_IXGRP )
 strcat(string, "x");
 else
 strcat(string, "-");
 if( buf.st_mode & S_IROTH )
 strcat(string, "r");
 else
 strcat(string, "-");
 if( buf.st_mode & S_IWOTH )
 strcat(string, "w");
 else
 strcat(string, "-");
 if( buf.st_mode & S_IXOTH )
 strcat(string, "x");
 else
 strcat(string, "-");
 
 // to get user id
 struct passwd *pwd;
 if ((pwd = getpwuid(buf.st_uid)) != NULL)
 sprintf(string + strlen(string)," %s",pwd->pw_name);//printf(" %s", pwd->pw_name);
 else
 sprintf(string + strlen(string)," %d",buf.st_uid); //printf(" %d", buf.st_uid);
 
 // to get group id
 struct group *grp;
 if ((grp = getgrgid(buf.st_gid)) != NULL)
 sprintf(string + strlen(string)," %s",grp->gr_name);// printf(" %s", grp->gr_name);
 else
 sprintf(string + strlen(string)," %d",buf.st_gid);//printf(" %d", buf.st_gid);
 
 // to get file size
 sprintf(string + strlen(string)," %lld",buf.st_size);//printf(" %lld", buf.st_size);
 
 // to get date modified
 time_t t = buf.st_mtime;
 struct tm lt;
 localtime_r(&t, &lt);
 char timbuf[80];
 strftime(timbuf, sizeof(timbuf), "%b %d %X", &lt);
 sprintf(string + strlen(string)," %s",timbuf);//printf(" %s",timbuf);
 
 //print filename
 sprintf(string + strlen(string)," %s",direntp->d_name);//printf(" %s\n", path);
 
 // putting string into list so it can be sorted
 list = realloc(list, (count+1)*sizeof(*list));
 list[count] = malloc(sizeof(string));
 strcpy(list[count],string);
 count ++;
 
 // printf("%s\n",string);
 }
for (int i = 0; i < count - 1; i++){
 for (int j = 0; j < count - i - 1; j++){
 if (strcasecmp(list[j], list[j + 1]) > 0) {
 char* temp;
 temp = list[j];
 list[j] = list[j+1];
 list[j+1] = temp;
 }
 }
}
for (int i = 0; i < (count) ;i++){
 printf("%s\n",list[i]);
}
for(int i = 0;i < count;i++){
 free(list[i]);
}
closedir(dir_ptr);
}

Answer 1

• The function is doing too much. Stick to SRP:

  • One function to iterate the directory
  • One function to collect the information about one file
  • One function to sort it
  • One function to print it
  • One function to rule them all

• fprintf(stderr,"ls1: cannot open %s\n", dirname) is better than nothing, but it loses an important information -- why exactly did opendir fail? Perhaps, a directory does not exist? or the user misses permissions? or something else? Use perror to give the important details.

• dirent->d_name[0] == '.' is dubious. There could be plenty of files of interest with a name starting with a dot. Why do you want to skip over them?

• path[266] -- What is the significance of 266? Avoid magic numbers.

• Ditto string[600].

• getpwid and getgrpid may fail and return NULL.

= string + strlen(string) is an anti-pattern. As you print more and more data, and ask for the length over and over again, you are just wasting cycles. sprintf tells you exactly how many byte it printed. Consider something along the lines

 char * end = line;
 ....
 size_t bytes = sprintf(end, ....);
 end += bytes;
 ....

• malloc and realloc may fail. Something you should take care about.
• To sort the results, you surely want something better than bubble sort.

Answer 2

 char path[266];
 
 snprintf(path, 266, "%s/%s", dirname, direntp->d_name);

Even though these two lines are close together, it's very easy to update one and not the other. Prefer a single constant (and better, use a provided one):

char path[PATH_MAX + 1]
snprintf(path, sizeof path, "%s/%s", dirname, direntp->d_name);

---

 list = realloc(list, (count+1)*sizeof(*list));

This is a dangerous pattern. If the realloc() fails, list becomes null, and we have no pointer to the memory block - it's leaked. The proper pattern is:

void *new_mem = realloc(list, (count+1) * sizeof *list);
if (!new_mem) {
 free(list);
 goto failure_return; /* or other cleanup */
}
list = new_mem;

---

Repeated strcat() wastes processor effort - it needs to start at the beginning of the string every time. It's more efficient if we just track the insert position ourselves.