Skip to content

Overlay cleanups after #666#783

Draft
mtrmac wants to merge 6 commits intocontainers:mainfrom
mtrmac:overlay
Draft

Overlay cleanups after #666#783
mtrmac wants to merge 6 commits intocontainers:mainfrom
mtrmac:overlay

Conversation

@mtrmac
Copy link
Copy Markdown
Contributor

@mtrmac mtrmac commented Apr 23, 2026

Following up on #666 (review) .

Marked as draft:

  • This should pass a Podman test suite
  • To allow early review. Cc: @giuseppe

@github-actions github-actions Bot added the storage Related to "storage" package label Apr 23, 2026
@packit-as-a-service
Copy link
Copy Markdown

Packit jobs failed. @containers/packit-build please check.

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Copy link
Copy Markdown
Contributor Author

mtrmac commented Apr 23, 2026

Podman test PR: containers/podman#28571 .

}
return nil, err
}
var res []string
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we know how many items are in the sequence, worth preallocating?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code uses SplitSeq, so we don’t actually trivially know. We could scan one more time to count that, and have everyone carefully think about the required +1… And anyway this is the legacy path…


Actually the lowerLayers path is currently unnecessarily creating an iterator and then allocating anyway. I’ll fix that.


Hypothetically all of these could return iterators instead of allocating slices, but that would complicate error handling and the current uses of strings.Join.

mtrmac added 5 commits April 23, 2026 16:32
... to be less confusing with a future getLowerLayerIDs.

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
getLowerForParent never returns "" on success
(and we are on a branch where parent != "").

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This avoids Readlink() on layer IDs we _know_ are not links
(and a risk of Readlink() actually finding something at that relative
path), and simplifies users to again use unambiguous data contents.

Then we can also use ordinary d.dir() to look up the directory to Stat().

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Don't bother with the d.dir() filesystem lookups;
they will be equal iff the layer IDs are equal.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Only moves unchanged code, should not change behavior.

Also document the difference between getLowerDirs and
getLowerDiffPaths.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Copy link
Copy Markdown
Contributor Author

mtrmac commented Apr 23, 2026

(The rebase moves getLowerLayerIDs earlier, so that a later commit does not move it again.)

mtrmac added a commit to mtrmac/libpod that referenced this pull request Apr 23, 2026
> go mod edit -replace go.podman.io/storage=github.com/mtrmac/container-libs/storage@overlay

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

storage Related to "storage" package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants