In load methods, track the inconsistency we want to fix

... as an error value instead of just a boolean.  That
will allow extending the logic to more kinds of inconsistencies,
and consolidates the specifics of the inconsistency knowledge
into a single place.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>

containers.go

@@ -278,7 +278,6 @@ func (r *containerStore) datapath(id, key string) string {
 // The caller must hold r.lockfile for reading _or_ writing; lockedForWriting is true
 // if it is held for writing.
 func (r *containerStore) load(lockedForWriting bool) error {
-	needSave := false
 	rpath := r.containerspath()
 	data, err := os.ReadFile(rpath)
 	if err != nil && !os.IsNotExist(err) {
@@ -295,6 +294,7 @@ func (r *containerStore) load(lockedForWriting bool) error {
 	layers := make(map[string]*Container)
 	ids := make(map[string]*Container)
 	names := make(map[string]*Container)
+	var errorToResolveBySaving error // == nil
 	for n, container := range containers {
 		idlist = append(idlist, container.ID)
 		ids[container.ID] = containers[n]
@@ -302,7 +302,7 @@ func (r *containerStore) load(lockedForWriting bool) error {
 		for _, name := range container.Names {
 			if conflict, ok := names[name]; ok {
 				r.removeName(conflict, name)
-				needSave = true
+				errorToResolveBySaving = errors.New("container store is inconsistent and the current caller does not hold a write lock")
 			}
 			names[name] = containers[n]
 		}
@@ -313,10 +313,10 @@ func (r *containerStore) load(lockedForWriting bool) error {
 	r.byid = ids
 	r.bylayer = layers
 	r.byname = names
-	if needSave {
+	if errorToResolveBySaving != nil {
 		if !lockedForWriting {
 			// Eventually, the callers should be modified to retry with a write lock, instead.
-			return errors.New("container store is inconsistent and the current caller does not hold a write lock")
+			return errorToResolveBySaving
 		}
 		return r.Save()
 	}

images.go

@@ -346,7 +346,6 @@ func (i *Image) recomputeDigests() error {
 // The caller must hold r.lockfile for reading _or_ writing; lockedForWriting is true
 // if it is held for writing.
 func (r *imageStore) load(lockedForWriting bool) error {
-	shouldSave := false
 	rpath := r.imagespath()
 	data, err := os.ReadFile(rpath)
 	if err != nil && !os.IsNotExist(err) {
@@ -363,13 +362,14 @@ func (r *imageStore) load(lockedForWriting bool) error {
 	ids := make(map[string]*Image)
 	names := make(map[string]*Image)
 	digests := make(map[digest.Digest][]*Image)
+	var errorToResolveBySaving error // == nil
 	for n, image := range images {
 		ids[image.ID] = images[n]
 		idlist = append(idlist, image.ID)
 		for _, name := range image.Names {
 			if conflict, ok := names[name]; ok {
 				r.removeName(conflict, name)
-				shouldSave = true
+				errorToResolveBySaving = ErrDuplicateImageNames
 			}
 		}
 		// Compute the digest list.
@@ -386,16 +386,16 @@ func (r *imageStore) load(lockedForWriting bool) error {
 		image.ReadOnly = !r.lockfile.IsReadWrite()
 	}
 
-	if shouldSave && (!r.lockfile.IsReadWrite() || !lockedForWriting) {
+	if errorToResolveBySaving != nil && (!r.lockfile.IsReadWrite() || !lockedForWriting) {
 		// Eventually, the callers should be modified to retry with a write lock if IsReadWrite && !lockedForWriting, instead.
-		return ErrDuplicateImageNames
+		return errorToResolveBySaving
 	}
 	r.images = images
 	r.idindex = truncindex.NewTruncIndex(idlist) // Invalid values in idlist are ignored: they are not a reason to refuse processing the whole store.
 	r.byid = ids
 	r.byname = names
 	r.bydigest = digests
-	if shouldSave {
+	if errorToResolveBySaving != nil {
 		return r.Save()
 	}
 	return nil

layers.go

@@ -459,7 +459,6 @@ func (r *layerStore) layerspath() string {
 // The caller must hold r.lockfile for reading _or_ writing; lockedForWriting is true
 // if it is held for writing.
 func (r *layerStore) load(lockedForWriting bool) error {
-	shouldSave := false
 	rpath := r.layerspath()
 	info, err := os.Stat(rpath)
 	if err != nil {
@@ -485,6 +484,7 @@ func (r *layerStore) load(lockedForWriting bool) error {
 	names := make(map[string]*Layer)
 	compressedsums := make(map[digest.Digest][]string)
 	uncompressedsums := make(map[digest.Digest][]string)
+	var errorToResolveBySaving error // == nil
 	if r.lockfile.IsReadWrite() {
 		selinux.ClearLabels()
 	}
@@ -494,7 +494,7 @@ func (r *layerStore) load(lockedForWriting bool) error {
 		for _, name := range layer.Names {
 			if conflict, ok := names[name]; ok {
 				r.removeName(conflict, name)
-				shouldSave = true
+				errorToResolveBySaving = ErrDuplicateLayerNames
 			}
 			names[name] = layers[n]
 		}
@@ -510,9 +510,9 @@ func (r *layerStore) load(lockedForWriting bool) error {
 		layer.ReadOnly = !r.lockfile.IsReadWrite()
 	}
 
-	if shouldSave && (!r.lockfile.IsReadWrite() || !lockedForWriting) {
+	if errorToResolveBySaving != nil && (!r.lockfile.IsReadWrite() || !lockedForWriting) {
 		// Eventually, the callers should be modified to retry with a write lock if IsReadWrite && !lockedForWriting, instead.
-		return ErrDuplicateLayerNames
+		return errorToResolveBySaving
 	}
 	r.layers = layers
 	r.idindex = truncindex.NewTruncIndex(idlist) // Invalid values in idlist are ignored: they are not a reason to refuse processing the whole store.
@@ -548,11 +548,11 @@ func (r *layerStore) load(lockedForWriting bool) error {
 						incompleteDeletionErrors = multierror.Append(incompleteDeletionErrors,
 							fmt.Errorf("deleting layer %#v: %w", layer.ID, err))
 					}
-					shouldSave = true
+					errorToResolveBySaving = errors.New("incomplete layer") // Essentially just "yes, do save", for now.
 				}
 			}
 		}
-		if shouldSave {
+		if errorToResolveBySaving != nil {
 			if err := r.saveLayers(); err != nil {
 				return err
 			}