On Fri, Nov 07, 2025 at 03:08:25PM +0000, Simon Horman wrote:

On Thu, Nov 06, 2025 at 04:49:48PM -0800, Bobby Eshleman wrote:

...

quoted

@@ -90,15 +85,19 @@ vm_ssh() {
 }
 
 cleanup() {
-	if [[ -s "${QEMU_PIDFILE}" ]]; then
-		pkill -SIGTERM -F "${QEMU_PIDFILE}" > /dev/null 2>&1
-	fi
+	local pidfile
 
-	# If failure occurred during or before qemu start up, then we need
-	# to clean this up ourselves.
-	if [[ -e "${QEMU_PIDFILE}" ]]; then
-		rm "${QEMU_PIDFILE}"
-	fi
+	for pidfile in "${PIDFILES[@]}"; do
+		if [[ -s "${pidfile}" ]]; then
+			pkill -SIGTERM -F "${pidfile}" > /dev/null 2>&1
+		fi
+
+		# If failure occurred during or before qemu start up, then we need
+		# to clean this up ourselves.
+		if [[ -e "${pidfile}" ]]; then
+			rm "${pidfile}"
+		fi
+	done
 }

Hi Bobby,

This is completely untested, but it looks to me
like cleanup() could be implemented more succinctly like this.

cleanup() {
	terminate_pidfiles "${PIDFILES[@]}"
}

Oh right! I reverted the deletion and completely forgot about
terminate_pidfiles().

quoted

 
 check_args() {

@@ -188,10 +187,35 @@ handle_build() {
 	popd &>/dev/null
 }
 
+create_pidfile() {
+	local pidfile
+
+	pidfile=$(mktemp "${PIDFILE_TEMPLATE}")
+	PIDFILES+=("${pidfile}")
+
+	echo "${pidfile}"
+}
+
+terminate_pidfiles() {
+	local pidfile
+
+	for pidfile in "$@"; do
+		if [[ -s "${pidfile}" ]]; then
+			pkill -SIGTERM -F "${pidfile}" > /dev/null 2>&1
+		fi
+
+		if [[ -e "${pidfile}" ]]; then
+			rm -f "${pidfile}"
+		fi
+	done

I think it would be useful to remove $pidfile from $PIDFILES.
This might be easier to implement if PIDFILES was an associative array.

Using an associative makes sense, this way we can trim the set.

quoted

+}
+

...

quoted

@@ -498,7 +529,8 @@ handle_build
 echo "1..${#ARGS[@]}"
 
 log_host "Booting up VM"
-vm_start
+pidfile="$(create_pidfile)"
+vm_start "${pidfile}"
 vm_wait_for_ssh
 log_host "VM booted up"
 

quoted

@@ -522,6 +554,8 @@ for arg in "${ARGS[@]}"; do
 	cnt_total=$(( cnt_total + 1 ))
 done
 
+terminate_pidfiles "${pidfile}"

I am assuming that there will be more calls to terminate_pidfiles
in subsequent patch-sets.

Else I think terminate_pidfiles can be removed
and instead we can rely on cleanup().

Indeed, later patches will use terminate_pidfiles() in between spin up /
shut down of multiple VMs.


Thanks again, will incorporate your feedback in the next!

Best,
Bobby