From 5d38d64fa42881c571db1566cfcb350a3da31269 Mon Sep 17 00:00:00 2001
From: Dietmar Maurer <dietmar@proxmox.com>
Date: Fri, 11 Apr 2014 11:32:32 +0200
Subject: [PATCH] improve concurrent update handling

compute digest per section.
---
 src/PVE/API2/Firewall/Cluster.pm |  8 +--
 src/PVE/API2/Firewall/Groups.pm  | 61 +++++++++++-----------
 src/PVE/API2/Firewall/Host.pm    |  8 +--
 src/PVE/API2/Firewall/IPSet.pm   | 89 +++++++++++++++-----------------
 src/PVE/API2/Firewall/Rules.pm   | 27 +++++-----
 src/PVE/API2/Firewall/VM.pm      |  7 +--
 src/PVE/Firewall.pm              | 78 ++++++++++++++++------------
 7 files changed, 139 insertions(+), 139 deletions(-)

diff --git a/src/PVE/API2/Firewall/Cluster.pm b/src/PVE/API2/Firewall/Cluster.pm
index 10ad062..8d575da 100644
--- a/src/PVE/API2/Firewall/Cluster.pm
+++ b/src/PVE/API2/Firewall/Cluster.pm
@@ -85,9 +85,7 @@ __PACKAGE__->register_method({
 
 	my $cluster_conf = PVE::Firewall::load_clusterfw_conf();
 
-	my $options = $cluster_conf->{options};
-
-	return $options;
+	return PVE::Firewall::copy_opject_with_digest($cluster_conf->{options});
     }});
 
 my $option_properties = {
@@ -121,6 +119,7 @@ __PACKAGE__->register_method({
 		description => "A list of settings you want to delete.",
 		optional => 1,
 	    },
+	    digest => get_standard_option('pve-config-digest'),
 	}),
     },
     returns => { type => "null" },
@@ -129,6 +128,9 @@ __PACKAGE__->register_method({
 
 	my $cluster_conf = PVE::Firewall::load_clusterfw_conf();
 
+	my (undef, $digest) = PVE::Firewall::copy_opject_with_digest($cluster_conf->{options});
+	PVE::Tools::assert_if_modified($digest, $param->{digest});
+
 	if ($param->{delete}) {
 	    foreach my $opt (PVE::Tools::split_list($param->{delete})) {
 		raise_param_exc({ delete => "no such option '$opt'" }) 
diff --git a/src/PVE/API2/Firewall/Groups.pm b/src/PVE/API2/Firewall/Groups.pm
index 5577d43..c5fdc8b 100644
--- a/src/PVE/API2/Firewall/Groups.pm
+++ b/src/PVE/API2/Firewall/Groups.pm
@@ -12,6 +12,25 @@ use Data::Dumper; # fixme: remove
 
 use base qw(PVE::RESTHandler);
 
+my $get_security_group_list = sub {
+    my ($cluster_conf) = @_;
+
+    my $res = [];
+    foreach my $group (keys %{$cluster_conf->{groups}}) {
+	my $data = { 
+	    name => $group,
+	};
+	if (my $comment = $cluster_conf->{group_comments}->{$group}) {
+	    $data->{comment} = $comment;
+	}
+	push @$res, $data;
+    }
+
+    my ($list, $digest) = PVE::Firewall::copy_list_with_digest($res);
+
+    return wantarray ? ($list, $digest) : $list;
+};
+
 __PACKAGE__->register_method({
     name => 'list_security_groups',
     path => '',
@@ -40,22 +59,7 @@ __PACKAGE__->register_method({
 
 	my $cluster_conf = PVE::Firewall::load_clusterfw_conf();
 
-	my $digest = $cluster_conf->{digest};
-
-	my $res = [];
-	foreach my $group (keys %{$cluster_conf->{groups}}) {
-	    my $data = { 
-		name => $group,
-		digest => $digest,
-		count => scalar(@{$cluster_conf->{groups}->{$group}}) 
-	    };
-	    if (my $comment = $cluster_conf->{group_comments}->{$group}) {
-		$data->{comment} = $comment;
-	    }
-	    push @$res, $data;
-	}
-
-	return $res;
+	return &$get_security_group_list($cluster_conf);
     }});
 
 __PACKAGE__->register_method({
@@ -85,20 +89,13 @@ __PACKAGE__->register_method({
 
 	my $cluster_conf = PVE::Firewall::load_clusterfw_conf();
 
-	my $digest = $cluster_conf->{digest};
-
-	PVE::Tools::assert_if_modified($digest, $param->{digest});
-
-	if (!$param->{rename}) {	
-	    foreach my $name (keys %{$cluster_conf->{groups}}) {
-		raise_param_exc({ name => "Security group '$name' already exists" }) 
-		    if $name eq $param->{name};
-	    }
-	}
-
 	if ($param->{rename}) {
+	    my (undef, $digest) = &$get_security_group_list($cluster_conf);
+	    PVE::Tools::assert_if_modified($digest, $param->{digest});
+
 	    raise_param_exc({ name => "Security group '$param->{rename}' does not exists" }) 
 		if !$cluster_conf->{groups}->{$param->{rename}};
+
 	    my $data = delete $cluster_conf->{groups}->{$param->{rename}};
 	    $cluster_conf->{groups}->{$param->{name}} = $data;
 	    if (my $comment = delete $cluster_conf->{group_comments}->{$param->{rename}}) {
@@ -106,6 +103,11 @@ __PACKAGE__->register_method({
 	    }
 	    $cluster_conf->{group_comments}->{$param->{name}} = $param->{comment} if defined($param->{comment});
 	} else {
+	    foreach my $name (keys %{$cluster_conf->{groups}}) {
+		raise_param_exc({ name => "Security group '$name' already exists" }) 
+		    if $name eq $param->{name};
+	    }
+
 	    $cluster_conf->{groups}->{$param->{name}} = [];
 	    $cluster_conf->{group_comments}->{$param->{name}} = $param->{comment} if defined($param->{comment});
 	}
@@ -134,10 +136,11 @@ __PACKAGE__->register_method({
 	    
 	my $cluster_conf = PVE::Firewall::load_clusterfw_conf();
 
-	PVE::Tools::assert_if_modified($cluster_conf->{digest}, $param->{digest});
-
 	return undef if !$cluster_conf->{groups}->{$param->{name}};
 
+	my (undef, $digest) = &$get_security_group_list($cluster_conf);
+	PVE::Tools::assert_if_modified($digest, $param->{digest});
+
 	die "Security group '$param->{name}' is not empty\n" 
 	    if scalar(@{$cluster_conf->{groups}->{$param->{name}}});
 
diff --git a/src/PVE/API2/Firewall/Host.pm b/src/PVE/API2/Firewall/Host.pm
index 0ebdcce..680bc58 100644
--- a/src/PVE/API2/Firewall/Host.pm
+++ b/src/PVE/API2/Firewall/Host.pm
@@ -68,13 +68,7 @@ __PACKAGE__->register_method({
 
 	my $hostfw_conf = PVE::Firewall::load_hostfw_conf();
 
-	my $options = $hostfw_conf->{options} || {};
-
-	my $digest = $hostfw_conf->{digest};
-
-	$options->{digest} = $digest;
-
-	return $options;
+	return PVE::Firewall::copy_opject_with_digest($hostfw_conf->{options});
     }});
 
 1;
diff --git a/src/PVE/API2/Firewall/IPSet.pm b/src/PVE/API2/Firewall/IPSet.pm
index 0e73dd4..c473f13 100644
--- a/src/PVE/API2/Firewall/IPSet.pm
+++ b/src/PVE/API2/Firewall/IPSet.pm
@@ -97,18 +97,7 @@ sub register_get_ipset {
 
 	    my ($fw_conf, $ipset) = $class->load_config($param);
 
-	    my $digest = $fw_conf->{digest};
-
-	    my $res = [];
-	    foreach my $entry (@$ipset) {
-		my $data = {digest => $digest};
-		foreach my $k (qw(cidr comment nomatch)) {
-		    $data->{$k} = $entry->{$k} if $entry->{$k};
-		}
-		push @$res, $data;
-	    }
-
-	    return $res;
+	    return PVE::Firewall::copy_list_with_digest($ipset);
 	}});
 }
 
@@ -180,11 +169,11 @@ sub register_read_ip {
 	    my ($param) = @_;
 
 	    my ($fw_conf, $ipset) = $class->load_config($param);
-	    my $digest = $fw_conf->{digest};
 
-	    foreach my $entry (@$ipset) {
+	    my $list = PVE::Firewall::copy_list_with_digest($ipset);
+
+	    foreach my $entry (@$list) {
 		if ($entry->{cidr} eq $param->{cidr}) {
-		    $entry->{digest} = $digest;
 		    return $entry;
 		}
 	    }
@@ -220,7 +209,9 @@ sub register_update_ip {
 
 	    my ($fw_conf, $ipset) = $class->load_config($param);
 
-	    PVE::Tools::assert_if_modified($fw_conf->{digest}, $param->{digest});
+	    my (undef, $digest) = PVE::Firewall::copy_list_with_digest($ipset);
+	    PVE::Tools::assert_if_modified($digest, $param->{digest});
+	    warn "TEST:$digest:$param->{digest}:\n";
 
 	    foreach my $entry (@$ipset) {
 		if($entry->{cidr} eq $param->{cidr}) {
@@ -260,7 +251,8 @@ sub register_delete_ip {
 
 	    my ($fw_conf, $ipset) = $class->load_config($param);
 
-	    PVE::Tools::assert_if_modified($fw_conf->{digest}, $param->{digest});
+	    my (undef, $digest) = PVE::Firewall::copy_list_with_digest($ipset);
+	    PVE::Tools::assert_if_modified($digest, $param->{digest});
 
 	    my $new = [];
    
@@ -320,6 +312,25 @@ use PVE::Firewall;
 
 use base qw(PVE::RESTHandler);
 
+my $get_ipset_list = sub {
+    my ($fw_conf) = @_;
+
+    my $res = [];
+    foreach my $name (keys %{$fw_conf->{ipset}}) {
+	my $data = { 
+	    name => $name,
+	};
+	if (my $comment = $fw_conf->{ipset_comments}->{$name}) {
+	    $data->{comment} = $comment;
+	}
+	push @$res, $data;
+    }
+
+    my ($list, $digest) = PVE::Firewall::copy_list_with_digest($res);
+
+    return wantarray ? ($list, $digest) : $list;
+};
+
 sub register_index {
     my ($class) = @_;
 
@@ -351,22 +362,7 @@ sub register_index {
 	    
 	    my $fw_conf = $class->load_config();
 
-	    my $digest = $fw_conf->{digest};
-
-	    my $res = [];
-	    foreach my $name (keys %{$fw_conf->{ipset}}) {
-		my $data = { 
-		    name => $name,
-		    digest => $digest,
-		    count => scalar(@{$fw_conf->{ipset}->{$name}}) 
-		};
-		if (my $comment = $fw_conf->{ipset_comments}->{$name}) {
-		    $data->{comment} = $comment;
-		}
-		push @$res, $data;
-	    }
-
-	    return $res;
+	    return &$get_ipset_list($fw_conf); 
 	}});
 }
 
@@ -400,27 +396,25 @@ sub register_create {
 	    
 	    my $fw_conf = $class->load_config();
 
-	    my $digest = $fw_conf->{digest};
-
-	    PVE::Tools::assert_if_modified($digest, $param->{digest});
-
-	    if (!$param->{rename}) {
-		foreach my $name (keys %{$fw_conf->{ipset}}) {
-		    raise_param_exc({ name => "IPSet '$name' already exists" }) 
-			if $name eq $param->{name};
-		}
-	    }
-
 	    if ($param->{rename}) {
+		my (undef, $digest) = &$get_ipset_list($fw_conf);
+		PVE::Tools::assert_if_modified($digest, $param->{digest});
+
 		raise_param_exc({ name => "IPSet '$param->{rename}' does not exists" }) 
 		    if !$fw_conf->{ipset}->{$param->{rename}};
+
 		my $data = delete $fw_conf->{ipset}->{$param->{rename}};
 		$fw_conf->{ipset}->{$param->{name}} = $data;
 		if (my $comment = delete $fw_conf->{ipset_comments}->{$param->{rename}}) {
 		    $fw_conf->{ipset_comments}->{$param->{name}} = $comment;
 		}
 		$fw_conf->{ipset_comments}->{$param->{name}} = $param->{comment} if defined($param->{comment});
-	    } else {
+	    } else { 
+		foreach my $name (keys %{$fw_conf->{ipset}}) {
+		    raise_param_exc({ name => "IPSet '$name' already exists" }) 
+			if $name eq $param->{name};
+		}
+
 		$fw_conf->{ipset}->{$param->{name}} = [];
 		$fw_conf->{ipset_comments}->{$param->{name}} = $param->{comment} if defined($param->{comment});
 	    }
@@ -453,10 +447,11 @@ sub register_delete {
 	    
 	    my $fw_conf = $class->load_config();
 
-	    PVE::Tools::assert_if_modified($fw_conf->{digest}, $param->{digest});
-
 	    return undef if !$fw_conf->{ipset}->{$param->{name}};
 
+	    my (undef, $digest) = &$get_ipset_list($fw_conf);
+	    PVE::Tools::assert_if_modified($digest, $param->{digest});
+
 	    die "IPSet '$param->{name}' is not empty\n" 
 		if scalar(@{$fw_conf->{ipset}->{$param->{name}}});
 
diff --git a/src/PVE/API2/Firewall/Rules.pm b/src/PVE/API2/Firewall/Rules.pm
index 0054dfc..65fe8a6 100644
--- a/src/PVE/API2/Firewall/Rules.pm
+++ b/src/PVE/API2/Firewall/Rules.pm
@@ -82,16 +82,14 @@ sub register_get_rules {
 
 	    my ($fw_conf, $rules) = $class->load_config($param);
 
-	    my $digest = $fw_conf->{digest};
-
-	    my $res = [];
+	    my ($list, $digest) = PVE::Firewall::copy_list_with_digest($rules);
 
 	    my $ind = 0;
-	    foreach my $rule (@$rules) {
-		push @$res, PVE::Firewall::cleanup_fw_rule($rule, $digest, $ind++);
+	    foreach my $rule (@$list) {
+		$rule->{pos} = $ind++;
 	    }
 
-	    return $res;
+	    return $list;
 	}});
 }
 
@@ -124,13 +122,14 @@ sub register_get_rule {
 
 	    my ($fw_conf, $rules) = $class->load_config($param);
 
-	    my $digest = $fw_conf->{digest};
+	    my ($list, $digest) = PVE::Firewall::copy_list_with_digest($rules);
 	
-	    die "no rule at position $param->{pos}\n" if $param->{pos} >= scalar(@$rules);
+	    die "no rule at position $param->{pos}\n" if $param->{pos} >= scalar(@$list);
 	
-	    my $rule = $rules->[$param->{pos}];
-	    
-	    return PVE::Firewall::cleanup_fw_rule($rule, $digest, $param->{pos});
+	    my $rule = $list->[$param->{pos}];
+	    $rule->{pos} = $param->{pos};
+
+	    return $rule;
 	}});
 }
 
@@ -212,7 +211,8 @@ sub register_update_rule {
 
 	    my ($fw_conf, $rules) = $class->load_config($param);
 
-	    PVE::Tools::assert_if_modified($fw_conf->{digest}, $param->{digest});
+	    my (undef, $digest) = PVE::Firewall::copy_list_with_digest($rules);
+	    PVE::Tools::assert_if_modified($digest, $param->{digest});
 
 	    die "no rule at position $param->{pos}\n" if $param->{pos} >= scalar(@$rules);
 	
@@ -274,7 +274,8 @@ sub register_delete_rule {
 
 	    my ($fw_conf, $rules) = $class->load_config($param);
 
-	    PVE::Tools::assert_if_modified($fw_conf->{digest}, $param->{digest});
+	    my (undef, $digest) = PVE::Firewall::copy_list_with_digest($rules);
+	    PVE::Tools::assert_if_modified($digest, $param->{digest});
 	
 	    die "no rule at position $param->{pos}\n" if $param->{pos} >= scalar(@$rules);
 	
diff --git a/src/PVE/API2/Firewall/VM.pm b/src/PVE/API2/Firewall/VM.pm
index 3d39978..b143a39 100644
--- a/src/PVE/API2/Firewall/VM.pm
+++ b/src/PVE/API2/Firewall/VM.pm
@@ -77,13 +77,8 @@ __PACKAGE__->register_method({
 
 	my $vmfw_conf = PVE::Firewall::load_vmfw_conf($vmid);
 
-	my $options = $vmfw_conf->{options} || {};
+	return PVE::Firewall::copy_opject_with_digest($vmfw_conf->{options});
 
-	my $digest = $vmfw_conf->{digest};
-
-	$options->{digest} = $digest;
-
-	return $options;
     }});
 
 1;
diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
index 93e53f5..ca57a24 100644
--- a/src/PVE/Firewall.pm
+++ b/src/PVE/Firewall.pm
@@ -784,6 +784,50 @@ sub pve_fw_verify_protocol_spec {
 
 # helper function for API
 
+sub copy_opject_with_digest {
+    my ($object) = @_;
+
+    my $sha = Digest::SHA->new('sha1');
+
+    my $res = {};
+    foreach my $k (sort keys %$object) {
+	my $v = $object->{$k};
+	$res->{$k} = $v;
+	$sha->add($k, ':', $v, "\n");
+    }
+
+    my $digest = $sha->b64digest;
+
+    $res->{digest} = $digest;
+
+    return wantarray ? ($res, $digest) : $res;
+}
+
+sub copy_list_with_digest {
+    my ($list) = @_;
+
+    my $sha = Digest::SHA->new('sha1');
+
+    my $res = [];
+    foreach my $entry (@$list) {
+	my $data = {};
+	foreach my $k (sort keys %$entry) {
+	    my $v = $entry->{$k};
+	    $data->{$k} = $v;
+	    $sha->add($k, ':', $v, "\n");
+	}
+	push @$res, $data;
+    }
+
+    my $digest = $sha->b64digest;
+
+    foreach my $entry (@$res) {
+	$entry->{digest} = $digest;
+    }
+
+    return wantarray ? ($res, $digest) : $res;
+}
+
 my $rule_properties = {
     pos => {
 	description => "Update rule at position <pos>.",
@@ -841,23 +885,6 @@ my $rule_properties = {
     },
 };
 
-sub cleanup_fw_rule {
-    my ($rule, $digest, $pos) = @_;
-
-    my $r = {};
-
-    foreach my $k (keys %$rule) {
-	next if !$rule_properties->{$k};
-	my $v = $rule->{$k};
-	next if !defined($v);
-	$r->{$k} = $v;
-	$r->{digest} = $digest;
-	$r->{pos} = $pos;
-    }
-
-    return $r;
-}
-
 sub add_rule_properties {
     my ($properties) = @_;
 
@@ -1864,11 +1891,7 @@ sub parse_vm_fw_rules {
 
     my $section;
 
-    my $digest = Digest::SHA->new('sha1');
-
     while (defined(my $line = <$fh>)) {
-	$digest->add($line);
-
 	next if $line =~ m/^#/;
 	next if $line =~ m/^\s*$/;
 
@@ -1906,8 +1929,6 @@ sub parse_vm_fw_rules {
 	push @{$res->{$section}}, $rule;
     }
 
-    $res->{digest} = $digest->b64digest;
-
     return $res;
 }
 
@@ -1918,11 +1939,7 @@ sub parse_host_fw_rules {
 
     my $section;
 
-    my $digest = Digest::SHA->new('sha1');
-
     while (defined(my $line = <$fh>)) {
-	$digest->add($line);
-
 	next if $line =~ m/^#/;
 	next if $line =~ m/^\s*$/;
 
@@ -1960,8 +1977,6 @@ sub parse_host_fw_rules {
 	push @{$res->{$section}}, $rule;
     }
 
-    $res->{digest} = $digest->b64digest;
-
     return $res;
 }
 
@@ -1980,11 +1995,7 @@ sub parse_cluster_fw_rules {
 	ipset_comments => {}, 
     };
 
-    my $digest = Digest::SHA->new('sha1');
-
     while (defined(my $line = <$fh>)) {
-	$digest->add($line);
-
 	next if $line =~ m/^#/;
 	next if $line =~ m/^\s*$/;
 
@@ -2072,7 +2083,6 @@ sub parse_cluster_fw_rules {
 	}
     }
 
-    $res->{digest} = $digest->b64digest;
     return $res;
 }
 
-- 
2.39.2