Stop returning `nil, false` in plugins (#51)

* handler: Fix documentation of plugin chaining

This condition was backwards: if the boolean is false, the next plugins
are called and any invalid or nil packet is very likely to cause crashes
in the next plugin. OTOH if true, we stop execution anyway and don't
send a message when resp == nil, so it's OK to be nil or invalid then

Signed-off-by: Anatole Denis <anatole@unverle.fr>

* plugins/file: Avoid nil returns to next plugin

`return nil, false` passes a nil dhcp message to the next plugin in
chain, which is almost guaranteed to crash or fail in some other way.
The proper way to ignore a valid message (because we don't know what to
do with it) is to return the resp argument. The ipv4 version of the
plugin already does so.

Add a note about file being a terminating plugin when a response is
chosen

Signed-off-by: Anatole Denis <anatole@unverle.fr>

* plugins/server_id: Uniformize error returns

`return resp, false` after a log.Fatal is misleading, since the program
will have crashed at this point. `return nil, true` is still dead code
but conveys the meaning of "abort here" better

Same thing for the DHCPv4 version of the code which was forgotten in the
original commit.

Remove the test for `resp == nil` since the previous commits in this
series removed the possibility of receiving a nil resp as argument

Fixes: 4a73abd6 ("plugins/server_id: Abort when ServerID is nil")
Signed-off-by: Anatole Denis <anatole@unverle.fr>

handler/handler.go

@@ -11,8 +11,8 @@ import (
 // The two input packets are the original request, and a response packet.
 // The response packet may or may not be modified by the function, and
 // the result will be returned by the handler.
-// If the returned boolean is false, the returned packet may be nil or
-// invalid.
+// If the returned boolean is true, the returned packet may be nil or
+// invalid, in which case no response will be sent.
 type Handler6 func(req, resp dhcpv6.DHCPv6) (dhcpv6.DHCPv6, bool)
 
 // Handler4 behaves like Handler6, but for DHCPv4 packets.

plugins/file/plugin.go

@@ -121,14 +121,15 @@ func LoadDHCPv6Records(filename string) (map[string]net.IP, error) {
 func Handler6(req, resp dhcpv6.DHCPv6) (dhcpv6.DHCPv6, bool) {
 	mac, err := dhcpv6.ExtractMAC(req)
 	if err != nil {
-		return nil, false
+		log.Warningf("Could not find client MAC, passing")
+		return resp, false
 	}
 	log.Printf("looking up an IP address for MAC %s", mac.String())
 
 	ipaddr, ok := StaticRecords[mac.String()]
 	if !ok {
 		log.Warningf("MAC address %s is unknown", mac.String())
-		return nil, false
+		return resp, false
 	}
 	log.Printf("found IP address %s for MAC %s", ipaddr, mac.String())
 	resp.AddOption(&dhcpv6.OptIANA{
@@ -167,6 +168,7 @@ func Handler6(req, resp dhcpv6.DHCPv6) (dhcpv6.DHCPv6, bool) {
 			}
 		}
 	}
+	// XXX: We should maybe allow other plugins to run after this to add other options/handle non-IANA requests
 	return resp, true
 }
 

plugins/server_id/plugin.go

@@ -29,7 +29,7 @@ var (
 func Handler6(req, resp dhcpv6.DHCPv6) (dhcpv6.DHCPv6, bool) {
 	if v6ServerID == nil {
 		log.Fatal("BUG: Plugin is running uninitialized!")
-		return resp, false
+		return nil, true
 	}
 
 	msg, err := req.GetInnerMessage()
@@ -70,8 +70,9 @@ func Handler6(req, resp dhcpv6.DHCPv6) (dhcpv6.DHCPv6, bool) {
 
 // Handler4 handles DHCPv4 packets for the server_id plugin.
 func Handler4(req, resp *dhcpv4.DHCPv4) (*dhcpv4.DHCPv4, bool) {
-	if v4ServerID == nil || resp == nil {
-		return resp, false
+	if v4ServerID == nil {
+		log.Fatal("BUG: Plugin is running uninitialized!")
+		return nil, true
 	}
 	if req.OpCode != dhcpv4.OpcodeBootRequest {
 		log.Warningf("not a BootRequest, ignoring")