diff --git a/pixiecore/dhcp.go b/pixiecore/dhcp.go index 9886b30..96d49e9 100644 --- a/pixiecore/dhcp.go +++ b/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) - // 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 fmt.Errorf("Receiving DHCP packet: %s", err) } 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 + return fmt.Errorf("Received DHCP packet with no interface information (this is a violation of dhcp4.Conn's contract, please file a bug)") } if err = s.isBootDHCP(pkt); err != nil { diff --git a/pixiecore/http.go b/pixiecore/http.go index ef8dc11..e1730eb 100644 --- a/pixiecore/http.go +++ b/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) - // 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 fmt.Errorf("HTTP server shut down: %s", err) } + return nil } func (s *Server) handleIpxe(w http.ResponseWriter, r *http.Request) { diff --git a/pixiecore/pixiecore.go b/pixiecore/pixiecore.go index 263e2b9..b9b2045 100644 --- a/pixiecore/pixiecore.go +++ b/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 s.servePXE(pxe) - go s.serveTFTP(tftp) - go s.serveHTTP(http) - select {} + go func() { s.errs <- s.serveDHCP(dhcp) }() + go func() { s.errs <- s.servePXE(pxe) }() + go func() { s.errs <- s.serveTFTP(tftp) }() + go func() { s.errs <- s.serveHTTP(http) }() + + // 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: + } } diff --git a/pixiecore/pxe.go b/pixiecore/pxe.go index 1767c02..b17f693 100644 --- a/pixiecore/pxe.go +++ b/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 - // 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 + return fmt.Errorf("Couldn't get interface metadata on PXE port: %s", err) } for { n, msg, addr, err := l.ReadFrom(buf) if err != nil { - s.log("PXE", "Receiving packet: %s", err) - return + return fmt.Errorf("Receiving packet: %s", err) } 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{ diff --git a/pixiecore/tftp.go b/pixiecore/tftp.go index 3490beb..327d365 100644 --- a/pixiecore/tftp.go +++ b/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 - // 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 fmt.Errorf("TFTP server shut down: %s", err) } + return nil } func (s *Server) logTFTPTransfer(clientAddr net.Addr, path string, err error) {