From 09e8a21ebc788ae2a1b24dad63068b780ad98336 Mon Sep 17 00:00:00 2001 From: Albert Chu Date: Thu, 15 Feb 2024 12:54:05 -0800 Subject: [PATCH 01/10] man: fix redfishpower(8) cut and paste errors Problem: The "cycle" configs and commands have clear cut and paste errors from the off config/commands. Fix cut and paste errors. --- man/redfishpower.8.in | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/man/redfishpower.8.in b/man/redfishpower.8.in index 5fe2e54d..076afd79 100644 --- a/man/redfishpower.8.in +++ b/man/redfishpower.8.in @@ -67,7 +67,7 @@ Set path and optional post data to turn on node. Set path and optional post data to turn off node. .TP .I "setcyclepath [data]" -Set path and optional post data to turn cycle node. +Set path and optional post data to cycle node. .TP .I "settimeout " Set command timeout in seconds. @@ -82,7 +82,7 @@ Turn on all nodes or specified subset of nodes. Will return "ok" after confirma Turn off all nodes or specified subset of nodes. Will return "ok" after confirmation "off" has completed. .TP .I "cycle [nodes]" -Turn off all nodes or specified subset of nodes. +Power cycle all nodes or specified subset of nodes. .SH "UPDATING REDFISHPOWER DEVICE FILES" .LP From f960fb4c5340d8c2d3c5372e397a44b6202d684c Mon Sep 17 00:00:00 2001 From: Albert Chu Date: Sat, 17 Feb 2024 12:15:44 -0800 Subject: [PATCH 02/10] man: document common header Problem: Typical configurations are documented in redfishpower(8) except for the common header. Document common header configuration. --- man/redfishpower.8.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/man/redfishpower.8.in b/man/redfishpower.8.in index 076afd79..931280a0 100644 --- a/man/redfishpower.8.in +++ b/man/redfishpower.8.in @@ -21,7 +21,7 @@ set simultaneously is limited by the file descriptor limit of the system call. .TP .I "-H, --header string" -Set extra HEADER to use. +Set extra HEADER to use. Typically is Content-Type:application/json. .TP .I "-S, --statpath string" Set Redfish path for obtaining power status. Typically is redfish/v1/Systems/1. From ba89491de74a716dcd5f0b6d4c2306632d5a5391 Mon Sep 17 00:00:00 2001 From: Albert Chu Date: Tue, 13 Feb 2024 16:01:52 -0800 Subject: [PATCH 03/10] redfishpower: fix tabbing / whitespace Problem: The tabbing / whitespace in a few functions was wrong. Fix the tabbing / whitespace. --- src/redfishpower/redfishpower.c | 66 ++++++++++++++++----------------- 1 file changed, 33 insertions(+), 33 deletions(-) diff --git a/src/redfishpower/redfishpower.c b/src/redfishpower/redfishpower.c index 49e5ab2a..45aa6d30 100644 --- a/src/redfishpower/redfishpower.c +++ b/src/redfishpower/redfishpower.c @@ -149,26 +149,26 @@ void help(void) static size_t output_cb(void *contents, size_t size, size_t nmemb, void *userp) { - size_t realsize = size * nmemb; - struct powermsg *pm = userp; - - if (pm->output) { - char *tmp = calloc(1, pm->output_len + realsize + 1); - if (!tmp) - err_exit(true, "calloc"); - memcpy(tmp, pm->output, pm->output_len); - memcpy(tmp + pm->output_len, contents, realsize); - pm->output_len += realsize; - free(pm->output); - pm->output = tmp; - } - else { - if (!(pm->output = calloc(1, realsize + 1))) - err_exit(true, "calloc"); - memcpy(pm->output, contents, realsize); - pm->output_len = realsize; - } - return realsize; + size_t realsize = size * nmemb; + struct powermsg *pm = userp; + + if (pm->output) { + char *tmp = calloc(1, pm->output_len + realsize + 1); + if (!tmp) + err_exit(true, "calloc"); + memcpy(tmp, pm->output, pm->output_len); + memcpy(tmp + pm->output_len, contents, realsize); + pm->output_len += realsize; + free(pm->output); + pm->output = tmp; + } + else { + if (!(pm->output = calloc(1, realsize + 1))) + err_exit(true, "calloc"); + memcpy(pm->output, contents, realsize); + pm->output_len = realsize; + } + return realsize; } static struct powermsg *powermsg_create(CURLM *mh, @@ -410,7 +410,7 @@ static void parse_onoff(struct powermsg *pm, const char **strp) } } -static void stat_process (struct powermsg *pm) +static void stat_process(struct powermsg *pm) { const char *str; parse_onoff(pm, &str); @@ -683,18 +683,18 @@ static void setpowerpath(char **av, char **path, char **postdata) static void settimeout(char **av) { - if (av[0]) { - char *endptr; - long tmp; - - errno = 0; - tmp = strtol (av[0], &endptr, 10); - if (errno - || endptr[0] != '\0' - || tmp <= 0) - printf("invalid timeout specified\n"); - cmd_timeout = tmp; - } + if (av[0]) { + char *endptr; + long tmp; + + errno = 0; + tmp = strtol (av[0], &endptr, 10); + if (errno + || endptr[0] != '\0' + || tmp <= 0) + printf("invalid timeout specified\n"); + cmd_timeout = tmp; + } } static void process_cmd(zlistx_t *activecmds, CURLM *mh, char **av, int *exitflag) From eb7c57d5cc5406aeb7b947a16b275050bba01cd7 Mon Sep 17 00:00:00 2001 From: Albert Chu Date: Wed, 14 Feb 2024 13:02:29 -0800 Subject: [PATCH 04/10] redfishpower: remove unnecessary initialization Problem: Some variables are initialized to NULL when it's unnecessary. Do not initialize variables unless its necessary. --- src/redfishpower/redfishpower.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/redfishpower/redfishpower.c b/src/redfishpower/redfishpower.c index 45aa6d30..bb6883ed 100644 --- a/src/redfishpower/redfishpower.c +++ b/src/redfishpower/redfishpower.c @@ -46,7 +46,7 @@ static char *cyclepath = NULL; static char *cyclepostdata = NULL; static int test_mode = 0; -static zhashx_t *test_power_status = NULL; +static zhashx_t *test_power_status; /* in seconds */ #define MESSAGE_TIMEOUT 10 @@ -329,9 +329,9 @@ static struct powermsg *stat_cmd_host(CURLM * mh, char *hostname) static void stat_cmd(zlistx_t *activecmds, CURLM *mh, char **av) { - hostlist_iterator_t itr = NULL; + hostlist_iterator_t itr; char *hostname; - hostlist_t *hostsptr = NULL; + hostlist_t *hostsptr; hostlist_t lhosts = NULL; if (!statpath) { @@ -452,9 +452,9 @@ static void power_cmd(zlistx_t *activecmds, const char *path, const char *postdata) { - hostlist_iterator_t itr = NULL; + hostlist_iterator_t itr; char *hostname; - hostlist_t *hostsptr = NULL; + hostlist_t *hostsptr; hostlist_t lhosts = NULL; if (!path) { @@ -1060,7 +1060,7 @@ int main(int argc, char *argv[]) } else { /* All hosts initially are off for testing */ - hostlist_iterator_t itr = NULL; + hostlist_iterator_t itr; char *hostname; if (!(itr = hostlist_iterator_create(hosts))) err_exit(true, "hostlist_iterator_create"); From cb76b96a4ffd861134810a7a9d5d9626a8703c01 Mon Sep 17 00:00:00 2001 From: Albert Chu Date: Tue, 13 Feb 2024 16:09:27 -0800 Subject: [PATCH 05/10] redfishpower: correct potential segfault Problem: Code in settimeout() may index outside of array bounds. Only check if a value is non-NULL if the prior value was non-NULL. --- src/redfishpower/redfishpower.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/redfishpower/redfishpower.c b/src/redfishpower/redfishpower.c index bb6883ed..781e5524 100644 --- a/src/redfishpower/redfishpower.c +++ b/src/redfishpower/redfishpower.c @@ -675,10 +675,11 @@ static void setpowerpath(char **av, char **path, char **postdata) xfree(*postdata); *postdata = NULL; } - if (av[0]) + if (av[0]) { (*path) = xstrdup(av[0]); - if (av[1]) - (*postdata) = xstrdup(av[1]); + if (av[1]) + (*postdata) = xstrdup(av[1]); + } } static void settimeout(char **av) From 60028539903aefff18cc6426bd0c1f8e1e0158bc Mon Sep 17 00:00:00 2001 From: Albert Chu Date: Tue, 13 Feb 2024 11:52:55 -0800 Subject: [PATCH 06/10] redfishpower: use terms consistently Problem: The following terms are used interchangeably within the redfishpower code: path & url data & postdata Solution: Consistently use the term "path" to describe "paths" input by the user for configuration and use the term "postdata" for http post data. The term "url" should only be used when the full URL is constructed (i.e. when http:// is prefixed). Update terms in the manpage as well. --- man/redfishpower.8.in | 6 +++--- src/redfishpower/redfishpower.c | 8 ++++---- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/man/redfishpower.8.in b/man/redfishpower.8.in index 931280a0..994a9086 100644 --- a/man/redfishpower.8.in +++ b/man/redfishpower.8.in @@ -60,13 +60,13 @@ Set extra HEADER to use. Do not specify data to clear. .I "setstatpath " Set path to obtain power status. .TP -.I "setonpath [data]" +.I "setonpath [postdata]" Set path and optional post data to turn on node. .TP -.I "setoffpath [data]" +.I "setoffpath [postdata]" Set path and optional post data to turn off node. .TP -.I "setcyclepath [data]" +.I "setcyclepath [postdata]" Set path and optional post data to cycle node. .TP .I "settimeout " diff --git a/src/redfishpower/redfishpower.c b/src/redfishpower/redfishpower.c index 781e5524..14612d20 100644 --- a/src/redfishpower/redfishpower.c +++ b/src/redfishpower/redfishpower.c @@ -136,10 +136,10 @@ void help(void) printf("Valid commands are:\n"); printf(" auth user:passwd\n"); printf(" setheader string\n"); - printf(" setstatpath url\n"); - printf(" setonpath url [data]\n"); - printf(" setoffpath url [data]\n"); - printf(" setcyclepath url [data]\n"); + printf(" setstatpath path\n"); + printf(" setonpath path [postdata]\n"); + printf(" setoffpath path [postdata]\n"); + printf(" setcyclepath path [postdata]\n"); printf(" settimeout seconds\n"); printf(" stat [nodes]\n"); printf(" on [nodes]\n"); From a4d006b9e9eaadcc0526b08b349a05b37b970028 Mon Sep 17 00:00:00 2001 From: Albert Chu Date: Wed, 21 Feb 2024 11:41:59 -0800 Subject: [PATCH 07/10] redfishpower: set delayedcmds list item destructor Problem: The item destructor was not set for the delayedcmds list. This is because items are typically moved from delayedcmds to activecmds, but there is always the chance of an error occuring. Set the item destructor for the delayedcmds list. --- src/redfishpower/redfishpower.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/redfishpower/redfishpower.c b/src/redfishpower/redfishpower.c index 14612d20..f8e16a7b 100644 --- a/src/redfishpower/redfishpower.c +++ b/src/redfishpower/redfishpower.c @@ -759,6 +759,7 @@ static void shell(CURLM *mh) if (!(delayedcmds = zlistx_new())) err_exit(true, "zlistx_new"); + zlistx_set_destructor(delayedcmds, cleanup_powermsg); while (exitflag == 0) { CURLMcode mc; From a77ca39b3422d2ba3d46a055c8a7c60a040c6a59 Mon Sep 17 00:00:00 2001 From: Albert Chu Date: Wed, 21 Feb 2024 08:34:08 -0800 Subject: [PATCH 08/10] redfishpower: add cleanup function for globals Problem: Globals are initialized in a init function, but we manually cleanup globals inside main(). Add a cleanup function to call when cleaning up globals. --- src/redfishpower/redfishpower.c | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/src/redfishpower/redfishpower.c b/src/redfishpower/redfishpower.c index f8e16a7b..2127f8df 100644 --- a/src/redfishpower/redfishpower.c +++ b/src/redfishpower/redfishpower.c @@ -998,6 +998,22 @@ static void init_redfishpower(char *argv[]) err_exit(false, "zhashx_new error"); } +static void cleanup_redfishpower(void) +{ + xfree(userpwd); + xfree(statpath); + xfree(onpath); + xfree(onpostdata); + xfree(offpath); + xfree(offpostdata); + xfree(cyclepath); + xfree(cyclepostdata); + + hostlist_destroy(hosts); + + zhashx_destroy(&test_power_status); +} + int main(int argc, char *argv[]) { CURLM *mh = NULL; @@ -1079,16 +1095,7 @@ int main(int argc, char *argv[]) if (!test_mode) curl_multi_cleanup(mh); - xfree(userpwd); - hostlist_destroy(hosts); - xfree(statpath); - xfree(onpath); - xfree(onpostdata); - xfree(offpath); - xfree(offpostdata); - xfree(cyclepath); - xfree(cyclepostdata); - zhashx_destroy(&test_power_status); + cleanup_redfishpower(); exit(0); } From 37b3b946601e5f30e00106bb0fa132ef71bec6d1 Mon Sep 17 00:00:00 2001 From: Albert Chu Date: Thu, 22 Feb 2024 08:19:17 -0800 Subject: [PATCH 09/10] redfishpower: add option to test host errors Problem: It would be nice to cover failures in redfishpower's test mode. When running under --test-mode, support a new --test-error option that allows users to set a group of hosts to permanently return errors when communicating with the hosts. --- src/redfishpower/redfishpower.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/src/redfishpower/redfishpower.c b/src/redfishpower/redfishpower.c index 2127f8df..8b58155d 100644 --- a/src/redfishpower/redfishpower.c +++ b/src/redfishpower/redfishpower.c @@ -46,6 +46,7 @@ static char *cyclepath = NULL; static char *cyclepostdata = NULL; static int test_mode = 0; +static hostlist_t test_fail_power_cmd_hosts; static zhashx_t *test_power_status; /* in seconds */ @@ -113,7 +114,7 @@ struct powermsg { err_exit(false, "curl_easy_setopt: %s", curl_easy_strerror(_ec)); \ } while(0) -#define OPTIONS "h:H:S:O:F:C:P:G:D:Tv" +#define OPTIONS "h:H:S:O:F:C:P:G:D:TEv" static struct option longopts[] = { {"hostname", required_argument, 0, 'h' }, {"header", required_argument, 0, 'H' }, @@ -125,6 +126,7 @@ static struct option longopts[] = { {"offpostdata", required_argument, 0, 'G' }, {"cyclepostdata", required_argument, 0, 'D' }, {"test-mode", no_argument, 0, 'T' }, + {"test-fail-power-cmd-hosts", required_argument, 0, 'E' }, {"verbose", no_argument, 0, 'v' }, {0,0,0,0}, }; @@ -958,7 +960,10 @@ static void shell(CURLM *mh) /* in test mode we assume all activecmds complete immediately */ struct powermsg *pm = zlistx_first(activecmds); while (pm) { - power_cmd_process(delayedcmds, pm); + if (hostlist_find(test_fail_power_cmd_hosts, pm->hostname) >= 0) + printf("%s: %s\n", pm->hostname, "error"); + else + power_cmd_process(delayedcmds, pm); fflush(stdout); zlistx_detach_cur(activecmds); pm = zlistx_next(activecmds); @@ -994,6 +999,9 @@ static void init_redfishpower(char *argv[]) if (!(hosts = hostlist_create(NULL))) err_exit(true, "hostlist_create error"); + if (!(test_fail_power_cmd_hosts = hostlist_create(NULL))) + err_exit(true, "hostlist_create error"); + if (!(test_power_status = zhashx_new ())) err_exit(false, "zhashx_new error"); } @@ -1011,6 +1019,7 @@ static void cleanup_redfishpower(void) hostlist_destroy(hosts); + hostlist_destroy(test_fail_power_cmd_hosts); zhashx_destroy(&test_power_status); } @@ -1055,6 +1064,10 @@ int main(int argc, char *argv[]) case 'T': /* --test-mode */ test_mode = 1; break; + case 'E': /* --test-fail-power-cmd-hosts */ + if (!hostlist_push(test_fail_power_cmd_hosts, optarg)) + err_exit(true, "hostlist_push error on %s", optarg); + break; case 'v': /* --verbose */ verbose = 1; break; From 07b3a3eaa2d8f1ec8b7f5d76e282ef4d3df36267 Mon Sep 17 00:00:00 2001 From: Albert Chu Date: Thu, 15 Feb 2024 22:04:32 -0800 Subject: [PATCH 10/10] t: cover redfishpower host errors Problem: There is no coverage for when targetted hosts return errors. Add basic coverage in t0029-redfish.t. --- t/t0029-redfish.t | 42 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/t/t0029-redfish.t b/t/t0029-redfish.t index 7d4590e8..de4ee360 100755 --- a/t/t0029-redfish.t +++ b/t/t0029-redfish.t @@ -20,6 +20,10 @@ makeoutput() { printf "unknown: %s\n" $3 } +# +# redfishpower core tests +# + test_expect_success 'create powerman.conf for 16 cray redfish nodes' ' cat >powerman.conf <<-EOT listen "$testaddr" @@ -102,6 +106,44 @@ test_expect_success 'stop powerman daemon' ' kill -15 $(cat powermand.pid) && wait ' + +# +# redfishpower fail hosts coverage +# + +test_expect_success 'create powerman.conf for 16 cray redfish nodes (failhosts)' ' + cat >powerman_fail_hosts.conf <<-EOT + listen "$testaddr" + include "$devicesdir/redfishpower-cray-r272z30.dev" + device "d0" "redfishpower-cray-r272z30" "$redfishdir/redfishpower -h t[0-15] --test-mode --test-fail-power-cmd-hosts=t[8-15] |&" + node "t[0-15]" "d0" + EOT +' +test_expect_success 'start powerman daemon and wait for it to start (failhosts)' ' + $powermand -Y -c powerman_fail_hosts.conf & + echo $! >powermand.pid && + $powerman --retry-connect=100 --server-host=$testaddr -d +' +test_expect_success 'powerman -q shows t[0-7] off, t[8-15] unknown' ' + $powerman -h $testaddr -q >test_failhosts_query.out && + makeoutput "" "t[0-7]" "t[8-15]" >test_failhosts_query.exp && + test_cmp test_failhosts_query.exp test_failhosts_query.out +' +test_expect_success 'powerman -1 t[0-15] completes' ' + $powerman -h $testaddr -1 t[0-15] >test_failhosts_on.out && + echo Command completed successfully >test_failhosts_on.exp && + test_cmp test_failhosts_on.exp test_failhosts_on.out +' +test_expect_success 'powerman -q shows t[0-7] on' ' + $powerman -h $testaddr -q >test_failhosts_query2.out && + makeoutput "t[0-7]" "" "t[8-15]" >test_failhosts_query2.exp && + test_cmp test_failhosts_query2.exp test_failhosts_query2.out +' +test_expect_success 'stop powerman daemon (failhosts)' ' + kill -15 $(cat powermand.pid) && + wait +' + test_done # vi: set ft=sh