diff --git a/libpod/container_internal.go b/libpod/container_internal.go index f64117c6a9..fb78553a44 100644 --- a/libpod/container_internal.go +++ b/libpod/container_internal.go @@ -1816,6 +1816,14 @@ func (c *Container) cleanupStorage() error { } var cleanupErr error + reportErrorf := func(msg string, args ...any) { + err := fmt.Errorf(msg, args...) // Always use fmt.Errorf instead of just logrus.Errorf(…) because the format string probably contains %w + if cleanupErr == nil { + cleanupErr = err + } else { + logrus.Errorf("%s", err.Error()) + } + } markUnmounted := func() { c.state.Mountpoint = "" @@ -1823,11 +1831,7 @@ func (c *Container) cleanupStorage() error { if c.valid { if err := c.save(); err != nil { - if cleanupErr != nil { - logrus.Errorf("Unmounting container %s: %v", c.ID(), err) - } else { - cleanupErr = err - } + reportErrorf("unmounting container %s: %w", c.ID(), err) } } } @@ -1836,40 +1840,24 @@ func (c *Container) cleanupStorage() error { if c.config.RootfsOverlay { overlayBasePath := filepath.Dir(c.state.Mountpoint) if err := overlay.Unmount(overlayBasePath); err != nil { - if cleanupErr != nil { - logrus.Errorf("Failed to clean up overlay mounts for %s: %v", c.ID(), err) - } else { - cleanupErr = fmt.Errorf("failed to clean up overlay mounts for %s: %w", c.ID(), err) - } + reportErrorf("failed to clean up overlay mounts for %s: %w", c.ID(), err) } } if c.config.RootfsMapping != nil { if err := unix.Unmount(c.config.Rootfs, 0); err != nil && err != unix.EINVAL { - if cleanupErr != nil { - logrus.Errorf("Unmounting idmapped rootfs for container %s after mount error: %v", c.ID(), err) - } else { - cleanupErr = fmt.Errorf("unmounting idmapped rootfs for container %s after mount error: %w", c.ID(), err) - } + reportErrorf("unmounting idmapped rootfs for container %s after mount error: %w", c.ID(), err) } } for _, containerMount := range c.config.Mounts { if err := c.unmountSHM(containerMount); err != nil { - if cleanupErr != nil { - logrus.Errorf("Unmounting container %s: %v", c.ID(), err) - } else { - cleanupErr = fmt.Errorf("unmounting container %s: %w", c.ID(), err) - } + reportErrorf("unmounting container %s: %w", c.ID(), err) } } if err := c.cleanupOverlayMounts(); err != nil { // If the container can't remove content report the error - if cleanupErr != nil { - logrus.Errorf("Failed to clean up overlay mounts for %s: %v", c.ID(), err) - } else { - cleanupErr = fmt.Errorf("failed to clean up overlay mounts for %s: %w", c.ID(), err) - } + reportErrorf("failed to clean up overlay mounts for %s: %w", c.ID(), err) } if c.config.Rootfs != "" { @@ -1885,11 +1873,7 @@ func (c *Container) cleanupStorage() error { if errors.Is(err, storage.ErrNotAContainer) || errors.Is(err, storage.ErrContainerUnknown) || errors.Is(err, storage.ErrLayerNotMounted) { logrus.Errorf("Storage for container %s has been removed", c.ID()) } else { - if cleanupErr != nil { - logrus.Errorf("Cleaning up container %s storage: %v", c.ID(), cleanupErr) - } else { - cleanupErr = fmt.Errorf("cleaning up container %s storage: %w", c.ID(), cleanupErr) - } + reportErrorf("cleaning up container %s storage: %w", c.ID(), err) } } @@ -1897,11 +1881,7 @@ func (c *Container) cleanupStorage() error { for _, v := range c.config.NamedVolumes { vol, err := c.runtime.state.Volume(v.Name) if err != nil { - if cleanupErr != nil { - logrus.Errorf("Retrieving named volume %s for container %s: %v", v.Name, c.ID(), err) - } else { - cleanupErr = fmt.Errorf("retrieving named volume %s for container %s: %w", v.Name, c.ID(), err) - } + reportErrorf("retrieving named volume %s for container %s: %w", v.Name, c.ID(), err) // We need to try and unmount every volume, so continue // if they fail. @@ -1911,11 +1891,7 @@ func (c *Container) cleanupStorage() error { if vol.needsMount() { vol.lock.Lock() if err := vol.unmount(false); err != nil { - if cleanupErr != nil { - logrus.Errorf("Unmounting volume %s for container %s: %v", vol.Name(), c.ID(), err) - } else { - cleanupErr = fmt.Errorf("unmounting volume %s for container %s: %w", vol.Name(), c.ID(), err) - } + reportErrorf("unmounting volume %s for container %s: %w", vol.Name(), c.ID(), err) } vol.lock.Unlock() } diff --git a/libpod/runtime_ctr.go b/libpod/runtime_ctr.go index 648118c374..e13fb66654 100644 --- a/libpod/runtime_ctr.go +++ b/libpod/runtime_ctr.go @@ -766,12 +766,20 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force, remo } var cleanupErr error + reportErrorf := func(msg string, args ...any) { + err := fmt.Errorf(msg, args...) // Always use fmt.Errorf instead of just logrus.Errorf(…) because the format string probably contains %w + if cleanupErr == nil { + cleanupErr = err + } else { + logrus.Errorf("%s", err.Error()) + } + } // Clean up network namespace, cgroups, mounts. // Do this before we set ContainerStateRemoving, to ensure that we can // actually remove from the OCI runtime. if err := c.cleanup(ctx); err != nil { - cleanupErr = fmt.Errorf("cleaning up container %s: %w", c.ID(), err) + reportErrorf("cleaning up container %s: %w", c.ID(), err) } // Remove all active exec sessions @@ -779,11 +787,7 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force, remo // after setting the state to ContainerStateRemoving will prevent that the container is // restarted if err := c.removeAllExecSessions(); err != nil { - if cleanupErr == nil { - cleanupErr = err - } else { - logrus.Errorf("Remove exec sessions: %v", err) - } + reportErrorf("removing exec sessions: %w", err) } // Set ContainerStateRemoving as an intermediate state (we may get @@ -792,31 +796,19 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force, remo if err := c.save(); err != nil { if !errors.Is(err, define.ErrCtrRemoved) { - if cleanupErr == nil { - cleanupErr = err - } else { - logrus.Errorf("Saving container: %v", err) - } + reportErrorf("saving container: %w", err) } } // Stop the container's storage if err := c.teardownStorage(); err != nil { - if cleanupErr == nil { - cleanupErr = err - } else { - logrus.Errorf("Cleaning up storage: %v", err) - } + reportErrorf("cleaning up storage: %w", err) } // Remove the container's CID file on container removal. if cidFile, ok := c.config.Spec.Annotations[define.InspectAnnotationCIDFile]; ok { if err := os.Remove(cidFile); err != nil && !errors.Is(err, os.ErrNotExist) { - if cleanupErr == nil { - cleanupErr = err - } else { - logrus.Errorf("Cleaning up CID file: %v", err) - } + reportErrorf("cleaning up CID file: %w", err) } } // Remove the container from the state @@ -824,29 +816,17 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force, remo // If we're removing the pod, the container will be evicted // from the state elsewhere if err := r.state.RemoveContainerFromPod(pod, c); err != nil { - if cleanupErr == nil { - cleanupErr = err - } else { - logrus.Errorf("Removing container %s from database: %v", c.ID(), err) - } + reportErrorf("removing container %s from database: %w", c.ID(), err) } } else { if err := r.state.RemoveContainer(c); err != nil { - if cleanupErr == nil { - cleanupErr = err - } else { - logrus.Errorf("Removing container %s from database: %v", c.ID(), err) - } + reportErrorf("removing container %s from database: %w", c.ID(), err) } } // Deallocate the container's lock if err := c.lock.Free(); err != nil { - if cleanupErr == nil && !os.IsNotExist(err) { - cleanupErr = fmt.Errorf("freeing lock for container %s: %w", c.ID(), err) - } else { - logrus.Errorf("Free container lock: %v", err) - } + reportErrorf("freeing lock for container %s: %w", c.ID(), err) } // Set container as invalid so it can no longer be used