Comment 2 for bug 612975

Revision history for this message
Colin Watson (cjwatson) wrote :

Thanks for the patch. A number of comments:

--upstart-only is incorrect, as in other bugs. Please see dh_installinit(1) for documentation of --upstart-only.

You've deleted invoke-rc.d calls but not replaced them with anything.

There are a couple of serious typos (as follows), as well as some minor ones in the changelog and comments:

+ [ -f /etc/default/dhcp3-relay ] && ./etc/default/dhcp3-rlay
+ exec /sur/sbin/dhcrelay -q $OPTIONS $IFCMD $SERVERS

Regarding the first line, as well as the spelling mistake, please note that you need a space after the '.' - this is a shell command, not part of a path. The ntp patch had the same mistake.

The dhcp3-relay init script aborts if /etc/default/dhcp3-relay doesn't exist, while the Upstart job carries on anyway. This is unsafe. The same goes for the dhcp3-server job.

Something needs to be done about the update-rc.d call in dhcp3-server.postinst.

Please port over the /etc/ltsp/dhcpd.conf handling from the init script. If we leave this out we'll likely just get bug reports and have to reintroduce it later - might as well get it right from the start.

+ [ -e /var/lib/dhcp3/dhcpd.leases~ ] || chown dhcpd:dhcpd /var/lib/dhcp3/dhcpd.leases~

This is backwards - should be && not ||.

+ exec /usr/sbin/dhcpd3 -q -pf /var/run/dhcp3-server/dhcpd.pid-cf $CONFIG_FILE $INTERFACES

Missing space between "pid" and "-cf". Can we drop the "-pf /var/run/dhcp3-server/dhcpd.pid", please? We shouldn't need pidfiles with Upstart, in general, since it can track daemons itself.