From 859d1791b83625cce8c20ee80298ba6ccc30497c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Fri, 21 Oct 2022 22:34:17 +0200 Subject: [PATCH] In load methods, track the inconsistency we want to fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... 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č --- containers.go | 8 ++++---- images.go | 10 +++++----- layers.go | 12 ++++++------ 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/containers.go b/containers.go index 19cf5206f..4cd3ebaa5 100644 --- a/containers.go +++ b/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() } diff --git a/images.go b/images.go index 1e144b17f..40a2b00e5 100644 --- a/images.go +++ b/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 diff --git a/layers.go b/layers.go index 3571ffd69..9c70284ae 100644 --- a/layers.go +++ b/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 }