Skip to content

Commit 32374d9

Browse files
mtrmacmtrmac
committed
Do not Close the ImageSource in UnparsedImage/Image
Remove the .Close() methods from UnparsedImage/Image, which closed the underlying ImageSource. Instead, just require the caller to ensure that the ImageSource is not closed as long as the UnparsedImage/Image are used. This allows using several independent UnparsedImage/Image instances for a shared ImageSource; notably independent Image objects for the individual image instances in a manifest list. (copy.Image is already simpler although it is only using a single instance.) To keep ImageReference.NewImage simple and not to break all the external callers of this, also add a simple ImageCloser wrapper which retains the ImageSource closing functionality, and return it from image.FromSource and ImageReference.NewImage implementations. (It's very likely many of the NewImage callers would be surprised by how this handles manifest lists, and it is very tempting to break this API, at least by renaming, to force the callers to consider this; however, this would be better done after eliminating the need of ImageReference.NewImage entirely, by replacing the specialized types.Image extensions with something else, first.) Signed-off-by: Miloslav Trmač <[email protected]>
1 parent f3c0826 commit 32374d9

23 files changed

+190
-170
lines changed

‎copy/copy.go‎

Lines changed: 5 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -134,6 +134,11 @@ func Image(policyContext *signature.PolicyContext, destRef, srcRef types.ImageRe
134134
if err != nil {
135135
return errors.Wrapf(err, "Error initializing source %s", transports.ImageName(srcRef))
136136
}
137+
defer func() {
138+
if err := rawSource.Close(); err != nil {
139+
retErr = errors.Wrapf(retErr, " (src: %v)", err)
140+
}
141+
}()
137142

138143
c := &copier{
139144
copiedBlobs: make(map[digest.Digest]digest.Digest),
@@ -146,13 +151,6 @@ func Image(policyContext *signature.PolicyContext, destRef, srcRef types.ImageRe
146151
}
147152

148153
unparsedImage := image.UnparsedInstance(rawSource, nil)
149-
defer func() {
150-
if unparsedImage != nil {
151-
if err := unparsedImage.Close(); err != nil {
152-
retErr = errors.Wrapf(retErr, " (unparsed: %v)", err)
153-
}
154-
}
155-
}()
156154
multiImage, err := isMultiImage(unparsedImage)
157155
if err != nil {
158156
return errors.Wrapf(err, "Error determining manifest MIME type for %s", transports.ImageName(srcRef))
@@ -171,12 +169,6 @@ func Image(policyContext *signature.PolicyContext, destRef, srcRef types.ImageRe
171169
if err != nil {
172170
return errors.Wrapf(err, "Error initializing image from source %s", transports.ImageName(srcRef))
173171
}
174-
unparsedImage = nil
175-
defer func() {
176-
if err := src.Close(); err != nil {
177-
retErr = errors.Wrapf(retErr, " (source: %v)", err)
178-
}
179-
}()
180172

181173
if err := checkImageDestinationForCurrentRuntimeOS(src, dest); err != nil {
182174
return err

‎copy/manifest_test.go‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -35,9 +35,6 @@ type fakeImageSource string
3535
func (f fakeImageSource) Reference() types.ImageReference {
3636
panic("Unexpected call to a mock function")
3737
}
38-
func (f fakeImageSource) Close() error {
39-
panic("Unexpected call to a mock function")
40-
}
4138
func (f fakeImageSource) Manifest() ([]byte, string, error) {
4239
if string(f) == "" {
4340
return nil, "", errors.New("Manifest() directed to fail")

‎directory/directory_transport.go‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -134,11 +134,12 @@ func (ref dirReference) PolicyConfigurationNamespaces() []string {
134134
return res
135135
}
136136

137-
// NewImage returns a types.Image for this reference, possibly specialized for this ImageTransport.
138-
// The caller must call .Close() on the returned Image.
137+
// NewImage returns a types.ImageCloser for this reference, possibly specialized for this ImageTransport.
138+
// The caller must call .Close() on the returned ImageCloser.
139139
// NOTE: If any kind of signature verification should happen, build an UnparsedImage from the value returned by NewImageSource,
140140
// verify that UnparsedImage, and convert it into a real Image via image.FromUnparsedImage.
141-
func (ref dirReference) NewImage(ctx *types.SystemContext) (types.Image, error) {
141+
// WARNING: This may not do the right thing for a manifest list, see image.FromSource for details.
142+
func (ref dirReference) NewImage(ctx *types.SystemContext) (types.ImageCloser, error) {
142143
src := newImageSource(ref)
143144
return image.FromSource(src)
144145
}

‎docker/archive/transport.go‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -125,11 +125,12 @@ func (ref archiveReference) PolicyConfigurationNamespaces() []string {
125125
return []string{}
126126
}
127127

128-
// NewImage returns a types.Image for this reference, possibly specialized for this ImageTransport.
129-
// The caller must call .Close() on the returned Image.
128+
// NewImage returns a types.ImageCloser for this reference, possibly specialized for this ImageTransport.
129+
// The caller must call .Close() on the returned ImageCloser.
130130
// NOTE: If any kind of signature verification should happen, build an UnparsedImage from the value returned by NewImageSource,
131131
// verify that UnparsedImage, and convert it into a real Image via image.FromUnparsedImage.
132-
func (ref archiveReference) NewImage(ctx *types.SystemContext) (types.Image, error) {
132+
// WARNING: This may not do the right thing for a manifest list, see image.FromSource for details.
133+
func (ref archiveReference) NewImage(ctx *types.SystemContext) (types.ImageCloser, error) {
133134
src := newImageSource(ctx, ref)
134135
return ctrImage.FromSource(src)
135136
}

‎docker/daemon/daemon_transport.go‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -151,9 +151,12 @@ func (ref daemonReference) PolicyConfigurationNamespaces() []string {
151151
return []string{}
152152
}
153153

154-
// NewImage returns a types.Image for this reference.
155-
// The caller must call .Close() on the returned Image.
156-
func (ref daemonReference) NewImage(ctx *types.SystemContext) (types.Image, error) {
154+
// NewImage returns a types.ImageCloser for this reference, possibly specialized for this ImageTransport.
155+
// The caller must call .Close() on the returned ImageCloser.
156+
// NOTE: If any kind of signature verification should happen, build an UnparsedImage from the value returned by NewImageSource,
157+
// verify that UnparsedImage, and convert it into a real Image via image.FromUnparsedImage.
158+
// WARNING: This may not do the right thing for a manifest list, see image.FromSource for details.
159+
func (ref daemonReference) NewImage(ctx *types.SystemContext) (types.ImageCloser, error) {
157160
src, err := newImageSource(ctx, ref)
158161
if err != nil {
159162
return nil, err

‎docker/docker_image.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,17 +12,17 @@ import (
1212
"github.com/pkg/errors"
1313
)
1414

15-
// Image is a Docker-specific implementation of types.Image with a few extra methods
15+
// Image is a Docker-specific implementation of types.ImageCloser with a few extra methods
1616
// which are specific to Docker.
1717
type Image struct {
18-
types.Image
18+
types.ImageCloser
1919
src *dockerImageSource
2020
}
2121

2222
// newImage returns a new Image interface type after setting up
2323
// a client to the registry hosting the given image.
2424
// The caller must call .Close() on the returned Image.
25-
func newImage(ctx *types.SystemContext, ref dockerReference) (types.Image, error) {
25+
func newImage(ctx *types.SystemContext, ref dockerReference) (types.ImageCloser, error) {
2626
s, err := newImageSource(ctx, ref)
2727
if err != nil {
2828
return nil, err
@@ -31,7 +31,7 @@ func newImage(ctx *types.SystemContext, ref dockerReference) (types.Image, error
3131
if err != nil {
3232
return nil, err
3333
}
34-
return &Image{Image: img, src: s}, nil
34+
return &Image{ImageCloser: img, src: s}, nil
3535
}
3636

3737
// SourceRefFullName returns a fully expanded name for the repository this image is in.

‎docker/docker_transport.go‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -122,11 +122,12 @@ func (ref dockerReference) PolicyConfigurationNamespaces() []string {
122122
return policyconfiguration.DockerReferenceNamespaces(ref.ref)
123123
}
124124

125-
// NewImage returns a types.Image for this reference, possibly specialized for this ImageTransport.
126-
// The caller must call .Close() on the returned Image.
125+
// NewImage returns a types.ImageCloser for this reference, possibly specialized for this ImageTransport.
126+
// The caller must call .Close() on the returned ImageCloser.
127127
// NOTE: If any kind of signature verification should happen, build an UnparsedImage from the value returned by NewImageSource,
128128
// verify that UnparsedImage, and convert it into a real Image via image.FromUnparsedImage.
129-
func (ref dockerReference) NewImage(ctx *types.SystemContext) (types.Image, error) {
129+
// WARNING: This may not do the right thing for a manifest list, see image.FromSource for details.
130+
func (ref dockerReference) NewImage(ctx *types.SystemContext) (types.ImageCloser, error) {
130131
return newImage(ctx, ref)
131132
}
132133

‎image/docker_schema2_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -320,7 +320,7 @@ func (ref refImageReferenceMock) PolicyConfigurationIdentity() string {
320320
func (ref refImageReferenceMock) PolicyConfigurationNamespaces() []string {
321321
panic("unexpected call to a mock function")
322322
}
323-
func (ref refImageReferenceMock) NewImage(ctx *types.SystemContext) (types.Image, error) {
323+
func (ref refImageReferenceMock) NewImage(ctx *types.SystemContext) (types.ImageCloser, error) {
324324
panic("unexpected call to a mock function")
325325
}
326326
func (ref refImageReferenceMock) NewImageSource(ctx *types.SystemContext) (types.ImageSource, error) {

‎image/memory.go‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -33,11 +33,6 @@ func (i *memoryImage) Reference() types.ImageReference {
3333
return nil
3434
}
3535

36-
// Close removes resources associated with an initialized UnparsedImage, if any.
37-
func (i *memoryImage) Close() error {
38-
return nil
39-
}
40-
4136
// Size returns the size of the image as stored, if known, or -1 if not.
4237
func (i *memoryImage) Size() (int64, error) {
4338
return -1, nil

‎image/sourced.go‎

Lines changed: 26 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -7,11 +7,19 @@ import (
77
"github.com/containers/image/types"
88
)
99

10-
// FromSource returns a types.Image implementation for the default instance of source.
10+
// imageCloser implements types.ImageCloser, perhaps allowing simple users
11+
// to use a single object without having keep a reference to a types.ImageSource
12+
// only to call types.ImageSource.Close().
13+
type imageCloser struct {
14+
types.Image
15+
src types.ImageSource
16+
}
17+
18+
// FromSource returns a types.ImageCloser implementation for the default instance of source.
1119
// If source is a manifest list, .Manifest() still returns the manifest list,
1220
// but other methods transparently return data from an appropriate image instance.
1321
//
14-
// The caller must call .Close() on the returned Image.
22+
// The caller must call .Close() on the returned ImageCloser.
1523
//
1624
// FromSource “takes ownership” of the input ImageSource and will call src.Close()
1725
// when the image is closed. (This does not prevent callers from using both the
@@ -20,8 +28,19 @@ import (
2028
//
2129
// NOTE: If any kind of signature verification should happen, build an UnparsedImage from the value returned by NewImageSource,
2230
// verify that UnparsedImage, and convert it into a real Image via image.FromUnparsedImage instead of calling this function.
23-
func FromSource(src types.ImageSource) (types.Image, error) {
24-
return FromUnparsedImage(UnparsedInstance(src, nil))
31+
func FromSource(src types.ImageSource) (types.ImageCloser, error) {
32+
img, err := FromUnparsedImage(UnparsedInstance(src, nil))
33+
if err != nil {
34+
return nil, err
35+
}
36+
return &imageCloser{
37+
Image: img,
38+
src: src,
39+
}, nil
40+
}
41+
42+
func (ic *imageCloser) Close() error {
43+
return ic.src.Close()
2544
}
2645

2746
// sourcedImage is a general set of utilities for working with container images,
@@ -40,20 +59,15 @@ type sourcedImage struct {
4059
}
4160

4261
// FromUnparsedImage returns a types.Image implementation for unparsed.
43-
// The caller must call .Close() on the returned Image.
62+
// If unparsed represents a manifest list, .Manifest() still returns the manifest list,
63+
// but other methods transparently return data from an appropriate single image.
4464
//
45-
// FromSource “takes ownership” of the input UnparsedImage and will call uparsed.Close()
46-
// when the image is closed. (This does not prevent callers from using both the
47-
// UnparsedImage and ImageSource objects simultaneously, but it means that they only need to
48-
// keep a reference to the Image.)
65+
// The Image must not be used after the underlying ImageSource is Close()d.
4966
func FromUnparsedImage(unparsed *UnparsedImage) (types.Image, error) {
5067
// Note that the input parameter above is specifically *image.UnparsedImage, not types.UnparsedImage:
5168
// we want to be able to use unparsed.src. We could make that an explicit interface, but, well,
5269
// this is the only UnparsedImage implementation around, anyway.
5370

54-
// Also, we do not explicitly implement types.Image.Close; we let the implementation fall through to
55-
// unparsed.Close.
56-
5771
// NOTE: It is essential for signature verification that all parsing done in this object happens on the same manifest which is returned by unparsed.Manifest().
5872
manifestBlob, manifestMIMEType, err := unparsed.Manifest()
5973
if err != nil {

0 commit comments

Comments
 (0)