Skip to main content
added 446 characters in body
Source Link
chux
  • 33.6k
  • 2
  • 38
  • 92
  1. Drop though cases. The code appears to be OK. Yet it looks like it might be missing a break; at the end of case '?'. Recommend to add a comment at the end of a case that intentionally lacks a break, return,etc.

         case '?':
             if (optopt == 'c')
                 fprintf(stderr, "Option -%c requires an argument.\n", optopt);
             else if (isprint (optopt))
                 fprintf(stderr, "Unknown option `-%c'.\n", optopt);
             else
                 fprintf(stderr,
                         "Unknown option character `\\x%x'.\n",
                         optopt);
    
             // Add comment
             // Drop though
    
         default: {
             return 1;
         }
    
  2. Remove debug code

             printf("sh OpenShell version 0.1(a)\n");
             printf("Version: %s\n", VERSION);
    
           //  printf ("%s / %s / %s / %s\n",
             //         program_name, version,
               //       build_date, build_git_sha);
    
  3. Obviously missing include files and other code needed before main().

     int main(int argc, char *argv[]) {
         bool donotrun = false;
         struct sigaction new_action, old_action;
         hashtable_t *hashtable = ht_create(65536);
    
  4. Inconsistent indentation. An if() without {} is tolerable, yet not preferred, yet then code breaks a line that would look fine as one. Suggest re-formating.

     // if (old_action.sa_handler != SIG_IGN)
     //     sigaction(SIGINT, &new_action, NULL);
     // ...
     // i = getopt_long(argc, argv, "pc:fvh",
     //                 options, &index);
    
     if (old_action.sa_handler != SIG_IGN) {
         sigaction(SIGINT, &new_action, NULL);
     }
     ...
     i = getopt_long(argc, argv, "pc:fvh", options, &index);
    
  5. Inconsistent formating in `switch statement concerning blank lines. This and other parts hints that OP is not using an automated formatter. Save time. Use an automated formatter. Often these are incorporated within a design environment. Stand alone ones exist. Avoid manual formatting.

            exit(EXIT_SUCCESS);
    
        }
        case 'h': {
            usage();
            exit(EXIT_SUCCESS);
    
        }
        case 'c': {
            cvalue = optarg;
            command(cvalue, hashtable, background);
            exit(EXIT_SUCCESS);
        }
    
        case 'f': {
    
  6. getPath(); is undefined, naked and uncommented. Certainly such a call could return an error, one that is not handled here.

  7. Buffer over flow potential. There is nothing special about 128. Code needs protection/re-work.

             char str[128];
             strcpy(str, line);  // Unsafe
    
  8. For array indexing use size_t.

             // int cc;
             size_t cc;
    
             cc = strlen(buf);
    
  9. Avoid a hacker exploit. buf[0] could be 0 and then buf[cc - 1] is UB. Instead test.

     if (fgets(buf, CMD_LEN - 1, fp) == NULL) {
     ...
     cc = strlen(buf);
     //if (buf[cc - 1] == '\n')
     if (cc > 0 && buf[cc - 1] == '\n')
    
  10. Simplification

    //char buffer[2]; //buffer[0] = '|'; //buffer[1] = '\0'; char buffer[] = "|";

  11. Avoid allocation by size of type. Instead by the size of the referenced variable. This post does not show struct pipeline. OTOH, with the right code, we can have less need for it. BTW, pedantic point: good to do allocation math leading with sizeof as that insures math at least at size_t width. Consider malloc(BUFFER_SIZE * chunks[i].size * sizeof *(pipe[i].data)) could overflow the BUFFER_SIZE * chunks[i].size first done in that order.

     // pipe[i].option = malloc(sizeof(int) * 10);
     pipe[i].option = malloc(sizeof *(pipe[i].option) * 10);
    
     // pipe[i].data = malloc(sizeof(char **) * BUFFER_SIZE * chunks[i].size);
     pipe[i].data = malloc(sizeof *(pipe[i].data) * BUFFER_SIZE * chunks[i].size);
    
  12. Allocation test good, but needs an exit. Good to test, but then what? Suggest returning a failure code too. IMO, better to include some diagnostic info like __LINE__, __FUNC__, etc.

    if (pipe == NULL) {
        // fprintf(stderr, "malloc failed!\n");
       fprintf(stderr, "malloc failed! %d\n", __LINE__);
        // add
        return(EXIT_FAILURE);
    }
    
  13. Some systems have a multitude of special integer types. Rather than guess at their width and potentially truncate, just go for the largest supported on printing

     // fprintf(stderr, "pid %ld:\n", (long) pid);
     fprintf(stderr, "pid %lld:\n", (long long) pid);
     // or
     fprintf(stderr, "pid %jd:\n", (intmax_t) pid);
    
  14. Unclear if the result of list_split() is always chunks->pipes > 0. If it could be 0, then malloc(0) returning NULL is not a sign of OOM.

    struct str_list *chunks = list_split(cmd, buffer);
    struct pipeline *pipe = malloc(chunks->pipes * sizeof *pipe);
    // if (pipe == NULL) {
    if (pipe == NULL && chunks->pipes > 0) {
        fprintf(stderr, "malloc failed!\n");
    }
    
  15. Code is certainly wrong - passing an unknown value in reti to exec_builtin(). With that - code is broken - review done.

    int * reti; if (exec_builtin(reti, cmd)) { return 0; }

  1. Drop though cases. The code appears to be OK. Yet it looks like it might be missing a break; at the end of case '?'. Recommend to add a comment at the end of a case that intentionally lacks a break, return,etc.

         case '?':
             if (optopt == 'c')
                 fprintf(stderr, "Option -%c requires an argument.\n", optopt);
             else if (isprint (optopt))
                 fprintf(stderr, "Unknown option `-%c'.\n", optopt);
             else
                 fprintf(stderr,
                         "Unknown option character `\\x%x'.\n",
                         optopt);
    
             // Add comment
             // Drop though
    
         default: {
             return 1;
         }
    
  2. Remove debug code

             printf("sh OpenShell version 0.1(a)\n");
             printf("Version: %s\n", VERSION);
    
           //  printf ("%s / %s / %s / %s\n",
             //         program_name, version,
               //       build_date, build_git_sha);
    
  3. Obviously missing include files and other code needed before main().

     int main(int argc, char *argv[]) {
         bool donotrun = false;
         struct sigaction new_action, old_action;
         hashtable_t *hashtable = ht_create(65536);
    
  4. Inconsistent indentation. An if() without {} is tolerable, yet not preferred, yet then code breaks a line that would look fine as one. Suggest re-formating.

     // if (old_action.sa_handler != SIG_IGN)
     //     sigaction(SIGINT, &new_action, NULL);
     // ...
     // i = getopt_long(argc, argv, "pc:fvh",
     //                 options, &index);
    
     if (old_action.sa_handler != SIG_IGN) {
         sigaction(SIGINT, &new_action, NULL);
     }
     ...
     i = getopt_long(argc, argv, "pc:fvh", options, &index);
    
  5. Inconsistent formating in `switch statement concerning blank lines. This and other parts hints that OP is not using an automated formatter. Save time. Use an automated formatter. Often these are incorporated within a design environment. Stand alone ones exist. Avoid manual formatting.

            exit(EXIT_SUCCESS);
    
        }
        case 'h': {
            usage();
            exit(EXIT_SUCCESS);
    
        }
        case 'c': {
            cvalue = optarg;
            command(cvalue, hashtable, background);
            exit(EXIT_SUCCESS);
        }
    
        case 'f': {
    
  6. getPath(); is undefined, naked and uncommented. Certainly such a call could return an error, one that is not handled here.

  7. Buffer over flow potential. There is nothing special about 128. Code needs protection/re-work.

             char str[128];
             strcpy(str, line);  // Unsafe
    
  8. For array indexing use size_t.

             // int cc;
             size_t cc;
    
             cc = strlen(buf);
    
  9. Avoid a hacker exploit. buf[0] could be 0 and then buf[cc - 1] is UB. Instead test.

     if (fgets(buf, CMD_LEN - 1, fp) == NULL) {
     ...
     cc = strlen(buf);
     //if (buf[cc - 1] == '\n')
     if (cc > 0 && buf[cc - 1] == '\n')
    
  10. Simplification

    //char buffer[2]; //buffer[0] = '|'; //buffer[1] = '\0'; char buffer[] = "|";

  11. Avoid allocation by size of type. Instead by the size of the referenced variable. This post does not show struct pipeline. OTOH, with the right code, we can have less need for it. BTW, pedantic point: good to do allocation math leading with sizeof as that insures math at least at size_t width. Consider malloc(BUFFER_SIZE * chunks[i].size * sizeof *(pipe[i].data)) could overflow the BUFFER_SIZE * chunks[i].size first done in that order.

     // pipe[i].option = malloc(sizeof(int) * 10);
     pipe[i].option = malloc(sizeof *(pipe[i].option) * 10);
    
     // pipe[i].data = malloc(sizeof(char **) * BUFFER_SIZE * chunks[i].size);
     pipe[i].data = malloc(sizeof *(pipe[i].data) * BUFFER_SIZE * chunks[i].size);
    
  12. Allocation test good, but needs an exit. Good to test, but then what? Suggest returning a failure code too. IMO, better to include some diagnostic info like __LINE__, __FUNC__, etc.

    if (pipe == NULL) {
        // fprintf(stderr, "malloc failed!\n");
       fprintf(stderr, "malloc failed! %d\n", __LINE__);
        // add
        return(EXIT_FAILURE);
    }
    
  13. Some systems have a multitude of special integer types. Rather than guess at their width and potentially truncate, just go for the largest supported on printing

     // fprintf(stderr, "pid %ld:\n", (long) pid);
     fprintf(stderr, "pid %lld:\n", (long long) pid);
     // or
     fprintf(stderr, "pid %jd:\n", (intmax_t) pid);
    
  1. Drop though cases. The code appears to be OK. Yet it looks like it might be missing a break; at the end of case '?'. Recommend to add a comment at the end of a case that intentionally lacks a break, return,etc.

         case '?':
             if (optopt == 'c')
                 fprintf(stderr, "Option -%c requires an argument.\n", optopt);
             else if (isprint (optopt))
                 fprintf(stderr, "Unknown option `-%c'.\n", optopt);
             else
                 fprintf(stderr,
                         "Unknown option character `\\x%x'.\n",
                         optopt);
    
             // Add comment
             // Drop though
    
         default: {
             return 1;
         }
    
  2. Remove debug code

             printf("sh OpenShell version 0.1(a)\n");
             printf("Version: %s\n", VERSION);
    
           //  printf ("%s / %s / %s / %s\n",
             //         program_name, version,
               //       build_date, build_git_sha);
    
  3. Obviously missing include files and other code needed before main().

     int main(int argc, char *argv[]) {
         bool donotrun = false;
         struct sigaction new_action, old_action;
         hashtable_t *hashtable = ht_create(65536);
    
  4. Inconsistent indentation. An if() without {} is tolerable, yet not preferred, yet then code breaks a line that would look fine as one. Suggest re-formating.

     // if (old_action.sa_handler != SIG_IGN)
     //     sigaction(SIGINT, &new_action, NULL);
     // ...
     // i = getopt_long(argc, argv, "pc:fvh",
     //                 options, &index);
    
     if (old_action.sa_handler != SIG_IGN) {
         sigaction(SIGINT, &new_action, NULL);
     }
     ...
     i = getopt_long(argc, argv, "pc:fvh", options, &index);
    
  5. Inconsistent formating in `switch statement concerning blank lines. This and other parts hints that OP is not using an automated formatter. Save time. Use an automated formatter. Often these are incorporated within a design environment. Stand alone ones exist. Avoid manual formatting.

            exit(EXIT_SUCCESS);
    
        }
        case 'h': {
            usage();
            exit(EXIT_SUCCESS);
    
        }
        case 'c': {
            cvalue = optarg;
            command(cvalue, hashtable, background);
            exit(EXIT_SUCCESS);
        }
    
        case 'f': {
    
  6. getPath(); is undefined, naked and uncommented. Certainly such a call could return an error, one that is not handled here.

  7. Buffer over flow potential. There is nothing special about 128. Code needs protection/re-work.

             char str[128];
             strcpy(str, line);  // Unsafe
    
  8. For array indexing use size_t.

             // int cc;
             size_t cc;
    
             cc = strlen(buf);
    
  9. Avoid a hacker exploit. buf[0] could be 0 and then buf[cc - 1] is UB. Instead test.

     if (fgets(buf, CMD_LEN - 1, fp) == NULL) {
     ...
     cc = strlen(buf);
     //if (buf[cc - 1] == '\n')
     if (cc > 0 && buf[cc - 1] == '\n')
    
  10. Simplification

    //char buffer[2]; //buffer[0] = '|'; //buffer[1] = '\0'; char buffer[] = "|";

  11. Avoid allocation by size of type. Instead by the size of the referenced variable. This post does not show struct pipeline. OTOH, with the right code, we can have less need for it. BTW, pedantic point: good to do allocation math leading with sizeof as that insures math at least at size_t width. Consider malloc(BUFFER_SIZE * chunks[i].size * sizeof *(pipe[i].data)) could overflow the BUFFER_SIZE * chunks[i].size first done in that order.

     // pipe[i].option = malloc(sizeof(int) * 10);
     pipe[i].option = malloc(sizeof *(pipe[i].option) * 10);
    
     // pipe[i].data = malloc(sizeof(char **) * BUFFER_SIZE * chunks[i].size);
     pipe[i].data = malloc(sizeof *(pipe[i].data) * BUFFER_SIZE * chunks[i].size);
    
  12. Allocation test good, but needs an exit. Good to test, but then what? Suggest returning a failure code too. IMO, better to include some diagnostic info like __LINE__, __FUNC__, etc.

    if (pipe == NULL) {
        // fprintf(stderr, "malloc failed!\n");
       fprintf(stderr, "malloc failed! %d\n", __LINE__);
        // add
        return(EXIT_FAILURE);
    }
    
  13. Some systems have a multitude of special integer types. Rather than guess at their width and potentially truncate, just go for the largest supported on printing

     // fprintf(stderr, "pid %ld:\n", (long) pid);
     fprintf(stderr, "pid %lld:\n", (long long) pid);
     // or
     fprintf(stderr, "pid %jd:\n", (intmax_t) pid);
    
  14. Unclear if the result of list_split() is always chunks->pipes > 0. If it could be 0, then malloc(0) returning NULL is not a sign of OOM.

    struct str_list *chunks = list_split(cmd, buffer);
    struct pipeline *pipe = malloc(chunks->pipes * sizeof *pipe);
    // if (pipe == NULL) {
    if (pipe == NULL && chunks->pipes > 0) {
        fprintf(stderr, "malloc failed!\n");
    }
    
  15. Code is certainly wrong - passing an unknown value in reti to exec_builtin(). With that - code is broken - review done.

    int * reti; if (exec_builtin(reti, cmd)) { return 0; }

added 446 characters in body
Source Link
chux
  • 33.6k
  • 2
  • 38
  • 92
  1. Drop though cases. The code appears to be OK. Yet it looks like it might be missing a break; at the end of case '?'. Recommend to add a comment at the end of a case that intentionally lacks a break, return,etc.

         case '?':
             if (optopt == 'c')
                 fprintf(stderr, "Option -%c requires an argument.\n", optopt);
             else if (isprint (optopt))
                 fprintf(stderr, "Unknown option `-%c'.\n", optopt);
             else
                 fprintf(stderr,
                         "Unknown option character `\\x%x'.\n",
                         optopt);
    
             // Add comment
             // Drop though
    
         default: {
             return 1;
         }
    
  2. Remove debug code

             printf("sh OpenShell version 0.1(a)\n");
             printf("Version: %s\n", VERSION);
    
           //  printf ("%s / %s / %s / %s\n",
             //         program_name, version,
               //       build_date, build_git_sha);
    
  3. Obviously missing include files and other code needed before main().

     int main(int argc, char *argv[]) {
         bool donotrun = false;
         struct sigaction new_action, old_action;
         hashtable_t *hashtable = ht_create(65536);
    
  4. Inconsistent indentation. An if() without {} is tolerable, yet not preferred, yet then code breaks a line that would look fine as one. Suggest re-formating.

     // if (old_action.sa_handler != SIG_IGN)
     //     sigaction(SIGINT, &new_action, NULL);
     // ...
     // i = getopt_long(argc, argv, "pc:fvh",
     //                 options, &index);
    
     if (old_action.sa_handler != SIG_IGN) {
         sigaction(SIGINT, &new_action, NULL);
     }
     ...
     i = getopt_long(argc, argv, "pc:fvh", options, &index);
    
  5. Inconsistent formating in `switch statement concerning blank lines. This and other parts hints that OP is not using an automated formatter. Save time. Use an automated formatter. Often these are incorporated within a design environment. Stand alone ones exist. Avoid manual formatting.

            exit(EXIT_SUCCESS);
    
        }
        case 'h': {
            usage();
            exit(EXIT_SUCCESS);
    
        }
        case 'c': {
            cvalue = optarg;
            command(cvalue, hashtable, background);
            exit(EXIT_SUCCESS);
        }
    
        case 'f': {
    
  6. getPath(); is undefined, naked and uncommented. Certainly such a call could return an error, one that is not handled here.

  7. Buffer over flow potential. There is nothing special about 128. Code needs protection/re-work.

             char str[128];
             strcpy(str, line);  // Unsafe
    
  8. For array indexing use size_t.

             // int cc;
             size_t cc;
    
             cc = strlen(buf);
    
  9. Avoid a hacker exploit. buf[0] could be 0 and then buf[cc - 1] is UB. Instead test.

     if (fgets(buf, CMD_LEN - 1, fp) == NULL) {
     ...
     cc = strlen(buf);
     //if (buf[cc - 1] == '\n')
     if (cc > 0 && buf[cc - 1] == '\n')
    
  10. Simplification

    //char buffer[2]; //buffer[0] = '|'; //buffer[1] = '\0'; char buffer[] = "|";

  11. Avoid allocation by size of type. Instead by the size of the referenced variable. This post does not show struct pipeline. OTOH, with the right code, we can have less need for it. BTW, pedantic point: good to do allocation math leading with sizeof as that insures math at least at size_t width. Consider malloc(BUFFER_SIZE * chunks[i].size * sizeof *(pipe[i].data)) could overflow the BUFFER_SIZE * chunks[i].size first done in that order.

     // pipe[i].option = malloc(sizeof(int) * 10);
     pipe[i].option = malloc(sizeof *(pipe[i].option) * 10);
    
     // pipe[i].data = malloc(sizeof(char **) * BUFFER_SIZE * chunks[i].size);
     pipe[i].data = malloc(sizeof *(pipe[i].data) * BUFFER_SIZE * chunks[i].size);
    
  12. Allocation test good, but needs an exit. Good to test, but then what? Suggest returning a failure code too. IMO, better to include some diagnostic info like __LINE__, __FUNC__, etc.

    if (pipe == NULL) {
        // fprintf(stderr, "malloc failed!\n");
       fprintf(stderr, "malloc failed! %d\n", __LINE__);
        // add
        return(EXIT_FAILURE);
    }
    
  13. Some systems have a multitude of special integer types. Rather than guess at their width and potentially truncate, just go for the largest supported on printing

     // fprintf(stderr, "pid %ld:\n", (long) pid);
     fprintf(stderr, "pid %lld:\n", (long long) pid);
     // or
     fprintf(stderr, "pid %jd:\n", (intmax_t) pid);
    
  1. Drop though cases. The code appears to be OK. Yet it looks like it might be missing a break; at the end of case '?'. Recommend to add a comment at the end of a case that intentionally lacks a break, return,etc.

         case '?':
             if (optopt == 'c')
                 fprintf(stderr, "Option -%c requires an argument.\n", optopt);
             else if (isprint (optopt))
                 fprintf(stderr, "Unknown option `-%c'.\n", optopt);
             else
                 fprintf(stderr,
                         "Unknown option character `\\x%x'.\n",
                         optopt);
    
             // Add comment
             // Drop though
    
         default: {
             return 1;
         }
    
  2. Remove debug code

             printf("sh OpenShell version 0.1(a)\n");
             printf("Version: %s\n", VERSION);
    
           //  printf ("%s / %s / %s / %s\n",
             //         program_name, version,
               //       build_date, build_git_sha);
    
  3. Obviously missing include files and other code needed before main().

     int main(int argc, char *argv[]) {
         bool donotrun = false;
         struct sigaction new_action, old_action;
         hashtable_t *hashtable = ht_create(65536);
    
  4. Inconsistent indentation. An if() without {} is tolerable, yet not preferred, yet then code breaks a line that would look fine as one. Suggest re-formating.

     // if (old_action.sa_handler != SIG_IGN)
     //     sigaction(SIGINT, &new_action, NULL);
     // ...
     // i = getopt_long(argc, argv, "pc:fvh",
     //                 options, &index);
    
     if (old_action.sa_handler != SIG_IGN) {
         sigaction(SIGINT, &new_action, NULL);
     }
     ...
     i = getopt_long(argc, argv, "pc:fvh", options, &index);
    
  5. Inconsistent formating in `switch statement concerning blank lines. This and other parts hints that OP is not using an automated formatter. Save time. Use an automated formatter. Often these are incorporated within a design environment. Stand alone ones exist. Avoid manual formatting.

            exit(EXIT_SUCCESS);
    
        }
        case 'h': {
            usage();
            exit(EXIT_SUCCESS);
    
        }
        case 'c': {
            cvalue = optarg;
            command(cvalue, hashtable, background);
            exit(EXIT_SUCCESS);
        }
    
        case 'f': {
    
  6. getPath(); is undefined, naked and uncommented. Certainly such a call could return an error, one that is not handled here.

  7. Buffer over flow potential. There is nothing special about 128. Code needs protection/re-work.

             char str[128];
             strcpy(str, line);  // Unsafe
    
  8. For array indexing use size_t.

             // int cc;
             size_t cc;
    
             cc = strlen(buf);
    
  9. Avoid a hacker exploit. buf[0] could be 0 and then buf[cc - 1] is UB. Instead test.

     if (fgets(buf, CMD_LEN - 1, fp) == NULL) {
     ...
     cc = strlen(buf);
     //if (buf[cc - 1] == '\n')
     if (cc > 0 && buf[cc - 1] == '\n')
    
  10. Simplification

    //char buffer[2]; //buffer[0] = '|'; //buffer[1] = '\0'; char buffer[] = "|";

  11. Avoid allocation by size of type. Instead by the size of the referenced variable. This post does not show struct pipeline. OTOH, with the right code, we can have less need for it.

     // pipe[i].option = malloc(sizeof(int) * 10);
     pipe[i].option = malloc(sizeof *(pipe[i].option) * 10);
    
  12. Allocation test good, but needs an exit. Good to test, but then what? Suggest returning a failure code too. IMO, better to include some diagnostic info like __LINE__, __FUNC__, etc.

    if (pipe == NULL) {
        // fprintf(stderr, "malloc failed!\n");
       fprintf(stderr, "malloc failed! %d\n", __LINE__);
        // add
        return(EXIT_FAILURE);
    }
    
  13. Some systems have a multitude of special integer types. Rather than guess at their width and potentially truncate, just go for the largest supported on printing

     // fprintf(stderr, "pid %ld:\n", (long) pid);
     fprintf(stderr, "pid %lld:\n", (long long) pid);
     // or
     fprintf(stderr, "pid %jd:\n", (intmax_t) pid);
    
  1. Drop though cases. The code appears to be OK. Yet it looks like it might be missing a break; at the end of case '?'. Recommend to add a comment at the end of a case that intentionally lacks a break, return,etc.

         case '?':
             if (optopt == 'c')
                 fprintf(stderr, "Option -%c requires an argument.\n", optopt);
             else if (isprint (optopt))
                 fprintf(stderr, "Unknown option `-%c'.\n", optopt);
             else
                 fprintf(stderr,
                         "Unknown option character `\\x%x'.\n",
                         optopt);
    
             // Add comment
             // Drop though
    
         default: {
             return 1;
         }
    
  2. Remove debug code

             printf("sh OpenShell version 0.1(a)\n");
             printf("Version: %s\n", VERSION);
    
           //  printf ("%s / %s / %s / %s\n",
             //         program_name, version,
               //       build_date, build_git_sha);
    
  3. Obviously missing include files and other code needed before main().

     int main(int argc, char *argv[]) {
         bool donotrun = false;
         struct sigaction new_action, old_action;
         hashtable_t *hashtable = ht_create(65536);
    
  4. Inconsistent indentation. An if() without {} is tolerable, yet not preferred, yet then code breaks a line that would look fine as one. Suggest re-formating.

     // if (old_action.sa_handler != SIG_IGN)
     //     sigaction(SIGINT, &new_action, NULL);
     // ...
     // i = getopt_long(argc, argv, "pc:fvh",
     //                 options, &index);
    
     if (old_action.sa_handler != SIG_IGN) {
         sigaction(SIGINT, &new_action, NULL);
     }
     ...
     i = getopt_long(argc, argv, "pc:fvh", options, &index);
    
  5. Inconsistent formating in `switch statement concerning blank lines. This and other parts hints that OP is not using an automated formatter. Save time. Use an automated formatter. Often these are incorporated within a design environment. Stand alone ones exist. Avoid manual formatting.

            exit(EXIT_SUCCESS);
    
        }
        case 'h': {
            usage();
            exit(EXIT_SUCCESS);
    
        }
        case 'c': {
            cvalue = optarg;
            command(cvalue, hashtable, background);
            exit(EXIT_SUCCESS);
        }
    
        case 'f': {
    
  6. getPath(); is undefined, naked and uncommented. Certainly such a call could return an error, one that is not handled here.

  7. Buffer over flow potential. There is nothing special about 128. Code needs protection/re-work.

             char str[128];
             strcpy(str, line);  // Unsafe
    
  8. For array indexing use size_t.

             // int cc;
             size_t cc;
    
             cc = strlen(buf);
    
  9. Avoid a hacker exploit. buf[0] could be 0 and then buf[cc - 1] is UB. Instead test.

     if (fgets(buf, CMD_LEN - 1, fp) == NULL) {
     ...
     cc = strlen(buf);
     //if (buf[cc - 1] == '\n')
     if (cc > 0 && buf[cc - 1] == '\n')
    
  10. Simplification

    //char buffer[2]; //buffer[0] = '|'; //buffer[1] = '\0'; char buffer[] = "|";

  11. Avoid allocation by size of type. Instead by the size of the referenced variable. This post does not show struct pipeline. OTOH, with the right code, we can have less need for it. BTW, pedantic point: good to do allocation math leading with sizeof as that insures math at least at size_t width. Consider malloc(BUFFER_SIZE * chunks[i].size * sizeof *(pipe[i].data)) could overflow the BUFFER_SIZE * chunks[i].size first done in that order.

     // pipe[i].option = malloc(sizeof(int) * 10);
     pipe[i].option = malloc(sizeof *(pipe[i].option) * 10);
    
     // pipe[i].data = malloc(sizeof(char **) * BUFFER_SIZE * chunks[i].size);
     pipe[i].data = malloc(sizeof *(pipe[i].data) * BUFFER_SIZE * chunks[i].size);
    
  12. Allocation test good, but needs an exit. Good to test, but then what? Suggest returning a failure code too. IMO, better to include some diagnostic info like __LINE__, __FUNC__, etc.

    if (pipe == NULL) {
        // fprintf(stderr, "malloc failed!\n");
       fprintf(stderr, "malloc failed! %d\n", __LINE__);
        // add
        return(EXIT_FAILURE);
    }
    
  13. Some systems have a multitude of special integer types. Rather than guess at their width and potentially truncate, just go for the largest supported on printing

     // fprintf(stderr, "pid %ld:\n", (long) pid);
     fprintf(stderr, "pid %lld:\n", (long long) pid);
     // or
     fprintf(stderr, "pid %jd:\n", (intmax_t) pid);
    
Source Link
chux
  • 33.6k
  • 2
  • 38
  • 92

There is lots of small stuff that muddies seeing the overall flow.

My apologizes for only a primarily low-level review.

  1. Drop though cases. The code appears to be OK. Yet it looks like it might be missing a break; at the end of case '?'. Recommend to add a comment at the end of a case that intentionally lacks a break, return,etc.

         case '?':
             if (optopt == 'c')
                 fprintf(stderr, "Option -%c requires an argument.\n", optopt);
             else if (isprint (optopt))
                 fprintf(stderr, "Unknown option `-%c'.\n", optopt);
             else
                 fprintf(stderr,
                         "Unknown option character `\\x%x'.\n",
                         optopt);
    
             // Add comment
             // Drop though
    
         default: {
             return 1;
         }
    
  2. Remove debug code

             printf("sh OpenShell version 0.1(a)\n");
             printf("Version: %s\n", VERSION);
    
           //  printf ("%s / %s / %s / %s\n",
             //         program_name, version,
               //       build_date, build_git_sha);
    
  3. Obviously missing include files and other code needed before main().

     int main(int argc, char *argv[]) {
         bool donotrun = false;
         struct sigaction new_action, old_action;
         hashtable_t *hashtable = ht_create(65536);
    
  4. Inconsistent indentation. An if() without {} is tolerable, yet not preferred, yet then code breaks a line that would look fine as one. Suggest re-formating.

     // if (old_action.sa_handler != SIG_IGN)
     //     sigaction(SIGINT, &new_action, NULL);
     // ...
     // i = getopt_long(argc, argv, "pc:fvh",
     //                 options, &index);
    
     if (old_action.sa_handler != SIG_IGN) {
         sigaction(SIGINT, &new_action, NULL);
     }
     ...
     i = getopt_long(argc, argv, "pc:fvh", options, &index);
    
  5. Inconsistent formating in `switch statement concerning blank lines. This and other parts hints that OP is not using an automated formatter. Save time. Use an automated formatter. Often these are incorporated within a design environment. Stand alone ones exist. Avoid manual formatting.

            exit(EXIT_SUCCESS);
    
        }
        case 'h': {
            usage();
            exit(EXIT_SUCCESS);
    
        }
        case 'c': {
            cvalue = optarg;
            command(cvalue, hashtable, background);
            exit(EXIT_SUCCESS);
        }
    
        case 'f': {
    
  6. getPath(); is undefined, naked and uncommented. Certainly such a call could return an error, one that is not handled here.

  7. Buffer over flow potential. There is nothing special about 128. Code needs protection/re-work.

             char str[128];
             strcpy(str, line);  // Unsafe
    
  8. For array indexing use size_t.

             // int cc;
             size_t cc;
    
             cc = strlen(buf);
    
  9. Avoid a hacker exploit. buf[0] could be 0 and then buf[cc - 1] is UB. Instead test.

     if (fgets(buf, CMD_LEN - 1, fp) == NULL) {
     ...
     cc = strlen(buf);
     //if (buf[cc - 1] == '\n')
     if (cc > 0 && buf[cc - 1] == '\n')
    
  10. Simplification

    //char buffer[2]; //buffer[0] = '|'; //buffer[1] = '\0'; char buffer[] = "|";

  11. Avoid allocation by size of type. Instead by the size of the referenced variable. This post does not show struct pipeline. OTOH, with the right code, we can have less need for it.

     // pipe[i].option = malloc(sizeof(int) * 10);
     pipe[i].option = malloc(sizeof *(pipe[i].option) * 10);
    
  12. Allocation test good, but needs an exit. Good to test, but then what? Suggest returning a failure code too. IMO, better to include some diagnostic info like __LINE__, __FUNC__, etc.

    if (pipe == NULL) {
        // fprintf(stderr, "malloc failed!\n");
       fprintf(stderr, "malloc failed! %d\n", __LINE__);
        // add
        return(EXIT_FAILURE);
    }
    
  13. Some systems have a multitude of special integer types. Rather than guess at their width and potentially truncate, just go for the largest supported on printing

     // fprintf(stderr, "pid %ld:\n", (long) pid);
     fprintf(stderr, "pid %lld:\n", (long long) pid);
     // or
     fprintf(stderr, "pid %jd:\n", (intmax_t) pid);