List directories (with information)

Question

For part of a project, I was given this directive:

The "ls" command will require you to use directory functions to list out what the filenames are in the directory. Functions required to make a "ls" command work are included in the <dirent.h> header file ( man dirent ). You will need to open a directory and iterate through the directory until you hit the end and display the filenames in that current directory. Now, the requirement for this piece of the lab requires an example of "ls –l" where you display most of the inode information on the file.

I've implemented this below. It seems to mimic ls -la quite well, even for all the edge cases I've been testing.

ls.c:

#include <stdio.h>
#include <stdlib.h>
#include <limits.h>
#include <sys/types.h>
#include <dirent.h>
#include <sys/stat.h>
#include <string.h>
#include <pwd.h>
#include <grp.h>
#include <time.h>
#ifndef streq
#define streq(x, y) (strcmp((x), (y)) == 0)
#endif
/*
 * This function takes a mode value and a char array
 * and puts into the char array the file type and the
 * nine letters that correspond to the bits in mode.
 * NOTE: It does not code setuid, setgid, and sticky
 * codes
 */
char* mode_to_letters(int mode)
{
 const int STR_SIZE = 11; // size of mode string + terminator
 char *str = malloc(STR_SIZE);
 memcpy(str, "----------", STR_SIZE); // default
 if (S_ISDIR(mode)) str[0] = 'd'; // directory
 if (S_ISCHR(mode)) str[0] = 'c'; // char devices
 if (S_ISBLK(mode)) str[0] = 'b'; // block device
 if (mode & S_IRUSR) str[1] = 'r'; // 3 bits for user
 if (mode & S_IWUSR) str[2] = 'w';
 if (mode & S_IXUSR) str[3] = 'x';
 if ( mode & S_IRGRP ) str[4] = 'r'; // 3 bits for group
 if ( mode & S_IWGRP ) str[5] = 'w';
 if ( mode & S_IXGRP ) str[6] = 'x';
 if ( mode & S_IROTH ) str[7] = 'r'; // 3 bits for other
 if ( mode & S_IWOTH ) str[8] = 'w';
 if ( mode & S_IXOTH ) str[9] = 'x';
 return str;
}
// returns pointer to username associated with uid, uses getpw()
char* uid_to_name( uid_t uid )
{
 struct passwd *pw_ptr = {0};
 char *numstr = malloc(10);
 if ((pw_ptr = getpwuid(uid)) == NULL)
 {
 sprintf(numstr,"%d", uid);
 return numstr;
 }
 else
 return pw_ptr->pw_name ;
}
// returns pointer to group number gid. used getgrgid(3)
char* gid_to_name(gid_t gid)
{
 struct group *grp_ptr = NULL;
 char *numstr = malloc(10);
 if ((grp_ptr = getgrgid(gid)) == NULL)
 {
 sprintf(numstr, "%d", gid);
 return numstr;
 }
 else
 return grp_ptr->gr_name;
}
// display the info about 'filename'. The info is stored in struct at *info_p
void show_file_info(char *filename, struct stat *info_p)
{
 char *modestr = mode_to_letters(info_p->st_mode);
 fprintf(stdout, "%s %2d %-9s %-6s %5lld %.12s %s\n",
 modestr,
 info_p->st_nlink,
 uid_to_name(info_p->st_uid),
 gid_to_name(info_p->st_gid),
 info_p->st_size,
 4 + ctime(&info_p->st_mtime),
 filename);
 free(modestr);
}
void dostat(char *filename, char* suffix)
{
 struct stat info = {0};
 char temp[PATH_MAX] = "";
 snprintf(temp, sizeof(temp), "%s/%s", suffix, filename);
 if (stat(temp, &info) == -1)
 perror(temp);
 else
 show_file_info(filename, &info);
}
// list files in directory called dirname
void do_ls(char dirname[])
{
 DIR *dir_ptr = NULL; // the directory
 struct dirent *direntp = NULL; // each entry
 if ((dir_ptr = opendir(dirname)) == NULL)
 fprintf(stderr, "my_ls: cannot open %s\n", dirname);
 else
 {
 while ((direntp = readdir(dir_ptr)))
 {
 dostat(direntp->d_name, dirname);
 }
 closedir(dir_ptr);
 }
}
int main(int ac, char *av[])
{
 if (ac == 1)
 do_ls( "." );
 else
 while (--ac)
 {
 printf("%s:\n", *++av);
 do_ls(*av);
 fputc('\n', stdout);
 }
}

All suggestions are welcome!

Answer 1

Don't used dynamic memory when the stack will do just fine

uid_to_name() and gid_to_name() may end up mallocing the returned string. This only complicates things (caller has to free) and decreases performance. Your code could leak memory because of that:

fprintf(stdout, "%s %2d %-9s %-6s %5lld %.12s %s\n",
 modestr,
 info_p->st_nlink,
 uid_to_name(info_p->st_uid),
 gid_to_name(info_p->st_gid),
 info_p->st_size,
 4 + ctime(&info_p->st_mtime),
 filename);

You've only freed modestr.

So why not pass in the buffer instead?

char* uid_to_name(uid_t uid, char* buffer, size_t bufSize);
char* gid_to_name(gid_t gid, char* buffer, size_t bufSize);

Then those could be called with a stack buffer:

char uid_name_buf[10];
char gid_name_buf[10];
fprintf(stdout, "%s %2d %-9s %-6s %5lld %.12s %s\n",
 modestr,
 info_p->st_nlink,
 uid_to_name(info_p->st_uid, uid_name_buf, sizeof(uid_name_buf)),
 gid_to_name(info_p->st_gid, gid_name_buf, sizeof(gid_name_buf)),
 info_p->st_size,
 4 + ctime(&info_p->st_mtime),
 filename);

A little more verbose, but not really, since for the current code to be fully correct you'd have to free the returned pointer if it was allocated. The same thing can be applied to mode_to_letters().

Miscellaneous:

• Mark pointers you are only reading from as const, specially function parameters! This is very important to convey the correct intent!

• Prefer snprintf since it will check for overflow - you're not using it consistently.

• Every function but main could be static, but I'm just being pedantic.

• streq macro was never used, so could be removed.

• I see some minor inconsistencies with how () are spaced - some have extra whitespace padding and others don't - ClangFormat to the rescue?.

• int main(int ac, char *av[]) - you've misspelled argc and argv ;)