On 05/20/2011 01:39 PM, Eric Blake wrote:
On 05/18/2011 10:54 AM, Laine Stump wrote:
> ***
> Changes from v1:
>
> 1) everything outlined in my response to eblake's review
> 2) changed the names of the functions to match those in the netcf API.
> ***
>
> +dnl
> +dnl init script flavor
> +dnl
> +AC_MSG_CHECKING([for init script flavor])
> +AC_ARG_WITH([init-script],
> + [AC_HELP_STRING([--with-init-script=@<:@redhat|auto|none@:>@],
> + [Style of init script to install @<:@default=auto@:>@])])
> +if test "x$with_init_script" = "x" || test
"x$with_init_script" = "xauto"; then
> + if test "$prefix" = "$HOME" || test
"$cross_compiling" = yes || test ! -f /etc/redhat-release; then
Good enough for now; we probably ought to push the $HOME check back to
libvirt, and/or improve both projects to do even stricter checking to
only do this when $sysconfdir is /etc (since init scripts don't make
sense in any other directory), but that would be a later patch and does
not hold up this.
I wasn't sure which of the two possibilities to use when adding this in
(check for = $HOME or check for != "/etc"), but when I saw jdenemar's
response to your message about it on libvir-list, I decided to take the
less restrictive approach. (That's also why I didn't make a patch for
the same issue in libvirt yet.)
> + with_init_script=none
> + else
> + with_init_script=redhat
> + fi
> +fi
> +AM_CONDITIONAL([NETCF_INIT_SCRIPT_RED_HAT], test x$with_init_script = xredhat)
> +AC_MSG_RESULT($with_init_script)
M4 quoting and long lines:
AM_CONDITIONAL([NETCF_INIT_SCRIPT_RED_HAT],
[test x$with_init_script = xredhat])
AC_MSG_RESULT([$with_init_script])
NB: Those lines were lifted directly from libvirt's configure.ac. Also,
the AM_CONDITIONAL line isn't > 80 characters (I folded it anyway)
> @@ -0,0 +1,223 @@
> +#!/bin/sh
> +#
> +# netcf-transaction: save/restore current network interface configuration
> +#
> +# chkconfig: - 09 91
> +# 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 \
s/boottime/boot time/ (twice)
> +# usage [val]
> +# Display usage string, then exit with VAL (defaults to 2).
> +usage() {
> + echo "Usage: $0
{change-begin|change-commit|change-rollback|snapshot-dir|start|stop|status|restart|condrestart|try-restart|reload|force-reload}"
Use $"", so "Usage" can get translated.
> +retval=0
> +case "$1" in
> + # commands required in all Fedora initscripts
> + start|restart|reload|force-reload|condrestart|try-restart)
> + echo -n $"Running $prog $1: "
> + change_rollback
> + retval=$?
> + echo
> + ;;
> + stop|status)
> + ;;
Shouldn't status output something? It might be nice to have status
output whether an uncommitted transaction exists, and how many rollback
points exist. But that can be an add-on patch, if you want to push this
as a starting point.
After the first 20 times a rollback is done, the count of rollbacks will
always be 20, so there's not much lasting use of putting that in. I did
put in a message stating "There is an open transaction" or "No open
transaction".
You addressed my findings from v1, so ACK with those additional nits
above addressed.
Thanks! I made the changes noted above (plus a couple other minor
things, e.g. printing out that there's been a rollback when one happens,
so that it shows up in the system boot logs) and pushed.
Related C API coming up!