Hi all!

It has been a long time since I posted, I have been working on a rewrite of NOS!
I would like for some peer review and feedback on the code I have written. it is not ready to compile by any means, but I would love to hear feedback on the code I have written.

The repo is located at https://www.github.com/nkeck720/nos.

Cheers![/url]

Stupid post removed

Don't feel bad. We are all busy with our own things. Perhaps when someone has some time they can take a look.

I took a quick look just now and I do have one comment for something I noticed immediately. Magic constants. There are many places in your code where you use raw numbers. fasm and fasmarm have this problem also, so I am not criticising just you on this point. Perhaps you can consider making named assignments and/or structure names for some of the most used numbers. Especially for numbers that are not defined in some spec or other unchanging situation and thus could be changed to another value. E.g. there are seven days in a week and this is not likely to change, ever, so making a hard coded number for that is not so bad. But other things that are mainly arbitrary, like a segment at 0x2000, could probably benefit from being a named constant.

Thanks, will take note. I can understand others being busy, I know I am sometimes.

nkeck72,

I did a complete review of your NOS software.
It got my attention because you seem to be willing to think out of the box, meaning that you have made some rather unusual development choices. One can only respect that and I am very eager to see where it will lead you...

++++++++++++++++++++++++++++++++++++++++++++++++

BOOTLOAD.ASM
------------
(1. When using "ORG 7C00h" in a bootloader we indicate that the offsets used in the program shall be relative to linear address 0.
This also means that you are required to initialize the DS segment register to zero in stead of the 07C0h value that you used. Luckily your current bootloader never depends on this mistake and so everything seems OK.

(2. You initialized the SS segment register with the value 009Ch, but from your memory layout I learn that you want your 4KB stack at 9C00h:0000h. Only you can know what it needs to be.

(3. Directly after you have set up video mode 3, the cursor will be in the upper left corner. You don't have to explicitly put it there. Moreover the function that sets the cursor uses the BH register to represent the display page for which you want to set the cursor position. You omitted this value and thus it's possible that BIOS used a page different from what you need which is the 0 display page.

(4. You store the bootdrive code on the stack, but you ignore its value when it comes to resetting the disk system, loading the FSB, and loading the kernel. For those operations you persistently use code 0 which is the first floppy drive. Then why store it?

(5. When you get a "RST.ERR" you display it alright but then you permit the CPU to execute whatever bytes that happen to follow. This could potentially harm your computer! You should always crash in an orderly manner by the use of an endless loop. The other 2 errors solve it with a jump to the "STOP" label.

Tip: Since all 3 error messages end with the same 4 characters " ERR" you could have 2 of them just jump to the tail of the 1st one and save a lot of bytes.

(6. Why do you set video mode 3 again after a succesful disk reset? The screen was already set and is still empty.

(7. The BIOS teletype function 0Eh uses the BH register to represent the display page you want the text to be put on. You don't set it up but luckily as it is the BX register is clear.

KERNEL.ASM
----------
(8. The instruction "mov [boot_drv], dl" will fail because at this point the DS register holds the value 07C0h!
The file "KERNEL.ASM" is compiled by FASM using an implicit "ORG 0". This is fine, but since the loader has put this program at the segmented address 1000h:0000h you still have to initialize the DS segment register to 1000h.
Alternatively write the following:


(9. The "mbr_clear" loop will clear the same byte 512 times! You must increment the BX register within the loop.

(10. The label "system_error_preapi" does not exist in the file "KERNEL.ASM". Also it's a bit of paranoia to test if push/pop work!

(11. The labels "api_load_error" and "disk_error" don't exist in the file "KERNEL.ASM".

(12. Just before the "get_api" label you redundantly clear out ES.

(13. At the label "get_api" you disable the interrupts much too soon. You only need this around the 2 instructions that effectively write in the IVT.

(14. Because the "c1" loop will push the 5 characters of the text "INT21" starting at the last character you need to set the SI register at 17.

(15. After the filename is verified, you correctly pop the CHSL values but much of the comments you wrote here are wrong!

(16. You've loaded the API at segmented address 8000h:0000h. This means that in the IVT the offset 0000h is stored at the address [es:21h*4] and that the segment 8000h is stored at the address [es:21h*4+2]. You've erroneously done it the other way round!

(17. The comment ";; 0xAA00FF55 #<filename1>* #<filename2>* ... 0x0D" is confusing. The mention 0xAA00FF55 signals a dword where your intent is to have 4 bytes 0xAA, 0x00, 0xFF, 0x55. You can still write it as a dword provided you use little-endian notation: 0x55FF00AA.

(18. The "pop_off_loop" loop will place every character at the same address. You must decrement BX right before the loop instruction. But even then you'll end up with the first character of the filename at address 0FFF1h in stead of 0FFF0h.
Additionally the terminating zero won't be correctly placed because by incrementing BH you add 256 to the address!
This snippet will resolve it:


(19. In order to process a second driver you preserve the ES and CX registers. ES is fine but CX has a known value of 0 at this point and more importantly you don't need it to remain. But you do need the SI register to be preserved and incremented because it still points at the "*" asterisk character. For correct counting you also need to reset the BH counter.


(20. The loop that clears the 6000h segment sadly does not clear the byte at address 0, (the byte that has 0xAA). To clear an entire 64KB segment you need to initialize the CX register with 0 in stead of 0FFFFh.

(21. Beware that at the point where you start processing the commandline the ES segment register still holds the value 6000h and not the 1000h that you might think.

(22. At the label "external_command", where you say you reload the FSB file (to make sure it is up to date) you erroneously load the bootsector since you specify CL=1. Needs to be CL=2.

(23. A bit further down when you want to load the file for the external command you specify 4000h:0000h as the address of the filename, but this filename is at address 9000h:0000h since it comes from the commandline.

(24. At the label "bad_prog_file" the stack still contains a value that needs popping. You'll end up filling the stack with garbage.

(25. At the label "remove_footer_flat" you wrote "mov [es:bx], ret_opcode". This can only work if you change the definition for ret_opcode using an EQU:


(26. When a flat program is present at 4000h:0000h you change the first 4 bytes from 0x46, 0x45, 0x46, 0xFF to 0x46, 0xC3, 0x00, 0x00.
When you later invoke such a flat program using "call 4000h:0001h" you hope to return immediately (I hope this behaviour is temporal) but since you called it using a FAR CALL you must also return through a FAR RET. Just change the definition for ret_opcode:


(27. Contrary to your comments, you forget to setup the stack for this user program.


(28. The user program will certainly have destroyed your DS segment register, so first make it point at the kernel:


(29. The loop at "clear_code_flat" will only zero the whole 65536 bytes if you initalize CX=0 in stead of cx=0FFFFh.
Also you commented "ES is already set" but the user program will have destroyed it!

(30. At the label "no_flat_footer" the jump to "clear_code_flat" will fail because you forget to setup the CX register. Additionally in this case "old_ss" and "old_sp" will not have correct values.

(31. The "run_segmented_prog" is very much unfinished business, and so I didn't review it properly.

INT21.ASM
---------
(32. Under BIOS the carriage return (13) brings the cursor to the leftside of the current line and the linefeed (10) brings the cursor one line down but in the same column. So to obtain the newline (\n) that you are accustomed to from LINUX you need to output both these numbers with the BIOS teletype function. I strongly advise you to change the single 13 into the pair 13,10 everywhere you defined a text for displaying.
Your API function 1 that writes a string to the screen has a lot of issues. I wrote this version to remedie all its problems at once:


(33. To have the carry flag set (or cleared) when returning from an interrupt you can't just write:

The IRET instruction overwrites the flags with the copy that was pushed on the stack when the interrupt started!
Use any of the following:


(34. FASM can't use an instruction like "mov ah, byte ptr [9000h:0FFFFh]". You must transfer the segment part to a segment register first:

Please note that this memory address most probably belongs to the BIOS. It has its Extended BIOS Data Area (EBDA) here. You can use BIOS function C1h on int 15h to retrieve the start of the EBDA and then use the byte immediately below this area for your purposes.

FSB.ASM
-------
(35. The info in the FSB file is wrong. It says that the kernel starts at sector 3 and has a length of 2 sectors. This means it occupies the sectors 3 and 4. But then looking at the CHSL info for the INT21 file I see that it starts at sector 4 and also has a length of 2 sectors! Both files can't be in the same sector. You should change the sector component of the position of INT21 to a 5 and you should also not forget to account for the extra space in the number of free blocks:
2876(2880) --> 2874(2880)

++++++++++++++++++++++++++++++++++++++++++++++++

At the conclusion of this extensive review I can only hope that the considerable number of issues that I found doesn't discourage you too much. It would be a waste. Not only of the time I invested in reviewing, but especially because I think that your project has great potential. Therefore I urge you to address all of these issues one by one. You can only get better.

Awesome review.

SeproMan - as revolution said - great review, I admire your attitude. Since the code is on the github already I suggest to use github issues tracker further as it will be easier to track and fix reported problems.

nkeck72 I suggest to either use Continuous Integration approach or test the code before you do a push request from your local repository. The problem with most assemblers is that there are very little possible warning during compilation so usually you can't use those to track down bugs, but on the other hand errors (just like in case of any hll compile) end up in breaking assembly process resulting in no executable. If you can't compile the code you push it is the most basic sign that there is something wrong with your source.

Sorry guys if I'm boring you with this basic stuff - those are only my 2 cents...

Wow, amazing review Serpo! I will get to fixing these issues when I get the time. I'm sure that review took some time, I appreciate it man! Regarding issue #26, (#27 on GitHub) I wanted to change the *end* of the program file to those four bytes. Does my code change the beginning instead?

ACP - I would test it but without a complete kernel I cannot yet compile. Right now GitHub is my only "backup" of my code until I get a real backup plan going, so there are a lot of commits with incomplete code. Probably not the best practice but I'm trying to use what I've got, ya know?

I will indeed start using the GitHub tracker, and will put up the reported issues and check them off as they get fixed. Seems a bit easier in terms of notifications (FASM board has decided to stop sending me emails about when someone responds to my threads, even when I tell it to send me emails :/).