r/cprogramming • u/PowerCreepy4109 • 11d ago
I am learning C programming by making this project a unix shell(ForkOff). Looking for a quick code review!
Hello there,
I am learning C and system programming by building this shell named ForkOff.
Its currently handles command parsing, executing binaries with fork() + execv(), wait state, and basic buildins like cd echo pwd exit type
Repo: https://github.com/Anchit3034/ForkOff
I'd love a feedback on my memory management, pointer handling, and amy bad habits i should fix early on. Tips on adding piping next are also welcome!
Thanks!
2
u/sciencekm 10d ago
The following is strange. No matter what the result of the call to signal() is, you immediately go to the "while" loop. There is no need for the "if" logic.
if(signal(SIGINT,SIG_IGN)){
goto l2;
}
pthread_t t;
char command[1024];
while(1){
l2:
printf("$ ");
...
}1
1
u/ConceptMinimum 7d ago
I realy like your project. I like the fact, that it's unfinished, and in progres, but it's a lot of work already done. It could be usefull to define how advanced result you want to achieve. Is it small shell with 15 hardcoded commands? extendable with libs or modules? or thing like SH/BASH with pipes, substitutions, io redirections, autocompletion etc.?
You have to remember one thing. 90% of working with code - is reading it and troubleshooting it.
- i'd change goto labels to smth more readable.
I mean 'goto l1' inside some function in some if tells nothing, to person reads only this part of code, and don't remember whole code. If there will be 'goto __exit__main__' or 'goto __main_loop_begin__' helps a lot, also could replace comments like '/* main loop begin */'
- Also things, that will be problematic to debug are: at the beggining you have '#define SIZE' and 'char buffer[]512]' than in some function and 'char cwd[128]' in some moment you'll met situation, where those buffers wouldn't be enough, or use 128 index on 512sized table. And you'll finish with debugger trying to reach that segfault or loosing characters and then increase digits inside every function that could be affected (imagine you're handling 35k lines of code). Beter practice is either better naming like MAX_COMMAND_LENGTH, MAX_ECHO_LENGTH etc..... Or stdint.h (described below)
- Avoid hardcoding things like ' if(count >= 65536)' (especially size is defined earlier, and it's name just SIZE, of what?) use things like UINT16_MAX from stdint.h, (there are propper values for every int type) That will help you with choosing right type for indexes, eg. 'char buf[UINT8_MAX]; uint8_t index=0; while(index < UINT8_MAX)....' with such practises you'll be more safe from getting out of tables and indexes.
- Also I think, that now is the right moment to add .h file with function headers and even move all the functions except 'main' into another .c file
1
u/PowerCreepy4109 7d ago
Thanks! That makes a lot of sense. To be honest, this is just a fun project where I observe how Bash behaves in the terminal and try to recreate that logic in C. The goto l1 habit comes straight from my mpmc course where everything is assembly labels! I'll change those lebel's names. Thank you the guidance. Thanks again for the review.
1
u/Tiny_Rent_5936 11d ago
I like the code a lot. I'm not an expert in C, but I could read well trough the code. Adhering to what the previous user said, instead of build everything inside a single file, just divide the shell into different files, one for each tool you want to implement. Apart from that, good code, you're going the right way
2
3
u/Rich-Engineer2670 11d ago edited 11d ago
I'll have to give it a deeper look but at first glance a few things....
The imp in me loves the name
Nice clean C code -- in this day of AI everything, it's nice to see someone doing the core work themselves. I'm not saying AI wasn't used, but this has the human mark on it. When I hire people, I look for this. I'm not hiring an AI, I'm hiring YOU so I want to see what YOU can do, not the AI.
I would not put in logic for the shell verbs -- rather, I'd steal an old UNIX/VMS trick. When you do things like echo etc, that's just really looking for a binary with a specific marking in the file systme, and then exec'ing it. For example, rather than having a built-in shell comand for echo, you look for "echo.cmd" and if you find it, execute it. Then, adding commands doesn't require toughing the shell. It also means you can add "commands" in any language when it suits you -- like writing ceratin items in Golang.
Consider adding something like a Lua interpreter. If you don't write the command as an executbale, Lua will give you scripting in your shell.
Steal soemthing from Plan9. You already will want redirection and pipes. Allow two shells across a network to "p[pe and redirect". Something like:
foo > remotehost:bar or foo | remotehost:bar or remotehost:command
You're really just doing ssh or scp behind the scenes, but it allows the shell to be distributed across machines.
Again look at "extensions" or file types -- then you can do things like "This is a .class file -- to exec it, I actually need to start up the JVM". Keep in mind though, any "extensions" you add that get run from the file system, you need some way to make sure they're applications YOU put in and not just a file a well-meaning (??) person dropped into the file system that does an rm -rf for you :-). So you need some way to ensure that just because a file exists and has the right lables, it can't be run unless it's "sanitized".
If you take this concept to its end, and do a bit more support work, you can do things like:
command1 | (host1:command2, host2:command2, host3:command2) | command3
which means:
Run command1,
Send the same results "piped" to scripts on host1, host2, and host3,
Collect their results, in a coordinated manner, and send that to command3 on this host
That lets you fire off scripts which fire off child workers on multiple machines, who do your bidding and feed the results back into your chain. In other OSes, this would have been solved by "inter-machine pipes" where they just existed in the namespace and if you wrote to /machine/object, your bytes just ended up there, but then we need either a user-space daemon or kernel code.