Comment by jstrieb
2 years ago
While I agree in general that shell script is not usually fun to read, this particular code is really not bad.
Not sure if this will sway you, but for what it's worth, I did read the bash script before running it, and it's actually very well-structured. Functionality is nicely broken into functions, variables are sensibly named, there are some helpful comments, there is no crazy control flow or indirection, and there is minimal use of esoteric commands. Overall this repo contains some of the most readable shell scripts I've seen.
Reflecting on what these scripts actually do, it makes sense that the code is fairly straightforward. At its core it really just wants to run one command: the one to start QEMU. All of the other code is checking out the local system for whether to set certain arguments to that one command, and maybe downloading some files if necessary.
I do see that it is better structured, but as any other bash script it relies heavily on global variables.
For example, `--delete-vm` is effectively `rm -rf $(dirname ${disk_img})`, but the function takes no arguments. It's getting the folder name from the global variable `$VMDIR`, which is set by the handling of the `--vm` option (another global variable named $VM) to `$(dirname ${disk_img})`, which in turn relies on sourcing a script named `$VM`.
First, when it works, it'll `rm -rf` the parent path of the VMs disk_img variable is set to, irrespective of whether it exists or is valid as dirname doesn't check that - it just tries to snip the end of the string. Enter an arbitrary string, and you'll `rm -rf` your current working directory as `dirname` just return ".".
Second, it does not handle relative paths. If you you pass `--vm somedir/name` with `disk_img` just set to the relative file name, it will not resolve`$VMDIR` relative to "somedir"- `dirname` will return ".", resulting in your current working directory being wiped rather than the VM directory.
Third, you're relying on the flow of global variables across several code paths in a huge bash script, not to mention global variables from a sourced bash script that could accidentally mess up quickemu's state, to protect you against even more broken rm -rf behavior. This is fragile and easily messed up by future changes.
The core functionality of just piecing together a qemu instantiation is an entirely fine and safe use of bash, and the script is well-organized for a bash script... But all the extra functionality makes this convoluted, fragile, and one bug away from rm -rf'ing your home folder.