11
\$\begingroup\$

I've made vast improvements to my previous ls implementation, including:

  • Added new flags:
    • -g (omit owner listing)
    • -o (omit group listing)
    • -r (reverse with sorting)
  • Removed a flag (which wasn't working entirely):
    • -R
  • Added long options for applicable flags (as defined)
  • Defined a single buffer size
  • Used more proper stat field types
  • Replaced sprintf() with snprintf()
  • Used function pointers to remove duplication with sorting
  • Greatly improved modularity to break up larger functions
  • Made opts global to work with comparer functions
  • Actually sort in reverse order as opposed to just displaying in reverse. This also greatly simplified the display code.

I was also considering POSIX regex for the -a flag (include hidden files) and a prospective -A flag (include hidden files except for the implied . and .. directories). Since I couldn't get it to work with the latter, I decided to leave it out. Keeping it just for the former might've then been a little overkill.

I realize that there's still no sorting done with the -d option, and I'm working on that. If the logic around it can be simplified, then it should make it easier to change.

#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 <string.h>
#include <strings.h>
#include <sysexits.h>
#include <time.h>
#include <unistd.h>

#define BUF_SIZE 1024

const char* current_dir = ".";

struct Options
{
    bool using_a_opt;
    bool using_d_opt;
    bool using_g_opt;
    bool using_h_opt;
    bool using_i_opt;
    bool using_l_opt;
    bool using_o_opt;
    bool using_p_opt;
    bool using_Q_opt;
    bool using_r_opt;
    bool using_S_opt;
    bool using_t_opt;
    bool using_U_opt;
} opts;

void init_opts()
{
    opts.using_a_opt = false;
    opts.using_d_opt = false;
    opts.using_g_opt = false;
    opts.using_h_opt = false;
    opts.using_i_opt = false;
    opts.using_l_opt = false;
    opts.using_o_opt = false;
    opts.using_p_opt = false;
    opts.using_Q_opt = false;
    opts.using_r_opt = false;
    opts.using_S_opt = false;
    opts.using_t_opt = false;
    opts.using_U_opt = false;
}

void get_opts(int count, char* args[])
{
    init_opts();

    static 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' },
        { "",                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  }
    };

    int opt;

    while ((opt = getopt_long(count, args, 
        "adghilopQrStU", long_opts, NULL)) != -1)
    {
        switch (opt)
        {
            case 'a': opts.using_a_opt = true; break;
            case 'd': opts.using_d_opt = true; break;
            case 'g': opts.using_g_opt = true; break;
            case 'h': opts.using_h_opt = true; break;
            case 'i': opts.using_i_opt = true; break;
            case 'l': opts.using_l_opt = true; break;
            case 'o': opts.using_o_opt = true; break;
            case 'p': opts.using_p_opt = true; break;
            case 'Q': opts.using_Q_opt = true; break;
            case 'r': opts.using_r_opt = true; break;
            case 'S': opts.using_S_opt = true; break;
            case 't': opts.using_t_opt = true; break;
            case 'U': opts.using_U_opt = true; break;
            case '?': exit(EX_USAGE);
        }
    }
}

void print_permissions(mode_t mode)
{
    putchar((mode & S_IRUSR) ? 'r' : '-');
    putchar((mode & S_IWUSR) ? 'w' : '-');
    putchar((mode & S_IXUSR) ? 'x' : '-');
    putchar((mode & S_IRGRP) ? 'r' : '-');
    putchar((mode & S_IWGRP) ? 'w' : '-');
    putchar((mode & S_IXGRP) ? 'x' : '-');
    putchar((mode & S_IROTH) ? 'r' : '-');
    putchar((mode & S_IWOTH) ? 'w' : '-');
    putchar((mode & S_IXOTH) ? 'x' : '-');
}

void print_filetype(mode_t mode)
{
    switch (mode & S_IFMT)
    {
        case S_IFREG:  putchar('-'); break;
        case S_IFDIR:  putchar('d'); break;
        case S_IFLNK:  putchar('l'); break;
        case S_IFCHR:  putchar('c'); break;
        case S_IFBLK:  putchar('b'); break;
        case S_IFSOCK: putchar('s'); break;
        case S_IFIFO:  putchar('f'); break;
    }
}

void print_readable_filesize(off_t size)
{
    const char* units[] = { "", "K", "M", "G", "T" };
    int i;

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

    char filesize_buf[BUF_SIZE];

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

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

void print_time(time_t mod_time)
{
    // get current time with year
    time_t curr_time;
    time(&curr_time);
    struct tm* t = localtime(&curr_time);
    const int curr_mon = t->tm_mon;
    const int curr_yr = 1970 + t->tm_year;

    // get modified time and year
    t = localtime(&mod_time);
    const int mod_mon = t->tm_mon;
    const int mod_yr = 1970 + t->tm_year;

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

    char time_buf[BUF_SIZE];

    const size_t bytes_written = 
        strftime(time_buf, sizeof(time_buf), format, t);

    if (bytes_written <= 0)
    {
        exit(EX_IOERR);
    }

    printf("%s", time_buf);
}

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

    const int bytes_written = 
        snprintf(path_buf, sizeof(path_buf), 
        "%s/%s", current_dir, filename);

    if (bytes_written <= 0)
    {
        exit(EX_IOERR);
    }

    struct stat sb;

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

    return sb;
}

int is_dir(const char* filename)
{
    struct stat sb = get_stats(filename);

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

    return sb.st_mode & S_IFDIR;
}

bool is_in_dir(const char* dir, const char* filename)
{
    DIR* dfd = opendir(dir);

    if (!dfd)
    {
        perror(dir);
        exit(EX_IOERR);
    }

    struct dirent* dp = readdir(dfd);

    while (dp)
    {
        if (strcmp(filename, dp->d_name) == 0)
        {
            closedir(dfd);
            return true;
        }      

        dp = readdir(dfd);
    }

    fprintf(stderr, "file \'%s\' not found\n", filename);

    closedir(dfd);

    return false;
}

void print_link(const char* dir, const char* filename)
{
    char path_buf[BUF_SIZE];

    const int bytes_written = 
        snprintf(path_buf, sizeof(path_buf), 
        "%s/%s", dir, filename);

    if (bytes_written <= 0)
    {
        exit(EX_IOERR);
    }

    char link_buf[BUF_SIZE];
    const ssize_t count = 
        readlink(path_buf, link_buf, sizeof(link_buf));

    if (count == -1)
    {
        perror("readlink");
        exit(EX_IOERR);
    }

    link_buf[count] = '\0';

    if (opts.using_Q_opt)
    {
        printf(" \"%s\" -> \"%s\"\n", filename, link_buf);
    }
    else
    {
        printf(" %s -> %s \n", filename, link_buf);
    }
}

void print_name(const char* filename)
{
    if (opts.using_Q_opt)
    {
        printf(" \"%s\"", filename);
    }
    else
    {
        printf(" %s", filename);
    }

    if (opts.using_p_opt && is_dir(filename))
    {
        putchar('/');
    }

    putchar('\n');
}

void display_stats(const char* dir, const char* filename)
{
    if (!is_in_dir(dir, filename))
    {
        return;
    }

    if (!opts.using_g_opt 
        && !opts.using_o_opt 
        && !opts.using_l_opt)
    {
        if (opts.using_Q_opt)
        {
            printf("\"%s\"\n", filename);
        }
        else
        {
            printf("%s\n", filename);
        }

        return;
    }

    struct stat sb = get_stats(filename);

    // print inode number
    if (opts.using_i_opt)
    {
        printf("%ju ", (uintmax_t)sb.st_ino);
    }

    print_filetype(sb.st_mode);

    print_permissions(sb.st_mode);

    printf(" %jd ", (intmax_t)sb.st_nlink);

    // print owner
    if (!opts.using_g_opt)
    {
        printf("%10s ", getpwuid(sb.st_uid)->pw_name);
    }

    // print group
    if (!opts.using_o_opt)
    {
        printf("%10s", getgrgid(sb.st_gid)->gr_name);
    }

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

    print_time(sb.st_mtime);

    // print name/link
    if ((sb.st_mode & S_IFMT) == S_IFLNK)
    {
        print_link(dir, filename);
    }
    else
    {
        print_name(filename);
    }
}

int compare_size(const void* p1, const void* p2)
{
    const char* str1 = *(const char**)p1;
    const char* str2 = *(const char**)p2;

    const off_t size1 = get_stats(str1).st_size;
    const off_t size2 = get_stats(str2).st_size;

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

    return (!opts.using_r_opt) ? return_value : -return_value;
}

int compare_time(const void* p1, const void* p2)
{
    const char* str1 = *(const char**)p1;
    const char* str2 = *(const char**)p2;

    const time_t time1 = get_stats(str1).st_mtime;
    const time_t time2 = get_stats(str2).st_mtime;

    const int return_value = (time2 > time1) - (time2 < time1);

    return (!opts.using_r_opt) ? return_value : -return_value;
}

int compare_lex(const void* p1, const void* p2)
{
    const char* str1 = *(const char**)p1;
    const char* str2 = *(const char**)p2;

    return (!opts.using_r_opt) 
        ? strcasecmp(str1, str2)
        : -strcasecmp(str1, str2);
}

void sort_dir(char** entries, size_t entries_count)
{
    int(*comparer)(const void*, const void*);

    if (opts.using_S_opt)
    {
        comparer = compare_size;
    }
    else if (opts.using_t_opt)
    {
        comparer = compare_time;
    }
    else
    {
        comparer = compare_lex;
    }

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

void structure_dir_entries(DIR* dfd, char** entries, size_t* entries_count)
{
    struct dirent* dp = readdir(dfd);
    *entries_count = 0;

    while (dp)
    {
        const bool omit_hidden = !opts.using_a_opt
                              && dp->d_name[0] == '.';

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

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

            entries[*entries_count] = dp->d_name;
            ++(*entries_count);
        }

        dp = readdir(dfd);
    }

    if (!opts.using_U_opt)
    {
        sort_dir(entries, *entries_count);
    }
}

void display_dir_entries(const char* dir)
{
    char** entries = malloc(BUF_SIZE * sizeof(char*));

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

    DIR* dfd = opendir(dir);

    size_t entries_count;

    structure_dir_entries(dfd, entries, &entries_count);

    for (size_t i = 0; i < entries_count; ++i)
    {
        display_stats(dir, entries[i]);
    }

    closedir(dfd);

    free(entries);
}

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

    if (no_dirs_given)
    {
        display_stats(".", ".");
    }

    for (int i = optind; i < argc; ++i)
    {
        display_stats(".", argv[i]);
    }
}

void scan_dir_entries(int argc, char* argv[])
{
    const bool multiple_dirs = (argc - optind) >= 2;

    for (int i = optind; i < argc; ++i)
    {
        if (!is_dir(argv[i]))
        {
            display_stats(".", argv[i]);
            continue;
        }

        if (!is_in_dir(".", argv[i]))
        {
            continue;
        }

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

        display_dir_entries(argv[i]);
    }
}

int main(int argc, char* argv[])
{
    get_opts(argc, argv);

    if (opts.using_d_opt)
    {
        display_dir_args(argc, argv);
        return EX_OK;
    }
    else
    {
        if (optind == argc)
        {
            display_dir_entries(".");
            return EX_OK;
        }

        scan_dir_entries(argc, argv);
    }
}
\$\endgroup\$
5
\$\begingroup\$

This looks pretty good over all, but I have a few things that jumped out at me.

Bugs

structure_dir_entries has a leak and likely a crash in it if BUF_SIZE actually overflows. You're basically doing this:

void f(char* buf) {
    buf = malloc(1);
}

void use_f(void) {
    char* b = NULL;
    f(b);
    free(b);
}

It looks like you need to check for absolute paths:

    corbin@mint ~/Desktop $ ./ls ..
    Desktop
    dev
    Documents
    Downloads
    corbin@mint ~/Desktop $ ./ls /home/corbin
    .//home/corbin: No such file or directory

Semantics / Technical Improvements

Your options should have names. using_a_opt is meaningless, when it could instead be named what the option does. Not only will this make the code easier to understand, it will tuck away the logic of flag <-> option mapping in the option parsing code.

Imagine you didn't write the code, and you came across this:

if (!opts.using_g_opt 
    && !opts.using_o_opt 
    && !opts.using_l_opt)
{

You'd almost certainly be confused and likely curse the previous programmer a bit.


opts shouldn't be a global. I know it's slightly convenient to have it be a global, but it creates a strange dependency where you have to remember to initialize it before using it. It seems inappropriate for the code consuming something to initialize it. It'd make more sense for it to be non-global, then the initialization and creation would be closer. Also, if you were to add unit testing to this, the global is likely going to complicate that.


A lot of your functions handle program flow and shouldn't. For example, your functions that handle getting file metadata shouldn't kill program execution. This makes unit testing annoying, and it makes the functions less usable.


It almost certainly doesn't matter since this is a single file executable and not a library, but any function you don't want to purposely expose should be marked static (i.e. probably all of your functions except for main). It's much better to over-static than leave something non-static and find out someone is using it when you try to actually encapsulate it.


There's some mixture of logic and output that shouldn't be there. For example, is_in_dir returns a boolean to signify if the file is in the directory or not. I wouldn't expect this function to output anything. The caller should output something if the caller wants output. Functions shouldn't handle both logic and output. They should handle one or the other (except of course for output functions having logic about output :p).


It's weird that print_link can fail. It might make more sense if you had some layer of code that would make and populate a struct, then print_link could operate on that in a way that it cannot fail. If I inherited some code and a print_* function had a chance of exiting the program, I would be pretty surprised. In a way, this is just a special case of functions should do either printing or logic, not both (and that's just a special case of "a function should only do one thing").

This also applies to a few other places too. For example, display_stats could take all of the data it needs instead of gathering it. The compare_* functions also seem a bit odd.

As a bonus of operating on the actual data and not what are essentially ways to obtain the data, you only have to obtain it once. In other words, you can get rid of a lot of the get_stats calls, which are likely relatively expensive (though there is of course a relatively small memory premium you pay for this).

This also has a nice effect of getting rid of some race conditions. For example, currently, if you call get_stats twice, the file might not exist the second time. If you only collect data up front once, you might print data that is out of date, but it will never be inconsistent.

Usability

There's a lot of exit scenarios that don't provide a very useful error to the user. I'd hate to have to go look up what the exit codes mean every time a program died on me. It'd be a bit friendlier to provide an error message along with exit code.


A --help option would be nice.

Style

int opt;

while ((opt = getopt_long(count, args, 
    "adghilopQrStU", long_opts, NULL)) != -1)
{

Whenever you have a loop like this, you can use a for loop to keep the loop variable scoped more tightly:

for (int opt; (opt = getopt_long(...)) != -1;)

Another example where you could use a for loop like this is in is_in_dir.


It looks like you're using an 80 character line limit. This obviously comes down to personal preference (even more than other style suggestions), but I think you should consider a larger limit. There's a lot of expressions squished down into 2-4 lines, that could be 1 or 2 lines instead. I know some people like to have 2 or even 3 things up and tiled horizontally while they work, but I think even with that, the limit could be 120.

There's now a 90% chance someone will yell at me.

\$\endgroup\$
  • \$\begingroup\$ How else should I handle the compare functions? Removing them would ease some troubles, but would make the program less useful. Also, the suggested for loop for getopt_long() isn't functioning in the same way. \$\endgroup\$ – Jamal Dec 25 '15 at 22:31
  • \$\begingroup\$ @Jamal Whoops, looks like I botched the parentheses on the getopt_long stuff. I'll fix it. As for the sorting, I just meant that it strange that the sorting works on file paths and not what's actually being sorted. Think about how you'd sort a collection of files in Java or C++ (with something like Boost.File). You'd most likely be operating on some kind of data type, not just a string. As it currently is, you'd have trouble unit testing that (at least without a lot of real files), and it's rather odd for a consumer of the sort functions that they can indirectly exit() the application. \$\endgroup\$ – Corbin Dec 25 '15 at 22:48
  • \$\begingroup\$ I'm only doing this because there may be problems trying to, for instance, populate a char[] with time_ts. Do I need to do some kind of casting here or something else, instead of keeping three different types of arrays by default? Would an array of void** be best? \$\endgroup\$ – Jamal Dec 25 '15 at 23:05
  • \$\begingroup\$ @Jamal You'd likely want some kind of File struct that you'd populate for each file/directory with name/size/type/etc. Then, in the qsort predicate, you'd cast the void* to File* and do your comparison logic. \$\endgroup\$ – Corbin Dec 25 '15 at 23:28
  • \$\begingroup\$ I can't believe I didn't think of that. Not only did I remove get_stats() from the preds, but I even removed the global dir! Such a huge relief. \$\endgroup\$ – Jamal Dec 26 '15 at 2:15
2
\$\begingroup\$
  1. Bug: minor.

    Code is allocating based on the sizeof(entries) when it should have used sizeof(*entries).

    Since both are pointers, they are likely the same size, so likely code does not fail.

    void structure_dir_entries(DIR* dfd, char** entries, size_t* entries_count) {
        ...
        // entries = realloc(entries, sizeof(entries) * BUF_SIZE)
        entries = realloc(entries, sizeof(*entries) * BUF_SIZE)
        // or simply
        entries = realloc(entries, sizeof *entries * BUF_SIZE)
    
  2. Suggest again to extend suffix array to cope with large sizes. Ref

    // const char* units[] = { "", "K", "M", "G", "T" };
    const char* units[] = { "", "K", "M", "G", "T", "P", "E", "Z", "Y" };
    
  3. Recommend using the size of the object and not the size of the type. This style is easier to code, maintain and less likely wrong.

    // qsort(entries, entries_count, sizeof(char*), comparer);
    qsort(entries, entries_count, sizeof *entries, comparer);
    

More

  1. Here is a subtle issue. Recommend to only call time(&curr_time); once in the life of the program. Else the calculations of current year/month are a moving target with the multiple calls of display_stats(). Could get a weird output on an end/start of month or New Year's. Something like a Y2K bug.

  2. Using an 80-character limit is fine, especial for SE whose code viewing window appears to be 89/93.

    12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789
             1         2         3         4         5         6         7         8      
    

.

123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123
         1         2         3         4         5         6         7         8         9
\$\endgroup\$
  • \$\begingroup\$ @Jamal Rolled back edit as that edit removed the demonstration of a 93 character code line fitting in an answer. \$\endgroup\$ – chux - Reinstate Monica Dec 30 '15 at 14:01
  • \$\begingroup\$ Then put a <!--- ---> between them so that they don't have to be separated with a period like that. \$\endgroup\$ – Jamal Dec 30 '15 at 18:01
  • \$\begingroup\$ Detail on need for "P", "E", "Z", "Y": In 2015 large hard disks sizes 10T bytes occur. Given Moore's law, expect the need for P about the year 2029 or so and E maybe 20 years after that. \$\endgroup\$ – chux - Reinstate Monica Dec 30 '15 at 18:02
  • \$\begingroup\$ @Jamal Will employ that idea next significant edit. \$\endgroup\$ – chux - Reinstate Monica Dec 30 '15 at 18:04
  • \$\begingroup\$ For the first point, was my mistake that I was getting the size of the double pointer as opposed to the single pointer (regardless of any differences in size)? Other than that, I no longer have that in my code as per @Corbin's advice. I now have an array of structs. \$\endgroup\$ – Jamal Dec 30 '15 at 19:54

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.