From db02fcc2bc9f3abb1a93a737219e5f609f907a19 Mon Sep 17 00:00:00 2001
From: Albert Chu <chu11@llnl.gov>
Date: Sat, 2 Mar 2024 15:15:36 -0800
Subject: [PATCH 1/5] t/simulators: add missing "goto" in vpcd simulator

Problem: The "temp" command was missing a "goto ok" statement after
completing the temperature request.  It lead to unexpected output.

Add the missing "goto ok".
---
 t/simulators/vpcd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/simulators/vpcd.c b/t/simulators/vpcd.c
index e9c89e00..56ba8e1f 100644
--- a/t/simulators/vpcd.c
+++ b/t/simulators/vpcd.c
@@ -305,6 +305,7 @@ _prompt_loop(void)
                 continue;
             }
             printf("plug %d: %d\n", i, temp[i]);
+            goto ok;
         }
         if (strcmp(buf, "temp *") == 0) {               /* temp * */
             for (i = 0; i < NUM_PLUGS; i++)

From 0c1a1d79381c6f19a0621998e5b053e498d7fbfe Mon Sep 17 00:00:00 2001
From: Albert Chu <chu11@llnl.gov>
Date: Sat, 2 Mar 2024 15:16:41 -0800
Subject: [PATCH 2/5] etc/devices: remove untested status scripts

Problem: Current powerman implementation will always
call the "all" status script instead of a "singleton"
status scripts for power status, beacon status, and temperature
status.  In the future this behavior will change.

Some devices specify both "all" and "singleton" status scripts.
This means the "singleton" status scripts are never used
and are untested, but with future changes they will begin to be
used.

Solution: Remove the singleton status scripts in appro-gb2.dev,
appro-greenblade.dev, and swpdu.dev.  This will preserve
current behavior of those device files.
---
 etc/devices/appro-gb2.dev        | 60 --------------------------------
 etc/devices/appro-greenblade.dev | 42 ----------------------
 etc/devices/swpdu.dev            |  6 ----
 3 files changed, 108 deletions(-)

diff --git a/etc/devices/appro-gb2.dev b/etc/devices/appro-gb2.dev
index bd2db9da..619a98b5 100644
--- a/etc/devices/appro-gb2.dev
+++ b/etc/devices/appro-gb2.dev
@@ -44,18 +44,6 @@ specification "sr5110" {
 		}
 		expect "-iSCB> "
 	}
-	script status {
-		send "pmnode %s\r"
-		expect "node([0-9]+): (on|off|n/a)"
-		setplugstate $1 $2 on="on" off="off"
-		expect "-iSCB> "
-	}
-	script status_beacon {
-		send "pmled %s\r"
-		expect "node([0-9]+): (on|off|n/a)"
-		setplugstate $1 $2 on="on" off="off"
-		expect "-iSCB> "
-	}
 	script on {
 		send "power on %s\r"
 		expect "-iSCB> "
@@ -119,18 +107,6 @@ specification "sr5110_gpu" {
 		}
 		expect "-iSCB> "
 	}
-	script status {
-		send "pmnode %s\r"
-		expect "node([0-9]+): (on|off|n/a)"
-		setplugstate $1 $2 on="on" off="off"
-		expect "-iSCB> "
-	}
-	script status_beacon {
-		send "pmled %s\r"
-		expect "node([0-9]+): (on|off|n/a)"
-		setplugstate $1 $2 on="on" off="off"
-		expect "-iSCB> "
-	}
 	script on {
 		send "power on %s\r"
 		expect "-iSCB> "
@@ -195,18 +171,6 @@ specification "sr8116" {
 		}
 		expect "-iSCB> "
 	}
-	script status {
-		send "pmnode %s\r"
-		expect "node([0-9]+): (on|off|n/a)"
-		setplugstate $1 $2 on="on" off="off"
-		expect "-iSCB> "
-	}
-	script status_beacon {
-		send "pmled %s\r"
-		expect "node([0-9]+): (on|off|n/a)"
-		setplugstate $1 $2 on="on" off="off"
-		expect "-iSCB> "
-	}
 	script on {
 		send "power on %s\r"
 		expect "-iSCB> "
@@ -270,18 +234,6 @@ specification "sr8116_gpu" {
 		}
 		expect "-iSCB> "
 	}
-	script status {
-		send "pmnode %s\r"
-		expect "node([0-9]+): (on|off|n/a)"
-		setplugstate $1 $2 on="on" off="off"
-		expect "-iSCB> "
-	}
-	script status_beacon {
-		send "pmled %s\r"
-		expect "node([0-9]+): (on|off|n/a)"
-		setplugstate $1 $2 on="on" off="off"
-		expect "-iSCB> "
-	}
 	script on {
 		send "power on %s\r"
 		expect "-iSCB> "
@@ -345,18 +297,6 @@ specification "sr8104" {
 		}
 		expect "-iSCB> "
 	}
-	script status {
-		send "pmnode %s\r"
-		expect "node([0-9]+): (on|off|n/a)"
-		setplugstate $1 $2 on="on" off="off"
-		expect "-iSCB> "
-	}
-	script status_beacon {
-		send "pmled %s\r"
-		expect "node([0-9]+): (on|off|n/a)"
-		setplugstate $1 $2 on="on" off="off"
-		expect "-iSCB> "
-	}
 	script on {
 		send "power on %s\r"
 		expect "-iSCB> "
diff --git a/etc/devices/appro-greenblade.dev b/etc/devices/appro-greenblade.dev
index eb7c11c6..d140d6c8 100644
--- a/etc/devices/appro-greenblade.dev
+++ b/etc/devices/appro-greenblade.dev
@@ -48,20 +48,6 @@ specification "iscb-gb" {
 		expect "ok"
 		expect "iSCB-[0-9]+:[0-9]> "
 	}
-	script status {
-		send "pmnode %s\r\n"
-		expect "node([0-9]+): (on|off|n/a)"
-		setplugstate $1 $2 on="on" off="off"
-		expect "ok"
-		expect "iSCB-[0-9]+:[0-9]> "
-	}
-	script status_beacon {
-		send "pmled %s\r\n"
-		expect "node([0-9]+): (on|off|n/a)"
-		setplugstate $1 $2 on="on" off="off"
-		expect "ok"
-		expect "iSCB-[0-9]+:[0-9]> "
-	}
 	script on {
 		send "on %s\r\n"
 		expect "ok"
@@ -138,20 +124,6 @@ specification "iscb-gbgpu" {
 		expect "ok"
 		expect "iSCB-[0-9]+:[0-9]> "
 	}
-	script status {
-		send "pmnode %s\r\n"
-		expect "node([0-9]+): (on|off|n/a)"
-		setplugstate $1 $2 on="on" off="off"
-		expect "ok"
-		expect "iSCB-[0-9]+:[0-9]> "
-	}
-	script status_beacon {
-		send "pmled %s\r\n"
-		expect "node([0-9]+): (on|off|n/a)"
-		setplugstate $1 $2 on="on" off="off"
-		expect "ok"
-		expect "iSCB-[0-9]+:[0-9]> "
-	}
 	script on {
 		send "on %s\r\n"
 		expect "ok"
@@ -228,20 +200,6 @@ specification "iscb-hybrid" {
 		expect "ok"
 		expect "iSCB-[0-9]+:[0-9]> "
 	}
-	script status {
-		send "pmnode %s\r\n"
-		expect "node([0-9]+): (on|off|n/a)"
-		setplugstate $1 $2 on="on" off="off"
-		expect "ok"
-		expect "iSCB-[0-9]+:[0-9]> "
-	}
-	script status_beacon {
-		send "pmled %s\r\n"
-		expect "node([0-9]+): (on|off|n/a)"
-		setplugstate $1 $2 on="on" off="off"
-		expect "ok"
-		expect "iSCB-[0-9]+:[0-9]> "
-	}
 	script on {
 		send "on %s\r\n"
 		expect "ok"
diff --git a/etc/devices/swpdu.dev b/etc/devices/swpdu.dev
index a8f54e26..4fe35934 100644
--- a/etc/devices/swpdu.dev
+++ b/etc/devices/swpdu.dev
@@ -38,12 +38,6 @@ specification "swpdu" {
 		}
 		expect "swpdu> "
 	}
-	script status {
-		send "status %s\r\n"
-		expect "port([0-9]+)[^\n]*(on|off|unknown|^n)"
-		setplugstate $1 $2 on="on" off="off"
-		expect "swpdu> "
-	}
 	script on {
 		send "on %s\r\n"
 		expect "swpdu> "

From 4e7ea642b69acce46e47f7704acbd96c4e898a35 Mon Sep 17 00:00:00 2001
From: Albert Chu <chu11@llnl.gov>
Date: Mon, 26 Feb 2024 16:22:25 -0800
Subject: [PATCH 3/5] powerman: do not always call "all" query script

Problem: When selecting a script to use for an action, the "all"
version of a script is always used if the action is a query action
(e.g. power status query, beacon query, etc.).

This can be problematic in situations where blades/nodes/etc. are
not fully populated, such as in a chassis.  We do not want to query
"all" things in that case.

Solution: Only call the all query script if all plugs are requested
OR the singlet version of the script is not defined."?  For device files
where unpopulated plugs may exist, it can define an all script and singlet
script for each scenario.
---
 src/powerman/device.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/powerman/device.c b/src/powerman/device.c
index 067d31ba..f18757ea 100644
--- a/src/powerman/device.c
+++ b/src/powerman/device.c
@@ -683,8 +683,12 @@ static int _enqueue_targeted_actions(Device * dev, int com, hostlist_t hl,
     pluglist_iterator_destroy(itr);
 
     /* Try _all version of script.
+     *
+     * - use if action is a query, unless no singlet version exists
+     *   (i.e. if there is no singlet version, we have to use the _all
+     *   version)
      */
-    if (all || _is_query_action(com)) {
+    if (all || (_is_query_action(com) && dev->scripts[com] == NULL)) {
         int ncom = _get_all_script(dev, com);
 
         if (ncom != -1) {

From 9992f964ca61da60aedcaf7949e6aa9eca5314e9 Mon Sep 17 00:00:00 2001
From: Albert Chu <chu11@llnl.gov>
Date: Fri, 1 Mar 2024 22:09:07 -0800
Subject: [PATCH 4/5] t: test which status script is used

Problem: There are no tests to see which status script (status vs status_all)
is chosen when both are specified in a device file.

Add coverage in t0004-status-query.t, t0007-temperature.t, and t0008-beacon.t.
---
 t/t0004-status-query.t | 66 ++++++++++++++++++++++++++++
 t/t0007-temperature.t  | 99 ++++++++++++++++++++++++++++++++++++++++++
 t/t0008-beacon.t       | 66 ++++++++++++++++++++++++++++
 3 files changed, 231 insertions(+)

diff --git a/t/t0004-status-query.t b/t/t0004-status-query.t
index da04a5e0..34fc0d43 100755
--- a/t/t0004-status-query.t
+++ b/t/t0004-status-query.t
@@ -145,6 +145,72 @@ test_expect_success 'stop powerman daemon and device server' '
 	wait &&
 	wait
 '
+test_expect_success 'create new powerman.conf with both status and status_all script' '
+	cat >powerman4.conf <<-EOT4
+	specification "vpc" {
+	    timeout 	5.0
+	    plug name { "0" "1" "2" "3" "4" "5" "6" "7" "8"
+	                "9" "10" "11" "12" "13" "14" "15" }
+	    script login {
+	        send "login\n"
+	        expect "[0-9]* OK\n"
+	        expect "[0-9]* vpc> "
+	    }
+	    script logout {
+	        send "logoff\n"
+	        expect "[0-9]* OK\n"
+	    }
+	    script status {
+	        send "stat %s\n"
+	        expect "plug ([0-9]+): (ON|OFF)\n"
+	        setplugstate \$1 \$2 on="ON" off="OFF"
+	        expect "[0-9]* OK\n"
+	        expect "[0-9]* vpc> "
+	    }
+	    script status_all {
+		send "stat *\n"
+		foreachplug {
+			expect "plug ([0-9]+): (ON|OFF)\n"
+			setplugstate \$1 \$2 on="ON" off="OFF"
+		}
+		expect "[0-9]* OK\n"
+		expect "[0-9]* vpc> "
+	    }
+	}
+	listen "$testaddr"
+	device "test0" "vpc" "$vpcd |&"
+	node "t[0-15]" "test0"
+	EOT4
+'
+test_expect_success 'start powerman daemon and wait for it to start' '
+	$powermand -c powerman4.conf &
+	echo $! >powermand4.pid &&
+	$powerman --retry-connect=100 --server-host=$testaddr -q >/dev/null
+'
+test_expect_success 'powerman -q works' '
+	$powerman -h $testaddr -q >all_query.out &&
+	makeoutput "" "t[0-15]" "" >all_query.exp &&
+	test_cmp all_query.exp all_query.out
+'
+test_expect_success 'powerman -q uses status_all script on all plugs' '
+	$powerman -h $testaddr -T -q >all_queryT.out &&
+	count=`grep "stat" all_queryT.out | wc -l` &&
+	test $count = 1
+'
+test_expect_success 'powerman -q t[1-15] works' '
+	$powerman -h $testaddr -q t[1-15] >most_query.out &&
+	makeoutput "" "t[1-15]" "" >most_query.exp &&
+	test_cmp most_query.exp most_query.out
+'
+test_expect_success 'powerman -q uses status script on not all plugs' '
+	$powerman -h $testaddr -T -q t[1-15] >most_queryT.out &&
+	count=`grep "stat" most_queryT.out | wc -l` &&
+	test $count = 15
+'
+test_expect_success 'stop powerman daemon' '
+	kill -15 $(cat powermand4.pid) &&
+	wait
+'
 
 test_done
 
diff --git a/t/t0007-temperature.t b/t/t0007-temperature.t
index 04aea275..a447fbce 100755
--- a/t/t0007-temperature.t
+++ b/t/t0007-temperature.t
@@ -66,6 +66,105 @@ test_expect_success 'stop powerman daemon' '
 	kill -15 $(cat powermand.pid) &&
 	wait
 '
+test_expect_success 'create new powerman.conf with both status_temp and status_temp_all script' '
+	cat >powerman2.conf <<-EOT2
+	specification "vpc" {
+	    timeout 	5.0
+	    plug name { "0" "1" "2" "3" "4" "5" "6" "7" "8"
+	                "9" "10" "11" "12" "13" "14" "15" }
+	    script login {
+	        send "login\n"
+	        expect "[0-9]* OK\n"
+	        expect "[0-9]* vpc> "
+	    }
+	    script logout {
+	        send "logoff\n"
+	        expect "[0-9]* OK\n"
+	    }
+	    script status_temp {
+	        send "temp %s\n"
+	        expect "plug ([0-9]+): ([0-9]+)\n"
+	        setplugstate \$1 \$2
+	        expect "[0-9]* OK\n"
+	        expect "[0-9]* vpc> "
+	    }
+	    script status_temp_all {
+		send "temp *\n"
+		foreachplug {
+			expect "plug ([0-9]+): ([0-9]+)\n"
+			setplugstate \$1 \$2
+		}
+		expect "[0-9]* OK\n"
+		expect "[0-9]* vpc> "
+	    }
+	}
+	listen "$testaddr"
+	device "test0" "vpc" "$vpcd |&"
+	node "t[0-15]" "test0"
+	EOT2
+'
+test_expect_success 'start powerman daemon and wait for it to start' '
+	$powermand -c powerman2.conf &
+	echo $! >powermand2.pid &&
+	$powerman --retry-connect=100 --server-host=$testaddr -t >/dev/null
+'
+test_expect_success 'powerman -t works' '
+	$powerman -h $testaddr -t >all_query.out &&
+	cat >all_query.exp <<-EOT &&
+	t0: 83
+	t1: 84
+	t2: 85
+	t3: 86
+	t4: 87
+	t5: 88
+	t6: 89
+	t7: 90
+	t8: 91
+	t9: 92
+	t10: 93
+	t11: 94
+	t12: 95
+	t13: 96
+	t14: 97
+	t15: 98
+	EOT
+	test_cmp all_query.exp all_query.out
+'
+test_expect_success 'powerman -t uses temp_status_all script on all plugs' '
+	$powerman -h $testaddr -T -t >all_queryT.out &&
+	count=`grep "temp" all_queryT.out | wc -l` &&
+	test $count = 1
+'
+test_expect_success 'powerman -t t[1-15] works' '
+	$powerman -h $testaddr -t t[1-15] >most_query.out &&
+	cat >most_query.exp <<-EOT &&
+	t1: 84
+	t2: 85
+	t3: 86
+	t4: 87
+	t5: 88
+	t6: 89
+	t7: 90
+	t8: 91
+	t9: 92
+	t10: 93
+	t11: 94
+	t12: 95
+	t13: 96
+	t14: 97
+	t15: 98
+	EOT
+	test_cmp most_query.exp most_query.out
+'
+test_expect_success 'powerman -t uses temp_status script on not all plugs' '
+	$powerman -h $testaddr -T -t t[1-15] >most_queryT.out &&
+	count=`grep "temp" most_queryT.out | wc -l` &&
+	test $count = 15
+'
+test_expect_success 'stop powerman daemon' '
+	kill -15 $(cat powermand2.pid) &&
+	wait
+'
 
 test_done
 
diff --git a/t/t0008-beacon.t b/t/t0008-beacon.t
index b8427d28..7d74decc 100755
--- a/t/t0008-beacon.t
+++ b/t/t0008-beacon.t
@@ -79,6 +79,72 @@ test_expect_success 'stop powerman daemon' '
 	kill -15 $(cat powermand.pid) &&
 	wait
 '
+test_expect_success 'create new powerman.conf with both status_beacon and status_beacon_all script' '
+	cat >powerman2.conf <<-EOT2
+	specification "vpc" {
+	    timeout 	5.0
+	    plug name { "0" "1" "2" "3" "4" "5" "6" "7" "8"
+	                "9" "10" "11" "12" "13" "14" "15" }
+	    script login {
+	        send "login\n"
+	        expect "[0-9]* OK\n"
+	        expect "[0-9]* vpc> "
+	    }
+	    script logout {
+	        send "logoff\n"
+	        expect "[0-9]* OK\n"
+	    }
+	    script status_beacon {
+	        send "beacon %s\n"
+	        expect "plug ([0-9]+): (ON|OFF)\n"
+	        setplugstate \$1 \$2 on="ON" off="OFF"
+	        expect "[0-9]* OK\n"
+	        expect "[0-9]* vpc> "
+	    }
+	    script status_beacon_all {
+		send "beacon *\n"
+		foreachplug {
+			expect "plug ([0-9]+): (ON|OFF)\n"
+			setplugstate \$1 \$2 on="ON" off="OFF"
+		}
+		expect "[0-9]* OK\n"
+		expect "[0-9]* vpc> "
+	    }
+	}
+	listen "$testaddr"
+	device "test0" "vpc" "$vpcd |&"
+	node "t[0-15]" "test0"
+	EOT2
+'
+test_expect_success 'start powerman daemon and wait for it to start' '
+	$powermand -c powerman2.conf &
+	echo $! >powermand2.pid &&
+	$powerman --retry-connect=100 --server-host=$testaddr -b >/dev/null
+'
+test_expect_success 'powerman -b works' '
+	$powerman -h $testaddr -b >all_query.out &&
+	makeoutput "" "t[0-15]" "" >all_query.exp &&
+	test_cmp all_query.exp all_query.out
+'
+test_expect_success 'powerman -b uses beacon_status_all script on all plugs' '
+	$powerman -h $testaddr -T -b >all_queryT.out &&
+	count=`grep "beacon" all_queryT.out | wc -l` &&
+	test $count = 1
+'
+test_expect_success 'powerman -b t[1-15] works' '
+	$powerman -h $testaddr -b t[1-15] >most_query.out &&
+	makeoutput "" "t[1-15]" "" >most_query.exp &&
+	test_cmp most_query.exp most_query.out
+'
+test_expect_success 'powerman -b uses beacon_status script on not all plugs' '
+	$powerman -h $testaddr -T -b t[1-15] >most_queryT.out &&
+	count=`grep "beacon" most_queryT.out | wc -l` &&
+	test $count = 15
+'
+test_expect_success 'stop powerman daemon' '
+	kill -15 $(cat powermand2.pid) &&
+	wait
+'
 
 test_done
 

From ac7726c8b371d67c6f2e5b28ce0ad24b190b81e1 Mon Sep 17 00:00:00 2001
From: Albert Chu <chu11@llnl.gov>
Date: Sat, 2 Mar 2024 15:31:11 -0800
Subject: [PATCH 5/5] man: add info on status scripts in powerman.dev(5)

Problem: Recent updates altered when an "all" vs "singleton"
status script is called.  However this change is not documented.

Add information on why a user might want to specify both an
"all" and "singleton" status script for power status, beacon status,
and temperature status.
---
 man/powerman.dev.5.in | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/man/powerman.dev.5.in b/man/powerman.dev.5.in
index aec15574..073c990e 100644
--- a/man/powerman.dev.5.in
+++ b/man/powerman.dev.5.in
@@ -81,7 +81,11 @@ Get device in a state so login script will work
 (though hopefully disconnecting will do that too).
 .TP
 .I "status_all, status[%s]"
-Obtain plug state for all plugs or only the specified plug.
+Obtain plug state for all plugs or only the specified plug.  In most
+cases, only one script needs to be specified.  In some hardware where
+unpopulated plugs may be problematic, it may be beneficial to specify
+both.  If both scripts are specified, the status_all script will be
+called only when all plug are requested.
 .TP
 .I "on_all, on_ranged[%s], on[%s]"
 Power on all plugs, a range of plugs, or the specified plug.
@@ -107,11 +111,19 @@ and a probe into the node attached to a non-standby power source.
 Obtain temperature reading for all plugs or only the specified plug.
 Temperature is obtained by sampling a thermocouple in the node.
 Results are reported as a text string - not interpreted by Powerman
-beyond any regex chopping done by the script.
+beyond any regex chopping done by the script.  In most cases, only one
+script needs to be specified.  In some hardware where unpopulated
+plugs may be problematic, it may be beneficial to specify both.  If
+both scripts are specified, the status_all script will be called only
+when all plug are requested.
 .TP
 .I "status_beacon_all, status_beacon[%s]"
-Obtain beacon state for all plugs or only the specified plug.
-Some RPC's include a way to flash a light on a node.
+Obtain beacon state for all plugs or only the specified plug.  Some
+RPC's include a way to flash a light on a node.  In most cases, only
+one script needs to be specified.  In some hardware where unpopulated
+plugs may be problematic, it may be beneficial to specify both.  If
+both scripts are specified, the status_all script will be called only
+when all plugs are requested.
 .TP
 .I "beacon_on[%s]"
 Flash beacon on the specified plug.