← Back to context

Comment by arghwhat

2 years ago

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.