A few minor Go style suggestions.

api/handler.go

@@ -12,8 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-// Handler for /api/
+// Package api provides a handler for /api/
-
 package api
 
 import (
@@ -47,8 +46,7 @@ var supportedApiVersions map[string]struct{} = map[string]struct{}{
 
 func RegisterHandlers(m manager.Manager) error {
 	http.HandleFunc(apiResource, func(w http.ResponseWriter, r *http.Request) {
-		err := handleRequest(m, w, r)
+		if err := handleRequest(m, w, r); err != nil {
-		if err != nil {
 			fmt.Fprintf(w, "%s", err)
 		}
 	})

client/client.go

@@ -12,6 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+// TODO(cAdvisor): Package comment.
 package cadvisor
 
 import (

container/container.go

@@ -12,27 +12,34 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+// Pacakge container defines types for sub-container events and also
+// defines an interface for container operation handlers.
 package container
 
 import "github.com/google/cadvisor/info"
 
-// Listing types.
+// ListType describes whether listing should be just for a
-const (
+// specific container or performed recurisvely.
-	LIST_SELF = iota
-	LIST_RECURSIVE
-)
-
 type ListType int
 
-// SubcontainerEvent types.
 const (
-	SUBCONTAINER_ADD = iota
+	ListSelf ListType = iota
-	SUBCONTAINER_DELETE
+	ListRecursive
 )
 
+// SubcontainerEventType indicated whether the event
+// specifies an addition or deletion.
+type SubcontainerEventType int
+
+const (
+	SubcontainerAdd SubcontainerEventType = iota
+	SubcontainerDelete
+)
+
+// SubcontainerEvent represents a
 type SubcontainerEvent struct {
 	// The type of event that occurred.
-	EventType int
+	EventType SubcontainerEventType
 
 	// The full container name of the container where the event occurred.
 	Name string

container/raw/handler.go

@@ -12,6 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+// TODO(cAdvosor): Package comment.
 package raw
 
 import (
@@ -166,8 +167,7 @@ func listDirectories(dirpath string, parent string, recursive bool, output map[s
 
 			// List subcontainers if asked to.
 			if recursive {
-				err := listDirectories(path.Join(dirpath, entry.Name()), name, true, output)
+				if err := listDirectories(path.Join(dirpath, entry.Name()), name, true, output); err != nil {
-				if err != nil {
 					return err
 				}
 			}
@@ -179,7 +179,7 @@ func listDirectories(dirpath string, parent string, recursive bool, output map[s
 func (self *rawContainerHandler) ListContainers(listType container.ListType) ([]info.ContainerReference, error) {
 	containers := make(map[string]struct{})
 	for _, subsystem := range self.cgroupSubsystems.mounts {
-		err := listDirectories(path.Join(subsystem.Mountpoint, self.name), self.name, listType == container.LIST_RECURSIVE, containers)
+		err := listDirectories(path.Join(subsystem.Mountpoint, self.name), self.name, listType == container.ListRecursive, containers)
 		if err != nil {
 			return nil, err
 		}
@@ -206,8 +206,7 @@ func (self *rawContainerHandler) ListProcesses(listType container.ListType) ([]i
 }
 
 func (self *rawContainerHandler) watchDirectory(dir string, containerName string) error {
-	err := self.watcher.AddWatch(dir, inotify.IN_CREATE|inotify.IN_DELETE|inotify.IN_MOVE)
+	if err := self.watcher.AddWatch(dir, inotify.IN_CREATE|inotify.IN_DELETE|inotify.IN_MOVE); err != nil {
-	if err != nil {
 		return err
 	}
 	self.watches[containerName] = struct{}{}
@@ -230,16 +229,16 @@ func (self *rawContainerHandler) watchDirectory(dir string, containerName string
 
 func (self *rawContainerHandler) processEvent(event *inotify.Event, events chan container.SubcontainerEvent) error {
 	// Convert the inotify event type to a container create or delete.
-	var eventType int
+	var eventType container.SubcontainerEventType
 	switch {
 	case (event.Mask & inotify.IN_CREATE) > 0:
-		eventType = container.SUBCONTAINER_ADD
+		eventType = container.SubcontainerAdd
 	case (event.Mask & inotify.IN_DELETE) > 0:
-		eventType = container.SUBCONTAINER_DELETE
+		eventType = container.SubcontainerDelete
 	case (event.Mask & inotify.IN_MOVED_FROM) > 0:
-		eventType = container.SUBCONTAINER_DELETE
+		eventType = container.SubcontainerDelete
 	case (event.Mask & inotify.IN_MOVED_TO) > 0:
-		eventType = container.SUBCONTAINER_ADD
+		eventType = container.SubcontainerAdd
 	default:
 		// Ignore other events.
 		return nil
@@ -260,18 +259,17 @@ func (self *rawContainerHandler) processEvent(event *inotify.Event, events chan
 
 	// Maintain the watch for the new or deleted container.
 	switch {
-	case eventType == container.SUBCONTAINER_ADD:
+	case eventType == container.SubcontainerAdd:
 		// If we've already seen this event, return.
 		if _, ok := self.watches[containerName]; ok {
 			return nil
 		}
 
 		// New container was created, watch it.
-		err := self.watchDirectory(event.Name, containerName)
+		if err := self.watchDirectory(event.Name, containerName); err != nil {
-		if err != nil {
 			return err
 		}
-	case eventType == container.SUBCONTAINER_DELETE:
+	case eventType == container.SubcontainerDelete:
 		// If we've already seen this event, return.
 		if _, ok := self.watches[containerName]; !ok {
 			return nil
@@ -279,8 +277,7 @@ func (self *rawContainerHandler) processEvent(event *inotify.Event, events chan
 		delete(self.watches, containerName)
 
 		// Container was deleted, stop watching for it.
-		err := self.watcher.RemoveWatch(event.Name)
+		if err := self.watcher.RemoveWatch(event.Name); err != nil {
-		if err != nil {
 			return err
 		}
 	default:
@@ -308,8 +305,7 @@ func (self *rawContainerHandler) WatchSubcontainers(events chan container.Subcon
 
 	// Watch this container (all its cgroups) and all subdirectories.
 	for _, mnt := range self.cgroupSubsystems.mounts {
-		err := self.watchDirectory(path.Join(mnt.Mountpoint, self.name), self.name)
+		if err := self.watchDirectory(path.Join(mnt.Mountpoint, self.name), self.name); err != nil {
-		if err != nil {
 			return err
 		}
 	}

manager/container.go

@@ -12,8 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-// Per-container manager.
+// Package mananger provides per-container manager support.
-
 package manager
 
 import (
@@ -80,8 +79,7 @@ func (c *containerData) GetInfo() (*containerInfo, error) {
 	// Make a copy of the info for the user.
 	c.lock.Lock()
 	defer c.lock.Unlock()
-	ret := c.info
+	return &c.info, nil
-	return &ret, nil
 }
 
 func newContainerData(containerName string, driver storage.StorageDriver, handler container.ContainerHandler) (*containerData, error) {
@@ -199,15 +197,14 @@ func (c *containerData) updateStats() error {
 	if err != nil {
 		return err
 	}
-	err = c.storageDriver.AddStats(ref, stats)
+	if err = c.storageDriver.AddStats(ref, stats); err != nil {
-	if err != nil {
 		return err
 	}
 	return nil
 }
 
 func (c *containerData) updateSubcontainers() error {
-	subcontainers, err := c.handler.ListContainers(container.LIST_SELF)
+	subcontainers, err := c.handler.ListContainers(container.ListSelf)
 	if err != nil {
 		return err
 	}

manager/container_test.go

@@ -48,13 +48,12 @@ func TestUpdateSubcontainers(t *testing.T) {
 		{Name: "/container/something"},
 	}
 	cd, mockHandler, _ := newTestContainerData(t)
-	mockHandler.On("ListContainers", container.LIST_SELF).Return(
+	mockHandler.On("ListContainers", container.ListSelf).Return(
 		subcontainers,
 		nil,
 	)
 
-	err := cd.updateSubcontainers()
+	if err := cd.updateSubcontainers(); err != nil {
-	if err != nil {
 		t.Fatal(err)
 	}
 
@@ -79,13 +78,12 @@ func TestUpdateSubcontainers(t *testing.T) {
 
 func TestUpdateSubcontainersWithError(t *testing.T) {
 	cd, mockHandler, _ := newTestContainerData(t)
-	mockHandler.On("ListContainers", container.LIST_SELF).Return(
+	mockHandler.On("ListContainers", container.ListSelf).Return(
 		[]info.ContainerReference{},
 		fmt.Errorf("some error"),
 	)
 
-	err := cd.updateSubcontainers()
+	if err := cd.updateSubcontainers(); err == nil {
-	if err == nil {
 		t.Fatal("updateSubcontainers should return error")
 	}
 	if len(cd.info.Subcontainers) != 0 {
@@ -107,8 +105,7 @@ func TestUpdateStats(t *testing.T) {
 
 	mockDriver.On("AddStats", info.ContainerReference{Name: mockHandler.Name}, stats).Return(nil)
 
-	err := cd.updateStats()
+	if err := cd.updateStats(); err != nil {
-	if err != nil {
 		t.Fatal(err)
 	}
 
@@ -143,7 +140,7 @@ func TestGetInfo(t *testing.T) {
 		spec,
 		nil,
 	)
-	mockHandler.On("ListContainers", container.LIST_SELF).Return(
+	mockHandler.On("ListContainers", container.ListSelf).Return(
 		subcontainers,
 		nil,
 	)

manager/manager.go

@@ -12,6 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+// TODO(cAdvisor): Package comment.
 package manager
 
 import (
@@ -30,6 +31,8 @@ import (
 
 var globalHousekeepingInterval = flag.Duration("global_housekeeping_interval", 1*time.Minute, "Interval between global housekeepings")
 
+// The Manager interface defines operations for starting a manager and getting
+// container and machine information.
 type Manager interface {
 	// Start the manager.
 	Start() error
@@ -50,6 +53,7 @@ type Manager interface {
 	GetVersionInfo() (*info.VersionInfo, error)
 }
 
+// New takes a driver and returns a new manager.
 func New(driver storage.StorageDriver) (Manager, error) {
 	if driver == nil {
 		return nil, fmt.Errorf("nil storage driver!")
@@ -90,13 +94,11 @@ type manager struct {
 // Start the container manager.
 func (self *manager) Start() error {
 	// Create root and then recover all containers.
-	err := self.createContainer("/")
+	if err := self.createContainer("/"); err != nil {
-	if err != nil {
 		return err
 	}
 	glog.Infof("Starting recovery of all containers")
-	err = self.detectSubcontainers("/")
+	if err := self.detectSubcontainers("/"); err != nil {
-	if err != nil {
 		return err
 	}
 	glog.Infof("Recovery completed")
@@ -253,13 +255,11 @@ func (self *manager) SubcontainersInfo(containerName string, query *info.Contain
 
 func (m *manager) GetMachineInfo() (*info.MachineInfo, error) {
 	// Copy and return the MachineInfo.
-	ret := m.machineInfo
+	return &m.machineInfo, nil
-	return &ret, nil
 }
 
 func (m *manager) GetVersionInfo() (*info.VersionInfo, error) {
-	ret := m.versionInfo
+	return &m.versionInfo, nil
-	return &ret, nil
 }
 
 // Create a container.
@@ -279,8 +279,7 @@ func (m *manager) createContainer(containerName string) error {
 		defer m.containersLock.Unlock()
 
 		// Check that the container didn't already exist
-		_, ok := m.containers[containerName]
+		if _, ok := m.containers[containerName]; ok {
-		if ok {
 			return true
 		}
 
@@ -335,9 +334,9 @@ func (m *manager) getContainersDiff(containerName string) (added []info.Containe
 	// Get all subcontainers recursively.
 	cont, ok := m.containers[containerName]
 	if !ok {
-		return nil, nil, fmt.Errorf("Failed to find container %q while checking for new containers", containerName)
+		return nil, nil, fmt.Errorf("failed to find container %q while checking for new containers", containerName)
 	}
-	allContainers, err := cont.handler.ListContainers(container.LIST_RECURSIVE)
+	allContainers, err := cont.handler.ListContainers(container.ListRecursive)
 	if err != nil {
 		return nil, nil, err
 	}
@@ -355,8 +354,7 @@ func (m *manager) getContainersDiff(containerName string) (added []info.Containe
 	// Added containers
 	for _, c := range allContainers {
 		delete(allContainersSet, c.Name)
-		_, ok := m.containers[c.Name]
+		if _, ok := m.containers[c.Name]; !ok {
-		if !ok {
 			added = append(added, c)
 		}
 	}
@@ -380,15 +378,14 @@ func (m *manager) detectSubcontainers(containerName string) error {
 	for _, cont := range added {
 		err = m.createContainer(cont.Name)
 		if err != nil {
-			glog.Errorf("Failed to create existing container: %s: %s", cont.Name, err)
+			glog.Errorf("failed to create existing container: %s: %s", cont.Name, err)
 		}
 	}
 
 	// Remove the old containers.
 	for _, cont := range removed {
-		err = m.destroyContainer(cont.Name)
+		if err = m.destroyContainer(cont.Name); err != nil {
-		if err != nil {
+			glog.Errorf("failed to destroy existing container: %s: %s", cont.Name, err)
-			glog.Errorf("Failed to destroy existing container: %s: %s", cont.Name, err)
 		}
 	}
 
@@ -396,8 +393,8 @@ func (m *manager) detectSubcontainers(containerName string) error {
 }
 
 func (self *manager) processEvent(event container.SubcontainerEvent) error {
-	var err error = nil
+	// TODO(cAdvisor): Why does this method always return nil? [satnam6502]
-	return err
+	return nil
 }
 
 // Watches for new containers started in the system. Runs forever unless there is a setup error.
@@ -415,14 +412,12 @@ func (self *manager) watchForNewContainers(quit chan error) error {
 
 	// Register for new subcontainers.
 	events := make(chan container.SubcontainerEvent, 16)
-	err := root.handler.WatchSubcontainers(events)
+	if err := root.handler.WatchSubcontainers(events); err != nil {
-	if err != nil {
 		return err
 	}
 
 	// There is a race between starting the watch and new container creation so we do a detection before we read new containers.
-	err = self.detectSubcontainers("/")
+	if err := self.detectSubcontainers("/"); err != nil {
-	if err != nil {
 		return err
 	}
 

manager/manager_test.go

@@ -83,7 +83,7 @@ func expectManagerWithContainers(containers []string, query *info.ContainerInfoR
 				nil,
 			)
 
-			h.On("ListContainers", container.LIST_SELF).Return(
+			h.On("ListContainers", container.ListSelf).Return(
 				[]info.ContainerReference(nil),
 				nil,
 			)
@@ -176,8 +176,7 @@ func TestNew(t *testing.T) {
 }
 
 func TestNewNilManager(t *testing.T) {
-	_, err := New(nil)
+	if _, err := New(nil); err == nil {
-	if err == nil {
 		t.Fatalf("Expected nil manager to return error")
 	}
 }