Drop though cases. The code appears to be OK. Yet it looks like it might be missing a
break;
at the end ofcase '?'
. Recommend to add a comment at the end of a case that intentionally lacks abreak
,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; }
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);
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);
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);
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': {
getPath();
is undefined, naked and uncommented. Certainly such a call could return an error, one that is not handled here.Buffer over flow potential. There is nothing special about 128. Code needs protection/re-work.
char str[128]; strcpy(str, line); // Unsafe
For array indexing use
size_t
.// int cc; size_t cc; cc = strlen(buf);
Avoid a hacker exploit.
buf[0]
could be 0 and thenbuf[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')
Simplification
//char buffer[2]; //buffer[0] = '|'; //buffer[1] = '\0'; char buffer[] = "|";
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 withsizeof
as that insures math at least atsize_t
width. Considermalloc(BUFFER_SIZE * chunks[i].size * sizeof *(pipe[i].data))
could overflow theBUFFER_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);
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); }
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);
Unclear if the result of
list_split()
is alwayschunks->pipes > 0
. If it could be 0, thenmalloc(0)
returningNULL
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"); }
Code is certainly wrong - passing an unknown value in
reti
toexec_builtin()
. With that - code is broken - review done.int * reti; if (exec_builtin(reti, cmd)) { return 0; }
Drop though cases. The code appears to be OK. Yet it looks like it might be missing a
break;
at the end ofcase '?'
. Recommend to add a comment at the end of a case that intentionally lacks abreak
,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; }
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);
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);
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);
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': {
getPath();
is undefined, naked and uncommented. Certainly such a call could return an error, one that is not handled here.Buffer over flow potential. There is nothing special about 128. Code needs protection/re-work.
char str[128]; strcpy(str, line); // Unsafe
For array indexing use
size_t
.// int cc; size_t cc; cc = strlen(buf);
Avoid a hacker exploit.
buf[0]
could be 0 and thenbuf[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')
Simplification
//char buffer[2]; //buffer[0] = '|'; //buffer[1] = '\0'; char buffer[] = "|";
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 withsizeof
as that insures math at least atsize_t
width. Considermalloc(BUFFER_SIZE * chunks[i].size * sizeof *(pipe[i].data))
could overflow theBUFFER_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);
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); }
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);
Drop though cases. The code appears to be OK. Yet it looks like it might be missing a
break;
at the end ofcase '?'
. Recommend to add a comment at the end of a case that intentionally lacks abreak
,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; }
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);
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);
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);
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': {
getPath();
is undefined, naked and uncommented. Certainly such a call could return an error, one that is not handled here.Buffer over flow potential. There is nothing special about 128. Code needs protection/re-work.
char str[128]; strcpy(str, line); // Unsafe
For array indexing use
size_t
.// int cc; size_t cc; cc = strlen(buf);
Avoid a hacker exploit.
buf[0]
could be 0 and thenbuf[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')
Simplification
//char buffer[2]; //buffer[0] = '|'; //buffer[1] = '\0'; char buffer[] = "|";
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 withsizeof
as that insures math at least atsize_t
width. Considermalloc(BUFFER_SIZE * chunks[i].size * sizeof *(pipe[i].data))
could overflow theBUFFER_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);
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); }
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);
Unclear if the result of
list_split()
is alwayschunks->pipes > 0
. If it could be 0, thenmalloc(0)
returningNULL
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"); }
Code is certainly wrong - passing an unknown value in
reti
toexec_builtin()
. With that - code is broken - review done.int * reti; if (exec_builtin(reti, cmd)) { return 0; }
Drop though cases. The code appears to be OK. Yet it looks like it might be missing a
break;
at the end ofcase '?'
. Recommend to add a comment at the end of a case that intentionally lacks abreak
,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; }
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);
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);
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);
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': {
getPath();
is undefined, naked and uncommented. Certainly such a call could return an error, one that is not handled here.Buffer over flow potential. There is nothing special about 128. Code needs protection/re-work.
char str[128]; strcpy(str, line); // Unsafe
For array indexing use
size_t
.// int cc; size_t cc; cc = strlen(buf);
Avoid a hacker exploit.
buf[0]
could be 0 and thenbuf[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')
Simplification
//char buffer[2]; //buffer[0] = '|'; //buffer[1] = '\0'; char buffer[] = "|";
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 withsizeof
as that insures math at least atsize_t
width. Considermalloc(BUFFER_SIZE * chunks[i].size * sizeof *(pipe[i].data))
could overflow theBUFFER_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);
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); }
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);
Drop though cases. The code appears to be OK. Yet it looks like it might be missing a
break;
at the end ofcase '?'
. Recommend to add a comment at the end of a case that intentionally lacks abreak
,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; }
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);
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);
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);
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': {
getPath();
is undefined, naked and uncommented. Certainly such a call could return an error, one that is not handled here.Buffer over flow potential. There is nothing special about 128. Code needs protection/re-work.
char str[128]; strcpy(str, line); // Unsafe
For array indexing use
size_t
.// int cc; size_t cc; cc = strlen(buf);
Avoid a hacker exploit.
buf[0]
could be 0 and thenbuf[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')
Simplification
//char buffer[2]; //buffer[0] = '|'; //buffer[1] = '\0'; char buffer[] = "|";
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);
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); }
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);
Drop though cases. The code appears to be OK. Yet it looks like it might be missing a
break;
at the end ofcase '?'
. Recommend to add a comment at the end of a case that intentionally lacks abreak
,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; }
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);
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);
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);
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': {
getPath();
is undefined, naked and uncommented. Certainly such a call could return an error, one that is not handled here.Buffer over flow potential. There is nothing special about 128. Code needs protection/re-work.
char str[128]; strcpy(str, line); // Unsafe
For array indexing use
size_t
.// int cc; size_t cc; cc = strlen(buf);
Avoid a hacker exploit.
buf[0]
could be 0 and thenbuf[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')
Simplification
//char buffer[2]; //buffer[0] = '|'; //buffer[1] = '\0'; char buffer[] = "|";
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 withsizeof
as that insures math at least atsize_t
width. Considermalloc(BUFFER_SIZE * chunks[i].size * sizeof *(pipe[i].data))
could overflow theBUFFER_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);
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); }
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);
There is lots of small stuff that muddies seeing the overall flow.
My apologizes for only a primarily low-level review.
Drop though cases. The code appears to be OK. Yet it looks like it might be missing a
break;
at the end ofcase '?'
. Recommend to add a comment at the end of a case that intentionally lacks abreak
,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; }
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);
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);
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);
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': {
getPath();
is undefined, naked and uncommented. Certainly such a call could return an error, one that is not handled here.Buffer over flow potential. There is nothing special about 128. Code needs protection/re-work.
char str[128]; strcpy(str, line); // Unsafe
For array indexing use
size_t
.// int cc; size_t cc; cc = strlen(buf);
Avoid a hacker exploit.
buf[0]
could be 0 and thenbuf[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')
Simplification
//char buffer[2]; //buffer[0] = '|'; //buffer[1] = '\0'; char buffer[] = "|";
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);
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); }
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);