Merge pull request #7949 from VersatusHPC/fix/table-commands

fix(table): refuse extra tabrestore fields and unknown chtab -d tables

perl-xCAT/xCAT/TableUtils.pm

@@ -2171,4 +2171,37 @@ sub rmnodegroups {
     $tabhd->setNodeAttribs($node, { groups => join(",", @newgroups) });
 }
 
+#-----------------------------------------------------------------------------
+
+
+=head3 delete_rows
+    delete the rows that match the keys from each named table
+
+    Arguments:
+      keys: a hash reference of key column values
+      names: the table names
+    Returns:
+      undef, or the name of the first table that does not open
+    Example:
+         my $missing = xCAT::TableUtils->delete_rows({ node => 'n1' }, 'nodehm', 'noderes');
+
+=cut
+
+#-----------------------------------------------------------------------------
+
+sub delete_rows {
+    my ($class, $keys, @names) = @_;
+
+    # Every table opens before the first delete, so a table that does not open leaves all of them unchanged.
+    my %tables;
+    foreach my $name (@names) {
+        $tables{$name} = xCAT::Table->new($name, -create => 1, -autocommit => 0) or return $name;
+    }
+    foreach my $name (@names) {
+        $tables{$name}->delEntries($keys);
+        $tables{$name}->commit;
+    }
+    return;
+}
+
 1;

xCAT-server/lib/xcat/plugins/tabutils.pm

@@ -430,6 +430,7 @@ sub tabrestore
         my $origline = $line;                  #save for error reporting
         my %record;
         my $col;
+        my $separated;    #a comma after the field of the last column starts one more field
         foreach $col (@colns)
         {
             if ($line =~ /^,/ or $line eq "")
@@ -438,7 +439,7 @@ sub tabrestore
                 if (!exists($auto_cols{$col})) {
                     $record{$col} = undef;
                 }
-                $line =~ s/^,//;
+                $separated = ($line =~ s/^,//);
             }
             elsif ($line =~ /^[^,]*"/)
             {    # We have stuff in quotes... pain...
@@ -501,7 +502,7 @@ sub tabrestore
                         elsif ($offset eq length($line) or $nextchar eq ',')
                         { #hit the end of the line or at least the end of the column
                             $ent = substr($line, 0, $offset, '');
-                            $line =~ s/^,//;
+                            $separated = ($line =~ s/^,//);
                             chop $ent;
                             $ent = substr($ent, 1);
                             $ent =~ s/""/"/g;
@@ -531,10 +532,10 @@ sub tabrestore
                 {
                     $record{$col} = $1;
                 }
-                $line =~ s/^([^,]+)(,|$)//;
+                $separated = ($line =~ s/^([^,]+)(,|$)//) && $2 eq ',';
             }
         }
-        if ($line)
+        if ($line ne '' or $separated)
         {
             $rollback = 1;
             $cb->({ error => "Too many fields on line $linenumber: $origline | $line", errorcode => 4 });

xCAT-server/sbin/chtab

@@ -6,6 +6,7 @@ BEGIN
 }
 use lib "$::XCATROOT/lib/perl";
 use xCAT::Table;
+use xCAT::TableUtils;
 use xCAT::NodeRange;
 use Getopt::Long;
 
@@ -97,10 +98,10 @@ if ($::DELETE) {
         usage;
         exit(1);
     }
-    for (@tables_to_del) {
-        $tables{$_} = xCAT::Table->new($_, -create => 1, -autocommit => 0);
-        $tables{$_}->delEntries(\%keyhash);
-        $tables{$_}->commit;
+    my $missing = xCAT::TableUtils->delete_rows(\%keyhash, @tables_to_del);
+    if (defined($missing)) {
+        print "Table $missing does not exist.\n";
+        exit(1);
     }
 }
 else {

xCAT-test/unit/tableutils_delete_rows.t

@@ -0,0 +1,47 @@
+#!/usr/bin/env perl
+use strict;
+use warnings;
+use FindBin;
+use lib "$FindBin::Bin/../../perl-xCAT";
+use File::Path qw(make_path);
+use File::Temp qw(tempdir);
+use Test::More;
+
+my $fixture = tempdir(CLEANUP => 1);
+make_path("$fixture/db");
+local $ENV{XCATCFG} = "SQLite:$fixture/db";
+require xCAT::Table;
+require xCAT::TableUtils;
+
+for my $table (qw(nodehm noderes)) {
+    xCAT::Table->new($table, -create => 1)->setAttribs({ node => $_ }, { comments => "row of $_" }) for qw(n1 n2 n3);
+}
+
+sub nodes {
+    my ($table) = @_;
+    return join(',', sort map { $_->{node} } xCAT::Table->new($table)->getAllAttribs('node'));
+}
+
+sub delete_rows {
+    my @args = @_;
+    my $missing = eval { xCAT::TableUtils->delete_rows(@args) };
+    return ($missing, $@);
+}
+
+my ($missing, $error) = delete_rows({ node => 'n1' }, 'nodehm', 'noderes');
+is($error,   '',    'rows are deleted from tables that open');
+is($missing, undef, 'no table is reported when every table opens');
+is(nodes('nodehm'),  'n2,n3', 'the row of the key is deleted from the first table');
+is(nodes('noderes'), 'n2,n3', 'the row of the key is deleted from the last table');
+
+for my $case ([ 'between two tables', 'nodehm', 'notatable', 'noderes' ], [ 'first', 'notatable', 'nodehm', 'noderes' ],
+    [ 'last', 'nodehm', 'noderes', 'notatable' ]) {
+    my ($label, @tables) = @$case;
+    ($missing, $error) = delete_rows({ node => 'n2' }, @tables);
+    is($error,   '',          "a missing table named $label does not die");
+    is($missing, 'notatable', "a missing table named $label is returned");
+    is(nodes('nodehm'),  'n2,n3', "nodehm keeps its rows when a missing table is named $label");
+    is(nodes('noderes'), 'n2,n3', "noderes keeps its rows when a missing table is named $label");
+}
+
+done_testing();

xCAT-test/unit/tabrestore_field_count.t

@@ -0,0 +1,55 @@
+#!/usr/bin/env perl
+use strict;
+use warnings;
+use FindBin;
+use lib "$FindBin::Bin/../lib", "$FindBin::Bin/../../perl-xCAT", "$FindBin::Bin/../../xCAT-server/lib/perl";
+use File::Path qw(make_path);
+use File::Temp qw(tempdir);
+use Test::More;
+use XCAT::Test::File qw(repo_path);
+
+my $fixture = tempdir(CLEANUP => 1);
+make_path("$fixture/db");
+local $ENV{XCATCFG} = "SQLite:$fixture/db";
+require xCAT::Table;
+require(repo_path('xCAT-server/lib/xcat/plugins/tabutils.pm'));
+
+# Restores the rows into the site table, which has 4 columns, over a row that was there before.
+sub restore {
+    my @rows = @_;
+    my $site = xCAT::Table->new('site', -create => 1);
+    $site->delEntries();
+    $site->setAttribs({ key => 'before' }, { value => 'kept' });
+    my @errors;
+    xCAT_plugin::tabutils::process_request({ command => ['tabrestore'], table => ['site'], data => [ '#key,value,comments,disable', @rows ] },
+        sub { push @errors, map { ref($_) ? @$_ : $_ } grep { defined } $_[0]{error} });
+    my %rows = map { ($_->{key} => join('|', map { $_ // '' } @{$_}{qw(value comments)})) } xCAT::Table->new('site')->getAllAttribs('key', 'value', 'comments');
+    return (join("\n", @errors), \%rows);
+}
+
+for my $case (
+    [ 'one field for each column',          '"k1","v1",,' ],
+    [ 'fewer fields than columns',          '"k1","v1"' ],
+    [ 'a quoted field that holds a comma',  '"k1","v1","a,b",', 'v1|a,b' ],
+    [ 'a quoted comment before the last field', '"k1","v1","note",', 'v1|note' ])
+{
+    my ($label, $row, $want) = @$case;
+    my ($errors, $rows) = restore($row);
+    is($errors, '', "$label: the row is restored without an error");
+    is_deeply($rows, { k1 => $want // 'v1|' }, "$label: the restore replaces the table with the row");
+}
+
+for my $case (
+    [ 'an empty field after the last column',         '"k1","v1",,,' ],
+    [ 'a field of 0 after the last column',           '"k1","v1",,,0' ],
+    [ 'an empty field after a quoted last column',    '"k1","v1",,"yes",' ],
+    [ 'an empty field after an unquoted last column', '"k1","v1",,yes,' ],
+    [ 'a text field after the last column',           '"k1","v1",,,extra' ])
+{
+    my ($label, $row) = @$case;
+    my ($errors, $rows) = restore('"k0","v0",,', $row);
+    like($errors, qr/Too many fields on line \d+: \Q$row\E/, "$label: the row is refused");
+    is_deeply($rows, { before => 'kept|' }, "$label: the restore is rolled back");
+}
+
+done_testing();