pixiecore: implement orderly shutdown on error or request.

pixiecore/dhcp.go

@@ -22,22 +22,14 @@ import (
 	"go.universe.tf/netboot/dhcp4"
 )
 
-func (s *Server) serveDHCP(conn *dhcp4.Conn) {
+func (s *Server) serveDHCP(conn *dhcp4.Conn) error {
 	for {
 		pkt, intf, err := conn.RecvDHCP()
 		if err != nil {
-			s.log("DHCP", "Receiving packet: %s", err)
+			return fmt.Errorf("Receiving DHCP packet: %s", err)
-			// TODO: fatal errors that return from one of the handler
-			// goroutines should plumb the error back to the
-			// coordinating goroutine, so that it can do an orderly
-			// shutdown and return the error from Serve(). This "log +
-			// randomly stop a piece of pixiecore" is a terrible
-			// kludge.
-			return
 		}
 		if intf == nil {
-			s.log("DHCP", "Received packet with no interface information (this is a violation of dhcp4.Conn's contract, please file a bug)")
+			return fmt.Errorf("Received DHCP packet with no interface information (this is a violation of dhcp4.Conn's contract, please file a bug)")
-			return
 		}
 
 		if err = s.isBootDHCP(pkt); err != nil {

pixiecore/http.go

@@ -26,20 +26,14 @@ import (
 	"text/template"
 )
 
-func (s *Server) serveHTTP(l net.Listener) {
+func (s *Server) serveHTTP(l net.Listener) error {
 	httpMux := http.NewServeMux()
 	httpMux.HandleFunc("/_/ipxe", s.handleIpxe)
 	httpMux.HandleFunc("/_/file", s.handleFile)
 	if err := http.Serve(l, httpMux); err != nil {
-		s.log("HTTP", "%s", err)
+		return fmt.Errorf("HTTP server shut down: %s", err)
-		// TODO: fatal errors that return from one of the handler
-		// goroutines should plumb the error back to the
-		// coordinating goroutine, so that it can do an orderly
-		// shutdown and return the error from Serve(). This "log +
-		// randomly stop a piece of pixiecore" is a terrible
-		// kludge.
-		return
 	}
+	return nil
 }
 
 func (s *Server) handleIpxe(w http.ResponseWriter, r *http.Request) {

pixiecore/pixiecore.go

@@ -153,6 +153,8 @@ type Server struct {
 	DHCPPort int
 	TFTPPort int
 	PXEPort  int
+
+	errs chan error
 }
 
 // Serve listens for machines attempting to boot, and uses Booter to
@@ -194,11 +196,30 @@ func (s *Server) Serve() error {
 		return err
 	}
 
-	// TODO: have something here for orderly shutdown when things go wrong.
+	// 5 buffer slots, one for each goroutine, plus one for
+	// Shutdown(). We only ever pull the first error out, but shutdown
+	// will likely generate some spurious errors from the other
+	// goroutines, and we want them to be able to dump them without
+	// blocking.
+	s.errs = make(chan error, 5)
 
-	go s.serveDHCP(dhcp)
+	go func() { s.errs <- s.serveDHCP(dhcp) }()
-	go s.servePXE(pxe)
+	go func() { s.errs <- s.servePXE(pxe) }()
-	go s.serveTFTP(tftp)
+	go func() { s.errs <- s.serveTFTP(tftp) }()
-	go s.serveHTTP(http)
+	go func() { s.errs <- s.serveHTTP(http) }()
-	select {}
+
+	// Wait for either a fatal error, or Shutdown().
+	err = <-s.errs
+	dhcp.Close()
+	tftp.Close()
+	pxe.Close()
+	http.Close()
+	return err
+}
+
+func (s *Server) Shutdown() {
+	select {
+	case s.errs <- nil:
+	default:
+	}
 }

pixiecore/pxe.go

@@ -29,25 +29,17 @@ import (
 // TianoCore EDK2 source code to figure out if what this is doing is
 // actually BINL, and if so rename everything.
 
-func (s *Server) servePXE(conn net.PacketConn) {
+func (s *Server) servePXE(conn net.PacketConn) error {
 	buf := make([]byte, 1024)
 	l := ipv4.NewPacketConn(conn)
 	if err := l.SetControlMessage(ipv4.FlagInterface, true); err != nil {
-		// TODO: fatal errors that return from one of the handler
+		return fmt.Errorf("Couldn't get interface metadata on PXE port: %s", err)
-		// goroutines should plumb the error back to the
-		// coordinating goroutine, so that it can do an orderly
-		// shutdown and return the error from Serve(). This "log +
-		// randomly stop a piece of pixiecore" is a terrible
-		// kludge.
-		s.log("PXE", "Couldn't get interface metadata on PXE port: %s", err)
-		return
 	}
 
 	for {
 		n, msg, addr, err := l.ReadFrom(buf)
 		if err != nil {
-			s.log("PXE", "Receiving packet: %s", err)
+			return fmt.Errorf("Receiving packet: %s", err)
-			return
 		}
 
 		pkt, err := dhcp4.Unmarshal(buf[:n])
@@ -86,6 +78,7 @@ func (s *Server) servePXE(conn net.PacketConn) {
 		bs, err := resp.Marshal()
 		if err != nil {
 			s.log("PXE", "Failed to marshal PXE offer for %s (%s): %s", pkt.HardwareAddr, addr, err)
+			continue
 		}
 
 		if _, err := l.WriteTo(bs, &ipv4.ControlMessage{

pixiecore/tftp.go

@@ -26,7 +26,7 @@ import (
 	"go.universe.tf/netboot/tftp"
 )
 
-func (s *Server) serveTFTP(l net.PacketConn) {
+func (s *Server) serveTFTP(l net.PacketConn) error {
 	ts := tftp.Server{
 		Handler:     s.handleTFTP,
 		InfoLog:     func(msg string) { s.debug("TFTP", msg) },
@@ -34,14 +34,9 @@ func (s *Server) serveTFTP(l net.PacketConn) {
 	}
 	err := ts.Serve(l)
 	if err != nil {
-		// TODO: fatal errors that return from one of the handler
+		return fmt.Errorf("TFTP server shut down: %s", err)
-		// goroutines should plumb the error back to the
-		// coordinating goroutine, so that it can do an orderly
-		// shutdown and return the error from Serve(). This "log +
-		// randomly stop a piece of pixiecore" is a terrible
-		// kludge.
-		s.log("TFTP", "Server shut down unexpectedly: %s", err)
 	}
+	return nil
 }
 
 func (s *Server) logTFTPTransfer(clientAddr net.Addr, path string, err error) {