From d5f882291d1b7b8ce1178fe988e3d296149bd440 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vin=C3=ADcius=20Ferr=C3=A3o?= <2031761+viniciusferrao@users.noreply.github.com> Date: Sun, 23 Aug 2026 02:16:51 -0300 Subject: [PATCH 1/2] fix(nodestat): accept the fping option that the usage message gives The manual page and the usage message of nodestat give the option -f|--usefping. The preprocessor of the request knows no option f, and the handler of the request spells the long name useping, so --usefping does nothing. An administrator who follows the manual page gets the nmap path, and gets no message that says why. The two places also read different specifications, so an option that one place accepts can reach the other place and take a different meaning. Put the specification in one routine, and let both places read that routine. Give the name usefping to the option, and keep useping as a second name. That spelling has worked since 2.14.2, so a site can have it in a script. The long name of the fping option starts with the same letters as usemon, so --use and --us become names that Getopt::Long cannot decide. Those two abbreviations select usemon today. Keep them with usemon, or an administrator who monitors with them loses the monitoring and gets no message. The change has two other effects. The abbreviations --use and --us no longer select fping as well, which they did only because the two places read different specifications. The bundles -mf and -fm now select both options, which they did not do before. Recovered from the lenovobuild branch. --- xCAT-server/lib/xcat/plugins/nodestat.pm | 34 +++++++++++++----------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/xCAT-server/lib/xcat/plugins/nodestat.pm b/xCAT-server/lib/xcat/plugins/nodestat.pm index 65f476022..e9fa159d2 100644 --- a/xCAT-server/lib/xcat/plugins/nodestat.pm +++ b/xCAT-server/lib/xcat/plugins/nodestat.pm @@ -116,6 +116,13 @@ sub getstat { =cut #------------------------------------------------------- +sub option_spec { + return ( + 'm|usemon|use|us', 'q|quiet', 'u|updatedb', 'p|powerstat', + 'f|usefping|useping', 'h|help', 'v|version', + ); +} + sub preprocess_request { my $req = shift; @@ -135,26 +142,23 @@ sub preprocess_request } # parse the options - $::UPDATE = 0; - $::QUIET = 0; - $::MON = 0; - $::POWER = 0; - #Getopt::Long::Configure("posix_default"); #Getopt::Long::Configure("no_gnu_compat"); Getopt::Long::Configure("bundling"); $Getopt::Long::ignorecase = 0; - if (!GetOptions( - 'm|usemon' => \$::MON, - 'q|quiet' => \$::QUIET, #this is a internal flag used by monitoring - 'u|updatedb' => \$::UPDATE, - 'p|powerstat' => \$::POWER, - 'h|help' => \$::HELP, - 'v|version' => \$::VERSION)) + my %opt; + if (!GetOptions(\%opt, option_spec())) { &usage($cb,1); return (1); } + $::MON = $opt{m} ? 1 : 0; + $::QUIET = $opt{q} ? 1 : 0; #this is a internal flag used by monitoring + $::UPDATE = $opt{u} ? 1 : 0; + $::POWER = $opt{p} ? 1 : 0; + $::USEFPING = $opt{f} ? 1 : 0; + $::HELP = $opt{h}; + $::VERSION = $opt{v}; if ($::HELP) { &usage($cb); return (0); @@ -922,9 +926,9 @@ sub process_request { my $usefping; if (ref $request->{arg}) { @ARGV = @{ $request->{arg} }; - GetOptions( - 'f|useping' => \$usefping - ); + my %opt; + GetOptions(\%opt, option_spec()); + $usefping = $opt{f}; } From eaa1e94a32f5aa50316ea04040601d5d62fcd4b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vin=C3=ADcius=20Ferr=C3=A3o?= <2031761+viniciusferrao@users.noreply.github.com> Date: Sun, 23 Aug 2026 02:16:52 -0300 Subject: [PATCH 2/2] test(nodestat): pin the fping option and the usemon abbreviations Add a unit test for the option that selects fping instead of nmap. The test takes the specification out of the plugin source and gives it to Getopt::Long with the settings that the daemon uses, so it drives the specification that the plugin ships. It shows that -f, --usefping and the older --useping each select fping, that --use and --us still select usemon and do not select fping, that the bundles -mf and -fm select both options, and that both places parse through the one specification. --- xCAT-test/unit/nodestat_usefping_option.t | 79 +++++++++++++++++++++++ 1 file changed, 79 insertions(+) create mode 100644 xCAT-test/unit/nodestat_usefping_option.t diff --git a/xCAT-test/unit/nodestat_usefping_option.t b/xCAT-test/unit/nodestat_usefping_option.t new file mode 100644 index 000000000..c460d692b --- /dev/null +++ b/xCAT-test/unit/nodestat_usefping_option.t @@ -0,0 +1,79 @@ +#!/usr/bin/env perl +use strict; +use warnings; + +use FindBin; +use File::Spec; +use Test::More; +use Getopt::Long qw(GetOptionsFromArray); + +my $plugin = File::Spec->catfile( $FindBin::Bin, '..', '..', + 'xCAT-server', 'lib', 'xcat', 'plugins', 'nodestat.pm' ); +plan skip_all => 'nodestat.pm not found' unless -r $plugin; + +open( my $fh, '<', $plugin ) or die "Unable to read $plugin: $!"; +my $source = do { local $/; <$fh> }; +close($fh); + +# nodestat parses its arguments twice, once in the preprocessor and once in the +# handler. Both have to read the same specification, or an option that one +# accepts is dropped or misread by the other. +my $calls = () = $source =~ /GetOptions\s*\(\s*\\%opt,\s*option_spec\(\)\s*\)/g; +is( $calls, 2, 'both places parse through the one specification' ); + +my ($routine) = $source =~ /(sub option_spec \{.*?\n\}\n)/s; +BAIL_OUT('could not extract option_spec from nodestat.pm') unless $routine; +eval "package NodestatSpec; $routine 1;" or BAIL_OUT("could not evaluate: $@"); + +# The daemon leaves Getopt::Long in pass_through, because xCAT::Usage sets it +# and the setting lasts for the life of the process. +sub parse { + my (@argv) = @_; + Getopt::Long::ConfigDefaults(); + Getopt::Long::Configure( 'pass_through', 'bundling' ); + $Getopt::Long::ignorecase = 0; + my %opt; + do { local $SIG{__WARN__} = sub { }; GetOptionsFromArray( \@argv, \%opt, NodestatSpec::option_spec() ) }; + return \%opt; +} + +# The option that the manual page and the usage message give. +foreach my $given (qw(-f --usefping)) { + ok( parse($given)->{f}, "$given selects fping" ); +} + +# The spelling that the code has carried since 2.14.2. A site can have it in a +# script, so it keeps working. +ok( parse('--useping')->{f}, '--useping still selects fping' ); + +# usemon owns these abbreviations. They were unambiguous before the fping +# option gained a long name beginning with the same letters, and an +# administrator who monitors with them must not silently lose monitoring. +foreach my $given (qw(--use --us --usemon -m)) { + my $opt = parse($given); + ok( $opt->{m}, "$given still selects usemon" ); + ok( !$opt->{f}, "$given does not select fping" ); +} + +# Short options bundle, so both orders have to give both settings. +foreach my $given (qw(-mf -fm)) { + my $opt = parse($given); + ok( $opt->{m} && $opt->{f}, "$given selects usemon and fping" ); +} + +# The other options keep their own letters. +is_deeply( + [ map { parse($_) } qw(-u -p -q) ], + [ { u => 1 }, { p => 1 }, { q => 1 } ], + 'the remaining options are unchanged' +); + +# The usage text is what an administrator types. +my ($usage) = $source =~ /(nodestat \[noderange\][^"]*)/; +ok( defined $usage, 'the usage message was found' ); +my ($long) = grep { /^f\|/ } NodestatSpec::option_spec(); +($long) = $long =~ /^f\|([^|]+)/; +ok( defined $usage && $usage =~ /--\Q$long\E/, + 'the usage message names the option that the code accepts' ); + +done_testing();