[lxc-devel] [lxd/master] Add function to consolidate Do's error handling

sean-jc on Github lxc-bot at linuxcontainers.org
Fri Jun 24 17:12:07 UTC 2016


A non-text attachment was scrubbed...
Name: not available
Type: text/x-mailbox
Size: 810 bytes
Desc: not available
URL: <http://lists.linuxcontainers.org/pipermail/lxc-devel/attachments/20160624/92498852/attachment.bin>
-------------- next part --------------
From b08c8faca061b084b45bb185fc66448f012d6d0a Mon Sep 17 00:00:00 2001
From: Sean Christopherson <sean.j.christopherson at intel.com>
Date: Fri, 24 Jun 2016 09:43:54 -0700
Subject: [PATCH] Add function to consolidate Do's error handling

Add a function that can be called inline with return to handle all
cleanup if an error occurs after migration has been started.  This
fixes a missing call to driver.Cleanup() and hopefully reduces the
probability of a similar bug occuring in the future.

Signed-off-by: Sean Christopherson <sean.j.christopherson at intel.com>
---
 lxd/migrate.go | 38 ++++++++++++++++++--------------------
 1 file changed, 18 insertions(+), 20 deletions(-)

diff --git a/lxd/migrate.go b/lxd/migrate.go
index 651b257..3ea1a76 100644
--- a/lxd/migrate.go
+++ b/lxd/migrate.go
@@ -302,37 +302,39 @@ func (s *migrationSourceWs) Do(op *operation) error {
 		driver, _ = rsyncMigrationSource(s.container)
 	}
 
-	if err := driver.SendWhileRunning(s.fsConn); err != nil {
+	// All failure paths need to do a few things to correctly handle errors before returning.
+	// Unfortunately, handling errors is not well-suited to defer as the code depends on the
+	// status of driver and the error value.  The error value is especially tricky due to the
+	// common case of creating a new err variable (intentional or not) due to scoping and use
+	// of ":=".  Capturing err in a closure for use in defer would be fragile, which defeats
+	// the purpose of using defer.  An abort function reduces the odds of mishandling errors
+	// without introducing the fragility of closing on err.
+	abort := func(err error) error {
 		driver.Cleanup()
 		s.sendControl(err)
 		return err
 	}
 
+	if err := driver.SendWhileRunning(s.fsConn); err != nil {
+		return abort(err)
+	}
+
 	if s.live {
 		if header.Criu == nil {
-			driver.Cleanup()
-			err := fmt.Errorf("Got no CRIU socket type for live migration")
-			s.sendControl(err)
-			return err
+			return abort(fmt.Errorf("Got no CRIU socket type for live migration"))
 		} else if *header.Criu != CRIUType_CRIU_RSYNC {
-			driver.Cleanup()
-			err := fmt.Errorf("Formats other than criu rsync not understood")
-			s.sendControl(err)
-			return err
+			return abort(fmt.Errorf("Formats other than criu rsync not understood"))
 		}
 
 		checkpointDir, err := ioutil.TempDir("", "lxd_checkpoint_")
 		if err != nil {
-			driver.Cleanup()
-			s.sendControl(err)
-			return err
+			return abort(err)
 		}
 		defer os.RemoveAll(checkpointDir)
 
 		err = s.container.Migrate(lxc.MIGRATE_DUMP, checkpointDir, "migration", true)
 		if err != nil {
-			s.sendControl(err)
-			return err
+			return abort(err)
 		}
 
 		/*
@@ -343,15 +345,11 @@ func (s *migrationSourceWs) Do(op *operation) error {
 		 * p.haul's protocol, it will make sense to do these in parallel.
 		 */
 		if err := RsyncSend(shared.AddSlash(checkpointDir), s.criuConn); err != nil {
-			driver.Cleanup()
-			s.sendControl(err)
-			return err
+			return abort(err)
 		}
 
 		if err := driver.SendAfterCheckpoint(s.fsConn); err != nil {
-			driver.Cleanup()
-			s.sendControl(err)
-			return err
+			return abort(err)
 		}
 	}
 


More information about the lxc-devel mailing list