I've very recently started using BackupPC, and I like it very much. It offers excellent flexibility while still being easy to manage. I do have a few ideas for improvement though. Here's the first.
Attached is a proof-of-concept patch that implements config file handling in three levels: - a default config file provided with the source or by a distro this file should not be modified by users (admins) - a config file with local overrides for the main config - config files with per-PC overrides The last two both use the override mechanism for per-PC config file editing introduced with BackupPC 3.0. The patch is not very big; it is based on the Debian 3.0.0-3 package. I have tested the changes fairly extensively in a test setup, and it all works nicely. Note that I'm not really a Perl coder, so a careful review of the patches would be appreciated. This is proof-of-concept, so a final implementation would still need some work. Details below. Cheers, Frans Pop Rationale --------- The current solution where a full main config file is included in /etc/backuppc has a few disadvantages, especially when upgrading to new versions if the admin makes lots of local changes in the main config file: - partly because of its size, it is quite difficult to see what the changes are between the old and new default config files - it is also quite a lot of work to merge the local changes into the new default config file (or vice versa) - this is worsened by the fact that the CGI config editor will write the file quite a bit differently than the default version provided - the alternative is that the admin keeps his local changes in the per-PC config files, but that means a lot of duplication and does not work for server settings anyway By handling the local changes in the same way as the per-PC configs are now handled, as overrides against the default config, these disadvantages are significantly reduced and the CGI interface still offers a good overview over the full config. It also becomes very easy to revert a setting to the default. The patches ----------- There are two patches. The first is the proof-of-concept implementation for the config file handling. It includes some comments (marked "FJP:") for things that I'm not completely sure about and of course any comments about it are very welcome. The patch also includes a few minor cleanups (e.g. removal of unused variables). The change in Lib.pm is fairly self-explaining. Overview of changes in CGI/EditConfig.pm: - localConf added to hold local overrides from config_local.pl - mainConf renamed to baseConf to reflect its new role - baseConf holds only the default config from config.pl when editing the main config - baseConf holds a merge of config.pl and config_local.pl when editing a per-PC config - overrides are now used for both the main config and per-PC configs, except for the Hosts file The second patch is somewhat separate. Currently mainConf also includes the Hosts data, but that seems redundant to me as changes are determined by comparing newConf and the current hosts file (in hostsDiffMesg), not by comparing mainConf and baseConf. The patch seems logical to me, especially when the comments I added in the first patch and removed in the second are taken into account. There is a minor regression after this second patch is applied. After adding a new host, it is not immediately added in the drop-down list. I was unable to determine how this "extended refresh" of the data is triggered. Note though that the current code is not entirely consistent in this respect: if a host is removed, the list is currently also not immediately updated. I hope that a solution that covers both cases is possible that works with my patch. ToDo and issues --------------- This implementation leaves the default config file as /etc/bpc/config.pl and the local overrides as /etc/bpc/config_local.pl. As the default config file no longer needs to be modified, it should be moved to /usr/share. So, you'd end up with: - /usr/share/backuppc/config.pl # default config file - /etc/backuppc/config.pl # local main config overrides - /etc/backuppc/$host.pl # per-PC config overrides I'm currently reading and writing the config_local.pl file as if it were just another host config file. It probably should be implemented a bit cleaner as it would now conflict with a host with the same name as the file. The config_local.pl file should probably include a reference to the default config file and maybe even the comments for config options. The handling of the hosts data is really separate from the main config. The way it is excluded from overrides can probably be implemented cleaner. I also wonder if it really makes sense to have both an option in the side bar to edit hosts _and_ have it lists as a "section" when editing the main config file. IMHO the last should be removed/suppressed. Optional future extension ------------------------- If the 3-level concept is adopted, the following extension could be considered to improve the management of changes in the default config on upgrades, which can help prevent unexpected changes in behavior of backups after an upgrade. - Include a "config version" in both config.pl and config_local.pl. - If a new release includes changes in config options (new/dropped options; changed defaults), increase the version in config.pl. - Include a file with the program listing changes per config version (with details about e.g. changes in defaults). - Display a big warning in the CGI interface if the versions in config.pl and config_local.pl do not match and possibly block backups. - Offer an overview of config changes between the two versions. Admins can then check if their local/host overrides need to be adjusted. - When the review of changes is complete, the admin updates the version in config_local.pl and backups are enabled again.
commit 0be50aa0856a226d37a80a117eded0ce11852f67
Author: Frans Pop <[EMAIL PROTECTED]>
Date: Sat Aug 11 16:16:57 2007 +0200
Support local overrides against main default configuration file
Initial 'proof of concept support' for having a fixed default main
configuration file and allowing admins to define local overrides against
that default file.
This will make local changes more visible and also prepares the way for
beter management of changes in the default configuration file on upgrades.
diff --git a/lib/BackupPC/CGI/EditConfig.pm b/lib/BackupPC/CGI/EditConfig.pm
index 4f12ff9..5a59158 100644
--- a/lib/BackupPC/CGI/EditConfig.pm
+++ b/lib/BackupPC/CGI/EditConfig.pm
@@ -338,15 +338,11 @@ our %ConfigMenu = (
sub action
{
- my $pc_dir = "$TopDir/pc";
- my($content, $contentHidden, $newConf, $override, $mainConf, $hostConf);
+ my($content, $contentHidden, $newConf, $override, $baseConf, $localConf, $hostConf);
my $errors = {};
my $host = $In{host};
my $menu = $In{menu} || "server";
- my $hosts_path = $Hosts;
- my $config_path = $host eq "" ? "/etc/backuppc/config.pl"
- : "/etc/backuppc/$host.pl";
my $Privileged = CheckPermission($host)
&& ($PrivAdmin || $Conf{CgiUserConfigEditEnable});
@@ -380,25 +376,32 @@ sub action
}
($newConf, $override) = inputParse($bpc, $userHost);
- $override = undef if ( $host eq "" );
} else {
#
# First time: pick up the current config settings
#
- $mainConf = $bpc->ConfigDataRead();
+ $baseConf = $bpc->ConfigDataRead();
+ $localConf = $bpc->ConfigDataRead("config_local");
+ $baseConf = { %$baseConf, %$localConf } if ( $host ne "" );
+ $hostConf = {};
+ $override = {};
+
if ( $host ne "" ) {
$hostConf = $bpc->ConfigDataRead($host);
- $override = {};
foreach my $param ( keys(%$hostConf) ) {
$override->{$param} = 1;
}
+ $newConf = { %$baseConf, %$hostConf };
} else {
- my $hostInfo = $bpc->HostInfoRead();
- $hostConf = {};
- $mainConf->{Hosts} = [map($hostInfo->{$_}, sort(keys(%$hostInfo)))];
+ my $hostInfo = $bpc->HostInfoRead();
+ $baseConf->{Hosts} = [map($hostInfo->{$_}, sort(keys(%$hostInfo)))];
+
+ foreach my $param ( keys(%$localConf) ) {
+ $override->{$param} = 1;
+ }
+ $newConf = { %$baseConf, %$localConf };
}
- $newConf = { %$mainConf, %$hostConf };
}
if ( $In{saveAction} ne "Save" && $In{newMenu} ne ""
@@ -673,8 +676,20 @@ EOF
if ( !$isError && $In{saveAction} eq "Save" ) {
my($mesg, $err);
+
+ # FJP: Are these really needed?
+ # FJP: If they are, shouldn't $baseConf->{hosts} be initialized as well?
+ # FJP: OTOH, it looks as if {hosts} may only be needed in $newConf anyway
+ $localConf = $bpc->ConfigDataRead("config_local") if ( !defined($localConf) );
+ if ( !defined($baseConf) ) {
+ $baseConf = $bpc->ConfigDataRead();
+ $baseConf = { %$baseConf, %$localConf } if ( $host ne "" );
+ }
+
if ( $host ne "" ) {
+ # FJP: Again, is this really needed?
$hostConf = $bpc->ConfigDataRead($host) if ( !defined($hostConf) );
+
my %hostConf2 = %$hostConf;
foreach my $param ( keys(%$newConf) ) {
if ( $override->{$param} ) {
@@ -683,11 +698,9 @@ EOF
delete($hostConf->{$param});
}
}
- $mesg = configDiffMesg($host, \%hostConf2, $hostConf);
- $err .= $bpc->ConfigDataWrite($host, $hostConf);
+ $mesg .= configDiffMesg($host, \%hostConf2, $hostConf);
+ $err .= $bpc->ConfigDataWrite($host, $hostConf);
} else {
- $mainConf = $bpc->ConfigDataRead() if ( !defined($mainConf) );
-
my $hostsSave = [];
my($hostsNew, $allHosts, $copyConf);
foreach my $entry ( @{$newConf->{Hosts}} ) {
@@ -716,15 +729,24 @@ EOF
foreach my $host ( keys(%$copyConf) ) {
my $confData = $bpc->ConfigDataRead($copyConf->{$host});
my $fromHost = $copyConf->{$host};
- $err .= $bpc->ConfigDataWrite($host, $confData);
$mesg .= eval("qq($Lang->{CfgEdit_Log_Copy_host_config})");
+ $err .= $bpc->ConfigDataWrite($host, $confData);
}
delete($newConf->{Hosts});
- $mesg .= configDiffMesg(undef, $mainConf, $newConf);
- $mainConf = { %$mainConf, %$newConf };
- $err .= $bpc->ConfigDataWrite(undef, $mainConf);
- $newConf->{Hosts} = $hostsSave;
+
+ my %localConf2 = %$localConf;
+ foreach my $param ( keys(%$newConf) ) {
+ if ( $override->{$param} ) {
+ $localConf->{$param} = $newConf->{$param}
+ } else {
+ delete($localConf->{$param});
+ }
+ }
+ $mesg .= configDiffMesg(undef, \%localConf2, $localConf);
+ $err .= $bpc->ConfigDataWrite("config_local", $localConf);
+
+ $newConf->{Hosts} = $hostsSave;
}
if ( defined($err) ) {
$tblContent .= <<EOF;
@@ -780,6 +802,8 @@ EOF
$doneParam->{$param} = 1;
+ # FJP: The exception for 'Hosts' can probably be done cleaner;
+ # maybe by setting a flag in its metadata
$tblContent .= fieldEditBuild($ConfigMeta{$param},
$param,
$newConf->{$param},
@@ -788,8 +812,8 @@ EOF
$comment,
$isError,
$paramInfo->{onchangeSubmit},
- defined($override) ? $param : undef,
- defined($override) ? $override->{$param} : undef
+ $param ne "Hosts" ? $param : undef,
+ $param ne "Hosts" ? $override->{$param} : undef,
);
if ( defined($paramInfo->{comment}) ) {
my $topDir = $bpc->TopDir;
@@ -846,36 +870,16 @@ EOF
}
if ( defined($In{menu}) || $In{saveAction} eq "Save" ) {
- if ( $In{saveAction} eq "Save" && !$userHost ) {
- #
- # Emit the new settings as orig_zZ_ parameters
- #
- $doneParam = {};
- foreach my $param ( keys(%ConfigMeta) ) {
- next if ( $doneParam->{$param} );
- next if ( $userHost
- && (!defined($bpc->{Conf}{CgiUserConfigEdit}{$param})
- || (!$PrivAdmin
- && !$bpc->{Conf}{CgiUserConfigEdit}{$param})) );
- $contentHidden .= fieldHiddenBuild($ConfigMeta{$param},
- $param,
- $newConf->{$param},
- "orig",
- );
- $doneParam->{$param} = 1;
- $In{modified} = 0;
- }
- } else {
- #
- # Just switching menus: copy all the orig_zZ_ input parameters
- #
- foreach my $var ( keys(%In) ) {
- next if ( $var !~ /^orig_zZ_/ );
- my $val = decode_utf8($In{$var});
- $contentHidden .= <<EOF;
+ # FJP: Not completely sure if this always needs to be executed
+ #
+ # Just switching menus: copy all the orig_zZ_ input parameters
+ #
+ foreach my $var ( keys(%In) ) {
+ next if ( $var !~ /^orig_zZ_/ );
+ my $val = decode_utf8($In{$var});
+ $contentHidden .= <<EOF;
<input type="hidden" name="$var" value="${EscHTML($val)}">
EOF
- }
}
} else {
#
@@ -890,7 +894,7 @@ EOF
&& !$bpc->{Conf}{CgiUserConfigEdit}{$param})) );
$contentHidden .= fieldHiddenBuild($ConfigMeta{$param},
$param,
- $mainConf->{$param},
+ $baseConf->{$param},
"orig",
);
$doneParam->{$param} = 1;
@@ -1481,7 +1485,7 @@ sub configDiffMesg
if ( $host ne "" ) {
$conf = "host $host config";
} else {
- $conf = "main config";
+ $conf = "local config";
}
foreach my $p ( keys(%ConfigMeta) ) {
diff --git a/lib/BackupPC/Lib.pm b/lib/BackupPC/Lib.pm
index 2fb1d49..5ed57f6 100644
--- a/lib/BackupPC/Lib.pm
+++ b/lib/BackupPC/Lib.pm
@@ -317,7 +317,7 @@ sub ConfigRead
my($ret);
#
- # Read main config file
+ # Read default config file
#
my($mesg, $config) = $bpc->{storage}->ConfigDataRead();
return $mesg if ( defined($mesg) );
@@ -325,6 +325,16 @@ sub ConfigRead
$bpc->{Conf} = $config;
#
+ # Read local config file
+ # For now: use '/etc/backuppc/config_local.pl'
+ # Eventually the default config file should be moved elsewhere and this
+ # should become '/etc/backuppc/config.pl'
+ #
+ my($mesg, $config) = $bpc->{storage}->ConfigDataRead("config_local");
+ return $mesg if ( defined($mesg) );
+ $bpc->{Conf} = { %{$bpc->{Conf}}, %$config };
+
+ #
# Read host config file
#
if ( $host ne "" ) {
commit a5192ec45b91c462f883ec5d8d8484b729074e98
Author: Frans Pop <[EMAIL PROTECTED]>
Date: Sun Aug 12 01:49:28 2007 +0200
Do not include hosts in baseConf
As far as I can tell there is no need to have hosts data in baseConf.
The check for changes is done by comparing against the hosts file, not
by checking against baseConf.
Small regression is that now the hosts dropdown list does not get updated
immediately after adding a new host. However, this already did not get
done of a host was removed, so a cleaner solution for that was probably
wanted anyway.
diff --git a/lib/BackupPC/CGI/EditConfig.pm b/lib/BackupPC/CGI/EditConfig.pm
index 5a59158..49b91e2 100644
--- a/lib/BackupPC/CGI/EditConfig.pm
+++ b/lib/BackupPC/CGI/EditConfig.pm
@@ -394,13 +394,13 @@ sub action
}
$newConf = { %$baseConf, %$hostConf };
} else {
- my $hostInfo = $bpc->HostInfoRead();
- $baseConf->{Hosts} = [map($hostInfo->{$_}, sort(keys(%$hostInfo)))];
-
foreach my $param ( keys(%$localConf) ) {
$override->{$param} = 1;
}
$newConf = { %$baseConf, %$localConf };
+
+ my $hostInfo = $bpc->HostInfoRead();
+ $newConf->{Hosts} = [map($hostInfo->{$_}, sort(keys(%$hostInfo)))];
}
}
@@ -678,8 +678,6 @@ EOF
my($mesg, $err);
# FJP: Are these really needed?
- # FJP: If they are, shouldn't $baseConf->{hosts} be initialized as well?
- # FJP: OTOH, it looks as if {hosts} may only be needed in $newConf anyway
$localConf = $bpc->ConfigDataRead("config_local") if ( !defined($localConf) );
if ( !defined($baseConf) ) {
$baseConf = $bpc->ConfigDataRead();
signature.asc
Description: This is a digitally signed message part.
------------------------------------------------------------------------- This SF.net email is sponsored by: Splunk Inc. Still grepping through log files to find problems? Stop. Now Search log events and configuration files using AJAX and a browser. Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________ BackupPC-devel mailing list BackupPC-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/backuppc-devel http://backuppc.sourceforge.net/
