From 221132b03b2f1ed9bfe9fe1d13e2e66430f5c7cd Mon Sep 17 00:00:00 2001
From: Ian Kelling <iank@fsf.org>
Date: Wed, 19 Mar 2025 10:14:58 -0400
Subject: [PATCH] lots of refactor and fixing

---
 .../files/simple/usr/local/bin/attach-vm-disk |  16 +-
 .../files/simple/usr/local/bin/create-vm      | 423 ++++++++++--------
 .../files/simple/usr/local/bin/savannah-virsh |   4 +
 .../simple/usr/local/lib/fsf-virsh-bash-lib   |  29 +-
 4 files changed, 265 insertions(+), 207 deletions(-)

diff --git a/roles/kvmhost/files/simple/usr/local/bin/attach-vm-disk b/roles/kvmhost/files/simple/usr/local/bin/attach-vm-disk
index 6e58bc1..2288ad1 100755
--- a/roles/kvmhost/files/simple/usr/local/bin/attach-vm-disk
+++ b/roles/kvmhost/files/simple/usr/local/bin/attach-vm-disk
@@ -20,14 +20,14 @@ One line description
 -h|--help  Print help and exit.
 
 
-For ceph disks, the path is the name attribute in its xml:
+For ceph disks, SOURCE_DEV_PATH is the name attribute in its xml:
 <source protocol='rbd' name='rbd/frontend2.savannah.gnu.org'>
 
 For non-ceph disks, the path is the dev attribute in its vm xml:
  <source dev='/dev/mapper/crypt-nfs2.savannah.gnu.org' index='4'/>
 
-The SOURCE_DEV_PATH has to be attached to SOURCE_VM_NAME. This is just so we can
-filter SOURCE_VM_NAME as a simple access control.
+The above xml must already exist for SOURCE_VM_NAME. This is so we
+can use access control to VMs.
 
 
 Note: Uses util-linux getopt option parsing: spaces between args and
@@ -51,7 +51,7 @@ check-source-dev() {
 
   found_source=false
   for attached_dev in "${attached_devs[@]}"; do
-    if [[ $source_dev == "$attached_dev" ]]; then
+    if [[ $source_dev_path == "$attached_dev" ]]; then
       found_source=true
       break
     fi
@@ -80,7 +80,7 @@ while true; do
   esac
   shift
 done
-read -r source_vm source_dev target_vm <<<"$@"
+read -r source_vm source_dev_path host <<<"$@"
 
 ##### end command line parsing ########
 
@@ -93,13 +93,13 @@ fi
 
 check-source-dev
 
-get-attached-devs $target_vm
+get-attached-devs $host
 
 disk_letters=( {a..z} )
 letter=${disk_letters[${#attached_devs[@]}]}
 
 if $ceph; then
-  add-ceph-disk $source_dev
+  attach-ceph-disk $source_dev_path
 else
-  add-local-disk $source_dev
+  attach-local-disk $source_dev_path
 fi
diff --git a/roles/kvmhost/files/simple/usr/local/bin/create-vm b/roles/kvmhost/files/simple/usr/local/bin/create-vm
index 3093408..63ea8af 100755
--- a/roles/kvmhost/files/simple/usr/local/bin/create-vm
+++ b/roles/kvmhost/files/simple/usr/local/bin/create-vm
@@ -21,6 +21,7 @@
 #
 
 set -e; . /usr/local/lib/bash-bear; set +e
+# shellcheck disable=SC1091
 # shellcheck source=../lib/fsf-virsh-bash-lib
 source /usr/local/lib/fsf-virsh-bash-lib
 
@@ -85,6 +86,10 @@ $0 [OPTONS] -d DISK_SIZE(MB) DISK_NAME/MOUNT_POINT FQDN
 or
 $0 -l
 
+DISTRO_RELEASE_CODENAME: Supported codenames are aramo (recommended),
+nabia, etiona, flidas, belenos and stretch. Codenames older than the
+recommended are more likely to expose a bug in this script.
+
 On vm hosts with local storage, disks are grouped into sets of 3 disks, in which a
 per-vm raid arrays are created by creating lvm logical volumes and then
 creating an mdadm or btrfs array. While the disks themselves are not
@@ -126,9 +131,6 @@ to undo what it has done. Be sure to read the output!
 
 Example: $0 www.gnu.org 5000 512 2 flidas
 
-Supported codenames are aramo (recommended), nabia, etiona, flidas,
-belenos and stretch. Codenames older than the recommended are more
-likely to expose a bug in this script.
 
 Example of adding a disk to the above vm:
 
@@ -168,7 +170,7 @@ cli-arg-check() {
   fi
 
   if $add_disk; then
-    if ! virsh list --name | grep -Fx $host &>/dev/null && mountpoint -q /mnt/$host; then
+    if ! vm-on && mountpoint -q /mnt/$host; then
       err "error: unmount the the vm from /mnt/$host first"
       exit 1
     fi
@@ -352,52 +354,43 @@ pop-cleanup() {
   unset "cleanup_cmds[-1]"
 }
 
-ceph-disk() {
-  pool=rbd
-  keyfile=/dev/shm/keyfile
+vm-on() {
+  virsh list --name | grep -Fx $host &>/dev/null
+}
 
-  # If adding disk, we reuse the same key
-  if $add_disk; then
-    read -r -p "Enter the VM's luks key so I can cryptsetup luksFormat the new disk " key_data
-    key_regex='^[[:alnum:]]+$'
-    if [[ ! $key_data =~ $key_regex ]] || (( ${#key_data} > 1000 || ${#key_data} < 30 )); then
-      echo "error: bad key"
-      exit 1
-    fi
-    printf "%s" "$key_data" >$keyfile
-    push-cleanup "rm -f $keyfile"
-  else
 
-    e writing generated password to $keyfile
-    pwgen 128 -s -1 | tr -d '\n' >$keyfile
-    push-cleanup "rm -f $keyfile"
+mk-decrypting-grub-image() {
+  # These kinds of images are mainly useful with ceph.
+  e writing generated password to $keyfile
+  pwgen 128 -s -1 | tr -d '\n' >$keyfile
+  push-cleanup "rm -f $keyfile"
 
-    m tee /dev/shm/grub.cfg <<EOF
+  m tee /dev/shm/grub.cfg <<EOF
 cryptomount -k /keyfile hd0
 linux (crypto0)/vmlinuz root=/dev/mapper/$dname-crypt0 ro elevator=noop console=tty0 console=ttyS0,115200 biosdevname=0 net.ifnames=0
 initrd (crypto0)/initrd.img
 boot
 EOF
-    m dd if=/dev/zero of=/dev/shm/memdisk bs=1M count=1
-    m mkfs.ext2 /dev/shm/memdisk
-    push-cleanup "rm -f /dev/shm/memdisk /dev/shm/grub.cfg"
-    m mkdir -p /dev/shm/memdiskmount
-    m mount /dev/shm/memdisk /dev/shm/memdiskmount
-    push-cleanup "umount /dev/shm/memdiskmount"
-    m cp $keyfile /dev/shm/memdiskmount/
-    m umount /dev/shm/memdiskmount
-    pop-cleanup
+  m dd if=/dev/zero of=/dev/shm/memdisk bs=1M count=1
+  m mkfs.ext2 /dev/shm/memdisk
+  push-cleanup "rm -f /dev/shm/memdisk /dev/shm/grub.cfg"
+  m mkdir -p /dev/shm/memdiskmount
+  m mount /dev/shm/memdisk /dev/shm/memdiskmount
+  push-cleanup "umount /dev/shm/memdiskmount"
+  m cp $keyfile /dev/shm/memdiskmount/
+  m umount /dev/shm/memdiskmount
+  pop-cleanup
 
-    m grub-mkimage -C xz -c /dev/shm/grub.cfg  -O i386-pc  -o /var/lib/libvirt/images/grub-$host.bin  biosdisk ext2 linux xfs normal luks help crypto cryptodisk zfscrypt gcry_sha512 gcry_sha256 echo cat memdisk -m /dev/shm/memdisk
-    rm -f /dev/shm/memdisk /dev/shm/grub.cfg
-    pop-cleanup
-    # This is slightly out of order because it is simpler than doing in
-    # proper order and removing cleanup_cmds[-2].
-    push-cleanup "rm -f /var/lib/libvirt/images/grub-$host.bin"
-  fi
+  m grub-mkimage -C xz -c /dev/shm/grub.cfg  -O i386-pc  -o /var/lib/libvirt/images/grub-$host.bin  biosdisk ext2 linux xfs normal luks help crypto cryptodisk zfscrypt gcry_sha512 gcry_sha256 echo cat memdisk -m /dev/shm/memdisk
+  rm -f /dev/shm/memdisk /dev/shm/grub.cfg
+  pop-cleanup
+  # This is slightly out of order because it is simpler than doing in
+  # proper order and removing cleanup_cmds[-2].
+  push-cleanup "rm -f /var/lib/libvirt/images/grub-$host.bin"
 
-  m rbd create $dname --size $new_disk_mb -p $pool --image-format=2 --image-feature exclusive-lock,object-map,fast-diff,layering
-  push-cleanup "rbd rm $dname"
+}
+
+mkfs-and-mount-ceph-disk() {
   nbd_dev=$(rbd-nbd map $dname)
   push-cleanup "rbd-nbd unmap $nbd_dev"
 
@@ -410,70 +403,175 @@ EOF
   push-cleanup "umount $target"
   vm_internal_disk=$luks_dev
 
-  if $add_disk; then
-    if virsh list --name | grep -Fx $host &>/dev/null; then
-      # If the vm is running, record the commands to be run manually inside it.
-      inside_vm_cmds=true
-      cat >/root/generated-to-run-in-vm <<EOF
+}
+
+maybe-get-ceph-live-add-disk-steps() {
+  if ! $add_disk && ! vm-on; then
+    return 0
+  fi
+
+  inside_vm_cmds=true
+  cat >/root/generated-to-run-in-vm <<EOF
+# todo: accept cli argument which alters these commands for swap partition, or multiple device btrfs, etc.
 set -x
 mknod /dev/sd$letter b 8 $disk_num
+tmpf=\$(mktemp)
+/boot/keyscript.sh >\$tmpf
+cryptsetup luksFormat -y --cipher aes-xts-plain64 --hash sha256 --use-urandom --key-size 256  /dev/sd$letter --key-file=\$tmpf
 echo "$dname-crypt0 /dev/sd$letter none luks,discard,keyscript=/boot/keyscript.sh,initramfs" >> /etc/crypttab
 mkdir -p $disk_mountpoint /etc
-echo "$luks_dev $disk_mountpoint $fs_type $fs_opts 0 $fs_passno" >>/etc/fstab
+echo "/dev/mapper/$dname-crypt0 $disk_mountpoint ext4 $fs_opts 0 $fs_passno" >>/etc/fstab
 cryptdisks_start $dname-crypt0
+rm \$tmpf
+mkfs.ext4 /dev/mapper/$dname-crypt0
 mount $disk_mountpoint
 update-initramfs -k all -u
 EOF
+}
 
-    else
-      m mount-vm -k $keyfile --first-disk $host
-      push-cleanup "umount-vm $host"
+chroot-add-ceph-disk() {
+  if [[ ! -e /mnt/$host/dev/sd$letter ]]; then
+    m mknod /mnt/$host/dev/sd$letter b 8 $disk_num
+  fi
+  echo "$dname-crypt0 /dev/sd$letter none luks,discard,keyscript=/boot/keyscript.sh,initramfs" >> /mnt/$host/etc/crypttab
 
-      if [[ ! -e /mnt/$host/dev/sd$letter ]]; then
-        m mknod /mnt/$host/dev/sd$letter b 8 $disk_num
-      fi
-      echo "$dname-crypt0 /dev/sd$letter none luks,discard,keyscript=/boot/keyscript.sh,initramfs" >> /mnt/$host/etc/crypttab
-
-      cd /mnt/$host
-      for d in proc sys dev dev/pts; do
-        m mount -ov bind /$d $d;
-        push-cleanup "umount $PWD/$d"
-      done
-      # note, this is normal output here:
-      # cryptsetup: WARNING: Couldn't determine cipher modules to load for
-      # tmp.fsf.org-data-crypt0
-      m chroot . update-initramfs -k all -u
-      for d in dev/pts dev sys proc; do
-        m umount $d
-        pop-cleanup
-      done
-      cd
-
-      m mkdir -p /mnt/$host/$disk_mountpoint /mnt/$host/etc
-      echo "$luks_dev $disk_mountpoint $fs_type $fs_opts 0 $fs_passno" >>/mnt/$host/etc/fstab
-      m umount-vm $host
-      pop-cleanup
+  cd /mnt/$host
+  for d in proc sys dev dev/pts; do
+    m mount -o bind /$d $d;
+    push-cleanup "umount $PWD/$d"
+  done
+  # note, this is normal output here:
+  # cryptsetup: WARNING: Couldn't determine cipher modules to load for
+  # tmp.fsf.org-data-crypt0
+  m chroot . update-initramfs -k all -u
+  for d in dev/pts dev sys proc; do
+    m umount $d
+    pop-cleanup
+  done
+  cd
 
-    fi
-  else
-    # Note: debootstrap would create these, I just want to setup the basic
-    # disk unlocking all in one place earlier on the script for
-    # convenience.
-    m mkdir -p $target/{boot,etc,dev}
-
-    m mknod $target/dev/sda b 8 0
-    m mkdir -p $target/dev/mapper
-    crypt_dev=$(readlink -f /dev/mapper/$dname-crypt0)
-    m cp -a $crypt_dev $target/dev/mapper/$dname-crypt0
-    m tee $target/boot/keyscript.sh <<EOF
+  m mkdir -p /mnt/$host/$disk_mountpoint /mnt/$host/etc
+  echo "$luks_dev $disk_mountpoint $fs_type $fs_opts 0 $fs_passno" >>/mnt/$host/etc/fstab
+  m umount-vm $host
+  pop-cleanup
+}
+
+chroot-add-ceph-root-disk() {
+  # Note: debootstrap would create these, I just want to setup the basic
+  # disk unlocking all in one place earlier on the script for
+  # convenience.
+  m mkdir -p $target/{boot,etc,dev}
+
+  m mknod $target/dev/sda b 8 0
+  m mkdir -p $target/dev/mapper
+  crypt_dev=$(readlink -f /dev/mapper/$dname-crypt0)
+  m cp -a $crypt_dev $target/dev/mapper/$dname-crypt0
+  m tee $target/boot/keyscript.sh <<EOF
 #!/bin/sh
 echo -n $(cat $keyfile)
 EOF
-    chmod 700 $target/boot/keyscript.sh $target/boot/
-    echo "$dname-crypt0 /dev/sda none luks,discard,keyscript=/boot/keyscript.sh,initramfs" > $target/etc/crypttab
+  chmod 700 $target/boot/keyscript.sh $target/boot/
+  echo "$dname-crypt0 /dev/sda none luks,discard,keyscript=/boot/keyscript.sh,initramfs" > $target/etc/crypttab
+
+}
+
+## calls a bunch of functions
+ceph-disk() {
+  pool=rbd
+  keyfile=/dev/shm/keyfile
+
+  m rbd create $dname --size $new_disk_mb -p $pool --image-format=2 --image-feature exclusive-lock,object-map,fast-diff,layering
+  push-cleanup "rbd rm $dname"
+
+  if $add_disk && vm-on; then return 0; fi
+
+  if $add_disk; then
+    m mount-vm --first-disk $host
+    push-cleanup "umount-vm $host"
+    /mnt/$host/boot/keyscript.sh >$keyfile
+  else
+    mk-decrypting-grub-image
+  fi
+
+  mkfs-and-mount-ceph-disk
+
+  if $add_disk; then
+    chroot-add-ceph-disk
+  else
+    chroot-add-ceph-root-disk
+  fi
+
+}
+
+close-disk() {
+  if mountpoint -q $target; then
+    m umount $target
+    pop-cleanup
+  fi
+  if [[ -s $luks_dev ]]; then
+    m cryptsetup luksClose $luks_dev
+    pop-cleanup
+  fi
+  if [[ -e $nbd_dev ]]; then
+    m rbd-nbd unmap $nbd_dev
+    pop-cleanup
   fi
 }
 
+install-dependencies() {
+  # https://dsa.debian.org/howto/install-kvm/
+  pkgs=(libvirt-dev virtinst pwgen libosinfo-bin)
+  for p in ${pkgs[@]}; do
+    if ! dpkg -s -- $p |& grep -Fx "Status: install ok installed" &> /dev/null; then
+      m apt-get install -y --no-install-recommends ${pkgs[@]}
+      break
+    fi
+  done
+}
+
+mk-common-grub-images() {
+  name_suf=
+  boot_prefix=
+  kernel_args=
+  mk-grub-image
+  bootsym-image
+
+  name_suf="-btrfs"
+  boot_prefix=/root
+  kernel_args=rootflags=subvol=root
+  mk-grub-image
+  bootsym-image
+}
+
+set-disk-letter() {
+  letter=a
+  if $add_disk; then
+    xpath=/domain/devices/disk/source/@dev
+    if $doceph; then
+      xpath="/domain/devices/disk/source[@protocol='rbd']/@name"
+    fi
+    tmpf=$(mktemp)
+    virsh dumpxml $host >$tmpf
+    disk_num=$( xmllint --xpath "count($xpath)" $tmpf)
+    disk_letters=( {a..z} )
+    letter=${disk_letters[disk_num]}
+  fi
+}
+
+attach-disk() {
+  if $doceph; then
+    attach-ceph-disk $pool/$dname
+    return 0
+  fi
+
+  for (( i=0; i < ${#vm_disks[@]}; i++ )); do
+    disk=${vm_disks[i]}
+    letter=${disk_letters[disk_num+i]}
+    attach-local-disk $disk $host
+  done
+}
+
+
 
 # vg-disk() {
 #   m lvcreate $vg -L $new_disk_mb -n $dname
@@ -769,6 +867,29 @@ EOF
   push-cleanup "umount $target"
 } # End nonceph-disk
 
+nonceph-maybe-mount-extra-disk() {
+  if ! $add_disk; then
+    return
+  fi
+
+  if vm-on; then
+    inside_vm_cmds=true
+    cat >/root/generated-to-run-in-vm <<EOF
+mkdir -p $disk_mountpoint /etc
+echo "/dev/vd$letter $disk_mountpoint $fs_type $fs_opts 0 $fs_passno" >>/etc/fstab
+mount $disk_mountpoint
+EOF
+
+  else
+    m mount-vm --first-disk $host
+    push-cleanup "umount-vm $host"
+    m mkdir -p /mnt/$host/$disk_mountpoint /mnt/$host/etc
+    echo "/dev/vd$letter $disk_mountpoint $fs_type $fs_opts 0 $fs_passno" >>/mnt/$host/etc/fstab
+    m umount-vm $host
+    pop-cleanup
+  fi
+
+}
 
 
 bootsym-image() {
@@ -1133,6 +1254,7 @@ create-vm() {
   # virt-install, so just add them here.
   if $doceph; then
     m virsh destroy $host
+    attach-ceph-disk $pool/$dname
     virsh dumpxml $host > /tmp/tmp.xml
     #sed "1s#># xmlns:qemu=\'http://libvirt.org/schemas/domain/qemu/1.0'>#; s#</domain>#<qemu:commandline><qemu:arg value='-set'/><qemu:arg value='device.scsi0-0-0-0.min_io_size=4194304'/><qemu:arg value='-set'/><qemu:arg value='device.scsi0-0-0-0.opt_io_size=4194304'/></qemu:commandline></domain>#" /tmp/tmp.xml -i
 
@@ -1162,11 +1284,15 @@ EOF
 
   if [[ $keyfile ]]; then
     echo "LUKS KEY	: $(cat $keyfile)"
-    if $doceph; then
-      echo "RBD IMAGE	: $pool/$host"
+    if [[ $keyfile == /dev/shm/* ]]; then
+      rm -f $keyfile
     fi
   fi
 
+  if $doceph; then
+    echo "RBD IMAGE	: $pool/$host"
+  fi
+
   if $import; then
     e "--import specified, stopping the empty vm"
     m virsh destroy $host
@@ -1183,12 +1309,10 @@ export LC_ALL=C
 
 pre="${0##*/}:"
 
-for pkg in jq ; do
-  if ! type -p $pkg &>/dev/null; then
-    echo "installing missing dependency: $pkg"
-    apt-get -y install $pkg
-  fi
-done
+if ! type -p jq &>/dev/null; then
+  echo "installing missing dependency: jq"
+  apt-get -y install jq
+fi
 
 ##### Begin command line parsing ########
 
@@ -1229,7 +1353,7 @@ done
 if $add_disk; then
   read -r new_disk_mb disk_arg host <<<"$@"
   disk_name_suf=-${disk_arg%%/*}
-  disk_mountpoint=${disk_arg#*/}
+  disk_mountpoint=/${disk_arg#*/}
 else
   read -r new_disk_mb ram cpus release host <<<"$@"
 fi
@@ -1267,71 +1391,20 @@ if $dolist; then
   exit 0
 fi
 
-# Install required packages
-# https://dsa.debian.org/howto/install-kvm/
-pkgs=(libvirt-dev virtinst pwgen libosinfo-bin)
-for p in ${pkgs[@]}; do
-  if ! dpkg -s -- $p |& grep -Fx "Status: install ok installed" &> /dev/null; then
-    m apt-get install -y --no-install-recommends ${pkgs[@]}
-    break
-  fi
-done
-target=$(mktemp -d)
-
-
-###### Begin make common grub images if they dont exist ######
-name_suf=
-boot_prefix=
-kernel_args=
-mk-grub-image
-bootsym-image
+install-dependencies
 
-name_suf="-btrfs"
-boot_prefix=/root
-kernel_args=rootflags=subvol=root
-mk-grub-image
-bootsym-image
-###### End make common grub images if they dont exist ######
+target=$(mktemp -d)
 
+mk-common-grub-images
 
-letter=a
-if $add_disk; then
-  xpath=/domain/devices/disk/source/@dev
-  if $doceph; then
-    xpath="/domain/devices/disk/source[@protocol='rbd']/@name"
-  fi
-  tmpf=$(mktemp)
-  virsh dumpxml $host >$tmpf
-  disk_num=$( xmllint --xpath "count($xpath)" $tmpf)
-  disk_letters=( {a..z} )
-  letter=${disk_letters[disk_num]}
-fi
+set-disk-letter
 
 if $doceph; then
   ceph-df
   ceph-disk
 else
   nonceph-disk
-fi
-
-if ! $doceph && $add_disk; then
-  if virsh list --name | grep -Fx $host &>/dev/null; then
-    # If the VM is running, record the commands to be run manually inside it.
-    inside_vm_cmds=true
-    cat >/root/generated-to-run-in-vm <<EOF
-mkdir -p $disk_mountpoint /etc
-echo "/dev/vd$letter $disk_mountpoint $fs_type $fs_opts 0 $fs_passno" >>/etc/fstab
-mount $disk_mountpoint
-EOF
-
-  else
-    m mount-vm --first-disk $host
-    push-cleanup "umount-vm $host"
-    m mkdir -p /mnt/$host/$disk_mountpoint /mnt/$host/etc
-    echo "/dev/vd$letter $disk_mountpoint $fs_type $fs_opts 0 $fs_passno" >>/mnt/$host/etc/fstab
-    m umount-vm $host
-    pop-cleanup
-  fi
+  nonceph-maybe-mount-extra-disk
 fi
 
 if ! $import && ! $add_disk; then
@@ -1339,49 +1412,15 @@ if ! $import && ! $add_disk; then
   os-prep
 fi
 
-m umount $target
-pop-cleanup
-
-
-# For ceph, decryption is done inside the VM.
-if $doceph; then
-  m cryptsetup luksClose $luks_dev
-  pop-cleanup
-  m rbd-nbd unmap $nbd_dev
-  pop-cleanup
-fi
-
+close-disk
 
 if $add_disk; then
-  if $doceph; then
-    ceph-add-disk $pool/$dname $host
-  else
-    # Note: On a vg disk in kvmhost2, we had this:
-    #  <driver name='qemu' type='raw' cache='writethrough'/>
-    #  but my research says that we do not want that cache mode.
-
-    # libvirt will add a elements like these into the disk:
-    # <alias name='virtio-disk0'/>
-    # <address type='pci' domain='0x0000' bus='0x04' slot='0x00' function='0x0'/>
-    # and the index here:
-    # <source dev='$disk' index='3'/>
-
-    for (( i=0; i < ${#vm_disks[@]}; i++ )); do
-      disk=${vm_disks[i]}
-      letter=${disk_letters[disk_num+i]}
-      # TODO After we upgrade all our kvmhosts, check out virsh attach-disk.
-      add-local-disk $disk $host
-    done
-  fi
+  attach-disk
 else
   create-vm
 fi
 
-# we dont keep ceph keyfiles
-if [[ $keyfile == /dev/shm/* ]]; then
-  rm -f $keyfile
-fi
-
+maybe-get-ceph-live-add-disk-steps
 
 if $inside_vm_cmds; then
   cat <<EOF
@@ -1394,8 +1433,10 @@ EOF
   cat /root/generated-to-run-in-vm
 fi
 
-if ! backup-crypt-luks-keys; then
-  echo "$0: this was the last command of create-vm, so just sort out any error and hopefully continue to use the vm"
+if type -p backup-crypt-luks-keys &>/dev/null; then
+  if  ! backup-crypt-luks-keys; then
+    echo "$0: this was the last command of create-vm, so just sort out any error and hopefully continue to use the vm"
+  fi
 fi
 
 exit 0
diff --git a/roles/kvmhost/files/simple/usr/local/bin/savannah-virsh b/roles/kvmhost/files/simple/usr/local/bin/savannah-virsh
index c9d1717..dab2b51 100755
--- a/roles/kvmhost/files/simple/usr/local/bin/savannah-virsh
+++ b/roles/kvmhost/files/simple/usr/local/bin/savannah-virsh
@@ -118,6 +118,10 @@ if (( args_len > 1000 )); then
   arg-die args_len 1000
 fi
 
+if [[ ! $args ]]; then
+  usage
+fi
+
 if [[ ! $args =~ $initial_input_regex ]]; then
   arg-die initial regex
 fi
diff --git a/roles/kvmhost/files/simple/usr/local/lib/fsf-virsh-bash-lib b/roles/kvmhost/files/simple/usr/local/lib/fsf-virsh-bash-lib
index bf762bc..23e977b 100644
--- a/roles/kvmhost/files/simple/usr/local/lib/fsf-virsh-bash-lib
+++ b/roles/kvmhost/files/simple/usr/local/lib/fsf-virsh-bash-lib
@@ -1,8 +1,10 @@
 #!/bin/bash
 
-# Input var: $letter , as in /dev/sd$letter . I could make it an arg, but meh.
-add-ceph-disk() {
-  local tmpf rbd_path="$1" target_vm="$2"
+# Input variables:
+#  $letter , as in /dev/sd$letter.
+#  $host: target vm name.
+attach-ceph-disk() {
+  local tmpf rbd_path="$1"
   tmpf=$(mktemp)
   tee $tmpf <<EOF
 <disk type='network' device='disk'>
@@ -18,13 +20,24 @@ add-ceph-disk() {
   <target dev='sd$letter' bus='scsi'/>
 </disk>
 EOF
-  virsh attach-device $target_vm $tmpf --persistent
+  virsh attach-device $host $tmpf --persistent
 }
 
-# Input var: $letter , as in /dev/sd$letter
-add-local-disk() {
-  local tmpf disk_path="$1" target_vm="$2"
+# Input variables:
+#  $letter , as in /dev/sd$letter.
+#  $host: target vm name.
+attach-local-disk() {
+  local tmpf disk_path="$1"
   tmpf=$(mktemp)
+  # Note: On a vg disk in kvmhost2, we had this:
+  #  <driver name='qemu' type='raw' cache='writethrough'/>
+  #  but my research says that we do not want that cache mode.
+
+  # libvirt will add a elements like these into the disk:
+  # <alias name='virtio-disk0'/>
+  # <address type='pci' domain='0x0000' bus='0x04' slot='0x00' function='0x0'/>
+  # and the index here:
+  # <source dev='$disk' index='3'/>
   tee $tmpf <<EOF
 <disk type='block' device='disk'>
   <driver name='qemu' type='raw' cache='none' io='native' discard='unmap'/>
@@ -32,6 +45,6 @@ add-local-disk() {
   <target dev='vd$letter' bus='virtio'/>
 </disk>
 EOF
-  virsh attach-device $target_vm $tmpf --persistent
+  virsh attach-device $host $tmpf --persistent
   rm $tmpf
 }
-- 
2.25.1