Former professional software dev here.

https://www.reddit.com/r/programming/comments/5r0f7e/toaruos_10_a_hobby_operating_system/dd3rlwe/

First of all, good job. I wouldn't know how to build my own OS, or even where to start. I could learn much from you.

Second, I opened up kernel/main.c expecting the code cleanliness to at least meet my expectations, and was disappointed. I don't want to be a downer here, because I think you're exceptionally talented --- and I mean exceptionally --- however, there are a few professional tips you could learn. You don't have to listen to me, of course; however, I learned these long ago and they've been an immense help. Finally, I realize and respect that this is your personal project, but it may not be someday.

I'll work my way down through kernel/main.c. The top comment block is good, but could explain the file's purpose a bit.

uintptr_t initial_esp = 0;

(1) I have no idea what "initial_esp" is or why it is initialized to zero. Is its value a literal for a reason? Should it be INITIAL_ESP_VALUE or something, so I don't have to somehow (it can take days!) find and replace 40 magic zeros if the requirements change?

fs_node_t * ramdisk_mount(uintptr_t, size_t);

(2) I believe I understand what you're doing here, but an affirming comment would be helpful.

#define EARLY_LOG_DEVICE 0x3F8

(3) Good job not repeating 0x3F8 throughout your code.

struct pack_header { ... }

(4) What is this struct for? And is there a reason the size of the head[] array is a literal number 4 instead of a #define?

mboot_mag

(5) What's a "mag"?

ENABLE_EARLY_BOOT_LOG(0);

(6) I suggest changing this to ENABLE_EARLY_BOOT_LOG(LOG_LEVEL_WARN); or something like that.

"Didn't boot with multiboot, not sure how we got here."

(7) It would be cleaner if all of your literal strings were neatly organized in a central location.

/* Initialize core modules */

(8) Nicely organized. Great job.

mboot_ptr->flags & (1 << 3)

(9) Definitely should be broken out into its own function, like flag_something_is_set() for example

debug_print(NOTICE, "There %s %d module%s starting at 0x%x.", mboot_ptr->mods_count == 1 ? "is" : "are", mboot_ptr->mods_count, mboot_ptr->mods_count == 1 ? "" : "s", mboot_ptr->mods_addr);

(10) Inline ternaries really slow down someone who's trying to scan your code quickly. If it's not performance-critical, definitely break them out into helper functions.

(mboot_ptr->mods_count > 0)

(11) Literal zero here is fine, since I'm assuming that it means a count of zero. My preference would be NO_MODULES, but a zero is fine. Also, don't be afraid to actually say "modules" instead of "mod" when you aren't using an interpreted language.

(uintptr_t)mod + sizeof(mboot_mod_t) > last_mod

(12) This could be a lot more clear. How about a function that returns a boolean instead?

if (last_mod < module_end) { ... }

(13) What is happening here? I need to be able to figure this out quicker.

if (mmap->type == 2) { ... }

(14) The literals in this block need clarification: 2, 0x1000, 0xFFFFFFFF, 0xFFFFF000

char cmdline_[1024];

(15) How about DEFAULT_CMDLINE_BUF_SIZE or something?

check_result == 2

(16) What does 2 mean?

void * start = (void *)((uintptr_t)pack_header + 4096);

(17) Again, 4096 could be a descriptive #define

(result != 1)

(18) *Maybe (!result_valid()) instead?"

map_vfs_directory("/dev");

(19) Replacing "/dev" with something like DEVICE_DIRECTORY_NAME (or whatever) would be much safer.

(1) I have no idea what "initial_esp" is or why it is initialized to zero. Is its value a literal for a reason? Should it be INITIAL_ESP_VALUE or something, so I don't have to somehow (it can take days!) find and replace 40 magic zeros if the requirements change?

I'd imagine it's the initial value to be loaded into the ESP register on x86 (which is the 32-bit stack pointer, IIRC). Though depending on whether that variable itself is actually modified later on, they might be better off declaring it as a constant.

(3) Good job not repeating 0x3F8 throughout your code.

Unless they're targeting very old systems, it would probably be better to go with a const than a define.

(10) Inline ternaries really slow down someone who's trying to scan your code quickly. If it's not performance-critical, definitely break them out into helper functions.

Functions add overhead, but appropriate use of whitespace would help make these things more readable.