Fix some dead code, missing error checks, shadowings.

I applied https://medium.com/@jgautheron/quality-pipeline-for-go-projects-497e34d6567 and was greeted with a deluge of warnings, most of which were not applicable or really fixable realistically. These are some of the first ones I decided to fix.
victorye88 · Sep 14, 2015 · af51346 · af51346
1 parent 896928a
commit af51346
Show file tree

Hide file tree

Showing 12 changed files with 50 additions and 22 deletions.
diff --git a/cmd/prometheus/config.go b/cmd/prometheus/config.go
@@ -304,5 +304,7 @@ func usage() {
 		}
 	}
 
-	t.Execute(os.Stdout, groups)
+	if err := t.Execute(os.Stdout, groups); err != nil {
+		panic(fmt.Errorf("error executing usage template: %s", err))
+	}
 }
diff --git a/promql/test.go b/promql/test.go
@@ -41,7 +41,6 @@ var (
 const (
 	testStartTime = model.Time(0)
 	epsilon       = 0.000001 // Relative error allowed for sample values.
-	maxErrorCount = 10
 )
 
 // Test is a sequence of read and write commands that are run

diff --git a/retrieval/discovery/file.go b/retrieval/discovery/file.go
@@ -179,7 +179,9 @@ func (fd *FileDiscovery) stop() {
 			}
 		}
 	}()
-	fd.watcher.Close()
+	if err := fd.watcher.Close(); err != nil {
+		log.Errorf("Error closing file watcher for %s: %s", fd.paths, err)
+	}
 
 	log.Debugf("File discovery for %s stopped.", fd.paths)
 }

diff --git a/storage/local/crashrecovery.go b/storage/local/crashrecovery.go
@@ -44,7 +44,9 @@ func (p *persistence) recoverFromCrash(fingerprintToSeries map[model.Fingerprint
 
 	// Delete the fingerprint mapping file as it might be stale or
 	// corrupt. We'll rebuild the mappings as we go.
-	os.Remove(p.mappingsFileName())
+	if err := os.RemoveAll(p.mappingsFileName()); err != nil {
+		return fmt.Errorf("couldn't remove old fingerprint mapping file %s: %s", p.mappingsFileName(), err)
+	}
 	// The mappings to rebuild.
 	fpm := fpMappings{}
 

diff --git a/storage/local/index/index.go b/storage/local/index/index.go
@@ -57,7 +57,9 @@ func (i *FingerprintMetricIndex) IndexBatch(mapping FingerprintMetricMapping) er
 	b := i.NewBatch()
 
 	for fp, m := range mapping {
-		b.Put(codable.Fingerprint(fp), codable.Metric(m))
+		if err := b.Put(codable.Fingerprint(fp), codable.Metric(m)); err != nil {
+			return err
+		}
 	}
 
 	return i.Commit(b)
@@ -72,7 +74,9 @@ func (i *FingerprintMetricIndex) UnindexBatch(mapping FingerprintMetricMapping)
 	b := i.NewBatch()
 
 	for fp := range mapping {
-		b.Delete(codable.Fingerprint(fp))
+		if err := b.Delete(codable.Fingerprint(fp)); err != nil {
+			return err
+		}
 	}
 
 	return i.Commit(b)
@@ -196,14 +200,18 @@ type LabelPairFingerprintIndex struct {
 //
 // While this method is fundamentally goroutine-safe, note that the order of
 // execution for multiple batches executed concurrently is undefined.
-func (i *LabelPairFingerprintIndex) IndexBatch(m LabelPairFingerprintsMapping) error {
+func (i *LabelPairFingerprintIndex) IndexBatch(m LabelPairFingerprintsMapping) (err error) {
 	batch := i.NewBatch()
 
 	for pair, fps := range m {
 		if len(fps) == 0 {
-			batch.Delete(codable.LabelPair(pair))
+			err = batch.Delete(codable.LabelPair(pair))
 		} else {
-			batch.Put(codable.LabelPair(pair), fps)
+			err = batch.Put(codable.LabelPair(pair), fps)
+		}
+
+		if err != nil {
+			return err
 		}
 	}
 

diff --git a/storage/local/persistence.go b/storage/local/persistence.go
@@ -1123,7 +1123,7 @@ func (p *persistence) fingerprintsModifiedBefore(beforeTime model.Time) ([]model
 	var fp codable.Fingerprint
 	var tr codable.TimeRange
 	fps := []model.Fingerprint{}
-	p.archivedFingerprintToTimeRange.ForEach(func(kv index.KeyValueAccessor) error {
+	err := p.archivedFingerprintToTimeRange.ForEach(func(kv index.KeyValueAccessor) error {
 		if err := kv.Value(&tr); err != nil {
 			return err
 		}
@@ -1135,7 +1135,7 @@ func (p *persistence) fingerprintsModifiedBefore(beforeTime model.Time) ([]model
 		}
 		return nil
 	})
-	return fps, nil
+	return fps, err
 }
 
 // archivedMetric retrieves the archived metric with the given fingerprint. This
@@ -1438,11 +1438,15 @@ func (p *persistence) checkpointFPMappings(fpm fpMappings) (err error) {
 	}
 
 	defer func() {
-		f.Sync()
+		syncErr := f.Sync()
 		closeErr := f.Close()
 		if err != nil {
 			return
 		}
+		err = syncErr
+		if err != nil {
+			return
+		}
 		err = closeErr
 		if err != nil {
 			return

diff --git a/storage/local/storage.go b/storage/local/storage.go
@@ -1111,7 +1111,9 @@ func (s *memorySeriesStorage) maintainArchivedSeries(fp model.Fingerprint, befor
 		s.seriesOps.WithLabelValues(archivePurge).Inc()
 		return
 	}
-	s.persistence.updateArchivedTimeRange(fp, newFirstTime, lastTime)
+	if err := s.persistence.updateArchivedTimeRange(fp, newFirstTime, lastTime); err != nil {
+		log.Errorf("Error updating archived time range for fingerprint %v: %s", fp, err)
+	}
 }
 
 // See persistence.loadChunks for detailed explanation.

diff --git a/storage/local/test_helpers.go b/storage/local/test_helpers.go
@@ -30,7 +30,9 @@ type testStorageCloser struct {
 }
 
 func (t *testStorageCloser) Close() {
-	t.storage.Stop()
+	if err := t.storage.Stop(); err != nil {
+		panic(err)
+	}
 	t.directory.Close()
 }
 

diff --git a/template/template.go b/template/template.go
@@ -272,11 +272,11 @@ func (te Expander) Expand() (result string, resultErr error) {
 		}
 	}()
 
-	var buffer bytes.Buffer
 	tmpl, err := text_template.New(te.name).Funcs(te.funcMap).Parse(te.text)
 	if err != nil {
 		return "", fmt.Errorf("error parsing template %v: %v", te.name, err)
 	}
+	var buffer bytes.Buffer
 	err = tmpl.Execute(&buffer, te.data)
 	if err != nil {
 		return "", fmt.Errorf("error executing template %v: %v", te.name, err)
@@ -296,7 +296,6 @@ func (te Expander) ExpandHTML(templateFiles []string) (result string, resultErr
 		}
 	}()
 
-	var buffer bytes.Buffer
 	tmpl := html_template.New(te.name).Funcs(html_template.FuncMap(te.funcMap))
 	tmpl.Funcs(html_template.FuncMap{
 		"tmpl": func(name string, data interface{}) (html_template.HTML, error) {
@@ -315,6 +314,7 @@ func (te Expander) ExpandHTML(templateFiles []string) (result string, resultErr
 			return "", fmt.Errorf("error parsing template files for %v: %v", te.name, err)
 		}
 	}
+	var buffer bytes.Buffer
 	err = tmpl.Execute(&buffer, te.data)
 	if err != nil {
 		return "", fmt.Errorf("error executing template %v: %v", te.name, err)

diff --git a/util/cli/cli.go b/util/cli/cli.go
@@ -156,13 +156,16 @@ func BasicHelp(app *App, ts string) func() string {
 		}
 
 		var buf bytes.Buffer
-		t.Execute(&buf, struct {
+		err := t.Execute(&buf, struct {
 			Name     string
 			Commands []command
 		}{
 			Name:     app.Name,
 			Commands: cmds,
 		})
+		if err != nil {
+			panic(fmt.Errorf("error executing help template: %s", err))
+		}
 		return strings.TrimSpace(buf.String())
 	}
 }
diff --git a/util/flock/flock_test.go b/util/flock/flock_test.go
@@ -27,7 +27,7 @@ func TestLocking(t *testing.T) {
 	}
 
 	// File must now exist.
-	if _, err := os.Stat(fileName); err != nil {
+	if _, err = os.Stat(fileName); err != nil {
 		t.Errorf("Could not stat file %q expected to exist: %s", fileName, err)
 	}
 
@@ -48,7 +48,7 @@ func TestLocking(t *testing.T) {
 	}
 
 	// File must still exist.
-	if _, err := os.Stat(fileName); err != nil {
+	if _, err = os.Stat(fileName); err != nil {
 		t.Errorf("Could not stat file %q expected to exist: %s", fileName, err)
 	}
 

diff --git a/util/httputil/client.go b/util/httputil/client.go
@@ -48,12 +48,16 @@ func NewDeadlineRoundTripper(timeout time.Duration, proxyURL *url.URL) http.Roun
 			start := time.Now()
 
 			c, err = net.DialTimeout(netw, addr, timeout)
+			if err != nil {
+				return nil, err
+			}
 
-			if err == nil {
-				c.SetDeadline(start.Add(timeout))
+			if err = c.SetDeadline(start.Add(timeout)); err != nil {
+				c.Close()
+				return nil, err
 			}
 
-			return
+			return c, nil
 		},
 	}
 }