From d4a8d09d1a7ced5c711fcc7a939986d22a0554eb Mon Sep 17 00:00:00 2001 From: Alexander Morozov Date: Tue, 3 Nov 2015 17:19:18 -0800 Subject: [PATCH] Do not rely on string comparison in truncindex Signed-off-by: Alexander Morozov --- api/server/router/local/image.go | 2 +- daemon/create.go | 16 +--------------- daemon/daemon.go | 2 +- daemon/errors.go | 23 +++++++++++++++++++++++ daemon/image_delete.go | 2 +- graph/graph.go | 5 ++++- integration-cli/docker_cli_rmi_test.go | 4 ++-- pkg/truncindex/truncindex.go | 5 ++++- 8 files changed, 37 insertions(+), 22 deletions(-) create mode 100644 daemon/errors.go diff --git a/api/server/router/local/image.go b/api/server/router/local/image.go index a0e14bb964..fd78ee2cda 100644 --- a/api/server/router/local/image.go +++ b/api/server/router/local/image.go @@ -242,7 +242,7 @@ func (s *router) deleteImages(ctx context.Context, w http.ResponseWriter, r *htt name := vars["name"] - if name == "" { + if strings.TrimSpace(name) == "" { return fmt.Errorf("image name cannot be blank") } diff --git a/daemon/create.go b/daemon/create.go index d9aa3b29a9..77517812f0 100644 --- a/daemon/create.go +++ b/daemon/create.go @@ -1,14 +1,10 @@ package daemon import ( - "strings" - "github.com/Sirupsen/logrus" "github.com/docker/docker/api/types" derr "github.com/docker/docker/errors" - "github.com/docker/docker/graph/tags" "github.com/docker/docker/image" - "github.com/docker/docker/pkg/parsers" "github.com/docker/docker/pkg/stringid" "github.com/docker/docker/runconfig" "github.com/docker/docker/volume" @@ -38,17 +34,7 @@ func (daemon *Daemon) ContainerCreate(params *ContainerCreateConfig) (types.Cont container, err := daemon.create(params) if err != nil { - if daemon.Graph().IsNotExist(err, params.Config.Image) { - if strings.Contains(params.Config.Image, "@") { - return types.ContainerCreateResponse{ID: "", Warnings: warnings}, derr.ErrorCodeNoSuchImageHash.WithArgs(params.Config.Image) - } - img, tag := parsers.ParseRepositoryTag(params.Config.Image) - if tag == "" { - tag = tags.DefaultTag - } - return types.ContainerCreateResponse{ID: "", Warnings: warnings}, derr.ErrorCodeNoSuchImageTag.WithArgs(img, tag) - } - return types.ContainerCreateResponse{ID: "", Warnings: warnings}, err + return types.ContainerCreateResponse{ID: "", Warnings: warnings}, daemon.graphNotExistToErrcode(params.Config.Image, err) } return types.ContainerCreateResponse{ID: container.ID, Warnings: warnings}, nil diff --git a/daemon/daemon.go b/daemon/daemon.go index dec9236884..456dfa8399 100644 --- a/daemon/daemon.go +++ b/daemon/daemon.go @@ -149,7 +149,7 @@ func (daemon *Daemon) Get(prefixOrName string) (*Container, error) { containerID, indexError := daemon.idIndex.Get(prefixOrName) if indexError != nil { // When truncindex defines an error type, use that instead - if strings.Contains(indexError.Error(), "no such id") { + if indexError == truncindex.ErrNotExist { return nil, derr.ErrorCodeNoSuchContainer.WithArgs(prefixOrName) } return nil, indexError diff --git a/daemon/errors.go b/daemon/errors.go new file mode 100644 index 0000000000..45a4882085 --- /dev/null +++ b/daemon/errors.go @@ -0,0 +1,23 @@ +package daemon + +import ( + "strings" + + derr "github.com/docker/docker/errors" + "github.com/docker/docker/graph/tags" + "github.com/docker/docker/pkg/parsers" +) + +func (d *Daemon) graphNotExistToErrcode(imageName string, err error) error { + if d.Graph().IsNotExist(err, imageName) { + if strings.Contains(imageName, "@") { + return derr.ErrorCodeNoSuchImageHash.WithArgs(imageName) + } + img, tag := parsers.ParseRepositoryTag(imageName) + if tag == "" { + tag = tags.DefaultTag + } + return derr.ErrorCodeNoSuchImageTag.WithArgs(img, tag) + } + return err +} diff --git a/daemon/image_delete.go b/daemon/image_delete.go index 3bcf023b42..ca8b82a1f3 100644 --- a/daemon/image_delete.go +++ b/daemon/image_delete.go @@ -55,7 +55,7 @@ func (daemon *Daemon) ImageDelete(imageRef string, force, prune bool) ([]types.I img, err := daemon.repositories.LookupImage(imageRef) if err != nil { - return nil, err + return nil, daemon.graphNotExistToErrcode(imageRef, err) } var removedRepositoryRef bool diff --git a/graph/graph.go b/graph/graph.go index d6a600c5f1..9072639733 100644 --- a/graph/graph.go +++ b/graph/graph.go @@ -221,7 +221,10 @@ func (graph *Graph) Exists(id string) bool { func (graph *Graph) Get(name string) (*image.Image, error) { id, err := graph.idIndex.Get(name) if err != nil { - return nil, fmt.Errorf("could not find image: %v", err) + if err == truncindex.ErrNotExist { + return nil, fmt.Errorf("image %s does not exist", name) + } + return nil, err } img, err := graph.loadImage(id) if err != nil { diff --git a/integration-cli/docker_cli_rmi_test.go b/integration-cli/docker_cli_rmi_test.go index 32472ea723..89bb9db10a 100644 --- a/integration-cli/docker_cli_rmi_test.go +++ b/integration-cli/docker_cli_rmi_test.go @@ -230,10 +230,10 @@ func (s *DockerSuite) TestRmiBlank(c *check.C) { c.Assert(out, checker.Contains, "image name cannot be blank", check.Commentf("out: %s", out)) out, _, err = dockerCmdWithError("rmi", " ") - // Should have failed to delete '' image + // Should have failed to delete ' ' image c.Assert(err, checker.NotNil) // Expected error message not generated - c.Assert(out, checker.Contains, "no such id", check.Commentf("out: %s", out)) + c.Assert(out, checker.Contains, "image name cannot be blank", check.Commentf("out: %s", out)) } func (s *DockerSuite) TestRmiContainerImageNotFound(c *check.C) { diff --git a/pkg/truncindex/truncindex.go b/pkg/truncindex/truncindex.go index 9c08c90d79..3037e97cab 100644 --- a/pkg/truncindex/truncindex.go +++ b/pkg/truncindex/truncindex.go @@ -22,6 +22,9 @@ var ( // ErrIllegalChar is returned when a space is in the ID ErrIllegalChar = errors.New("illegal character: ' '") + + // ErrNotExist is returned when ID or its prefix not found in index. + ErrNotExist = errors.New("ID does not exist") ) // TruncIndex allows the retrieval of string identifiers by any of their unique prefixes. @@ -116,7 +119,7 @@ func (idx *TruncIndex) Get(s string) (string, error) { if id != "" { return id, nil } - return "", fmt.Errorf("no such id: %s", s) + return "", ErrNotExist } // Iterate iterates over all stored IDs, and passes each of them to the given handler.