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.

>>17

C doesn't have booleans

READ DA STANDARD