Fix vfs unit test and port VFS to the new IDMappings

The test was failing because TarOptions was using a non-pointer for
ChownOpts, which meant the check for nil was never true, and
createTarFile was never using the hdr.UID/GID

Signed-off-by: Daniel Nephin <dnephin@docker.com>

daemon/archive_tarcopyoptions_unix.go

@@ -20,6 +20,6 @@ func (daemon *Daemon) tarCopyOptions(container *container.Container, noOverwrite
 
 	return &archive.TarOptions{
 		NoOverwriteDirNonDir: noOverwriteDirNonDir,
-		ChownOpts:            idtools.IDPair{UID: user.Uid, GID: user.Gid},
+		ChownOpts:            &idtools.IDPair{UID: user.Uid, GID: user.Gid},
 	}, nil
 }

daemon/graphdriver/devmapper/devmapper_test.go

@@ -4,6 +4,8 @@ package devmapper
 
 import (
 	"fmt"
+	"os"
+	"syscall"
 	"testing"
 	"time"
 
@@ -17,11 +19,51 @@ func init() {
 	defaultMetaDataLoopbackSize = 200 * 1024 * 1024
 	defaultBaseFsSize = 300 * 1024 * 1024
 	defaultUdevSyncOverride = true
-	if err := graphtest.InitLoopbacks(); err != nil {
+	if err := initLoopbacks(); err != nil {
 		panic(err)
 	}
 }
 
+// initLoopbacks ensures that the loopback devices are properly created within
+// the system running the device mapper tests.
+func initLoopbacks() error {
+	statT, err := getBaseLoopStats()
+	if err != nil {
+		return err
+	}
+	// create at least 8 loopback files, ya, that is a good number
+	for i := 0; i < 8; i++ {
+		loopPath := fmt.Sprintf("/dev/loop%d", i)
+		// only create new loopback files if they don't exist
+		if _, err := os.Stat(loopPath); err != nil {
+			if mkerr := syscall.Mknod(loopPath,
+				uint32(statT.Mode|syscall.S_IFBLK), int((7<<8)|(i&0xff)|((i&0xfff00)<<12))); mkerr != nil {
+				return mkerr
+			}
+			os.Chown(loopPath, int(statT.Uid), int(statT.Gid))
+		}
+	}
+	return nil
+}
+
+// getBaseLoopStats inspects /dev/loop0 to collect uid,gid, and mode for the
+// loop0 device on the system.  If it does not exist we assume 0,0,0660 for the
+// stat data
+func getBaseLoopStats() (*syscall.Stat_t, error) {
+	loop0, err := os.Stat("/dev/loop0")
+	if err != nil {
+		if os.IsNotExist(err) {
+			return &syscall.Stat_t{
+				Uid:  0,
+				Gid:  0,
+				Mode: 0660,
+			}, nil
+		}
+		return nil, err
+	}
+	return loop0.Sys().(*syscall.Stat_t), nil
+}
+
 // This avoids creating a new driver for each test if all tests are run
 // Make sure to put new tests between TestDevmapperSetup and TestDevmapperTeardown
 func TestDevmapperSetup(t *testing.T) {

daemon/graphdriver/graphtest/graphtest_unix.go

@@ -16,6 +16,8 @@ import (
 	"github.com/docker/docker/daemon/graphdriver"
 	"github.com/docker/docker/pkg/stringid"
 	"github.com/docker/go-units"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 var (
@@ -33,14 +35,9 @@ type Driver struct {
 
 func newDriver(t testing.TB, name string, options []string) *Driver {
 	root, err := ioutil.TempDir("", "docker-graphtest-")
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	if err := os.MkdirAll(root, 0755); err != nil {
-		t.Fatal(err)
-	}
+	require.NoError(t, err)
 
+	require.NoError(t, os.MkdirAll(root, 0755))
 	d, err := graphdriver.GetDriver(name, nil, graphdriver.Options{DriverOptions: options, Root: root})
 	if err != nil {
 		t.Logf("graphdriver: %v\n", err)
@@ -86,14 +83,11 @@ func DriverTestCreateEmpty(t testing.TB, drivername string, driverOptions ...str
 	driver := GetDriver(t, drivername, driverOptions...)
 	defer PutDriver(t)
 
-	if err := driver.Create("empty", "", nil); err != nil {
-		t.Fatal(err)
-	}
+	err := driver.Create("empty", "", nil)
+	require.NoError(t, err)
 
 	defer func() {
-		if err := driver.Remove("empty"); err != nil {
-			t.Fatal(err)
-		}
+		require.NoError(t, driver.Remove("empty"))
 	}()
 
 	if !driver.Exists("empty") {
@@ -101,21 +95,14 @@ func DriverTestCreateEmpty(t testing.TB, drivername string, driverOptions ...str
 	}
 
 	dir, err := driver.Get("empty", "")
-	if err != nil {
-		t.Fatal(err)
-	}
+	require.NoError(t, err)
 
 	verifyFile(t, dir, 0755|os.ModeDir, 0, 0)
 
 	// Verify that the directory is empty
 	fis, err := readDir(dir)
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	if len(fis) != 0 {
-		t.Fatal("New directory not empty")
-	}
+	require.NoError(t, err)
+	assert.Len(t, fis, 0)
 
 	driver.Put("empty")
 }
@@ -127,9 +114,7 @@ func DriverTestCreateBase(t testing.TB, drivername string, driverOptions ...stri
 
 	createBase(t, driver, "Base")
 	defer func() {
-		if err := driver.Remove("Base"); err != nil {
-			t.Fatal(err)
-		}
+		require.NoError(t, driver.Remove("Base"))
 	}()
 	verifyBase(t, driver, "Base")
 }
@@ -140,21 +125,14 @@ func DriverTestCreateSnap(t testing.TB, drivername string, driverOptions ...stri
 	defer PutDriver(t)
 
 	createBase(t, driver, "Base")
-
 	defer func() {
-		if err := driver.Remove("Base"); err != nil {
-			t.Fatal(err)
-		}
+		require.NoError(t, driver.Remove("Base"))
 	}()
 
-	if err := driver.Create("Snap", "Base", nil); err != nil {
-		t.Fatal(err)
-	}
-
+	err := driver.Create("Snap", "Base", nil)
+	require.NoError(t, err)
 	defer func() {
-		if err := driver.Remove("Snap"); err != nil {
-			t.Fatal(err)
-		}
+		require.NoError(t, driver.Remove("Snap"))
 	}()
 
 	verifyBase(t, driver, "Snap")

daemon/graphdriver/graphtest/testutil_unix.go

@@ -3,7 +3,6 @@
 package graphtest
 
 import (
-	"fmt"
 	"io/ioutil"
 	"os"
 	"path"
@@ -11,81 +10,24 @@ import (
 	"testing"
 
 	"github.com/docker/docker/daemon/graphdriver"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
-// InitLoopbacks ensures that the loopback devices are properly created within
-// the system running the device mapper tests.
-func InitLoopbacks() error {
-	statT, err := getBaseLoopStats()
-	if err != nil {
-		return err
-	}
-	// create at least 8 loopback files, ya, that is a good number
-	for i := 0; i < 8; i++ {
-		loopPath := fmt.Sprintf("/dev/loop%d", i)
-		// only create new loopback files if they don't exist
-		if _, err := os.Stat(loopPath); err != nil {
-			if mkerr := syscall.Mknod(loopPath,
-				uint32(statT.Mode|syscall.S_IFBLK), int((7<<8)|(i&0xff)|((i&0xfff00)<<12))); mkerr != nil {
-				return mkerr
-			}
-			os.Chown(loopPath, int(statT.Uid), int(statT.Gid))
-		}
-	}
-	return nil
-}
-
-// getBaseLoopStats inspects /dev/loop0 to collect uid,gid, and mode for the
-// loop0 device on the system.  If it does not exist we assume 0,0,0660 for the
-// stat data
-func getBaseLoopStats() (*syscall.Stat_t, error) {
-	loop0, err := os.Stat("/dev/loop0")
-	if err != nil {
-		if os.IsNotExist(err) {
-			return &syscall.Stat_t{
-				Uid:  0,
-				Gid:  0,
-				Mode: 0660,
-			}, nil
-		}
-		return nil, err
-	}
-	return loop0.Sys().(*syscall.Stat_t), nil
-}
-
 func verifyFile(t testing.TB, path string, mode os.FileMode, uid, gid uint32) {
 	fi, err := os.Stat(path)
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	if fi.Mode()&os.ModeType != mode&os.ModeType {
-		t.Fatalf("Expected %s type 0x%x, got 0x%x", path, mode&os.ModeType, fi.Mode()&os.ModeType)
-	}
+	require.NoError(t, err)
 
-	if fi.Mode()&os.ModePerm != mode&os.ModePerm {
-		t.Fatalf("Expected %s mode %o, got %o", path, mode&os.ModePerm, fi.Mode()&os.ModePerm)
-	}
-
-	if fi.Mode()&os.ModeSticky != mode&os.ModeSticky {
-		t.Fatalf("Expected %s sticky 0x%x, got 0x%x", path, mode&os.ModeSticky, fi.Mode()&os.ModeSticky)
-	}
-
-	if fi.Mode()&os.ModeSetuid != mode&os.ModeSetuid {
-		t.Fatalf("Expected %s setuid 0x%x, got 0x%x", path, mode&os.ModeSetuid, fi.Mode()&os.ModeSetuid)
-	}
-
-	if fi.Mode()&os.ModeSetgid != mode&os.ModeSetgid {
-		t.Fatalf("Expected %s setgid 0x%x, got 0x%x", path, mode&os.ModeSetgid, fi.Mode()&os.ModeSetgid)
-	}
+	actual := fi.Mode()
+	assert.Equal(t, mode&os.ModeType, actual&os.ModeType, path)
+	assert.Equal(t, mode&os.ModePerm, actual&os.ModePerm, path)
+	assert.Equal(t, mode&os.ModeSticky, actual&os.ModeSticky, path)
+	assert.Equal(t, mode&os.ModeSetuid, actual&os.ModeSetuid, path)
+	assert.Equal(t, mode&os.ModeSetgid, actual&os.ModeSetgid, path)
 
 	if stat, ok := fi.Sys().(*syscall.Stat_t); ok {
-		if stat.Uid != uid {
-			t.Fatalf("%s no owned by uid %d", path, uid)
-		}
-		if stat.Gid != gid {
-			t.Fatalf("%s not owned by gid %d", path, gid)
-		}
+		assert.Equal(t, uid, stat.Uid, path)
+		assert.Equal(t, gid, stat.Gid, path)
 	}
 }
 
@@ -94,35 +36,25 @@ func createBase(t testing.TB, driver graphdriver.Driver, name string) {
 	oldmask := syscall.Umask(0)
 	defer syscall.Umask(oldmask)
 
-	if err := driver.CreateReadWrite(name, "", nil); err != nil {
-		t.Fatal(err)
-	}
+	err := driver.CreateReadWrite(name, "", nil)
+	require.NoError(t, err)
 
 	dir, err := driver.Get(name, "")
-	if err != nil {
-		t.Fatal(err)
-	}
+	require.NoError(t, err)
 	defer driver.Put(name)
 
 	subdir := path.Join(dir, "a subdir")
-	if err := os.Mkdir(subdir, 0705|os.ModeSticky); err != nil {
-		t.Fatal(err)
-	}
-	if err := os.Chown(subdir, 1, 2); err != nil {
-		t.Fatal(err)
-	}
+	require.NoError(t, os.Mkdir(subdir, 0705|os.ModeSticky))
+	require.NoError(t, os.Chown(subdir, 1, 2))
 
 	file := path.Join(dir, "a file")
-	if err := ioutil.WriteFile(file, []byte("Some data"), 0222|os.ModeSetuid); err != nil {
-		t.Fatal(err)
-	}
+	err = ioutil.WriteFile(file, []byte("Some data"), 0222|os.ModeSetuid)
+	require.NoError(t, err)
 }
 
 func verifyBase(t testing.TB, driver graphdriver.Driver, name string) {
 	dir, err := driver.Get(name, "")
-	if err != nil {
-		t.Fatal(err)
-	}
+	require.NoError(t, err)
 	defer driver.Put(name)
 
 	subdir := path.Join(dir, "a subdir")
@@ -131,13 +63,7 @@ func verifyBase(t testing.TB, driver graphdriver.Driver, name string) {
 	file := path.Join(dir, "a file")
 	verifyFile(t, file, 0222|os.ModeSetuid, 0, 0)
 
-	fis, err := readDir(dir)
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	if len(fis) != 2 {
-		t.Fatal("Unexpected files in base image")
-	}
-
+	files, err := readDir(dir)
+	require.NoError(t, err)
+	assert.Len(t, files, 2)
 }

daemon/graphdriver/vfs/driver.go

@@ -9,7 +9,6 @@ import (
 	"github.com/docker/docker/pkg/chrootarchive"
 	"github.com/docker/docker/pkg/idtools"
 	"github.com/docker/docker/pkg/system"
-
 	"github.com/opencontainers/selinux/go-selinux/label"
 )
 
@@ -26,15 +25,14 @@ func init() {
 // This sets the home directory for the driver and returns NaiveDiffDriver.
 func Init(home string, options []string, uidMaps, gidMaps []idtools.IDMap) (graphdriver.Driver, error) {
 	d := &Driver{
-		home:    home,
-		uidMaps: uidMaps,
-		gidMaps: gidMaps,
+		home:       home,
+		idMappings: idtools.NewIDMappingsFromMaps(uidMaps, gidMaps),
 	}
-	rootUID, rootGID, err := idtools.GetRootUIDGID(uidMaps, gidMaps)
+	rootIDs, err := d.idMappings.RootPair()
 	if err != nil {
 		return nil, err
 	}
-	if err := idtools.MkdirAllAs(home, 0700, rootUID, rootGID); err != nil {
+	if err := idtools.MkdirAllAndChown(home, 0700, rootIDs); err != nil {
 		return nil, err
 	}
 	return graphdriver.NewNaiveDiffDriver(d, uidMaps, gidMaps), nil
@@ -45,9 +43,8 @@ func Init(home string, options []string, uidMaps, gidMaps []idtools.IDMap) (grap
 // In order to support layering, files are copied from the parent layer into the new layer. There is no copy-on-write support.
 // Driver must be wrapped in NaiveDiffDriver to be used as a graphdriver.Driver
 type Driver struct {
-	home    string
-	uidMaps []idtools.IDMap
-	gidMaps []idtools.IDMap
+	home       string
+	idMappings *idtools.IDMappings
 }
 
 func (d *Driver) String() string {
@@ -82,14 +79,14 @@ func (d *Driver) Create(id, parent string, opts *graphdriver.CreateOpts) error {
 	}
 
 	dir := d.dir(id)
-	rootUID, rootGID, err := idtools.GetRootUIDGID(d.uidMaps, d.gidMaps)
+	rootIDs, err := d.idMappings.RootPair()
 	if err != nil {
 		return err
 	}
-	if err := idtools.MkdirAllAs(filepath.Dir(dir), 0700, rootUID, rootGID); err != nil {
+	if err := idtools.MkdirAllAndChown(filepath.Dir(dir), 0700, rootIDs); err != nil {
 		return err
 	}
-	if err := idtools.MkdirAs(dir, 0755, rootUID, rootGID); err != nil {
+	if err := idtools.MkdirAndChown(dir, 0755, rootIDs); err != nil {
 		return err
 	}
 	labelOpts := []string{"level:s0"}
@@ -103,10 +100,7 @@ func (d *Driver) Create(id, parent string, opts *graphdriver.CreateOpts) error {
 	if err != nil {
 		return fmt.Errorf("%s: %s", parent, err)
 	}
-	if err := CopyWithTar(parentDir, dir); err != nil {
-		return err
-	}
-	return nil
+	return CopyWithTar(parentDir, dir)
 }
 
 func (d *Driver) dir(id string) string {

pkg/archive/archive.go

@@ -39,7 +39,7 @@ type (
 		NoLchown         bool
 		UIDMaps          []idtools.IDMap
 		GIDMaps          []idtools.IDMap
-		ChownOpts        idtools.IDPair
+		ChownOpts        *idtools.IDPair
 		IncludeSourceDir bool
 		// WhiteoutFormat is the expected on disk format for whiteout files.
 		// This format will be converted to the standard format on pack
@@ -920,7 +920,7 @@ loop:
 			}
 		}
 
-		if err := createTarFile(path, dest, hdr, trBuf, !options.NoLchown, &options.ChownOpts, options.InUserNS); err != nil {
+		if err := createTarFile(path, dest, hdr, trBuf, !options.NoLchown, options.ChownOpts, options.InUserNS); err != nil {
 			return err
 		}