6
\$\begingroup\$

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);
        }
    }
}
\$\endgroup\$
3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ #5: Accessing out of bounds would require the size to exceed pow(1024, 9) or about 1.2e27 and of course the size being larger than 64-bit. Do not expect such files sizes for 90 years. Yet easy enough to encode your suggested limit. \$\endgroup\$ – chux - Reinstate Monica Jan 1 '16 at 6:55
  • \$\begingroup\$ @chux: sure, but is still a good habit to check against potential out of bounds access. The input comes from external and should be treated as untrustworthy \$\endgroup\$ – ChrisWue Jan 1 '16 at 8:09
  • \$\begingroup\$ It's kind of funny to see a preference the complete opposite to my own in such a stark way. In hindsight, I probably should have made it clearer in my answer that for vs while in this scenario comes down almost purely to preference. I actually thought for being preferred over while when loop-scoped variables were introduced was pretty much universally standard in C and C++, but after Googling a bit, it appears I've imagine that. Goes to show what happens when I spend too much time isolated in my own specific style :). \$\endgroup\$ – Corbin Jan 2 '16 at 1:42
  • \$\begingroup\$ @corbin: yeah, I always try to explain the algorithm in English and then pick the language constructs which fit the natural description the closest. Technical things like variable scope tend to lose in that regards since I'd deem it slightly less important. But as usual not much is black and white when it comes to programming. \$\endgroup\$ – ChrisWue Jan 2 '16 at 5:35
  • 1
    \$\begingroup\$ Minor disagreement with #2. Various reasons including the space saving in data may be offset by less space efficient code. \$\endgroup\$ – chux - Reinstate Monica Jan 2 '16 at 20:45
1
\$\begingroup\$
  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]

  1. 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);
      }
    }
    
  2. Excessive use of passed structures when the structure could be efficiently passed by address.

\$\endgroup\$
  • \$\begingroup\$ When I have tried to get realloc() to work, I got seg faults when working with lower values. Maybe I'm still not doing it right. \$\endgroup\$ – Jamal Jan 2 '16 at 18:01
  • \$\begingroup\$ @Jamal Agree. See edit/ \$\endgroup\$ – chux - Reinstate Monica Jan 2 '16 at 20:13
  • \$\begingroup\$ I have been trying to use pointers to structs instead, but it ended up causing more problems for some reason. Maybe I'll try to get it working again later. \$\endgroup\$ – Jamal Jan 2 '16 at 20:16
  • \$\begingroup\$ Also, did you mean to first call malloc() in structure_dir_entries()? It's only calling realloc(). \$\endgroup\$ – Jamal Jan 2 '16 at 20:18
  • \$\begingroup\$ @Jamal No. Only entries = NULL; Call to malloc() not needed. \$\endgroup\$ – chux - Reinstate Monica Jan 2 '16 at 20:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.