Jonathan Wiltshire wrote:
> I worked through the diff of your proposed changes and integrated almost
> all of them - if I'm right, lots are formatting and style, some cruft
> removal, and some technical.
yes, sorry about not marking the changes specifically.
> The file gvpe.init particularly came mostly from the debhelper
> template, which is why it was weaker. The bits I'm left with are the
> ones I don't quite follow, here as a diff with annotations:
> init.d:
> @@ -31,16 +31,11 @@
> # merely create a single tunnel, it creates a real
> # network with multiple endpoints.
> ### END INIT INFO
>
> -PATH=/usr/local/sbin:/usr/local/bin:/sbin:/bin:/usr/sbin:/usr/bin
> -
> -DAEMON=/usr/sbin/gvpe
> NAME=gvpe
> DESC="GNU Virtual Private Ethernet daemon"
> -LOGDIR=/var/log/gvpe
> -
> PIDFILE=/var/run/$NAME.pid
>
> -test -x $DAEMON || exit 0
> +test -x /usr/bin/gvpectrl || exit 0
> Ok, I missed the fact that the PATH variable has wrong paths in (oops)
> so that can go in. But why remove the declaration of $NAME and move it
> further down?
do you mean $DAEMON instead of $NAME? $NAME isn't moved...
the DAEMON variable is set later because it's constructed based on the
values of $CIPHER and $DIGEST, which can be overriden by
/etc/default/gvpe. so it has to be set after sourcing the defaults
file.
> I take it the $LOGDIR variable goes because it doesn't get
> used - I assumed that log_daemon_msg and friends used it, probably a bad
> thing to assume.
yes, i tried to remove the unused boilerplate.
> Why test that /usr/bin/gvpectrl is executable but not $DAEMON?
the init script is a conf file which means it remains on the system on
package removal. (conf files are only removed on purge.) so the init
script needs a way to determine if the package is installed so you
usually just test if your daemon's binary still exists.
in this case, though, the path to the daemon is constructed, and it
could be constructed to a nonexistent path (say, a typo in
/etc/default/gvpe) and the init script wouldn't run. so instead test
for a stable binary name included in the package (gvpectrl in this case)
instead.
> @@ -73,8 +66,7 @@
> # Use this if you want the user to explicitly set 'RUN' in
> # /etc/default/
> if [ "x$RUN" != "xyes" ]; then
> - log_failure_msg "$NAME disabled, please adjust the configuration to
> your needs "
> - log_failure_msg "and then set RUN to 'yes' in /etc/default/$NAME to
> enable it."
> + log_failure_msg "$NAME disabled in /etc/default/$NAME."
> exit 1
>
> Is this change to the output text a preference, or is there a style for
> this sort of message that I've missed somewhere?
it looks odd because log_failure_msg prints out the string you gave it
and appends "failed!" in red letters, so your sequence prints out
"failed!" twice. it looks nicer if "failed!" is only printed once.
i don't know if there's a style guide. the message should probably fit
on an 80 column terminal though.
i have ten packages currently installed which install init scripts that
print out a 'disabled' message if they haven't been configured to start,
and they all use only a single line to print the warning. some of them
use log_warning_msg. i don't see any that use log_failure_msg.
> @@ -153,7 +145,7 @@
> log_end_msg 0
> exit 0
> fi
> - if start_server; then
> + if start_server ; then
>
> I think you missed this one ;)
sorry. the template you copied from had a lot of tabs-vs-spaces
confusion too.
> @@ -213,19 +205,11 @@
> fi
> ;;
> reload)
> - log_daemon_msg "Reloading configuration for $DESC" "$NAME"
> - if running; then
> - [ -x /usr/sbin/gvpectrl ] && /usr/sbin/gvpectrl -kHUP
> - log_end_msg 0
> - else
> - log_progress_msg "apparently not running"
> - log_end_msg 1
> - exit 1
> - fi
> + log_warning_msg "Reloading $NAME daemon: not implemented (use
> restart)."
> ;;
> *)
> N=/etc/init.d/$NAME
> - echo "Usage: $N
> {start|stop|force-stop|restart|reload|force-reload|status}" >&2
> + echo "Usage: $N
> {start|stop|force-stop|restart|force-reload|status}" >&2
>
> I only just today noticed that gvpectrl can send a HUP signal to the
> daemon, and tested that it reloads the configuration properly, so I've
> implemented reloading. Does it look correct?
hmm, i don't know how gvpectrl determines what process ID to send the
signal too. probably just reads the pid file?
if you just need to send SIGHUP to the gvpe daemon you might want to
use the $PIDFILE and send the signal yourself.
actually, the whole init script would probably be a lot shorter / neater
if you used start-stop-daemon :)
[ the last init script i wrote was /etc/init.d/unbound in the unbound
package if you need an example of an init script that uses
start-stop-daemon. ]
> debian/rules:
> @@ -20,7 +20,7 @@
> CROSS= --build $(DEB_BUILD_GNU_TYPE)
> endif
>
> -CONF=./configure $(CROSS) --prefix=/usr --includedir=$${prefix}/include
> --mandir=\$${prefix}/share/man --infodir=\$${prefix}/share/info
> --sysconfdir=/etc --localstatedir=/var --libexecdir=$${prefix}/lib/gvpe
> --disable-maintainer-mode --disable-dependency-tracking --enable-dns
> --enable-icmp --enable-rawip --enable-tcp --enable-udp
> CFLAGS="$(CFLAGS)" LDFLAGS="-Wl,-z,defs"
> +CONF=./configure $(CROSS) --prefix=/usr --includedir=$${prefix}/include
> --mandir=\$${prefix}/share/man --infodir=\$${prefix}/share/info
> --sysconfdir=/etc --localstatedir=/var --libexecdir=$${prefix}/lib/gvpe
> --disable-maintainer-mode --enable-max-mtu=9100 CFLAGS="$(CFLAGS)"
> LDFLAGS="-Wl,-z,defs"
>
> I gathered from the configure script output that extra protocols, like
> --enable-icmp and --enable-tcp and so on had to be explicitly enabled.
> Certainly using your configure command they don't appear at the end of
> the outpt summary; have I mis-interpreted this?
i removed --enable-icmp and --enable-tcp because they are already
enabled:
--disable-icmp enable icmp protocol support (default enabled).
--disable-tcp enable tcp protocol support (default enabled).
[ appears to be a braino here in the configure help output. ]
i removed --enable-rawip and --enable-udp because there doesn't appear
to be any such options (earlier version of gvpe perhaps?).
and according to the changelog:
- implemented dns tunneling (experimental now and in the future).
so i removed --enable-dns (tunneling IP over DNS in general is a huge
hack, if the author says his implementation will always be experimental,
may as well leave it in the default disabled state).
> I agree that building multiple binaries, though a bit convoluted, makes
> it easier to choose cipher preferences, so that makes sense. I wondered
> about debconf questions for them, but that would be new ground for me -
> do you think it a good idea? I guess it would be good to learn if
> nothing else.
debconf might be kind of extravagant. i'm planning on rolling out some
gvpe instances and i'll probably end up auto-generating /etc/gvpe/* and
/etc/default/gvpe. debconf might be ok if the whole thing could be
configured via prompts, but gvpe is probably too flexible to be
configured with debconf to most users' satisfaction. also, remember
that the gvpe config file has to be updated on every node when a new
node is added.
> Thanks for the help, and sorry for so many questions!
no problem. i'd be happy to sponsor your package.
--
Robert Edmonds
edmonds@debian.org
Attachment:
signature.asc
Description: Digital signature