Create single bsdiff logger

The bsdiff logger was shared globally between several goroutines and in
some cases, a goroutine would override the existing logger with a newly
created one.

This change centralizes the logger creation so that it is only created
once which prevents logger creation race conditions.

Signed-off-by: John Akre <john.w.akre@intel.com>

builder/builder.go

@@ -501,10 +501,18 @@ func (b *Builder) BuildDeltaPacks(from, to uint32, printReport bool) error {
 	bundleDir := filepath.Join(b.Config.Builder.ServerStateDir, "image")
 	fmt.Printf("Using %d workers\n", b.NumDeltaWorkers)
 	// Create all deltas first
-	err = swupd.CreateAllDeltas(outputDir, int(fromManifest.Header.Version), int(toManifest.Header.Version), b.NumDeltaWorkers)
+	bsdiffLog, logFile, err := swupd.CreateBsdiffLogger(b.Config.Builder.ServerStateDir)
 	if err != nil {
 		return err
 	}
+	defer func() {
+		_ = logFile.Close()
+	}()
+	err = swupd.CreateAllDeltas(outputDir, int(fromManifest.Header.Version), int(toManifest.Header.Version), b.NumDeltaWorkers, bsdiffLog)
+	if err != nil {
+		return err
+	}
+
 	// Create packs filling in any missing deltas
 	return createDeltaPacks(fromManifest, toManifest, printReport, outputDir, bundleDir, b.NumDeltaWorkers)
 }
@@ -571,6 +579,14 @@ func (b *Builder) BuildDeltaPacksPreviousVersions(prev, to uint32, printReport b
 	wg.Add(versionWorkers)
 	fmt.Printf("Using %d version threads and %d delta threads in each\n", versionWorkers, b.NumDeltaWorkers)
 
+	bsdiffLog, logFile, err := swupd.CreateBsdiffLogger(b.Config.Builder.ServerStateDir)
+	if err != nil {
+		return err
+	}
+	defer func() {
+		_ = logFile.Close()
+	}()
+
 	// If possible, run a thread for each version back so we don't get locked up
 	// at the end of a version doing some large/slow delta pack in serial. This way
 	// the large file(s) at the end of each version will run in parallel.
@@ -578,7 +594,7 @@ func (b *Builder) BuildDeltaPacksPreviousVersions(prev, to uint32, printReport b
 		go func() {
 			defer wg.Done()
 			for fromManifest := range versionQueue {
-				deltaErr := swupd.CreateAllDeltas(outputDir, int(fromManifest.Header.Version), int(toManifest.Header.Version), b.NumDeltaWorkers)
+				deltaErr := swupd.CreateAllDeltas(outputDir, int(fromManifest.Header.Version), int(toManifest.Header.Version), b.NumDeltaWorkers, bsdiffLog)
 				if deltaErr != nil {
 					deltaErrors = append(deltaErrors, deltaErr)
 				}

swupd/delta.go

@@ -47,13 +47,11 @@ type Delta struct {
 	to    *File
 }
 
-var bsdiffLog *log.Logger
-
 // CreateDeltasForManifest creates all delta files between the previous and current version of the
 // supplied manifest. Returns a list of deltas (which contains information about
 // individual delta errors). Returns error (and no deltas) if it can't assemble the delta
 // list. If number of workers is zero or less, 1 worker is used.
-func CreateDeltasForManifest(manifest, statedir string, from, to uint32, numWorkers int) ([]Delta, error) {
+func CreateDeltasForManifest(manifest, statedir string, from, to uint32, numWorkers int, bsdiffLog *log.Logger) ([]Delta, error) {
 	var c config
 
 	c, err := getConfig(statedir)
@@ -71,19 +69,22 @@ func CreateDeltasForManifest(manifest, statedir string, from, to uint32, numWork
 		return nil, err
 	}
 
-	return createDeltasFromManifests(&c, oldManifest, newManifest, numWorkers)
+	return createDeltasFromManifests(&c, oldManifest, newManifest, numWorkers, bsdiffLog)
 }
 
-func createDeltasFromManifests(c *config, oldManifest, newManifest *Manifest, numWorkers int) ([]Delta, error) {
-	logFile, err := os.OpenFile(filepath.Join(c.stateDir, "bsdiff_errors.log"), os.O_RDWR|os.O_CREATE|os.O_APPEND, 0666)
+// CreateBsdiffLogger creates a logger for the bsdiff_errors.log file at the directory
+// specified by stateDir.
+func CreateBsdiffLogger(stateDir string) (*log.Logger, *os.File, error) {
+	logFile, err := os.OpenFile(filepath.Join(stateDir, "bsdiff_errors.log"), os.O_RDWR|os.O_CREATE|os.O_APPEND, 0666)
 	if err != nil {
-		return nil, errors.Wrap(err, "Cannot create log file for delta creation")
+		return nil, nil, errors.Wrap(err, "Cannot create log file for delta creation")
 	}
-	defer func() {
-		_ = logFile.Close()
-	}()
-	bsdiffLog = log.New(logFile, "DELTA: ", log.Lshortfile)
+	bsdiffLog := log.New(logFile, "DELTA: ", log.Lshortfile)
 
+	return bsdiffLog, logFile, nil
+}
+
+func createDeltasFromManifests(c *config, oldManifest, newManifest *Manifest, numWorkers int, bsdiffLog *log.Logger) ([]Delta, error) {
 	deltas, err := findDeltas(c, oldManifest, newManifest)
 	if err != nil {
 		return nil, errors.Wrapf(err, "Failed to create deltas list %s", newManifest.Name)
@@ -105,7 +106,7 @@ func createDeltasFromManifests(c *config, oldManifest, newManifest *Manifest, nu
 		go func() {
 			defer wg.Done()
 			for delta := range deltaQueue {
-				delta.Error = createFileDelta(c, delta)
+				delta.Error = createFileDelta(c, delta, bsdiffLog)
 			}
 		}()
 	}
@@ -133,7 +134,7 @@ func CreateManifestDeltas(statedir string, fromManifest, toManifest *Manifest, n
 		return nil, err
 	}
 
-	logFile, err := os.OpenFile(filepath.Join(c.stateDir, "bsdiff_errors.log"), os.O_RDWR|os.O_CREATE|os.O_APPEND, 0666)
+	bsdiffLog, logFile, err := CreateBsdiffLogger(c.stateDir)
 	if err != nil {
 		return nil, errors.Wrap(err, "Cannot create log file for delta creation")
 	}
@@ -141,7 +142,6 @@ func CreateManifestDeltas(statedir string, fromManifest, toManifest *Manifest, n
 	defer func() {
 		_ = logFile.Close()
 	}()
-	bsdiffLog = log.New(logFile, "DELTA: ", log.Lshortfile)
 
 	toManifest.sortFilesName()
 	fromManifest.sortFilesName()
@@ -204,7 +204,7 @@ func CreateManifestDeltas(statedir string, fromManifest, toManifest *Manifest, n
 		go func() {
 			defer wg.Done()
 			for delta := range deltaQueue {
-				delta.Error = createManifestDelta(&c, delta)
+				delta.Error = createManifestDelta(&c, delta, bsdiffLog)
 			}
 		}()
 	}
@@ -246,21 +246,21 @@ func deltaTooLarge(c *config, delta *Delta, newPath string) bool {
 	return deltaSize >= fcSize
 }
 
-func createFileDelta(c *config, delta *Delta) error {
+func createFileDelta(c *config, delta *Delta, bsdiffLog *log.Logger) error {
 	oldPath := filepath.Join(c.imageBase, fmt.Sprint(delta.from.Version), "full", delta.from.Name)
 	newPath := filepath.Join(c.imageBase, fmt.Sprint(delta.to.Version), "full", delta.to.Name)
 
-	return createDelta(c, oldPath, newPath, delta)
+	return createDelta(c, oldPath, newPath, delta, bsdiffLog)
 }
 
-func createManifestDelta(c *config, delta *Delta) error {
+func createManifestDelta(c *config, delta *Delta, bsdiffLog *log.Logger) error {
 	oldPath := filepath.Join(c.stateDir, "www", fmt.Sprint(delta.from.Version), "Manifest."+delta.from.Name)
 	newPath := filepath.Join(c.stateDir, "www", fmt.Sprint(delta.to.Version), "Manifest."+delta.to.Name)
 
-	return createDelta(c, oldPath, newPath, delta)
+	return createDelta(c, oldPath, newPath, delta, bsdiffLog)
 }
 
-func createDelta(c *config, oldPath, newPath string, delta *Delta) error {
+func createDelta(c *config, oldPath, newPath string, delta *Delta, bsdiffLog *log.Logger) error {
 	if _, err := os.Stat(delta.Path); err == nil {
 		// Skip existing deltas. Not verifying since client is resilient about that.
 		return nil

swupd/helpers_test.go

@@ -255,7 +255,16 @@ func checkManifestMatches(t *testing.T, testDir, ver, name string, res ...*regex
 
 func mustCreateAllDeltas(t *testing.T, manifest, statedir string, from, to uint32) {
 	t.Helper()
-	deltas, err := CreateDeltasForManifest(manifest, statedir, from, to, 0)
+
+	bsdiffLog, logFile, err := CreateBsdiffLogger(statedir)
+	if err != nil {
+		t.Fatalf("couldn't create logger for %s: %s", manifest, err)
+	}
+	defer func() {
+		_ = logFile.Close()
+	}()
+
+	deltas, err := CreateDeltasForManifest(manifest, statedir, from, to, 0, bsdiffLog)
 	if err != nil {
 		t.Fatalf("couldn't create deltas for %s: %s", manifest, err)
 	}
@@ -273,7 +282,16 @@ func mustCreateAllDeltas(t *testing.T, manifest, statedir string, from, to uint3
 
 func tryCreateAllDeltas(t *testing.T, manifest, statedir string, from, to uint32) {
 	t.Helper()
-	_, err := CreateDeltasForManifest(manifest, statedir, from, to, 0)
+
+	bsdiffLog, logFile, err := CreateBsdiffLogger(statedir)
+	if err != nil {
+		t.Fatalf("couldn't create logger for %s: %s", manifest, err)
+	}
+	defer func() {
+		_ = logFile.Close()
+	}()
+
+	_, err = CreateDeltasForManifest(manifest, statedir, from, to, 0, bsdiffLog)
 	if err != nil {
 		t.Fatalf("couldn't create deltas for %s: %s", manifest, err)
 	}

swupd/packs.go

@@ -78,7 +78,7 @@ func (state PackState) String() string {
 // CreateAllDeltas builds all of the deltas using the full manifest from one
 // version to the next. This allows better concurrency and the pack creation
 // code can just worry about adding pre-existing files to packs.
-func CreateAllDeltas(outputDir string, fromVersion, toVersion, numWorkers int) error {
+func CreateAllDeltas(outputDir string, fromVersion, toVersion, numWorkers int, bsdiffLog *log.Logger) error {
 	// Don't try to make deltas for zero packs
 	if fromVersion == 0 {
 		return nil
@@ -105,7 +105,7 @@ func CreateAllDeltas(outputDir string, fromVersion, toVersion, numWorkers int) e
 		return err
 	}
 
-	_, err = createDeltasFromManifests(&c, fromManifest, toManifest, numWorkers)
+	_, err = createDeltasFromManifests(&c, fromManifest, toManifest, numWorkers, bsdiffLog)
 	if err != nil {
 		return err
 	}

swupd/packs_test.go

@@ -604,7 +604,21 @@ func mustValidateZeroPack(t *testing.T, manifestPath, packPath string) {
 func mustCreatePack(t *testing.T, name string, fromVersion, toVersion uint32, outputDir, chrootDir string) *PackInfo {
 	debugPacks = true
 	t.Helper()
-	err := CreateAllDeltas(outputDir, int(fromVersion), int(toVersion), 0)
+
+	c, err := getConfig(filepath.Join(outputDir, ".."))
+	if err != nil {
+		t.Fatalf("couldn't get config for %s: %s", name, err)
+	}
+
+	bsdiffLog, logFile, err := CreateBsdiffLogger(c.stateDir)
+	if err != nil {
+		t.Fatalf("couldn't create logger for %s: %s", name, err)
+	}
+	defer func() {
+		_ = logFile.Close()
+	}()
+
+	err = CreateAllDeltas(outputDir, int(fromVersion), int(toVersion), 0, bsdiffLog)
 	if err != nil {
 		t.Fatalf("error creating pack for bundle %s: %s", name, err)
 	}