Name: Anonymous 2017-01-30 23:39
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.
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.