On 05/13/2011 10:23 AM, Eric Blake wrote:
On 05/13/2011 01:22 AM, Laine Stump wrote:
> netcf-transaction is a shell script that can take a snapshot of
> network config files, and later either commit the changes made (by
> deleting the snapshot) or revert back to the original config. If
> uncommitted changes exist at boottime, netcf-transaction will
> automatically revert them.
>
> Available commands:
>
> snapshot-config: save a copy of the current network interface config
> commit-config: delete the saved snapshot
> rollback-config: archive the current state of config into a rollback
> directory, and copy the original snapshotted state
> back into place
> snapshot-dir: return the path to the snapshot directory.
>
> start, restart, reload, force-reload, condrestart, and try-restart are
> all synomyms for snapshot-config.
> ---
>
>
> +# This is for the initscript that handles network config change
> +# transactions.
> +install-data-local: install-init
> +
> +uninstall-local: uninstall-init
You should make these rules guarded by an automake conditional. See how
libvirt guards it by LIBVIRT_INIT_SCRIPT_RED_HAT (and corresponding code
in configure.ac to set that conditional); the rationale being that init
scripts should only be installed on systems likely to use it, and only
when $prefix looks like a system install and not an installation into $HOME.
I've done the first part by copying libvirt's configure.ac code, but for
the second (only installing if $prefix looks like a system install and
not $HOME) libvirt doesn't make that check (we just discussed on IRC
that that is a bug in libvirt's configure.ac).
My remaining question is: Do I only auto-set to "redhat" 'if test
"$prefix" = "/usr"', or 'if test "$prefix" !=
"$HOME"' ? I'm assuming
the latter when I repost my patch...
> +++ b/src/netcf-transaction.init.sh
> @@ -0,0 +1,145 @@
> +#!/bin/sh
This says sh but you are relying on bash extensions. While this works
for Red Hat init scripts (since Fedora and RHEL are still using bash for
/bin/sh), it may be worth making it explicit that you are using bash
extensions.
or get rid of the bash extensions...
> +#
> +# netcf-transaction: save/restore current network interface configuration
> +#
> +# chkconfig: - 09 91
> +# description: Potentially rolls back network interface config to a \
> +# known good state at boot time. Also includes utility \
> +# commands to create this "known good state", remove it \
> +# (ie, accept the current config as "known good"), and \
s/ie,/i.e.,/
done.
> +# to manually rollback to the known good state, if
it \
> +# exists.
> +
> +### BEGIN INIT INFO
> +# Provides: netcf-transaction
> +# Required-Start: $local_fs
> +# Short-Description: save/restore network configuration files
> +# Description: This script can save the current state of network config,
> +# and later revert to that config, or commit the new config
> +# (by deleting the snapshot). At boottime, if there are
> +# uncomitted changes to the network config, they are
s/uncomitted/uncommitted/
fixed
> +# reverted (and the discarded changes are archived
in
> +# /var/lib/netcf/network-rollback-*).
Shouldn't the two description strings be identical?
Yes, I suppose they should :-)
> +#
> +### END INIT INFO
> +
> +sysconfdir="@sysconfdir@"
> +localstatedir="@localstatedir@"
> +
> +netconfdir="$sysconfdir"/sysconfig/network-scripts
> +snapshotdir="$localstatedir"/lib/netcf/network-snapshot
> +rollbackdirbase="$localstatedir"/lib/netcf/network-rollback
> +
> +# Source function library.
> +test ! -r "$sysconfdir"/rc.d/init.d/functions ||
> + . "$sysconfdir"/rc.d/init.d/functions
> +
> +# take a snapshot of current network configuration scripts
> +snapshot_config ()
> +{
> + if test -e "$snapshotdir"
test -d is safer, given that you insist on using it as a directory
Except that the following mkdir -p will fail if anything exists by that
name - even a file or symbolic link. The intent of the test is to detect
something is already in place before we try (and potentially fail) to
create the directory.
> + then
> + echo "snapshot directory $snapshotdir already exists"
> + return 1;
> + fi
> + mkdir -p "$snapshotdir"
Check for mkdir failure. For that matter, if you are using mkdir -p,
which silently succeeds if the directory already exists, why are you
duplicating the check and failing if it exists? Either use 'mkdir
"$snapshotdir"' without -p, or drop the check for the directory already
existing.
I'm using mkdir -p in case the parent directory doesn't yet exist (the
first time a snapshot is taken, it won't), not to avoid the error if it
doesn't already exist.
But if it already exists, that means someone else has already begun a
transaction, and hasn't yet done a commit or rollback, and in our
discussions we decided that attempts to start a new transaction when one
is already open should result in failure of the new transaction. So I
really do need to both 1) check for existence beforehand, and 2) use
mkdir -p.
It is a reasonable idea to check for failure of mkdir, however (it would
take very odd circumstances, but in a world of root-squash nfs mounts,
anything can happen). I'll add that in.
> + for f in "$netconfdir"/{ifcfg,route,rule}-*
See above comment about bash extensions.
Okay, I'm removing all uses of {} in globs
> + do
> + test -f "$f"&& cp -p "$f"
"$snapshotdir"
You should test for cp failure. Otherwise, a failure on iteration 1
followed by success on iteration 2 makes the caller think this function
succeeded.
Okay. I'm now checking for failure of all instances of cp, mkdir, and rm.
In the case of rm, I set a non-0 return, but continue, because in all of
those cases the main objective is to get the old files back into the
config directory, and I want to make sure we do that even if there is
some strange failure to remove a backup file along the way.
> + done
> +}
> +
> +commit_config ()
> +{
> + rm -rf "$snapshotdir"
> +}
> +
> +# rollback network configuration to last snapshot (if one exists)
> +rollback_config ()
> +{
> + # return immediately if there is no snapshot
> + test ! -d "$snapshotdir"&& return 0
> +
> + # eliminate all but the last 20 rollback saves
> + ls -d "$rollbackdirbase"-* 2>/dev/null | head -n -20 |\
Ouch. Here, you are relying on ls sorting order, but you didn't specify
LC_ALL=C to guarantee which order you are relying on. Also, do you want
it strictly name-based, or do you also want to sort on timestamps to
discard the oldest directories?
Well, the directory contains a timestamp in its name, so the oldest
directories will be discarded (even if someone messes up the timestamps
somehow). I added in LC_ALL=C
> + while read r; do rm -rf "$r"; done
> +
> + # save a copy of the current config before rollback "just in case"
> + rollbackdir=$rollbackdirbase-`date +%Y.%m.%d-%H:%M:%S`
`` is out-dated. Use $() in init scripts.
Old habits die hard :-)
> + mkdir -p "$rollbackdir"
> + for f in "$netconfdir"/{ifcfg,route,rule}-*
> + do
> + test -f "$f"&& cp -p "$f"
"$rollbackdir"
Test for mkdir and cp failures.
Done.
> + done
> +
> + # There are 4 classes of files, each handled slightly differently to minimize
> + # disruption in services:
> + # 1) file in both, unmodified - just erase the snapshot copy
> + # 2) file in both, modified - copy the snapshot version over the current&
erase copy
> + # 3) file in current only - remove the file
> + # 4) file in snapshot only - copy the snapshot file to current
> + #
> + # We handle the 1st three cases in one loop going through all current config
> + # files, and what is left over in snapshotdir is, by definition, case 4.
> + #
> + # (NB: we can't mv the files, because then the selinux labels
> + # don't get reset properly.)
> +
> + for f in "$netconfdir"/{ifcfg,route,rule}-*
> + do
> + test ! -f "$f"&& continue
> +
> + rollbackf=$snapshotdir/`basename "$f"`
Again, prefer $().
Or, better yet, save a fork and use the POSIX-guaranteed:
rollbackf=$snapshotdir/${f##*/}
I've never seen that construct before, but I tried it and it works, so
who can argue against that? :-)
It's unlikely that $f will ever start with '-' (for that
to happen,
@sysconfdir@ has to be a relative path), but if it were possible, you
would want 'basename -- "$f"'.
> + if test -f "$rollbackf"
> + then
> + # Case (1)& (2) (only copies if they're different)
> + ! diff "$rollbackf" "$f">/dev/null
2>&1&& cp -pf "$rollbackf" "$f"
Rather than using diff for its exit status, I prefer 'cmp -s'.
Another typing habit I just carried over to the script. I switched to
cmp -s.
Check for cp failure.
Yup
> + rm -f "$rollbackf"
Check for rm failure.
Done
> + else
> + # Case (3)
> + rm -f "$f"
Check for rm failure.
and done.
>> + fi
>> + done
>> +
>> + # Case (4)
>> + for f in "$snapshotdir"/{ifcfg,route,rule}-*
>> + do
>> + test ! -f "$f"&& continue
>> + cp -pf "$f" "$netconfdir"
Check for cp failure.
and done.
> + done
> + rm -rf "$snapshotdir"
> +}
> +
> +case "$1" in
> + # commands required in all Fedora initscripts
> + start|restart|reload|force-reload|condrestart|try-restart)
> + echo -n $"Running $prog $1: "
echo -n is a bash-ism; worse, it is a configurable bash-ism (if I use
shopt -s xpg_echo, I will break this script). Use printf instead.
Heh. I looked in /etc/rc.d/init.d on F13 and saw that the only script
using printf was libvirt-guests. All the others are using echo -n.
$"" is a bash-ism; it's reasonable enough but if we
ever need to port
this script to non-RedHat systems, then we need to follow the
libvirt-guests.init script lead, and use gettext instead.
Ah - it seems like if I switch to using printf instead of echo -n, I
should also switch to using gettext.
Since I don't understand the details of gettext and shell scripts, or
the extra dependencies this creates, I'm going to leave the "'echo -n
$"..."' in place for now. We can revisit it later when someone other
than RHEL/Fedora wants to use the script.
> + rollback_config
> + retval=$?
> + echo
> + ;;
> + stop|status)
> + ;;
> +
> +
> + # specific to netcf-transaction
> + snapshot-config)
> + snapshot_config
> + ;;
> + commit-config)
> + commit_config
> + ;;
> + rollback-config)
> + rollback_config
> + retval=$?
> + ;;
> + snapshot-dir)
> + echo $snapshotdir
> + retval=0
> + ;;
> + *)
> + echo $"Usage: $0
{start|snapshot-config|commit-config|rollback-config|snapshot-dir}"
Should this also echo the names of all standard targets (such as
try-restart)?
I"m not sure if it's required, but libvirt-guests does it, so I'll do it
here too.
> + exit 2
Is it worth following the lead of libvirt-guests.init and having a usage
function, as well as supporting a --help argument that prints usage with
exit status 0?
Sure. I added it in.
> +esac
> +
> +exit $retval
Thanks for the review. a V2 will follow this reply.