From 032d4a95f0f041133dd8cc2b7d37245d1c78ac59 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= <mitr@redhat.com>
Date: Fri, 19 May 2023 22:26:11 +0200
Subject: [PATCH] Consolidate error handling in Runtime.removeContainer
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Use a helper to handle the cleanupErr logic instead of
copy&pasting it EIGHT times.

Also modifies the returned errors to be wrapped with a context,
and changes the text of the logged errors a bit.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
---
 libpod/runtime_ctr.go | 52 +++++++++++++------------------------------
 1 file changed, 16 insertions(+), 36 deletions(-)

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