From dc79403f7b38b9b98451df87ece02767eb4682d0 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 28 Dec 2023 12:51:42 +0100 Subject: [PATCH 1/9] daemon/cluster: remove/rename err-returns and remove naked returns Prevent accidentally shadowing these errors, which are used in defers. Signed-off-by: Sebastiaan van Stijn --- daemon/cluster/controllers/plugin/controller.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/daemon/cluster/controllers/plugin/controller.go b/daemon/cluster/controllers/plugin/controller.go index 6168afc578..6f1a2e9558 100644 --- a/daemon/cluster/controllers/plugin/controller.go +++ b/daemon/cluster/controllers/plugin/controller.go @@ -87,7 +87,7 @@ func (p *Controller) Update(ctx context.Context, t *api.Task) error { } // Prepare is the prepare phase from swarmkit -func (p *Controller) Prepare(ctx context.Context) (err error) { +func (p *Controller) Prepare(ctx context.Context) (retErr error) { p.logger.Debug("Prepare") remote, err := reference.ParseNormalizedNamed(p.spec.Remote) @@ -105,7 +105,7 @@ func (p *Controller) Prepare(ctx context.Context) (err error) { pl, err := p.backend.Get(p.spec.Name) defer func() { - if pl != nil && err == nil { + if pl != nil && retErr == nil { pl.Acquire() } }() From 088c180a9eee3f9c074f6879968d0fe940a92796 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 28 Dec 2023 13:07:59 +0100 Subject: [PATCH 2/9] daemon/containerd: remove named err-returns Signed-off-by: Sebastiaan van Stijn --- daemon/containerd/image_delete.go | 6 +++--- daemon/containerd/image_list.go | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/daemon/containerd/image_delete.go b/daemon/containerd/image_delete.go index e952c50586..3ed0b768de 100644 --- a/daemon/containerd/image_delete.go +++ b/daemon/containerd/image_delete.go @@ -203,7 +203,7 @@ func (i *ImageService) ImageDelete(ctx context.Context, imageRef string, force, // also deletes dangling parents if there is no conflict in doing so. // Parent images are removed quietly, and if there is any issue/conflict // it is logged but does not halt execution/an error is not returned. -func (i *ImageService) deleteAll(ctx context.Context, imgID image.ID, all []c8dimages.Image, c conflictType, prune bool) (records []imagetypes.DeleteResponse, err error) { +func (i *ImageService) deleteAll(ctx context.Context, imgID image.ID, all []c8dimages.Image, c conflictType, prune bool) (records []imagetypes.DeleteResponse, _ error) { // Workaround for: https://github.com/moby/buildkit/issues/3797 possiblyDeletedConfigs := map[digest.Digest]struct{}{} if len(all) > 0 && i.content != nil { @@ -236,6 +236,7 @@ func (i *ImageService) deleteAll(ctx context.Context, imgID image.ID, all []c8di var parents []c8dimages.Image if prune { // TODO(dmcgowan): Consider using GC labels to walk for deletion + var err error parents, err = i.parents(ctx, imgID) if err != nil { log.G(ctx).WithError(err).Warn("failed to get image parents") @@ -254,8 +255,7 @@ func (i *ImageService) deleteAll(ctx context.Context, imgID image.ID, all []c8di if !isDanglingImage(parent) { break } - err = i.imageDeleteHelper(ctx, parent, all, &records, conflictSoft) - if err != nil { + if err := i.imageDeleteHelper(ctx, parent, all, &records, conflictSoft); err != nil { log.G(ctx).WithError(err).Warn("failed to remove image parent") break } diff --git a/daemon/containerd/image_list.go b/daemon/containerd/image_list.go index 6701694a6f..bf426591e2 100644 --- a/daemon/containerd/image_list.go +++ b/daemon/containerd/image_list.go @@ -654,7 +654,7 @@ func setupLabelFilter(ctx context.Context, store content.Store, fltrs filters.Ar // It will be returned once a matching config is found. errFoundConfig := errors.New("success, found matching config") - err := c8dimages.Dispatch(ctx, presentChildrenHandler(store, c8dimages.HandlerFunc(func(ctx context.Context, desc ocispec.Descriptor) (subdescs []ocispec.Descriptor, err error) { + err := c8dimages.Dispatch(ctx, presentChildrenHandler(store, c8dimages.HandlerFunc(func(ctx context.Context, desc ocispec.Descriptor) (subdescs []ocispec.Descriptor, _ error) { if !c8dimages.IsConfigType(desc.MediaType) { return nil, nil } From 48220008d89c267a918728be0838847a9f67e586 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 28 Dec 2023 12:25:57 +0100 Subject: [PATCH 3/9] daemon/graphdriver: remove/rename err-returns and remove naked returns Prevent accidentally shadowing these errors, which are used in defers, and while at it, also fixed some linting warnings about unhandled errors. Signed-off-by: Sebastiaan van Stijn --- daemon/graphdriver/fsdiff.go | 8 +-- .../fuse-overlayfs/fuseoverlayfs.go | 4 +- daemon/graphdriver/overlay2/overlay.go | 4 +- daemon/graphdriver/windows/windows.go | 53 ++++++++++--------- 4 files changed, 35 insertions(+), 34 deletions(-) diff --git a/daemon/graphdriver/fsdiff.go b/daemon/graphdriver/fsdiff.go index 1bfe91ffbe..32d7175845 100644 --- a/daemon/graphdriver/fsdiff.go +++ b/daemon/graphdriver/fsdiff.go @@ -47,7 +47,7 @@ func NewNaiveDiffDriver(driver ProtoDriver, idMap user.IdentityMapping) Driver { // Diff produces an archive of the changes between the specified // layer and its parent layer which may be "". -func (gdw *NaiveDiffDriver) Diff(id, parent string) (arch io.ReadCloser, err error) { +func (gdw *NaiveDiffDriver) Diff(id, parent string) (arch io.ReadCloser, retErr error) { startTime := time.Now() driver := gdw.ProtoDriver @@ -58,8 +58,8 @@ func (gdw *NaiveDiffDriver) Diff(id, parent string) (arch io.ReadCloser, err err layerFs := layerRootFs defer func() { - if err != nil { - driver.Put(id) + if retErr != nil { + _ = driver.Put(id) } }() @@ -131,7 +131,7 @@ func (gdw *NaiveDiffDriver) Changes(id, parent string) ([]archive.Change, error) // ApplyDiff extracts the changeset from the given diff into the // layer with the specified id and parent, returning the size of the // new layer in bytes. -func (gdw *NaiveDiffDriver) ApplyDiff(id, parent string, diff io.Reader) (size int64, err error) { +func (gdw *NaiveDiffDriver) ApplyDiff(id, parent string, diff io.Reader) (size int64, _ error) { driver := gdw.ProtoDriver // Mount the root filesystem so we can apply the diff/layer. diff --git a/daemon/graphdriver/fuse-overlayfs/fuseoverlayfs.go b/daemon/graphdriver/fuse-overlayfs/fuseoverlayfs.go index 13596d64f9..cbfd63af7e 100644 --- a/daemon/graphdriver/fuse-overlayfs/fuseoverlayfs.go +++ b/daemon/graphdriver/fuse-overlayfs/fuseoverlayfs.go @@ -450,7 +450,7 @@ func (d *Driver) isParent(id, parent string) bool { } // ApplyDiff applies the new layer into a root -func (d *Driver) ApplyDiff(id string, parent string, diff io.Reader) (size int64, err error) { +func (d *Driver) ApplyDiff(id string, parent string, diff io.Reader) (size int64, _ error) { if !d.isParent(id, parent) { return d.naiveDiff.ApplyDiff(id, parent, diff) } @@ -480,7 +480,7 @@ func (d *Driver) getDiffPath(id string) string { // DiffSize calculates the changes between the specified id // and its parent and returns the size in bytes of the changes // relative to its base filesystem directory. -func (d *Driver) DiffSize(id, parent string) (size int64, err error) { +func (d *Driver) DiffSize(id, parent string) (int64, error) { return d.naiveDiff.DiffSize(id, parent) } diff --git a/daemon/graphdriver/overlay2/overlay.go b/daemon/graphdriver/overlay2/overlay.go index 6c496cfdd9..bdfee98c2c 100644 --- a/daemon/graphdriver/overlay2/overlay.go +++ b/daemon/graphdriver/overlay2/overlay.go @@ -674,7 +674,7 @@ func (d *Driver) isParent(id, parent string) bool { } // ApplyDiff applies the new layer into a root -func (d *Driver) ApplyDiff(id string, parent string, diff io.Reader) (size int64, err error) { +func (d *Driver) ApplyDiff(id string, parent string, diff io.Reader) (size int64, _ error) { if useNaiveDiff(d.home) || !d.isParent(id, parent) { return d.naiveDiff.ApplyDiff(id, parent, diff) } @@ -703,7 +703,7 @@ func (d *Driver) getDiffPath(id string) string { // DiffSize calculates the changes between the specified id // and its parent and returns the size in bytes of the changes // relative to its base filesystem directory. -func (d *Driver) DiffSize(id, parent string) (size int64, err error) { +func (d *Driver) DiffSize(id, parent string) (int64, error) { if useNaiveDiff(d.home) || !d.isParent(id, parent) { return d.naiveDiff.DiffSize(id, parent) } diff --git a/daemon/graphdriver/windows/windows.go b/daemon/graphdriver/windows/windows.go index e4b2962d30..2056038fa4 100644 --- a/daemon/graphdriver/windows/windows.go +++ b/daemon/graphdriver/windows/windows.go @@ -479,15 +479,15 @@ func (d *Driver) Cleanup() error { // Diff produces an archive of the changes between the specified // layer and its parent layer which may be "". // The layer should be mounted when calling this function -func (d *Driver) Diff(id, _ string) (_ io.ReadCloser, err error) { +func (d *Driver) Diff(id, _ string) (io.ReadCloser, error) { rID, err := d.resolveID(id) if err != nil { - return + return nil, err } layerChain, err := d.getLayerChain(rID) if err != nil { - return + return nil, err } // this is assuming that the layer is unmounted @@ -503,7 +503,7 @@ func (d *Driver) Diff(id, _ string) (_ io.ReadCloser, err error) { arch, err := d.exportLayer(rID, layerChain) if err != nil { prepare() - return + return nil, err } return ioutils.NewReadCloserWrapper(arch, func() error { err := arch.Close() @@ -604,20 +604,20 @@ func (d *Driver) ApplyDiff(id, parent string, diff io.Reader) (int64, error) { // DiffSize calculates the changes between the specified layer // and its parent and returns the size in bytes of the changes // relative to its base filesystem directory. -func (d *Driver) DiffSize(id, parent string) (size int64, err error) { +func (d *Driver) DiffSize(id, parent string) (int64, error) { rPId, err := d.resolveID(parent) if err != nil { - return + return 0, err } changes, err := d.Changes(id, rPId) if err != nil { - return + return 0, err } layerFs, err := d.Get(id, "") if err != nil { - return + return 0, err } defer d.Put(id) @@ -683,26 +683,26 @@ func (d *Driver) exportLayer(id string, parentLayerPaths []string) (io.ReadClose // writeBackupStreamFromTarAndSaveMutatedFiles reads data from a tar stream and // writes it to a backup stream, and also saves any files that will be mutated // by the import layer process to a backup location. -func writeBackupStreamFromTarAndSaveMutatedFiles(buf *bufio.Writer, w io.Writer, t *tar.Reader, hdr *tar.Header, root string) (nextHdr *tar.Header, err error) { - var bcdBackup *os.File - var bcdBackupWriter *winio.BackupFileWriter +func writeBackupStreamFromTarAndSaveMutatedFiles(buf *bufio.Writer, w io.Writer, t *tar.Reader, hdr *tar.Header, root string) (nextHdr *tar.Header, retErr error) { if backupPath, ok := mutatedFiles[hdr.Name]; ok { - bcdBackup, err = os.Create(filepath.Join(root, backupPath)) + bcdBackup, err := os.Create(filepath.Join(root, backupPath)) if err != nil { return nil, err } defer func() { - cerr := bcdBackup.Close() - if err == nil { - err = cerr + if err := bcdBackup.Close(); err != nil { + if retErr == nil { + retErr = err + } } }() - bcdBackupWriter = winio.NewBackupFileWriter(bcdBackup, false) + bcdBackupWriter := winio.NewBackupFileWriter(bcdBackup, false) defer func() { - cerr := bcdBackupWriter.Close() - if err == nil { - err = cerr + if err := bcdBackupWriter.Close(); err != nil { + if retErr == nil { + retErr = err + } } }() @@ -712,9 +712,10 @@ func writeBackupStreamFromTarAndSaveMutatedFiles(buf *bufio.Writer, w io.Writer, } defer func() { - ferr := buf.Flush() - if err == nil { - err = ferr + if err := buf.Flush(); err != nil { + if retErr == nil { + retErr = err + } } }() @@ -766,7 +767,7 @@ func writeLayerFromTar(r io.Reader, w hcsshim.LayerWriter, root string) (int64, } // importLayer adds a new layer to the tag and graph store based on the given data. -func (d *Driver) importLayer(id string, layerData io.Reader, parentLayerPaths []string) (size int64, err error) { +func (d *Driver) importLayer(id string, layerData io.Reader, parentLayerPaths []string) (size int64, _ error) { if !noreexec { cmd := reexec.Command(append([]string{"docker-windows-write-layer", d.info.HomeDir, id}, parentLayerPaths...)...) output := bytes.NewBuffer(nil) @@ -774,11 +775,11 @@ func (d *Driver) importLayer(id string, layerData io.Reader, parentLayerPaths [] cmd.Stdout = output cmd.Stderr = output - if err = cmd.Start(); err != nil { - return + if err := cmd.Start(); err != nil { + return 0, err } - if err = cmd.Wait(); err != nil { + if err := cmd.Wait(); err != nil { return 0, fmt.Errorf("re-exec error: %v: output: %s", err, output) } From ba15bbc422a4f63d100240c6438c753379ea71e7 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 28 Dec 2023 13:10:22 +0100 Subject: [PATCH 4/9] daemon/images: rename err-returns to prevent shadowing Prevent accidentally shadowing the error, which is used in a defer, and while at it, also fixed some linting warnings about unhandled errors. Signed-off-by: Sebastiaan van Stijn --- daemon/images/image_commit.go | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/daemon/images/image_commit.go b/daemon/images/image_commit.go index 27b7ea3fc0..1ac091ae96 100644 --- a/daemon/images/image_commit.go +++ b/daemon/images/image_commit.go @@ -78,14 +78,14 @@ func (i *ImageService) CommitImage(ctx context.Context, c backend.CommitConfig) return id, nil } -func exportContainerRw(layerStore layer.Store, id, mountLabel string) (arch io.ReadCloser, err error) { +func exportContainerRw(layerStore layer.Store, id, mountLabel string) (arch io.ReadCloser, retErr error) { rwlayer, err := layerStore.GetRWLayer(id) if err != nil { return nil, err } defer func() { - if err != nil { - layerStore.ReleaseRWLayer(rwlayer) + if retErr != nil { + _, _ = layerStore.ReleaseRWLayer(rwlayer) } }() @@ -93,23 +93,21 @@ func exportContainerRw(layerStore layer.Store, id, mountLabel string) (arch io.R // mount the layer if needed. But the Diff() function for windows requests that // the layer should be mounted when calling it. So we reserve this mount call // until windows driver can implement Diff() interface correctly. - _, err = rwlayer.Mount(mountLabel) - if err != nil { + if _, err := rwlayer.Mount(mountLabel); err != nil { return nil, err } archive, err := rwlayer.TarStream() if err != nil { - rwlayer.Unmount() + _ = rwlayer.Unmount() return nil, err } return ioutils.NewReadCloserWrapper(archive, func() error { - archive.Close() - err = rwlayer.Unmount() - layerStore.ReleaseRWLayer(rwlayer) - return err - }), - nil + _ = archive.Close() + err := rwlayer.Unmount() + _, _ = layerStore.ReleaseRWLayer(rwlayer) + return err + }), nil } // CommitBuildStep is used by the builder to create an image for each step in From 190ad0610d0637a656e39b786c855d38df38901b Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 28 Dec 2023 12:07:17 +0100 Subject: [PATCH 5/9] daemon/logger: remove/rename err-returns and linting warnings Prevent accidentally shadowing these errors, which are used in defers, and while at it, also fixed some linting warnings about unhandled errors, and defers created in a loop. Signed-off-by: Sebastiaan van Stijn --- daemon/logger/adapter_test.go | 2 +- daemon/logger/jsonfilelog/read.go | 2 +- daemon/logger/loggerutils/logfile.go | 6 +++--- daemon/logger/loggerutils/logfile_race_test.go | 6 +++--- daemon/logger/loggerutils/sharedtemp.go | 2 +- daemon/logger/plugin.go | 12 ++++++------ daemon/logger/proxy.go | 7 +++---- 7 files changed, 18 insertions(+), 19 deletions(-) diff --git a/daemon/logger/adapter_test.go b/daemon/logger/adapter_test.go index 5c89f350d8..00523d205d 100644 --- a/daemon/logger/adapter_test.go +++ b/daemon/logger/adapter_test.go @@ -69,7 +69,7 @@ func (l *mockLoggingPlugin) StopLogging(file string) error { return nil } -func (l *mockLoggingPlugin) Capabilities() (cap Capability, err error) { +func (l *mockLoggingPlugin) Capabilities() (Capability, error) { return Capability{ReadLogs: true}, nil } diff --git a/daemon/logger/jsonfilelog/read.go b/daemon/logger/jsonfilelog/read.go index 6627074fe2..ff235e6d3d 100644 --- a/daemon/logger/jsonfilelog/read.go +++ b/daemon/logger/jsonfilelog/read.go @@ -62,7 +62,7 @@ func (d *decoder) Close() { d.jl = nil } -func (d *decoder) Decode() (msg *logger.Message, err error) { +func (d *decoder) Decode() (*logger.Message, error) { if d.dec == nil { d.dec = json.NewDecoder(d.rdr) } diff --git a/daemon/logger/loggerutils/logfile.go b/daemon/logger/loggerutils/logfile.go index d6f3f38377..886617c3a1 100644 --- a/daemon/logger/loggerutils/logfile.go +++ b/daemon/logger/loggerutils/logfile.go @@ -121,12 +121,12 @@ type GetTailReaderFunc func(ctx context.Context, f SizeReaderAt, nLogLines int) // NewLogFile creates new LogFile func NewLogFile(logPath string, capacity int64, maxFiles int, compress bool, decodeFunc MakeDecoderFn, perms os.FileMode, getTailReader GetTailReaderFunc) (*LogFile, error) { - log, err := openFile(logPath, os.O_WRONLY|os.O_APPEND|os.O_CREATE, perms) + logFile, err := openFile(logPath, os.O_WRONLY|os.O_APPEND|os.O_CREATE, perms) if err != nil { return nil, err } - size, err := log.Seek(0, io.SeekEnd) + size, err := logFile.Seek(0, io.SeekEnd) if err != nil { return nil, err } @@ -141,7 +141,7 @@ func NewLogFile(logPath string, capacity int64, maxFiles int, compress bool, dec st <- logReadState{pos: pos} return &LogFile{ - f: log, + f: logFile, read: st, pos: pos, closed: make(chan struct{}), diff --git a/daemon/logger/loggerutils/logfile_race_test.go b/daemon/logger/loggerutils/logfile_race_test.go index 36fbb112cc..3cffeb7be8 100644 --- a/daemon/logger/loggerutils/logfile_race_test.go +++ b/daemon/logger/loggerutils/logfile_race_test.go @@ -43,14 +43,14 @@ func TestConcurrentLogging(t *testing.T) { for ct := 0; ct < containers; ct++ { ct := ct dir := t.TempDir() - g.Go(func() (err error) { + g.Go(func() (retErr error) { logfile, err := NewLogFile(filepath.Join(dir, "log.log"), capacity, maxFiles, compress, createDecoder, 0o644, getTailReader) if err != nil { return err } defer func() { - if cErr := logfile.Close(); cErr != nil && err == nil { - err = cErr + if err := logfile.Close(); err != nil && retErr == nil { + retErr = err } }() lg, ctx := errgroup.WithContext(ctx) diff --git a/daemon/logger/loggerutils/sharedtemp.go b/daemon/logger/loggerutils/sharedtemp.go index f5c8c1f035..fdf07115b6 100644 --- a/daemon/logger/loggerutils/sharedtemp.go +++ b/daemon/logger/loggerutils/sharedtemp.go @@ -140,7 +140,7 @@ func (c *sharedTempFileConverter) openExisting(st stfcState, id stfID, v sharedT return res.fr, res.err } -func (c *sharedTempFileConverter) convert(f *os.File) (converted *os.File, size int64, err error) { +func (c *sharedTempFileConverter) convert(f *os.File) (converted *os.File, size int64, _ error) { dst, err := os.CreateTemp(c.TempDir, "dockerdtemp.*") if err != nil { return nil, 0, err diff --git a/daemon/logger/plugin.go b/daemon/logger/plugin.go index 2916fbe55a..7e06fef52d 100644 --- a/daemon/logger/plugin.go +++ b/daemon/logger/plugin.go @@ -68,10 +68,10 @@ func makePluginClient(p plugingetter.CompatPlugin) (logPlugin, error) { } func makePluginCreator(name string, l logPlugin, scopePath func(s string) string) Creator { - return func(logCtx Info) (logger Logger, err error) { + return func(logCtx Info) (logger Logger, retErr error) { defer func() { - if err != nil { - pluginGetter.Get(name, extName, plugingetter.Release) + if retErr != nil { + _, _ = pluginGetter.Get(name, extName, plugingetter.Release) } }() @@ -90,9 +90,9 @@ func makePluginCreator(name string, l logPlugin, scopePath func(s string) string logInfo: logCtx, } - cap, err := a.plugin.Capabilities() + caps, err := a.plugin.Capabilities() if err == nil { - a.capabilities = cap + a.capabilities = caps } stream, err := openPluginStream(a) @@ -107,7 +107,7 @@ func makePluginCreator(name string, l logPlugin, scopePath func(s string) string return nil, errors.Wrapf(err, "error creating logger") } - if cap.ReadLogs { + if caps.ReadLogs { return &pluginAdapterWithRead{a}, nil } diff --git a/daemon/logger/proxy.go b/daemon/logger/proxy.go index 502271b240..5173c09758 100644 --- a/daemon/logger/proxy.go +++ b/daemon/logger/proxy.go @@ -73,10 +73,9 @@ type logPluginProxyCapabilitiesResponse struct { Err string } -func (pp *logPluginProxy) Capabilities() (cap Capability, err error) { +func (pp *logPluginProxy) Capabilities() (Capability, error) { var ret logPluginProxyCapabilitiesResponse - - if err = pp.Call("LogDriver.Capabilities", nil, &ret); err != nil { + if err := pp.Call("LogDriver.Capabilities", nil, &ret); err != nil { return Capability{}, err } @@ -92,7 +91,7 @@ type logPluginProxyReadLogsRequest struct { Config ReadConfig } -func (pp *logPluginProxy) ReadLogs(info Info, config ReadConfig) (stream io.ReadCloser, err error) { +func (pp *logPluginProxy) ReadLogs(info Info, config ReadConfig) (stream io.ReadCloser, _ error) { return pp.Stream("LogDriver.ReadLogs", logPluginProxyReadLogsRequest{ Info: info, Config: config, From 2145cf63091f4ec1ebc7a3b26e10b8be35678c77 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 28 Dec 2023 13:19:57 +0100 Subject: [PATCH 6/9] daemon: Daemon.ContainerStatPath, ContainerArchivePath: minor refactor - remove named error-returns - make error-handling slightly more idiomatic (check for non-nil errors) Signed-off-by: Sebastiaan van Stijn --- daemon/archive.go | 6 +++--- daemon/archive_unix.go | 10 +++++----- daemon/archive_windows.go | 22 +++++++++++----------- 3 files changed, 19 insertions(+), 19 deletions(-) diff --git a/daemon/archive.go b/daemon/archive.go index 5de94339c4..45a0c1e641 100644 --- a/daemon/archive.go +++ b/daemon/archive.go @@ -10,13 +10,13 @@ import ( // ContainerStatPath stats the filesystem resource at the specified path in the // container identified by the given name. -func (daemon *Daemon) ContainerStatPath(name string, path string) (stat *container.PathStat, err error) { +func (daemon *Daemon) ContainerStatPath(name string, path string) (*container.PathStat, error) { ctr, err := daemon.GetContainer(name) if err != nil { return nil, err } - stat, err = daemon.containerStatPath(ctr, path) + stat, err := daemon.containerStatPath(ctr, path) if err != nil { if os.IsNotExist(err) { return nil, containerFileNotFound{path, name} @@ -33,7 +33,7 @@ func (daemon *Daemon) ContainerStatPath(name string, path string) (stat *contain // ContainerArchivePath creates an archive of the filesystem resource at the // specified path in the container identified by the given name. Returns a // tar archive of the resource and whether it was a directory or a single file. -func (daemon *Daemon) ContainerArchivePath(name string, path string) (content io.ReadCloser, stat *container.PathStat, err error) { +func (daemon *Daemon) ContainerArchivePath(name string, path string) (content io.ReadCloser, stat *container.PathStat, _ error) { ctr, err := daemon.GetContainer(name) if err != nil { return nil, nil, err diff --git a/daemon/archive_unix.go b/daemon/archive_unix.go index 8f7db9a3b0..a32caf3074 100644 --- a/daemon/archive_unix.go +++ b/daemon/archive_unix.go @@ -20,7 +20,7 @@ import ( // containerStatPath stats the filesystem resource at the specified path in this // container. Returns stat info about the resource. -func (daemon *Daemon) containerStatPath(container *container.Container, path string) (stat *containertypes.PathStat, err error) { +func (daemon *Daemon) containerStatPath(container *container.Container, path string) (*containertypes.PathStat, error) { container.Lock() defer container.Unlock() @@ -36,11 +36,11 @@ func (daemon *Daemon) containerStatPath(container *container.Container, path str // containerArchivePath creates an archive of the filesystem resource at the specified // path in this container. Returns a tar archive of the resource and stat info // about the resource. -func (daemon *Daemon) containerArchivePath(container *container.Container, path string) (content io.ReadCloser, stat *containertypes.PathStat, err error) { +func (daemon *Daemon) containerArchivePath(container *container.Container, path string) (content io.ReadCloser, stat *containertypes.PathStat, retErr error) { container.Lock() defer func() { - if err != nil { + if retErr != nil { // Wait to unlock the container until the archive is fully read // (see the ReadCloseWrapper func below) or if there is an error // before that occurs. @@ -54,8 +54,8 @@ func (daemon *Daemon) containerArchivePath(container *container.Container, path } defer func() { - if err != nil { - cfs.Close() + if retErr != nil { + _ = cfs.Close() } }() diff --git a/daemon/archive_windows.go b/daemon/archive_windows.go index 2e1ff5d5f5..ed37a9d956 100644 --- a/daemon/archive_windows.go +++ b/daemon/archive_windows.go @@ -17,7 +17,7 @@ import ( // containerStatPath stats the filesystem resource at the specified path in this // container. Returns stat info about the resource. -func (daemon *Daemon) containerStatPath(container *container.Container, path string) (stat *containertypes.PathStat, err error) { +func (daemon *Daemon) containerStatPath(container *container.Container, path string) (*containertypes.PathStat, error) { container.Lock() defer container.Unlock() @@ -26,12 +26,12 @@ func (daemon *Daemon) containerStatPath(container *container.Container, path str return nil, err } - if err = daemon.Mount(container); err != nil { + if err := daemon.Mount(container); err != nil { return nil, err } defer daemon.Unmount(container) - err = daemon.mountVolumes(container) + err := daemon.mountVolumes(container) defer container.DetachAndUnmount(daemon.LogVolumeEvent) if err != nil { return nil, err @@ -51,11 +51,11 @@ func (daemon *Daemon) containerStatPath(container *container.Container, path str // containerArchivePath creates an archive of the filesystem resource at the specified // path in this container. Returns a tar archive of the resource and stat info // about the resource. -func (daemon *Daemon) containerArchivePath(container *container.Container, path string) (content io.ReadCloser, stat *containertypes.PathStat, err error) { +func (daemon *Daemon) containerArchivePath(container *container.Container, path string) (content io.ReadCloser, stat *containertypes.PathStat, retErr error) { container.Lock() defer func() { - if err != nil { + if retErr != nil { // Wait to unlock the container until the archive is fully read // (see the ReadCloseWrapper func below) or if there is an error // before that occurs. @@ -68,12 +68,12 @@ func (daemon *Daemon) containerArchivePath(container *container.Container, path return nil, nil, err } - if err = daemon.Mount(container); err != nil { + if err := daemon.Mount(container); err != nil { return nil, nil, err } defer func() { - if err != nil { + if retErr != nil { // unmount any volumes container.DetachAndUnmount(daemon.LogVolumeEvent) // unmount the container's rootfs @@ -81,7 +81,7 @@ func (daemon *Daemon) containerArchivePath(container *container.Container, path } }() - if err = daemon.mountVolumes(container); err != nil { + if err := daemon.mountVolumes(container); err != nil { return nil, nil, err } @@ -223,14 +223,14 @@ func (daemon *Daemon) containerExtractToDir(container *container.Container, path return nil } -func (daemon *Daemon) containerCopy(container *container.Container, resource string) (rc io.ReadCloser, err error) { +func (daemon *Daemon) containerCopy(container *container.Container, resource string) (_ io.ReadCloser, retErr error) { if resource[0] == '/' || resource[0] == '\\' { resource = resource[1:] } container.Lock() defer func() { - if err != nil { + if retErr != nil { // Wait to unlock the container until the archive is fully read // (see the ReadCloseWrapper func below) or if there is an error // before that occurs. @@ -248,7 +248,7 @@ func (daemon *Daemon) containerCopy(container *container.Container, resource str } defer func() { - if err != nil { + if retErr != nil { // unmount any volumes container.DetachAndUnmount(daemon.LogVolumeEvent) // unmount the container's rootfs From 3e586094fc51b71d9b2d8e59e1351e94cdb00bb6 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 28 Dec 2023 11:38:10 +0100 Subject: [PATCH 7/9] daemon: parseXXVersion: rewrite to be slightly more iodiomatic Signed-off-by: Sebastiaan van Stijn --- daemon/info_unix.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/daemon/info_unix.go b/daemon/info_unix.go index aa8e40afb2..49c85ccd42 100644 --- a/daemon/info_unix.go +++ b/daemon/info_unix.go @@ -339,7 +339,7 @@ func fillDriverWarnings(v *system.Info) { // Output example from `docker-init --version`: // // tini version 0.18.0 - git.fec3683 -func parseInitVersion(v string) (version string, commit string, err error) { +func parseInitVersion(v string) (version string, commit string, _ error) { parts := strings.Split(v, " - ") if len(parts) >= 2 { @@ -353,9 +353,9 @@ func parseInitVersion(v string) (version string, commit string, err error) { version = strings.TrimPrefix(parts[0], "tini version ") } if version == "" && commit == "" { - err = errors.Errorf("unknown output format: %s", v) + return "", "", errors.Errorf("unknown output format: %s", v) } - return version, commit, err + return version, commit, nil } // parseRuntimeVersion parses the output of `[runtime] --version` and extracts the @@ -366,7 +366,7 @@ func parseInitVersion(v string) (version string, commit string, err error) { // runc version 1.0.0-rc5+dev // commit: 69663f0bd4b60df09991c08812a60108003fa340 // spec: 1.0.0 -func parseRuntimeVersion(v string) (runtime, version, commit string, err error) { +func parseRuntimeVersion(v string) (runtime, version, commit string, _ error) { lines := strings.Split(strings.TrimSpace(v), "\n") for _, line := range lines { if strings.Contains(line, "version") { @@ -381,12 +381,12 @@ func parseRuntimeVersion(v string) (runtime, version, commit string, err error) } } if version == "" && commit == "" { - err = errors.Errorf("unknown output format: %s", v) + return runtime, "", "", errors.Errorf("unknown output format: %s", v) } - return runtime, version, commit, err + return runtime, version, commit, nil } -func parseDefaultRuntimeVersion(rts *runtimes) (runtime, version, commit string, err error) { +func parseDefaultRuntimeVersion(rts *runtimes) (runtime, version, commit string, _ error) { shim, opts, err := rts.Get(rts.Default) if err != nil { return "", "", "", err @@ -407,7 +407,7 @@ func parseDefaultRuntimeVersion(rts *runtimes) (runtime, version, commit string, if err != nil { return "", "", "", fmt.Errorf("failed to parse %s version: %w", rt, err) } - return runtime, version, commit, err + return runtime, version, commit, nil } func cgroupNamespacesEnabled(sysInfo *sysinfo.SysInfo, cfg *config.Config) bool { From 9ed975a247c7ba6b844d5cfaa7e6f653dd3b1c31 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 28 Dec 2023 11:40:52 +0100 Subject: [PATCH 8/9] daemon: NewDaemon: rename err-return Prevent accidentally shadowing the error, which is used in a defer. Signed-off-by: Sebastiaan van Stijn --- daemon/daemon.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/daemon/daemon.go b/daemon/daemon.go index 48797df673..cc47d98906 100644 --- a/daemon/daemon.go +++ b/daemon/daemon.go @@ -760,7 +760,7 @@ func (daemon *Daemon) IsSwarmCompatible() error { // NewDaemon sets up everything for the daemon to be able to service // requests from the webserver. -func NewDaemon(ctx context.Context, config *config.Config, pluginStore *plugin.Store, authzMiddleware *authorization.Middleware) (daemon *Daemon, err error) { +func NewDaemon(ctx context.Context, config *config.Config, pluginStore *plugin.Store, authzMiddleware *authorization.Middleware) (_ *Daemon, retErr error) { // Verify platform-specific requirements. // TODO(thaJeztah): this should be called before we try to create the daemon; perhaps together with the config validation. if err := checkSystem(); err != nil { @@ -841,7 +841,7 @@ func NewDaemon(ctx context.Context, config *config.Config, pluginStore *plugin.S // Ensure the daemon is properly shutdown if there is a failure during // initialization defer func() { - if err != nil { + if retErr != nil { // Use a fresh context here. Passed context could be cancelled. if err := d.Shutdown(context.Background()); err != nil { log.G(ctx).Error(err) From 19ccb75c628251f3a5885c6da6827fcc1970e873 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 28 Dec 2023 13:22:37 +0100 Subject: [PATCH 9/9] daemon: remove/rename err-returns and remove naked returns Prevent accidentally shadowing these errors, which are used in defers, and remove naked returns. Signed-off-by: Sebastiaan van Stijn --- daemon/container.go | 6 +++--- daemon/container_operations.go | 4 ++-- daemon/daemon_unix.go | 4 ++-- daemon/daemon_windows.go | 4 ++-- daemon/network.go | 5 +++-- daemon/oci_linux.go | 4 ++-- daemon/oci_windows.go | 5 +++-- daemon/oci_windows_test.go | 10 +++++----- daemon/stats_unix.go | 3 +-- 9 files changed, 23 insertions(+), 22 deletions(-) diff --git a/daemon/container.go b/daemon/container.go index b4852413c4..c4518d03dc 100644 --- a/daemon/container.go +++ b/daemon/container.go @@ -226,10 +226,10 @@ func (daemon *Daemon) setHostConfig(container *container.Container, hostConfig * // verifyContainerSettings performs validation of the hostconfig and config // structures. -func (daemon *Daemon) verifyContainerSettings(daemonCfg *configStore, hostConfig *containertypes.HostConfig, config *containertypes.Config, update bool) (warnings []string, err error) { +func (daemon *Daemon) verifyContainerSettings(daemonCfg *configStore, hostConfig *containertypes.HostConfig, config *containertypes.Config, update bool) (warnings []string, _ error) { // First perform verification of settings common across all platforms. - if err = validateContainerConfig(config); err != nil { - return warnings, err + if err := validateContainerConfig(config); err != nil { + return nil, err } warns, err := validateHostConfig(hostConfig) diff --git a/daemon/container_operations.go b/daemon/container_operations.go index d8e07cb4d1..f8a5dc6534 100644 --- a/daemon/container_operations.go +++ b/daemon/container_operations.go @@ -761,12 +761,12 @@ func (daemon *Daemon) connectToNetwork(ctx context.Context, cfg *config.Config, if !ctr.Managed { // add container name/alias to DNS if err := daemon.ActivateContainerServiceBinding(ctr.Name); err != nil { - return fmt.Errorf("Activate container service binding for %s failed: %v", ctr.Name, err) + return fmt.Errorf("activate container service binding for %s failed: %v", ctr.Name, err) } } if err := updateJoinInfo(ctr.NetworkSettings, n, ep); err != nil { - return fmt.Errorf("Updating join info failed: %v", err) + return fmt.Errorf("updating join info failed: %v", err) } ctr.NetworkSettings.Ports = getPortMapInfo(sb) diff --git a/daemon/daemon_unix.go b/daemon/daemon_unix.go index 6cf97e1159..7379752dd8 100644 --- a/daemon/daemon_unix.go +++ b/daemon/daemon_unix.go @@ -413,7 +413,7 @@ func adaptSharedNamespaceContainer(daemon containerGetter, hostConfig *container } // verifyPlatformContainerResources performs platform-specific validation of the container's resource-configuration -func verifyPlatformContainerResources(resources *containertypes.Resources, sysInfo *sysinfo.SysInfo, update bool) (warnings []string, err error) { +func verifyPlatformContainerResources(resources *containertypes.Resources, sysInfo *sysinfo.SysInfo, update bool) (warnings []string, _ error) { fixMemorySwappiness(resources) // memory subsystem checks and adjustments @@ -650,7 +650,7 @@ func isRunningSystemd() bool { // verifyPlatformContainerSettings performs platform-specific validation of the // hostconfig and config structures. -func verifyPlatformContainerSettings(daemon *Daemon, daemonCfg *configStore, hostConfig *containertypes.HostConfig, update bool) (warnings []string, err error) { +func verifyPlatformContainerSettings(daemon *Daemon, daemonCfg *configStore, hostConfig *containertypes.HostConfig, update bool) (warnings []string, _ error) { if hostConfig == nil { return nil, nil } diff --git a/daemon/daemon_windows.go b/daemon/daemon_windows.go index 33c7658f18..458969f9d2 100644 --- a/daemon/daemon_windows.go +++ b/daemon/daemon_windows.go @@ -67,7 +67,7 @@ func (daemon *Daemon) adaptContainerSettings(daemonCfg *config.Config, hostConfi } // verifyPlatformContainerResources performs platform-specific validation of the container's resource-configuration -func verifyPlatformContainerResources(resources *containertypes.Resources, isHyperv bool) (warnings []string, err error) { +func verifyPlatformContainerResources(resources *containertypes.Resources, isHyperv bool) (warnings []string, _ error) { fixMemorySwappiness(resources) if !isHyperv { // The processor resource controls are mutually exclusive on @@ -171,7 +171,7 @@ func verifyPlatformContainerResources(resources *containertypes.Resources, isHyp // verifyPlatformContainerSettings performs platform-specific validation of the // hostconfig and config structures. -func verifyPlatformContainerSettings(daemon *Daemon, daemonCfg *configStore, hostConfig *containertypes.HostConfig, update bool) (warnings []string, err error) { +func verifyPlatformContainerSettings(daemon *Daemon, daemonCfg *configStore, hostConfig *containertypes.HostConfig, update bool) (warnings []string, _ error) { if hostConfig == nil { return nil, nil } diff --git a/daemon/network.go b/daemon/network.go index 268aecbd24..dfbf043aa9 100644 --- a/daemon/network.go +++ b/daemon/network.go @@ -589,14 +589,14 @@ func (daemon *Daemon) deleteNetwork(nw *libnetwork.Network, dynamic bool) error } // GetNetworks returns a list of all networks -func (daemon *Daemon) GetNetworks(filter filters.Args, config backend.NetworkListConfig) (networks []networktypes.Inspect, err error) { +func (daemon *Daemon) GetNetworks(filter filters.Args, config backend.NetworkListConfig) ([]networktypes.Inspect, error) { var idx map[string]*libnetwork.Network if config.Detailed { idx = make(map[string]*libnetwork.Network) } allNetworks := daemon.getAllNetworks() - networks = make([]networktypes.Inspect, 0, len(allNetworks)) + networks := make([]networktypes.Inspect, 0, len(allNetworks)) for _, n := range allNetworks { nr := buildNetworkResource(n) networks = append(networks, nr) @@ -605,6 +605,7 @@ func (daemon *Daemon) GetNetworks(filter filters.Args, config backend.NetworkLis } } + var err error networks, err = network.FilterNetworks(networks, filter) if err != nil { return nil, err diff --git a/daemon/oci_linux.go b/daemon/oci_linux.go index b8d10377f0..c88b9f1f41 100644 --- a/daemon/oci_linux.go +++ b/daemon/oci_linux.go @@ -476,7 +476,7 @@ func inSlice(slice []string, s string) bool { // withMounts sets the container's mounts func withMounts(daemon *Daemon, daemonCfg *configStore, c *container.Container, mounts []container.Mount) coci.SpecOpts { - return func(ctx context.Context, _ coci.Client, _ *containers.Container, s *coci.Spec) (err error) { + return func(ctx context.Context, _ coci.Client, _ *containers.Container, s *coci.Spec) error { sortMounts(mounts) userMounts := make(map[string]struct{}) @@ -1003,7 +1003,7 @@ func WithUser(c *container.Container) coci.SpecOpts { } } -func (daemon *Daemon) createSpec(ctx context.Context, daemonCfg *configStore, c *container.Container, mounts []container.Mount) (retSpec *specs.Spec, err error) { +func (daemon *Daemon) createSpec(ctx context.Context, daemonCfg *configStore, c *container.Container, mounts []container.Mount) (retSpec *specs.Spec, _ error) { var ( opts []coci.SpecOpts s = oci.DefaultSpec() diff --git a/daemon/oci_windows.go b/daemon/oci_windows.go index df1de26305..393e8aa5ae 100644 --- a/daemon/oci_windows.go +++ b/daemon/oci_windows.go @@ -478,7 +478,7 @@ func (daemon *Daemon) mergeUlimits(c *containertypes.HostConfig, daemonCfg *conf // listing only the methods we care about here. // It's mainly useful to easily allow mocking the registry in tests. type registryKey interface { - GetStringValue(name string) (val string, valtype uint32, err error) + GetStringValue(name string) (val string, valType uint32, err error) Close() error } @@ -526,7 +526,8 @@ func readCredentialSpecFile(id, root, location string) (string, error) { return string(bcontents[:]), nil } -func setupWindowsDevices(devices []containertypes.DeviceMapping) (specDevices []specs.WindowsDevice, err error) { +func setupWindowsDevices(devices []containertypes.DeviceMapping) ([]specs.WindowsDevice, error) { + var specDevices []specs.WindowsDevice for _, deviceMapping := range devices { if strings.HasPrefix(deviceMapping.PathOnHost, "class/") { specDevices = append(specDevices, specs.WindowsDevice{ diff --git a/daemon/oci_windows_test.go b/daemon/oci_windows_test.go index a8508fa9f3..0f676056d0 100644 --- a/daemon/oci_windows_test.go +++ b/daemon/oci_windows_test.go @@ -111,7 +111,7 @@ func TestSetWindowsCredentialSpecInSpec(t *testing.T) { t.Run("happy path with a 'registry://' option", func(t *testing.T) { valueName := "my-cred-spec" key := &dummyRegistryKey{ - getStringValueFunc: func(name string) (val string, valtype uint32, err error) { + getStringValueFunc: func(name string) (val string, valType uint32, _ error) { assert.Equal(t, valueName, name) return dummyCredFileContents, 0, nil }, @@ -141,7 +141,7 @@ func TestSetWindowsCredentialSpecInSpec(t *testing.T) { t.Run("when using a 'registry://' option pointing to a value that doesn't exist, it fails gracefully", func(t *testing.T) { valueName := "my-cred-spec" key := &dummyRegistryKey{ - getStringValueFunc: func(name string) (val string, valtype uint32, err error) { + getStringValueFunc: func(name string) (val string, valType uint32, _ error) { assert.Equal(t, valueName, name) return "", 0, registry.ErrNotExist }, @@ -159,7 +159,7 @@ func TestSetWindowsCredentialSpecInSpec(t *testing.T) { dummyError := fmt.Errorf("dummy error") valueName := "my-cred-spec" key := &dummyRegistryKey{ - getStringValueFunc: func(name string) (val string, valtype uint32, err error) { + getStringValueFunc: func(name string) (val string, valType uint32, _ error) { assert.Equal(t, valueName, name) return "", 0, dummyError }, @@ -274,11 +274,11 @@ func TestSetWindowsCredentialSpecInSpec(t *testing.T) { /* Helpers below */ type dummyRegistryKey struct { - getStringValueFunc func(name string) (val string, valtype uint32, err error) + getStringValueFunc func(name string) (val string, valType uint32, err error) closed bool } -func (k *dummyRegistryKey) GetStringValue(name string) (val string, valtype uint32, err error) { +func (k *dummyRegistryKey) GetStringValue(name string) (val string, valType uint32, _ error) { return k.getStringValueFunc(name) } diff --git a/daemon/stats_unix.go b/daemon/stats_unix.go index f734f66b9e..6226f05d89 100644 --- a/daemon/stats_unix.go +++ b/daemon/stats_unix.go @@ -358,8 +358,7 @@ func getSystemCPUUsage() (cpuUsage uint64, cpuNum uint32, _ error) { } totalClockTicks += v } - cpuUsage = (totalClockTicks * nanoSecondsPerSecond) / - clockTicksPerSecond + cpuUsage = (totalClockTicks * nanoSecondsPerSecond) / clockTicksPerSecond } if '0' <= line[3] && line[3] <= '9' { cpuNum++