Community code review

shikhin · **Joined:** Sat Oct 09, 2010 3:35 am **Posts:** 274

Hi,

I've got two projects running at the moment -- Draumr is my x86(-64) OS, while Tart is my ARM(v6) OS, specifically targeted for the Raspberry Pi.

I learnt C via a bunch of tutorials and books; I learnt ASM for x86 via a bunch of tutorials too. At most, I'd say my knowledge of either is weak. Now, I've never really collaborated with someone for a long time, nor have I contributed to any large scale project. Thus, I'm really the only one who has ever seen all my code.

The point is, I doubt the quality of my own code. It works, but hey -- a lot of people went down saying that line. What I'm asking for is someone from here to take a look at any one of my projects, and perhaps use comments in GitHub or reply on the forums to point out obvious downfalls of my code & style. I'm not asking for someone finding bugs or anything (though that'd be nice too) -- what I'm asking is someone look for inefficient use of the language, or bad coding styles, or... :oops:

Now, the forums are probably a bad place to ask for this. I don't even have any kind of reward or money for anyone attempting to do the above. Note that I'm a little young to earn myself and spend money on this.

Regards,
Shikhin

NOTE: The x86 OS, Draumr, is under a major refactor, as you can see from the branch name. I'd like if someone reviews it completely, but someone just reviewing the bootloader part (which is what most of the *current* OS is anyway...) would be awesome.

bluemoon · **Posted:** Sat Jan 12, 2013 10:57 am

Shikhin wrote:

Now, the forums are probably a bad place to ask for this. I don't even have any kind of reward or money for anyone attempting to do the above. Note that I'm a little young to earn myself and spend money on this.

You have good attitude

I have a quick look on the A20 code and spot a few issues:
1. you comparing 4 bytes here while you only set 1 byte:

Code:

    mov byte [es:si], 0x00            ; 0x500 now has 0x00 stored in it.
    ; Here comes the tricky bit. If A20 has been enabled, then DS:DI would point to 0x100500.
    ; Else, DS:DI would point to 0x00500, so a move over there would overwrite ES:SI.
    mov byte [ds:di], 0xFF          
    mov eax, [es:si]
    cmp eax, 0xFF                     ; Now, if ES:SI is 0xFF, then A20 isn't enabled.

2. I'm not very sure if your A20Check work if the machine is on unreal mode or under some exotic conditions, but you actually don't need A20Check.
You can just call BIOS to enable it, BIOS will handle it properly (hopefully) if it's already enabled.
If you absolutely want to check the status, there is BIOS 2402 function.

---
Due to the compact nature of boot loader, I would prefer optimization and scarify some readability, so there is less point talking about code style there, except it really confusing when I see number jump lablels.

shikhin · **Joined:** Sat Oct 09, 2010 3:35 am **Posts:** 274

Hi,

bluemoon wrote:

1. you comparing 4 bytes here while you only set 1 byte:

Code:

    mov byte [es:si], 0x00            ; 0x500 now has 0x00 stored in it.
    ; Here comes the tricky bit. If A20 has been enabled, then DS:DI would point to 0x100500.
    ; Else, DS:DI would point to 0x00500, so a move over there would overwrite ES:SI.
    mov byte [ds:di], 0xFF          
    mov eax, [es:si]
    cmp eax, 0xFF                     ; Now, if ES:SI is 0xFF, then A20 isn't enabled.

Ah, true. I'll fix that up.

bluemoon wrote:

2. I'm not very sure if your A20Check work if the machine is on unreal mode or under some exotic conditions, but you actually don't need A20Check.
You can just call BIOS to enable it, BIOS will handle it properly (hopefully) if it's already enabled.
If you absolutely want to check the status, there is BIOS 2402 function.

There are several posts on the forum describing how that'd be borked for some machine. Moreover, that BIOS function actually doesn't work on one of my testbeds -- at least it doesn't match with results by A20Check.

bluemoon wrote:

Due to the compact nature of boot loader, I would prefer optimization and scarify some readability, so there is less point talking about code style there, except it really confusing when I see number jump lablels.

I don't think I did number jump labels anywhere -- those confuse the hell out of me too.

Regards,
Shikhin

Antti · **Posted:** Sat Jan 12, 2013 11:37 am

bluemoon wrote:

I'm not very sure if your A20Check work if the machine is on unreal mode or under some exotic conditions, but you actually don't need A20Check.

I would say that the code logic is fine. Always check the A20 status "manually." Perhaps there could also be a loop for testing the status if changes are not immediate. I do not understand this unreal / exotic aspect.

shikhin · **Joined:** Sat Oct 09, 2010 3:35 am **Posts:** 274

Hi,

Antti wrote:

bluemoon wrote:

I'm not very sure if your A20Check work if the machine is on unreal mode or under some exotic conditions, but you actually don't need A20Check.

I would say that the code logic is fine. Always check the A20 status "manually." Perhaps there could also be a loop for testing the status if changes are not immediate. I do not understand this unreal / exotic aspect.

I'm not sure why the changes won't be immediate -- should be pretty immediate, and can check right there.

Regards,
Shikhin

Antti · **Posted:** Sat Jan 12, 2013 12:04 pm

Shikhin wrote:

I'm not sure why the changes won't be immediate

Maybe the loop is more like a precaution than a strictly needed feature. However, it does not hurt to have it... Of course there should be some reasonable number of tries before giving up.

Antti · **Posted:** Sat Jan 12, 2013 12:31 pm

OK, I try to contribute something more meaningful to this thread. Take this code as an example where I made some additions:

Code:

 ; Prints a message on the screen using the BIOS.
 ;     SI -> the address of the null terminated string.
Print:
    pushad
    cld                               ; Added by Antti

.PrintLoop:
    lodsb                             ; Load the value at [@es:@si] in @al.
    test al, al                       ; If AL is the terminator character, stop printing.
    je .PrintDone

    mov ah, 0x0E
    mov bx, 0x0007                    ; Added by Antti
    push si                           ; Added by Antti
    int 0x10
    pop si                            ; Added by Antti
    jmp .PrintLoop                    ; Loop till the null character not found.

.PrintDone:
    popad                             ; Pop all general purpose registers to save them.
    ret

The main idea is to make this more robust by making minimum amounts of assumptions. I do not claim you should change it this way.

dozniak · **Posted:** Sat Jan 12, 2013 6:50 pm

Purely stylistic: I think you're making too nested directory trees.

shikhin · **Joined:** Sat Oct 09, 2010 3:35 am **Posts:** 274

Hi,

Antti wrote:

OK, I try to contribute something more meaningful to this thread. Take this code as an example where I made some additions:

Code:

 ; Prints a message on the screen using the BIOS.
 ;     SI -> the address of the null terminated string.
Print:
    pushad
    cld                               ; Added by Antti

.PrintLoop:
    lodsb                             ; Load the value at [@es:@si] in @al.
    test al, al                       ; If AL is the terminator character, stop printing.
    je .PrintDone

    mov ah, 0x0E
    mov bx, 0x0007                    ; Added by Antti
    push si                           ; Added by Antti
    int 0x10
    pop si                            ; Added by Antti
    jmp .PrintLoop                    ; Loop till the null character not found.

.PrintDone:
    popad                             ; Pop all general purpose registers to save them.
    ret

The main idea is to make this more robust by making minimum amounts of assumptions. I do not claim you should change it this way.

First of all, thanks for the suggestions! I'd like to apologize, since I don't have the specs ready (this thread was mostly a creation due to my self-confidence being low). The direction flag should be guaranteed clear, over here. As for bx, Ralf Brown's list says that BL only needs to be set for graphics mode. Afair, this is used for setting the color.

As far as the push/pop of si is concerned -- I get your point, and I'll fix that for all BIOS interrupts I use (not presume it doesn't disturb needed registers).

dozniak wrote:

Purely stylistic: I think you're making too nested directory trees.

That issue (more of an issue, since it disturbs normal navigation) has been noted, and I'll be fixing it soon. You don't want to get lost in directories anyway.

As for the general purpose of the thread, it's going well. Most of the things pointed out were little things, while I was expecting something like, "You idiot, you don't even know how to code -- go learn C first." If anyone has to say something similar to that, I'm more than happy for the criticism.

Regards,
Shikhin

OSDev.org

Community code review

Who is online