7
\$\begingroup\$

The M-Shell (msh) provides a basic command-line interface similar, and features some builtins (cd, exit, help, whoami, kill).

#ifdef _POSIX_C_SOURCE
#undef _POSIX_C_SOURCE
#endif

#ifdef _XOPEN_SOURCE
#undef _XOPEN_SOURCE
#endif

#define _POSIX_C_SOURCE 200819L
#define _XOPEN_SOURCE 700

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <stdbool.h>
#include <stdint.h>
#include <ctype.h>

#include <unistd.h>
#include <sys/wait.h>
#include <errno.h>
#include <pwd.h>
#include <libgen.h>

#define ARRAY_CARDINALITY(x) (sizeof (x) / sizeof ((x)[0]))

#define MSH_TOK_DELIM   " \t\r\n\v\f"

static int msh_cd(const char *const *argv);
static int msh_help(const char *const *argv);
static int msh_exit(const char *const *argv);
static int msh_kill(const char *const *argv);
static int msh_whoami(const char *const *argv);

/* 
*   List of builtin commands, followed by their corresponding functions. 
*/
static struct {
    const char *const builtin_str;
    int (* const builtin_func)(const char *const *);
} const builtin[] = {
    { "cd", &msh_cd },
    { "help", &msh_help },
    { "exit", &msh_exit },
    { "kill", &msh_kill },
    { "whoami", &msh_whoami },
};

static int msh_whoami(const char *const *argv)
{
    const uid_t uid = geteuid();
    const struct passwd *const pw = (errno = 0, getpwuid(uid));

    if (argv[1]) {
        fputs("-msh: extra operand to \"whoami\".\n", stderr);
    } else if (!pw || errno) {
        perror("-msh: ");
    } else {
        puts(pw->pw_name);
    }
    return 1;
}

static int msh_kill(const char *const *argv)
{
    if (!argv[1] || !argv[2]) {
        fputs("-msh: expected argument to \"kill\".\n", stderr);
    } else
        if (kill((pid_t) strtol(argv[2], 0, 10), (int) strtol(argv[1], 0, 10)) == -1) {
        perror("-msh ");
    }
    return 1;
}

static int msh_cd(const char *const *argv)
{
    if (!argv[1]) {
        fputs("-msh: expected argument to \"cd\".\n", stderr);
    } else if (chdir(argv[1])) {
        perror("-msh");
    }
    return 1;
}

static int msh_help(const char *const *argv)
{
    (void) argv;
    puts("Z-Shell\n"
         "Type program names and arguments, and hit enter.\n"
         "The following are built-in:\n");

    for (size_t i = 0; i < ARRAY_CARDINALITY(builtin); ++i) {
        puts(builtin[i].builtin_str);
    }

    puts("Use the man command for information on other programs.\n");
    return 1;
}

static int msh_exit(const char *const *argv)
{
    (void) argv;
    return 0;
}

/* Calls fork and execvp to duplicate and replace a process, returns 0 on failure and 1 on success
*/
static int msh_launch(const char *const *argv)
{
    int status = 0;
    int pid = fork();
    
    if (!pid) {
        if (execvp(argv[0], (char *const *) argv) == -1) {
            perror("msh: ");
            return 0;
        }
    }

    if (pid == -1) {
        perror("msh: ");
        return 0;
    }

    do {
        waitpid(pid, &status, WUNTRACED);
    } while (!WIFEXITED(status) && !WIFSIGNALED(status));

    return 1;
}

/* Returns 1 in the absence of commands or a pointer to a function if argv[0] was a built-in command.
*/
static int msh_execute(const char *const *argv)
{
    if (!argv[0]) {
        /* No commands were entered. */
        return 1;
    }

    for (size_t i = 0; i < ARRAY_CARDINALITY(builtin); ++i) {
        if (!strcmp(argv[0], builtin[i].builtin_str)) {
            return (*builtin[i].builtin_func) (argv);
        }
    }
    return msh_launch(argv);
}

/* Returns a char pointer on success, or a null pointer on failure.
*  Caller must free the line on success.
*  Otherwise, msh_read_line frees all allocations and set them to point to NULL.
*/
static char *msh_read_line(int *err_code)
{
    const size_t page_size = BUFSIZ;
    size_t position = 0;
    size_t size = 0;
    char *content = 0;

    clearerr(stdin);

    for (;;) {
        if (position >= size) {
            size += page_size;
            char *new = realloc(content, size);

            if (!new) {
                *err_code = ENOMEM;
                return 0;
            }
            content = new;
        }
        int c = getc(stdin);

        if (c == EOF || c == '\n') {
            if (feof(stdin)) {
                free(content);
                *err_code = EOF;
                return 0;
            } else {
                content[position] = '\0';
                return content;
            }
        } else {
            content[position] = (char) c;
        }
        position++;
    }
}

/* Returns a pointer to pointers to null-terminated strings, or a NULL pointer on failure. 
*  Does not free the passed char * in case of failure. 
*/
static char **msh_parse_args(char *line)
{
    const size_t page_size = 128;
    size_t position = 0;
    size_t size = 0;
    char **tokens = 0;

    for (char *next = line; (next = strtok(next, MSH_TOK_DELIM)); next = 0) {
        if (position >= size) {
            size += page_size;
            char **tmp = realloc(tokens, size * sizeof *tmp);

            if (!tmp) {
                free(tokens);
                return 0;
            }
            tokens = tmp;
        }
        tokens[position++] = next;
    }

    if (tokens) {
        tokens[position] = 0;
    }
    return tokens;
}

static int msh_loop(void)
{
    int status = 0;

    do {
        char *line = NULL;
        char **args = NULL;
       
        /* getuid() is always successful. */
        const uid_t uid = getuid();
        const struct passwd *const pw = getpwuid(uid);
    
        char *cwd = getcwd(0, 0);
        char *base_name = cwd ? basename(cwd) : 0;
        
        printf("%s:~/%s $ ", pw ? pw->pw_name : "", base_name? base_name : "");

        int err_code = 0;

        line = msh_read_line(&err_code);
        if (!line) {
            if (err_code == ENOMEM) {
                perror("-msh ");
            }
            fputc('\n', stdout);
            free(cwd);
            return 0;
        }
    
        if (!*line) {
            continue;
        }

        if (!(args = msh_parse_args(line))) {
            perror("-msh ");
            free(line);
            free(cwd);
            return 0;
        }

        status = msh_execute((const char *const *) args);
        free(line);
        free(args);
        free(cwd);
    } while (status);
    
    return 1;
}

int main(void)
{
    return !msh_loop() ? EXIT_FAILURE : EXIT_SUCCESS;
}

The final executable is 14552 bytes, does not have a large memory footprint:

3442:   ./msh
Address           Kbytes     RSS   Dirty Mode  Mapping
0000564a55b19000       4       4       0 r---- msh
0000564a55b1a000       4       4       0 r-x-- msh
0000564a55b1b000       4       4       0 r---- msh
0000564a55b1c000       4       4       4 r---- msh
0000564a55b1d000       4       4       4 rw--- msh
0000564a579fb000     132      16      16 rw---   [ anon ]
00007f1e932fa000      12       8       8 rw---   [ anon ]
00007f1e932fd000     160     160       0 r---- libc.so.6
00007f1e93325000    1620    1004       0 r-x-- libc.so.6
00007f1e934ba000     352     216       0 r---- libc.so.6
00007f1e93512000       4       0       0 ----- libc.so.6
00007f1e93513000      16      16      16 r---- libc.so.6
00007f1e93517000       8       8       8 rw--- libc.so.6
00007f1e93519000      52      24      24 rw---   [ anon ]
00007f1e93538000       8       4       4 rw---   [ anon ]
00007f1e9353a000       8       8       0 r---- ld-linux-x86-64.so.2
00007f1e9353c000     168     168       0 r-x-- ld-linux-x86-64.so.2
00007f1e93566000      44      44       0 r---- ld-linux-x86-64.so.2
00007f1e93572000       8       8       8 r---- ld-linux-x86-64.so.2
00007f1e93574000       8       8       8 rw--- ld-linux-x86-64.so.2
00007ffd60554000     132      12      12 rw---   [ stack ]
00007ffd605b7000      16       0       0 r----   [ anon ]
00007ffd605bb000       8       4       0 r-x--   [ anon ]
ffffffffff600000       4       0       0 --x--   [ anon ]
---------------- ------- ------- ------- 
total kB            2780    1728     112

and unlike other shells, reports no memory leaks under valgrind.

Review Goals:

Is the code structured properly?

General coding comments, style, etc.

Does any part of the code invoke undefined/implementation-defined behavior?

\$\endgroup\$
9
  • \$\begingroup\$ Clearly this shell is less capable than bash, which can do | pipes, and that's cool. It would be interesting to see some "SoTA comparisons" in the review context, to motivate this exercise. For example, the ash and dash shells were crafted with an explicit design goal of minimalism. The idea was to place a tiny shell interpreter on a small /boot partition, and to consume very little memory while /etc/rc and other bootup scripts are running. "Small" can mean "secure", fewer bugs. Perhaps there are some nice "small footprint" stats you'd like to highlight in this question? \$\endgroup\$
    – J_H
    Commented Jan 25 at 18:33
  • \$\begingroup\$ @J_H Well, would the result of pmap be of any interest? \$\endgroup\$
    – Harith
    Commented Jan 25 at 19:31
  • \$\begingroup\$ Sure. It’s often the case that a given program is best at “something”. I’m just looking for what that might be in this case. \$\endgroup\$
    – J_H
    Commented Jan 25 at 19:34
  • \$\begingroup\$ Have you tested thecd functionality and is it working as expected? Does it work repeatedly? \$\endgroup\$
    – pacmaninbw
    Commented Jan 27 at 19:30
  • 1
    \$\begingroup\$ In msh_kill you have an } else followed by a newline and indented if (...) {. It looks like you forgot a {, the closing } below the if statement is actually for the if, not the else. Consider adding appropriate brackets as right now it looks like you always perror("msh- "), when that should only occur if the kill fails. \$\endgroup\$
    – Andrakis
    Commented Jun 6 at 6:16

7 Answers 7

3
\$\begingroup\$

Good job! It definitely lives up to its name of "minimal shell". And while a small codebase doesn't always translate to a bug-free codebase, there's certainly fewer places for bugs to hide.

I really like the layering. It makes a lot of sense as you read the source.

name

static int msh_help( ... )
{
    ...
    puts("Z-Shell\n"

Z ?!?

I was kind expecting to see "msh" there.

automated tests

This submission would benefit from adding a test suite that exercises the target code.

NULL

https://linux.die.net/man/3/getcwd

As an extension to the POSIX.1-2001 standard, Linux (libc4, libc5, glibc) getcwd() allocates the buffer dynamically using malloc(3) if buf is NULL.

        char *cwd = getcwd(0, 0);

Call me old fashioned, but I would rather see getcwd(NULL, 0) there. (Yes, yes, I know, zero and NULL are nearly the same.)

check error status

Thank you for helpfully pointing out that getuid() always succeeds.

But some of the calls you make can fail, e.g. with ENOMEM on an unlucky call to getcwd().

I am very glad to see you carefully calling free().

goto outN

        line = msh_read_line( ... );
        if (!line) {
            ...
            free(cwd);

I feel that, instead of freeing cwd, this should be goto out3;.

        if (!( ... msh_parse_args(line))) {
            ...
            free(line);
            free(cwd);

Similarly, goto out2;.

And out1: would free line.

    out2:
        free(args);
    out3:
        free(cwd);

        return ret_val;

We would need to properly set up the return value.

The idea here is, for each allocate call, we write exactly one deallocate call, nicely paired up.

Following such a pattern should make it harder to accidentally introduce code defects.

allocation lifetime

*  Does not free the passed char * in case of failure. 
*/
static char **msh_parse_args(char *line)
{
            ...
            if ( ... ) {
                free(tokens);

I started writing this review remark thinking I would ask for caller to always be responsible for free'ing line.

But then I realized the comment was misleading me. I imagine it used to be true, at one time.

lint

static int msh_exit(const char *const *argv)
{
    (void) argv;

I assume you wrote this to silence a -Wall -pedantic type of warning, and that's great, I support such practice.

IDK, maybe assert that there's no arguments supplied? Which should keep the compiler happy, since we actually used the input parameter.

boolean

static int msh_execute( ... )

Rather than int, I feel this is more naturally a bool. Though the sense is negated.

OIC, launch can return arbitrary values, so maybe introduce a status type? Similar to EXIT_{FAILURE,SUCCESS} ?

I keep finding it harder than necessary to read things like this:

static int msh_launch( ... )
            ...
            return 0;

because it doesn't say something like return EXIT_SUCCESS.

I do like how you DRYed up help by looping and displaying each .builtin_str.

The whoami and similar routines are wonderfully simple and robust.


This codebase tackles modest goals, and accomplishes them, within a small memory footprint.

I would be willing to delegate or accept maintenance tasks on it.


EDIT for @pacmaninbw

(As far as "teaching" goes, I would rather teach folks to choose a high level language instead of portable assembly. Then we do a better job of conveying technical ideas to other humans. But hey, C is what it is, sometimes ya gotta use it, so I roll with the punches.)

Assume "mutex acquisition failed" is represented by a NULL return value.

Here is the usual (carefully structured, nested) pattern:

void do_stuff() {
    int ret = EXIT_FAILURE;

    if ((a = acquire_mutex("a")) == NULL) {
        goto out_a;
    }
    if ((b = acquire_mutex("b")) == NULL) {
        goto out_b;
    }
    if ((c = acquire_mutex("c")) == NULL) {
        goto out_c;
    }
    do_cool_stuff_while_holding(a, b, c);
    ret = EXIT_SUCCESS;

    unlock("c");
  out_c:
    unlock("b");
  out_b:
    unlock("a");
  out_a:
    return ret;
}

Notice that we obey lexical ordering of lock names, and carefully release them in reverse order, to prevent ugly scenarios like deadlock.

It doesn't matter what the resource is. Could be a lock, a malloc'd string, a file descriptor, whatever. The routine needs them in order to accomplish its contract. It acquires them in sequence. But sometimes things don't go smoothly. So it releases them in reverse sequence when it bails out.

If we don't follow this pattern, we may wind up replacing the if ... goto clauses with copy-pasta code. First if ... "release A, then bail". Second if ... "release B, A, then bail". (Wait, did I carefully release in the proper order?) Third if ... "release C, B, A, then bail".

Now someone refactors B, turning it into D. And correctly fixes 50% of the places where we free it. Sigh! Or someone messes up the unlock order.

Remember this is the exceptional path we're considering here, not the happy path. So test coverage is going to be challenging, and spotty.

Forcing every single release to always execute in the Happy Path is a boon to testing. So jumping into the middle of the "release" flow is a big win here.

Cultural digression: An engineer, perhaps Edsger Dijkstra or Niklaus Wirth, could nest structured elements so that a,b,c nest nicely, and so does the unwinding code for releasing resources. But hey, Linus don't code that way. So we roll with it, we don't write a bunch of trivial helpers, and we don't see the bulk of our logic inching further and further away from the left margin.

Here is a typical example in BIND9, that is, in some non-kernel code:

00331 static inline dns_rdatalist_t *
00332 newrdatalist(dns_message_t *msg) {
00337         if ( ... ) { ...
00339                 goto out;
00340         } ...
00355  out:
00356         if ( ... )
00357                 dns_rdatalist_init(rdatalist);
00358 
00359         return (rdatalist);
00360 }
\$\endgroup\$
3
  • \$\begingroup\$ I was going to up vote until I saw you suggest goto, why do you want to teach spaghetti code? \$\endgroup\$
    – pacmaninbw
    Commented Jan 29 at 2:51
  • 6
    \$\begingroup\$ It's a standard idiom in C code, e.g. in the linux kernel we see it ALL the time. And no, it's not spaghetti, it is structured. It's a pattern that is easy for humans to follow and causes fallible humans to write fewer code defects. // In a higher level language like lisp or python, we might throw an exception, confident that the Right Thing will happen and resources will be cleaned up on the way out. Or roughly equivalently, use a context manager. In C we do not enjoy that luxury. So we pair allocate with deallocate. Some form of goto fail, goto out, or goto outN accomplishes cleanup. \$\endgroup\$
    – J_H
    Commented Jan 29 at 2:56
  • 5
    \$\begingroup\$ Yes, this is the single widely-accepted use of goto in C. Using that pattern would likely have avoided the memory leak I mentioned in my answer. \$\endgroup\$ Commented Jan 30 at 17:03
4
\$\begingroup\$

Line near end-of-file

I'd expect input like "abc" then end-of-file to be considered a line, so do not return with *err_code = EOF unless nothing was read for that line.

// if (feof(stdin)) {
if (feof(stdin) && position == 0) {
  free(content);
  *err_code = EOF;
  return 0;
} 

Bug: Pedantic: input error

With input like: "abc" then input error, code treats that the same as "abc\n". I'd expect *err_code = EOF; or some other error condition.

        if (c == EOF || c == '\n') {
          if (c == '\n' || (feof(stdin) && position > 0)) {
            content[position] = '\0';
            return content;
          }
          free(content);
          *err_code = EOF;
          return 0;
        } else {
          ...
\$\endgroup\$
3
  • 1
    \$\begingroup\$ Text files are comprised of lines. Each line ends with a \n newline. Let's count how many lines are in a three-byte binary file: $ echo -n abc | wc -l. Now let's try a short text file: $ echo abc | wc -l. As expected, the results are zero lines and one line. \$\endgroup\$
    – J_H
    Commented Jun 4 at 18:00
  • \$\begingroup\$ @J_H I did not get the point you were trying to make. Pray expatiate. \$\endgroup\$
    – Harith
    Commented Jun 4 at 19:30
  • 1
    \$\begingroup\$ @Harith Shells work on text files full of commands as input. I was clarifying that we can produce a small binary file with $ echo -n abc > alpha.bin, a different kind of file from this text file: $ echo abc > alpha.txt. Sure, the OP code's behavior around EOFs could likely be improved. But I assert that caller did the wrong thing, and gets what he deserves, if instead of text lines the caller offers to the shell some binary input. The shell could choose to respond more robustly, but it's not clear that a spec requires it to treat an erroneous early EOF (without newline) in a certain way. \$\endgroup\$
    – J_H
    Commented Jun 4 at 19:42
3
\$\begingroup\$

Not a full review, but one thing that jumped out for now:

Feature-Test Macros

You have

#ifdef _POSIX_C_SOURCE
#undef _POSIX_C_SOURCE
#endif

#ifdef _XOPEN_SOURCE
#undef _XOPEN_SOURCE
#endif

#define _POSIX_C_SOURCE 200819L
#define _XOPEN_SOURCE 700

This silently overrides any previous definitions in your build system or another header. But if different developers have conflicting requirements for the API to use, you want to report the error and get the maintainers to fix it, not hide it from them. What happens if two files both do this, and end up with inconsistent ABI versions that cause a link error? How easy would that bug be to find?

I would usually recommend leaving in the definitions with no preprocessor guards, so if anyone else defines it inconsistently, the compiler tells you where the problem is.

However, your shop might set the version macros in the build system, or want to be able to test another setting by adding -D_XOPEN_SOURCE= in the makefile. To support that, you might want to set these macros by default, but allow them to be overridden on the command line:

#if !defined(_POSIX_C_SOURCE) && !defined(_XOPEN_SOURCE)
#  define _POSIX_C_SOURCE 200819L
#  define _XOPEN_SOURCE   700
#endif
\$\endgroup\$
2
\$\begingroup\$

The #ifdef here adds no value:

#ifdef _POSIX_C_SOURCE
#undef _POSIX_C_SOURCE
#endif

It's perfectly permissible and normal to #undef names whether or not they are defined.


I don't like the name hard-coded here and elsewhere:

    perror("-msh: ");

It's much better to store the name from argv[0] to match how the user accessed the program (which may have a leading - if it's invoked from login). Also, the extra : and space look weird alongside those that perror() produces.


Here's a memory leak:

    if (!*line) {
        continue;
    }

We need to free line and cwd before we continue and those variables end their scope.

It's quite easy to trigger this leak - here's what happens when I send just two newlines as input:

==1310940== HEAP SUMMARY:
==1310940==     in use at exit: 8,223 bytes in 2 blocks
==1310940==   total heap usage: 22 allocs, 20 frees, 30,928 bytes allocated
==1310940== 
==1310940== 31 bytes in 1 blocks are definitely lost in loss record 1 of 2
==1310940==    at 0x4845C43: realloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==1310940==    by 0x496F9E7: getcwd (getcwd.c:86)
==1310940==    by 0x109869: msh_loop (289119.c:233)
==1310940==    by 0x1099CF: main (289119.c:272)
==1310940== 
==1310940== 8,192 bytes in 1 blocks are definitely lost in loss record 2 of 2
==1310940==    at 0x4840718: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==1310940==    by 0x10967B: msh_read_line (289119.c:165)
==1310940==    by 0x1098E0: msh_loop (289119.c:240)
==1310940==    by 0x1099CF: main (289119.c:272)
\$\endgroup\$
2
  • \$\begingroup\$ Just when I thought I was not leaking any memory. :) \$\endgroup\$
    – Harith
    Commented Jan 30 at 18:48
  • 2
    \$\begingroup\$ If it helps, it was gcc -fanalyzer that alerted me to the leak. That's becoming a more useful option with each successive GCC version. \$\endgroup\$ Commented Feb 11 at 13:00
2
\$\begingroup\$

These declarations:

static int msh_cd(const char *const *argv);
static int msh_help(const char *const *argv);
static int msh_exit(const char *const *argv);
static int msh_kill(const char *const *argv);
static int msh_whoami(const char *const *argv);

can be tidied up with an alias:

typedef int cmd_t(const char *const *argv);

static cmd_t msh_ch;
static cmd_t msh_help;
static cmd_t msh_exit;
static cmd_t msh_kill;
static cmd_t msh_whoami;

This typedef:

typedef int cmd_t(const char *const *argv)

defines an alias for the type of function of the type int(const char *const argv).

This is probably not a big deal right now, but would be useful as the number of built-in functions increases: only a single change would be warranted to the function prototype.

As _t suffix is reserved in the ISO POSIX Standard, one might want to pick something else. builtin_func is an option.

\$\endgroup\$
2
\$\begingroup\$

Bug 1:

The shell exits when a single newline is provided. The culprit is likely this code:

if (!line) {
    if (err_code == ENOMEM) {
        perror("-msh ");
    }
    fputc('\n', stdout);
    free(cwd);
    return 0;
}

This also leaks memory, as @Toby shows in his answer.

Bug 2:

The shell exits with an unsuccessful status code for a line containing more than one whitespace (and only whitespaces):

user:~/user $
-msh : Success

This message is being printed from:

if (!(args = msh_parse_args(line))) {
    perror("-msh ");
    free(line);
    free(cwd);
    return 0;
}

msh_parse_args() is lacking in documentation. It states:

/* Returns a pointer to pointers to null-terminated strings, or a NULL pointer on failure. 
*  Does not free the passed char * in case of failure. 
*/

Yet nowhere does it mention that a null pointer is also returned when the there are no tokens in the line, or the line has only whitespaces.

Consider changing the documentation like so:

/* Returns a pointer to pointers to null-terminated strings, or a null pointer
 * on failure. The function does not free line in any case.
 *
 * Caller can distinguish between a memory allocation error and an empty line by
 * checking the value of *argc. A nonzero value indicates a memory allocation.

And changing the function like so:

static char **msh_parse_args(char *line, int *argc)
{
    ...
    size_t position = 0;
    ...

    for (...) {
        ...
        if (tmp == NULL) {
            free(tokens);
            *argc = (int) position;
            return NULL;
        }
        ...
    }
    ...
}

And changing the calling code to:

int argc = 0;

if ((args = msh_parse_args(line, &argc)) == NULL) {
    if (argc) {
        // Now fail
    }
}

This solves the bug.

What this code would really benefit from is fuzz-testing.

page_size should hold the page size:

In msh_parse_args(), this:

const size_t page_size = 128;

make no sense. Consider changing it to initial_size or the like.

\$\endgroup\$
0
2
\$\begingroup\$

First :: ACCOLADES! (very rare; very precious!):

Let me begin by complimenting this code for its layout and legibility.
This code is a pleasure to read!
Kudos to you!


More optimism, shorter distances:

/* existing code */
static int msh_loop(void)
{
    int status = 0;

    do {
        ...
        if (!line) { // error
            ...
            return 0;
        }
        if (!(args = msh_parse_args(line))) { // error
            ...
            return 0;
        }
        status = msh_execute((const char *const *) args);
        ...
    } while (status);
    return 1; // no error
}

int main(void)
{
    return !msh_loop() ? EXIT_FAILURE : EXIT_SUCCESS;
}

Shrinking-out some code, it becomes plain that the return code from msh_loop() doesn't jive with the exit code from main().

Simplify:

/* revised code */
static int msh_loop(void)
{
    int status = 0;

    do {
        ...
        if (!line) { // error
            ...
            return EXIT_FAILURE;
        }
        if (!(args = msh_parse_args(line))) { // error
            ...
            return EXIT_FAILURE;
        }
        status = msh_execute((const char *const *) args);
        ...
    } while (status);
    return EXIT_SUCCESS;
}

int main(void)
{
    return msh_loop();
}

This leaves main() looking quite anemic. Okay for now. Who knows when there may come a time to deal with program arguments?

This also reveals, however, that status of executing can be 0 if an error is encountered. This will terminate the do/while() loop, but then "happy as a clam 1" is being returned to the caller, main().

Really think control flow and status needs to be carefully examined and tested...


Proximate variables:

    do {
        char *line = NULL;
        char **args = NULL;

        /* getuid() is always successful. */
        const uid_t uid = getuid();
        const struct passwd *const pw = getpwuid(uid);

        char *cwd = getcwd(0, 0);
        char *base_name = cwd ? basename(cwd) : 0;

        printf("%s:~/%s $ ", pw ? pw->pw_name : "", base_name? base_name : "");
        ...
            free(cwd);
        ...
            free(cwd);
        ...
        free(cwd);
        ...

line isn't needed until Act II, and args not until Act III of the function, but they're both onstage when the curtain goes up on Act I of do/while();. The audience wonders why they both just stand there, staring blankly off into space...

base_name, pw and cwd are only needed for brief cameo appearances, but... Look at those egos. cwd has one exit, and two(!) curtain calls. Tell her she's only being paid for her one line of the drama.

Recommend factoring out several lines of the whole "prompt" generation code to new: msh_prompt().


Off by one error:

Imagine a command line from that place where it's always very, very warm...

static char **msh_parse_args(char *line)
{
    const size_t page_size = 128;
    ...
    char **tokens = 0;

    for (char *next = line; (next = strtok(next, MSH_TOK_DELIM)); next = 0) {
        if (position >= size) {
            size += page_size;
// ORDERING! rooms for 128 pointers, please!
            char **tmp = realloc(tokens, size * sizeof *tmp);
            ...
            tokens = tmp;
        }
// Okay, #0, #1, #2, ... #125, #126 #127... That's it! Full up!!
// Got 128 full elements... happy!
        tokens[position++] = next;
    }

    if (tokens) {
// Wot!! There ain't no element "tokens[ 128 ]"!!! Oops!!
        tokens[position] = 0;
    }
    return tokens;
}

Gone, and ain't comin' back:

        if (c == EOF || c == '\n') {
            if (feof(stdin)) {
                free(content);
                *err_code = EOF;
                return 0;
            } else {
                content[position] = '\0';
                return content;
            }
        } else {
            content[position] = (char) c;
        }
        position++;

(This could be a matter of taste...)
Once one of those wormholes (aka: return, break, continue, goto) has opened up, or been avoided(!), there's not much use of else to avoid getting onto an alternate path.

Some like to see } else { .... You can find your own preference. To me, if/else suggests there's a fork-in-the-road coming up and I'm always taken by surprise when one branch vectors off somewhere far away, ne'er to return to this region. Just a heads-up.

Look at the flow through msh_launch(). It correctly attempts execvp(), and correctly deals with the failure case. When successful, the "child binary" replaces one of the clones (processes) resulting from that fork(). The "parent" goes on living and doing parent-y things. Rightly so, there is no else in that function. Flow will never re-unite into a single path again.

Look closely. That bottom else makes a mysterious big-shot out of a simple assignment that will happen just before an increment.


Unfortunate variable names:

static char *msh_read_line(int *err_code) {
    const size_t page_size = BUFSIZ;
    size_t size = 0;
    ...
    for (;;) {
        if (position >= size) {
            size += page_size;

I lost 5 minutes thinking I'd found a problem, only to discover that
size and page_size were being used correctly here.

Recommend that constant move closer to the realloc() and be given a different name. (Perhaps allocSize?).

Just a small matter that tripped me up for a few minutes (my own fault). Preventable??


Time presses, so leaving this here with one final suggestion.

You know the function prototypes are extra baggage when the function definitions are sequenced-by-design. Less code... (Forward declaration of built_ins[], and its element count, may be a way to get rid of the function prototypes and allow msh_help() to be able to report itself as one element. Strive for fewer lines of code.


A long-ago workmate had a practice of prefixing function names (even static functions with limited file scope existence) and variable names (even those local to a function) with the name of the source file in which they resided. This meant that all-or-most of his for() blocks were spread across 3 or more lines.

Fabricated example:

    for( year_end_report_ind = 0;
        year_end_report_arr[ year_end_report_ind ].value > myAnnualIncome;
        year_end_report_ind += 1 )
    {
        year_end_report_function( year_end_report_arr[ year_end_report_ind ].summary );
        ...

Every function in this code is static. There's no need for the msh_ prefix. Less to read means more mental horsepower will be available to think about what is being read. Code must be Correct, and Clear is very important, too. I suggest Concise deserves a lot of effort, too. KISS!


On Being "Clever":

static int msh_whoami(const char *const *argv)
{
    const uid_t uid = geteuid();
    const struct passwd *const pw = (errno = 0, getpwuid(uid));
    ...

(This took some digging!) Saving one line of code by (unnecessarily) combining two operations inside parentheses would/should furrow the brow of any reviewer.

I don't run UNIX/Linux, so cannot easily test this...
It looks like 2 lines might be more expressive as:

static int msh_whoami(const char *const *argv)
{
    errno = 0;
    const struct passwd *const pw = getpwuid( geteuid() );
    ...

I can handle nested function calls. "Clever" constructs I reserve for my own personal-use (often obfuscated) code that is intended to never venture into the wild.


\$\endgroup\$
4
  • 2
    \$\begingroup\$ I like chunk_size for incremental allocations. But alloc_size is quite good, too. Definitely agree that the word page already has a meaning that misdirects here! \$\endgroup\$ Commented Jun 6 at 8:27
  • \$\begingroup\$ @TobySpeight No wonder what I was thinking an year ago, and then another 5 months ago when I last touched this code. I have even used page_size for the initial capacity in msh_parse_args(). \$\endgroup\$
    – Harith
    Commented Jun 6 at 8:53
  • 1
    \$\begingroup\$ @Fe2O3 Thank you for your time. As you find great pleasure in reading code, I found this new version of cdecl that supports all C and C++ standards and worked very great for debugging and testing declarations and macros. The code seems to be of high quality to me (Well, the author has been writing C for the past 35-40 years, so it is to be expected). I have already picked up many new things from the codebase. Thought you might like the code, if not the tool. :) \$\endgroup\$
    – Harith
    Commented Jun 6 at 8:55
  • 1
    \$\begingroup\$ @Harith Thank you for the link. I've now got it bookmarked in the hope that, one day in the not-to-distant future, my brain will grow back again. :-) Cheers! \$\endgroup\$
    – Fe2O3
    Commented Jun 6 at 9:01

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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