current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 6 down vote favorite

This is a script I made for testing a simple operating system.

It assembles the source and creates a boot image, then It automates the configuration of a Virtual Box machine. 

#!/bin/bash

OS_NAME="hello"
MEMORY_SIZE=8

build_main () {
    # remove old files
    rm -f $OS_NAME.bin
    rm -f $OS_NAME.flp
    rm -f $OS_NAME.iso

    # assemble OS
    nasm -f bin -o $OS_NAME.bin $OS_NAME.asm
    if [[ ! -f $OS_NAME.bin ]]
    then
        exit
    fi

    dd if=/dev/zero of=$OS_NAME.flp bs=512 count=2880
    dd conv=notrunc if=$OS_NAME.bin of=$OS_NAME.flp
    #  make CD image
    mkisofs -o $OS_NAME.iso -b $OS_NAME.flp .

    VBoxManage unregistervm "$OS_NAME" --delete
    rm -f $OS_NAME.vdi

    VBoxManage createvm --name "$OS_NAME" --register

    # settings
    VBoxManage modifyvm "$OS_NAME" --memory $MEMORY_SIZE --acpi on --$OS_NAME1 dvd
    VBoxManage modifyvm "$OS_NAME" --ostype other
    VBoxManage modifyvm "$OS_NAME" --cpuexecutioncap 90
    VBoxManage modifyvm "$OS_NAME" --firmware bios

    VBoxManage createvdi -filename "$OS_NAME.vdi" --size 8

    VBoxManage storagectl "$OS_NAME" --name "IDE Controller" --add ide
    VBoxManage storageattach "$OS_NAME" --storagectl "IDE Controller"  --port 0 --device 0 --type hdd --medium ./$OS_NAME.vdi
    VBoxManage storageattach "$OS_NAME" --storagectl "IDE Controller" --port 1 --device 0 --type dvddrive --medium ./$OS_NAME.iso

    # start the machine
    open -a VirtualBox
    VBoxManage startvm "$OS_NAME"
}

build_main

Here is a simple operating system to use for testing, hello.asm

    BITS 16

start:
    mov ax, 07C0h       ; Set up 4K stack space after this bootloader
    add ax, 288     ; (4096 + 512) / 16 bytes per paragraph
    mov ss, ax
    mov sp, 4096

    mov ax, 07C0h       ; Set data segment to where we are loaded
    mov ds, ax


    mov si, text_string ; Put string position into SI
    call print_string   ; Call our string-printing routine

    jmp $           ; Jump here - infinite loop!


    text_string db 'hello world', 0


print_string:           ; Routine: output string in SI to screen
    mov ah, 0Eh     ; int 10h 'print char' function

.repeat:
    lodsb           ; Get character from string
    cmp al, 0
    je .done        ; If char is zero, end of string
    int 10h         ; Otherwise, print it
    jmp .repeat

.done:
    ret


    times 510-($-$$) db 0   ; Pad remainder of boot sector with 0s
    dw 0xAA55       ; The standard PC boot signature
share|improve this question

2 Answers 2

up vote 3 down vote accepted

Overall it's a pretty nice script. I can mostly nitpick, with a few exceptions. Here we go, from top to bottom.

It might be a good idea to set the -e flag to make the script stop running when something unexpected goes wrong, like this:

#!/bin/bash -e

If a string doesn't contain special characters, then the quoting is redundant:

OS_NAME=hello    # short and sweet
OS_NAME="hello"  # quotes are redundant

It might be a good idea to rename MEMORY_SIZE to indicate the units, for example:

MEMORY_SIZE_MB=8

Quoting: although in the beginning I removed the quotes from OS_NAME="hello", if somebody fancies to use a name with spaces, it will be nice if the script can support it. So when using the variable $OS_NAME, it's probably good to use quotes everywhere, for example: 

rm -f "$OS_NAME.bin"
rm -f "$OS_NAME.flp"
rm -f "$OS_NAME.iso"

I don't have much experience with nasm, but probably you can simplify this:

nasm -f bin -o $OS_NAME.bin $OS_NAME.asm
if [[ ! -f $OS_NAME.bin ]]
then
    exit
fi

like this:

nasm -f bin -o "$OS_NAME.bin" "$OS_NAME.asm" || exit 1

Aside from shortening, I quoted $OS_NAME and used exit 1 to explicitly indicate that this is a failure.

dd if=/dev/zero of=$OS_NAME.flp bs=512 count=2880
dd conv=notrunc if=$OS_NAME.bin of=$OS_NAME.flp

Here too, remember to quote $OS_NAME. And the benefit of settings #!/bin/bash -e at the beginning is that if any of these fail unexpectedly, the script will halt immediately, which can be cleaner and easier to debug.

#  make CD image
mkisofs -o $OS_NAME.iso -b $OS_NAME.flp .

The comment is redundant, I think you can drop it. And of course quote $OS_NAME.

But the biggest concern here is that the ISO image will contain everything that was in the current directory, and the script doesn't do anything to guarantee that the directory contains only the necessary files. To avoid junk ending up in your ISO, I'd recommend using a freshly created temporary directory.

VBoxManage modifyvm "$OS_NAME" --memory $MEMORY_SIZE --acpi on --$OS_NAME1 dvd

The --$OS_NAME1 parameter is a typo? I'm not sure what's this supposed to be, and what should happen if the variable contains spaces.

# start the machine
open -a VirtualBox
VBoxManage startvm "$OS_NAME"

Again, redundant comment.

Judging by the open command, I guess your are on a Mac. It would be better if the script was portable, so that Linux users could benefit from it too.

share|improve this answer
up vote 1 down vote

In addition to a great answer from @janos, I would suggest to replace /bin/bash with /bin/sh in the shebang and [[ with [ in if statement. Your script looks portable between shells, why limit it to bash? In fact, bash isn't that fast when it comes to startup time, and replacing it with a smaller shell can give you a speed boost.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.