Implementation of the ls command with several options - follow-up 2

Question

I've made more improvements from previous iterations and my own, including:

1. Added the -n flag (print owner and group IDs instead of names)
2. Eliminated some bools by allowing certain flags to turn on others
3. Added a Date struct for keeping the current date in one place
4. Added an Entry struct to ease sorting and information-passing
5. Added SUID, GUID, and sticky bits to file permissions
6. Fixed a bug with displaying entries from certain inputted directories

Concerns:

• I still feel that print_entries() can be simplified, specifically to reduce the large number of conditionals. I've attempted to put the data into separate buffers and format them into a printf(), but this still resulted in quite a few conditionals.
• It still looks tacky to have bool reverse in Entry, but I especially wanted to avoid making this a global instead. Either this is the best that can be done, or this struct can be revised in some way.

I'll also add my compilation command:

gcc -std=gnu99 -Werror -Wall -Wextra -pedantic ls.c -o ls

#include <sys/stat.h>
#include <sys/types.h>
#include <dirent.h>
#include <getopt.h>
#include <grp.h>
#include <pwd.h>
#include <stdbool.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <strings.h>
#include <sysexits.h>
#include <time.h>
#include <unistd.h>

#define BUF_SIZE 1024

typedef struct
{
    bool show_hidden;
    bool list_dirents;
    bool no_list_owner;
    bool human_readable;
    bool show_inode_number;
    bool show_long_listings;
    bool show_id_numbers;
    bool no_list_group;
    bool append_indicator;
    bool quote_names;
    bool sort_reverse;
    bool sort_by_size;
    bool sort_by_mtime;
    bool no_sorting;
} Options;

typedef struct
{
    int curr_month;
    int curr_year;
} Date;

typedef struct
{
    const char* name;
    const char* dir;
    struct stat sb;
    bool reverse;
} Entry;

static void init_opts(int count, char* args[], Options* opts)
{
    static const struct option long_opts[] = {
        { "all",             optional_argument, NULL, 'a' },
        { "directory",       optional_argument, NULL, 'd' },
        { "",                optional_argument, NULL, 'g' },
        { "human-readable",  optional_argument, NULL, 'h' },
        { "inode",           optional_argument, NULL, 'i' },
        { "",                optional_argument, NULL, 'l' },
        { "numeric-uid-gid", optional_argument, NULL, 'n' },
        { "",                optional_argument, NULL, 'o' },
        { "file-type",       optional_argument, NULL, 'p' },
        { "quote-name",      optional_argument, NULL, 'Q' },
        { "reverse",         optional_argument, NULL, 'r' },
        { "",                optional_argument, NULL, 'S' },
        { "",                optional_argument, NULL, 't' },
        { "",                optional_argument, NULL, 'U' },
        { NULL,              0,                 NULL,  0  }
    };

    opts->show_hidden        = false;
    opts->list_dirents       = false;
    opts->no_list_owner      = false;
    opts->human_readable     = false;
    opts->show_inode_number  = false;
    opts->show_long_listings = false;
    opts->show_id_numbers    = false;
    opts->no_list_group      = false;
    opts->append_indicator   = false;
    opts->quote_names        = false;
    opts->sort_reverse       = false;
    opts->sort_by_size       = false;
    opts->sort_by_mtime      = false;
    opts->no_sorting         = false;

    for (int opt; (opt = getopt_long(count, args, 
        "adghilnopQrStU", long_opts, NULL)) != -1;)
    {
        switch (opt)
        {
            case 'a': opts->show_hidden        = true;
                      break;
            case 'd': opts->list_dirents       = true;
                      break;
            case 'g': opts->no_list_owner      = true;
                      opts->show_long_listings = true;
                      break;
            case 'h': opts->human_readable     = true;
                      break;
            case 'i': opts->show_inode_number  = true;
                      break;
            case 'l': opts->show_long_listings = true;
                      break;
            case 'n': opts->show_id_numbers    = true;
                      opts->show_long_listings = true;
                      break;
            case 'o': opts->no_list_group      = true;
                      opts->show_long_listings = true;
                      break;
            case 'p': opts->append_indicator   = true;
                      break;
            case 'Q': opts->quote_names        = true;
                      break;
            case 'r': opts->sort_reverse       = true;
                      break;
            case 'S': opts->sort_by_size       = true;
                      break;
            case 't': opts->sort_by_mtime      = true;
                      break;
            case 'U': opts->no_sorting         = true;
                      opts->sort_reverse       = false;
                      opts->sort_by_size       = false;
                      opts->sort_by_mtime      = false;
                      break;
            case '?': exit(EX_USAGE);
        }
    }
}

static void init_date(Date* t)
{
    time_t curr_time;
    time(&curr_time);
    const struct tm* tm = localtime(&curr_time);
    t->curr_month = tm->tm_mon;
    t->curr_year = 1900 + tm->tm_year;
}

static struct stat get_stats(const char* dir, const char* filename)
{
    char path_buf[BUF_SIZE];

    snprintf(path_buf, sizeof(path_buf), 
        "%s/%s", dir, filename);

    struct stat sb;

    if (lstat(path_buf, &sb) < 0)
    {   
        perror(path_buf);
        exit(EX_IOERR);
    }

    return sb;
}

static void print_file_indicator(Entry entry)
{
    if (S_ISDIR(entry.sb.st_mode))
    {
        putchar('/');
    }
}

static void print_permissions(mode_t mode)
{
    putchar((mode & S_IRUSR) ? 'r' : '-');
    putchar((mode & S_IWUSR) ? 'w' : '-');

    if (mode & S_IXUSR)
    {
        putchar('x');
    }
    else if (mode & S_ISUID)
    {
        putchar('s');
    }
    else if ((mode & S_ISUID)
        && !(mode & S_IXUSR))
    {
        putchar('S');
    }
    else
    {
        putchar('-');
    }

    putchar((mode & S_IRGRP) ? 'r' : '-');
    putchar((mode & S_IWGRP) ? 'w' : '-');

    if (mode & S_IXGRP)
    {
        putchar('x');
    }
    else if (mode & S_ISGID)
    {
        putchar('s');
    }
    else if ((mode & S_ISGID)
        && !(mode & S_IXGRP))
    {
        putchar('S');
    }
    else
    {
        putchar('-');
    }

    putchar((mode & S_IROTH) ? 'r' : '-');
    putchar((mode & S_IWOTH) ? 'w' : '-');

    if (mode & S_IXOTH)
    {
        putchar('x');
    }
    else if (mode & S_ISVTX)
    {
        putchar('t');
    }
    else if ((mode & S_ISVTX)
        && !(mode & S_IXOTH))
    {
        putchar('T');
    }
    else
    {
        putchar('-');
    }
}

static void print_filetype(mode_t mode)
{
    if (S_ISREG(mode))  putchar('-');
    if (S_ISDIR(mode))  putchar('d');
    if (S_ISLNK(mode))  putchar('l');
    if (S_ISCHR(mode))  putchar('c');
    if (S_ISBLK(mode))  putchar('b');
    if (S_ISSOCK(mode)) putchar('s');
    if (S_ISFIFO(mode)) putchar('f');
}

static void print_readable_size(off_t size)
{
    const char* units[] = {
        "", "K", "M", "G", "T", "P", "E", "Z", "Y"
    };

    int i;

    for (i = 0; size > 1024; ++i, size /= 1024);

    char size_buf[BUF_SIZE];

    snprintf(size_buf, sizeof(size_buf), 
        "%*jd%s ", i, (intmax_t)size, units[i]);

    printf(" %8s", size_buf);
}

static void print_time(Date date, time_t mtime)
{
    const struct tm* t = localtime(&mtime);

    const int mod_mon = t->tm_mon;
    const int mod_yr = 1900 + t->tm_year;

    // determine format based on modification date
    // (past six months)
    const char* format = (mod_yr == date.curr_year)
                      && (mod_mon >= (date.curr_month - 6))
                             ? "%b %e %H:%M"
                             : "%b %e  %Y";

    char time_buf[BUF_SIZE];
    strftime(time_buf, BUF_SIZE, format, t);
    printf("%s ", time_buf);
}

static void print_link(const Entry entry, Options opts)
{
    char path_buf[BUF_SIZE];

    snprintf(path_buf, sizeof(path_buf), 
        "%s/%s", entry.dir, entry.name);

    char link_buf[BUF_SIZE];

    const ssize_t count = 
        readlink(path_buf, link_buf, sizeof(link_buf));

    link_buf[count] = '\0';

    const char* link_format = 
            (!opts.quote_names) 
            ? "%s -> %s"
            : "\"%s\" -> \"%s\"";

    printf(link_format, entry.name, link_buf);
}

static void print_name(const Entry entry, Options opts)
{
    const char* format = 
            (!opts.quote_names) 
            ? "%s" 
            : "\"%s\"";

    printf(format, entry.name);
}

static void print_entry(const Entry entry, Options opts, Date date)
{
    if (opts.show_long_listings)
    {
        // print inode number
        if (opts.show_inode_number)
        {
            printf("%ju ", (uintmax_t)entry.sb.st_ino);
        }

        print_filetype(entry.sb.st_mode);
        print_permissions(entry.sb.st_mode);
        printf(" %jd ", (intmax_t)entry.sb.st_nlink);

        // print owner
        if (!opts.no_list_owner)
        {
            const char* owner_name = getpwuid(entry.sb.st_uid)->pw_name;
            const uid_t owner_id = getpwuid(entry.sb.st_uid)->pw_uid;

            if (opts.show_id_numbers)
            {
                printf("%10jd ", (intmax_t)owner_id);
            }
            else
            {
                printf("%10s ", owner_name);
            }
        }

        // print group
        if (!opts.no_list_group)
        {
            const char* group_name = getgrgid(entry.sb.st_gid)->gr_name;
            const gid_t group_id = getgrgid(entry.sb.st_gid)->gr_gid;

            if (opts.show_id_numbers)
            {
                printf("%10jd ", (intmax_t)group_id);
            }
            else
            {
                printf("%10s", group_name);
            }
        }

        // print file size
        if (opts.human_readable)
        {
            print_readable_size(entry.sb.st_size);
        }
        else
        {
            printf("%10jd ", (intmax_t)entry.sb.st_size);
        }

        print_time(date, entry.sb.st_mtime);

        // print name or link
        if (S_ISLNK(entry.sb.st_mode))
        {
            print_link(entry, opts);
        }
        else
        {
            print_name(entry, opts);
        }
    }
    else
    {
        print_name(entry, opts);
    }

    if (opts.append_indicator)
    {
        print_file_indicator(entry);
    }

    putchar('\n');
}

static int cmp_size(const void* p1, const void* p2)
{
    const Entry entry1 = *(const Entry*)p1;
    const Entry entry2 = *(const Entry*)p2;

    const off_t size1 = entry1.sb.st_size;
    const off_t size2 = entry2.sb.st_size;

    const int return_value = 
        (size2 > size1) - (size2 < size1);

    return (!entry1.reverse)
        ? return_value 
        : -return_value;
}

static int cmp_mtime(const void* p1, const void* p2)
{
    const Entry entry1 = *(const Entry*)p1;
    const Entry entry2 = *(const Entry*)p2;

    const time_t mtime1 = entry1.sb.st_mtime;
    const time_t mtime2 = entry2.sb.st_mtime;

    const int return_value 
        = (mtime2 > mtime1) - (mtime2 < mtime1);

    return (!entry1.reverse) 
        ? return_value 
        : -return_value;
}

static int cmp_lex(const void* p1, const void* p2)
{
    const Entry entry1 = *(const Entry*)p1;
    const Entry entry2 = *(const Entry*)p2;

    const char* name1 = entry1.name;
    const char* name2 = entry2.name;

    const int return_value 
        = strcasecmp(name1, name2);

    return (!entry1.reverse) 
        ? return_value 
        : -return_value;
}

static void sort_entries(Entry* entries, size_t entries_count, Options opts)
{
    int(*comparer)(const void*, const void*);

    if (opts.sort_by_size)
    {
        comparer = cmp_size;
    }
    else if (opts.sort_by_mtime)
    {
        comparer = cmp_mtime;
    }
    else
    {
        comparer = cmp_lex;
    }

    qsort(entries, entries_count, sizeof(*entries), comparer);
}

static void structure_dir_entries(const char* dir, DIR* dfd, 
    Entry* entries, size_t* entries_count, Options opts)
{
    *entries_count = 0;

    for (struct dirent* dp; (dp = readdir(dfd));)
    {
        const bool omit_hidden = !opts.show_hidden
                              && dp->d_name[0] == '.';

        if (!omit_hidden)
        {
            if (*entries_count >= BUF_SIZE * sizeof(*entries))
            {
                entries = realloc(
                    entries, BUF_SIZE * sizeof(*entries));

                if (!entries)
                {
                    perror("realloc");
                    abort();
                }
            }

            const Entry entry = { 
                dp->d_name,
                dir,
                get_stats(dir, dp->d_name),
                opts.sort_reverse
            };

            entries[*entries_count] = entry;
            ++(*entries_count);
        }
    }

    if (!opts.no_sorting)
    {
        sort_entries(entries, *entries_count, opts);
    }
}

static void display_dir_entries(const char* dir, Options opts, Date date)
{
    Entry* entries = malloc(BUF_SIZE * sizeof(Entry));

    if (!entries)
    {
        perror("malloc");
        abort();
    }

    DIR* dfd = opendir(dir);

    size_t entries_count;

    structure_dir_entries(dir, dfd, entries, &entries_count, opts);

    for (size_t i = 0; i < entries_count; ++i)
    {
        print_entry(entries[i], opts, date);
    }

    closedir(dfd);
    free(entries);
}

static void display_dir_args(int argc, char* argv[], Options opts, Date date)
{
    const bool no_dirs_given = (argc - optind) == 0;

    if (no_dirs_given)
    {
        const Entry dir_entry = {
            ".",
            ".",
            get_stats(".", "."),
            opts.sort_reverse
        };

        print_entry(dir_entry, opts, date);
    }
    else
    {
        for (int i = optind; i < argc; ++i)
        {
            const Entry dir_entry = {
                argv[i],
                argv[i],
                get_stats(".", argv[i]),
                opts.sort_reverse
            };

            print_entry(dir_entry, opts, date);
        }
    }
}

static void scan_dir_entries(int argc, char* argv[], Options opts, Date date)
{
    const bool multiple_dir_args = (argc - optind) >= 2;

    for (int i = optind; i < argc; ++i)
    {
        const Entry dir_entry = { 
            argv[i],
            argv[i],
            get_stats(".", argv[i]),
            opts.sort_reverse
        };

        if (!S_ISDIR(dir_entry.sb.st_mode))
        {
            print_entry(dir_entry, opts, date);
            continue;
        }

        if (multiple_dir_args)
        {
            printf("\n%s:\n", argv[i]);
        }

        display_dir_entries(argv[i], opts, date);
    }
}

int main(int argc, char* argv[])
{
    Options opts;
    init_opts(argc, argv, &opts);

    Date date;
    init_date(&date);

    if (opts.list_dirents)
    {
        display_dir_args(argc, argv, opts, date);
    }
    else
    {
        if (optind == argc)
        {
            display_dir_entries(".", opts, date);
        }
        else
        {
            scan_dir_entries(argc, argv, opts, date);
        }
    }
}

Answer 1

Your code looks nice and readable. A few things I would pick on:

1. This is not a nice use of a for loop:

   for (int opt; (opt = getopt_long(count, args, 
   "adghilnopQrStU", long_opts, NULL)) != -1;)
   

   It's only using one of the three components which make a for loop. Semantically a while loop expresses what you are doing a lot better:

   int opt;
   while ((opt = getopt_long(count, args, "adghilnopQrStU", long_opts, NULL)) != -1)
   

   I just saw that you had it this way before and changed it based on review. Shows that this is probably a preference thing but I'd prefer clear semantics over scoping limitations of variables any time since it improves readability.

   Same goes for this:

   for (struct dirent* dp; (dp = readdir(dfd));)
   

2. You can safe some space in your Options struct. Right now this is occupying one byte per flag - so at least a total of 14 bytes (depending on platform and compiler and compiler options additional padding bytes might be added between each flag). But C has bit fields which allow you to express this in a much more compact fashion:

   typedef struct
   {
       bool show_hidden : 1;
       bool list_dirents : 1;
       bool no_list_owner : 1;
       bool human_readable : 1;
       bool show_inode_number : 1;
       bool show_long_listings : 1;
       bool show_id_numbers : 1;
       bool no_list_group : 1;
       bool append_indicator : 1;
       bool quote_names : 1;
       bool sort_reverse : 1;
       bool sort_by_size : 1;
       bool sort_by_mtime : 1;
       bool no_sorting : 1;
   } Options;
   

   Now each flag will be one bit and the whole struct will probably only be about two bytes (might get padded to 4 or 8 bytes depending on platform and compiler and compiler options).

3. If you want to clear the Options struct you can do this via memset generically - then you won't forget to add another initializer if you add another flag:

   memset(opts, 0, sizeof(*opts));
   

   (in case you are worried about potentially de-referencing a NULL pointer - it's not a problem)

4. I'm pretty sure this is partially broken:

   if (mode & S_IXUSR)
   {
       putchar('x');
   }
   else if (mode & S_ISUID)
   {
       putchar('s');
   }
   else if ((mode & S_ISUID)
       && !(mode & S_IXUSR))
   {
       putchar('S');
   }
   else
   {
       putchar('-');
   }
   

   If the first if fails we know that !(mode & S_IXUSR) is true. It will then go to the second else if condition. If that fails we know that (mode & S_ISUID) is false. Hence the third condition !(mode & S_IXUSR) && (mode & S_ISUID) cannot be true as such it will print S. This bug has been copy-pasted for the other two blocks further down as well.

   These blocks look very similar in structure and ideally could be refactored into some single common logic but I haven't found a nice solution for this yet. Ideally you could map something like

   S_IXUSR           > 'x'
   S_ISUID           > 's'
   S_IXUSR | S_ISUID > 'S'
   everything else   > '-'
   

   And then your code could become something like (pseudo code):

   puts(permission_bits[mode & (S_IXUSR | S_ISUID)]);
   

   Unfortunately C has not standard mapping structure.

   OTOH a switch statement instead of the if-else cascade might do and would possibly have avoided the bug in the first place.

5. While highly unlikely in print_readable_size you should check that you're not accessing units out of bounds.

6. structure_dir_entries should return the count instead of passing an external variable by pointer:

   static size_t structure_dir_entries(const char* dir, DIR* dfd, 
                                       Entry* entries, Options opts)`
   

7. Another bug. This doesn't do what you probably think it does:

           entries = realloc(
               entries, BUF_SIZE * sizeof(*entries));
   

   sizeof(*entries) is the same as sizeof(Entry) and not the size of the entire array. So if entries already has BUF_SIZE members then the realloc is basically a no-op and your program will crash further down.

8. Regarding the reverse flag. I'd either add a pointer to the options struct to the entry so the compare functions have access to all the options or make the flag global.

Answer 2

1. Error: Lost pointers. structure_dir_entries() may reallocate entries and then that function never free the newly allocated memory. display_dir_entries() will then free the original value of entries causing a double-free. Likely code should be re-worked to pass the address of entries to structure_dir_entries().

   static void structure_dir_entries(const char* dir, DIR* dfd, 
       Entry* entries, size_t* entries_count, Options opts) {
     ... 
     entries = realloc(entries, BUF_SIZE * sizeof(*entries));
     ...
     sort_entries(entries, *entries_count, opts);
   }
   
   static void display_dir_entries(const char* dir, Options opts, Date date) {
       Entry* entries = malloc(BUF_SIZE * sizeof(Entry));
       ...
       structure_dir_entries(dir, dfd, entries, &entries_count, opts);
       ...
       free(entries);
   }
   

2. Minor: Inconsistent tm_year,tm_mon to curr_year,curr_month. A value of 0 in tm_year is 1900. A value of 0 in tm_mon is January. I would expect adjusting both (making January 1) or neither.

   // both
   t->curr_month = 1 + tm->tm_mon;
   t->curr_year = 1900 + tm->tm_year;
   
   // neither
   t->curr_month = tm->tm_mon;
   t->curr_year = tm->tm_year;
   

   Further as there is no strong need for the Date type, consider using struct tm date; and passing &date about.

[Edit]

3. Deeper review of structure_dir_entries() indicates it has more issues. It isn't allocating memory correctly, and linearly re-sizing. Suggest simplification: Do not allocate memory in calling function. Pass address of entries. Pass address of opts. Use positive logic (show = opts.show_hidden || dp->d_name[0] != '.';

   Untested code follows

   Entry* entries = NULL;
   size_t entries_count = 0;
   DIR* dfd = opendir(dir);
   if (dfd == NULL) Report_Error();
   
   structure_dir_entries(dir, dfd, &entries, &entries_count, &opts);
   ...
   free(entries);
   
   static void structure_dir_entries(const char* dir, DIR* dfd, Entry** entries, 
       size_t* entries_count, const Options *opts) {
     *entries = NULL;
     *entries_count = 0;
     size_t entries_n = 0;
   
     for (struct dirent* dp; (dp = readdir(dfd));) {
       bool show = opts->show_hidden || dp->d_name[0] != '.';
       if (show) {
         if (*entries_count == entries_n) {
           entries_n = entries_n * 2 + 1;  // double size
           (*entries) = realloc((*entries), sizeof(*entries) * entries_n);
           if ((*entries) == NULL) {
             perror("realloc"); abort();
           }
         }
         Entry entry = { dp->d_name, dir, get_stats(dir, dp->d_name), opts->sort_reverse };
         (*entries)[*entries_count] = entry;
         ++(*entries_count);
       }
     }
   
     if (!opts->no_sorting) {
       sort_entries((*entries), *entries_count, opts);
     }
   }
   

4. Excessive use of passed structures when the structure could be efficiently passed by address.