Address loop dev PR comments #4178

Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>

mount/losetup_linux.go

@@ -26,28 +26,17 @@ import (
 	"unsafe"
 
 	"github.com/pkg/errors"
+	"golang.org/x/sys/unix"
 )
 
 const (
 	loopControlPath = "/dev/loop-control"
 	loopDevFormat   = "/dev/loop%d"
 
-	// According to util-linux/include/loopdev.h
-	ioctlSetFd       = 0x4C00
-	ioctlClrFd       = 0x4C01
-	ioctlSetStatus64 = 0x4C04
-	ioctlGetFree     = 0x4C82
-
-	loFlagsReadonly = 1
-	//loFlagsUseAops   = 2
-	loFlagsAutoclear = 4
-	//loFlagsPartScan  = 8
-	loFlagsDirectIO = 16
-
 	ebusyString = "device or resource busy"
 )
 
-// parameters to control loop device setup
+// LoopParams parameters to control loop device setup
 type LoopParams struct {
 	// Loop device should forbid write
 	Readonly bool
@@ -58,29 +47,6 @@ type LoopParams struct {
 	Direct bool
 }
 
-// struct loop_info64 in util-linux/include/loopdev.h
-type loopInfo struct {
-	/*
-		device         uint64
-		inode          uint64
-		rdevice        uint64
-		offset         uint64
-		sizelimit      uint64
-		number         uint32
-		encryptType    uint32
-		encryptKeySize uint32
-	*/
-	_        [13]uint32
-	flags    uint32
-	fileName [64]byte
-	/*
-		cryptName  [64]byte
-		encryptKey [32]byte
-		init       [2]uint64
-	*/
-	_ [112]byte
-}
-
 func ioctl(fd, req, args uintptr) (uintptr, uintptr, error) {
 	r1, r2, errno := syscall.Syscall(syscall.SYS_IOCTL, fd, req, args)
 	if errno != 0 {
@@ -96,63 +62,70 @@ func getFreeLoopDev() (uint32, error) {
 		return 0, errors.Errorf("could not open %v: %v", loopControlPath, err)
 	}
 	defer ctrl.Close()
-	num, _, err := ioctl(ctrl.Fd(), ioctlGetFree, 0)
+	num, _, err := ioctl(ctrl.Fd(), unix.LOOP_CTL_GET_FREE, 0)
 	if err != nil {
 		return 0, errors.Wrap(err, "could not get free loop device")
 	}
 	return uint32(num), nil
 }
 
-func setupLoopDev(backingFile, loopDev string, param LoopParams) (devFile *os.File, err error) {
+func setupLoopDev(backingFile, loopDev string, param LoopParams) error {
 	// 1. Open backing file and loop device
-	oflags := os.O_RDWR
+	flags := os.O_RDWR
 	if param.Readonly {
-		oflags = os.O_RDONLY
+		flags = os.O_RDONLY
 	}
-	back, err := os.OpenFile(backingFile, oflags, 0)
+
+	back, err := os.OpenFile(backingFile, flags, 0)
 	if err != nil {
-		return nil, errors.Errorf("could not open backing file: %v", err)
+		return errors.Wrapf(err, "could not open backing file: %s", backingFile)
 	}
 	defer back.Close()
 
-	loopFile, err := os.OpenFile(loopDev, oflags, 0)
+	loop, err := os.OpenFile(loopDev, flags, 0)
 	if err != nil {
-		return nil, errors.Errorf("could not open loop device: %v", err)
+		return errors.Wrapf(err, "could not open loop device: %s", loopDev)
 	}
-	defer func() {
+	defer loop.Close()
-		if err != nil {
-			loopFile.Close()
-		}
-	}()
 
 	// 2. Set FD
-	if _, _, err = ioctl(loopFile.Fd(), ioctlSetFd, back.Fd()); err != nil {
+	if _, _, err = ioctl(loop.Fd(), unix.LOOP_SET_FD, back.Fd()); err != nil {
-		return nil, errors.Errorf("could not set loop fd: %v", err)
+		return errors.Wrapf(err, "could not set loop fd for device: %s", loopDev)
 	}
 
 	// 3. Set Info
-	info := loopInfo{}
+	info := unix.LoopInfo64{}
-	copy(info.fileName[:], []byte(backingFile))
+	copy(info.File_name[:], backingFile)
 	if param.Readonly {
-		info.flags |= loFlagsReadonly
+		info.Flags |= unix.LO_FLAGS_READ_ONLY
 	}
+
 	if param.Autoclear {
-		info.flags |= loFlagsAutoclear
+		info.Flags |= unix.LO_FLAGS_AUTOCLEAR
 	}
+
 	if param.Direct {
-		info.flags |= loFlagsAutoclear
+		info.Flags |= unix.LO_FLAGS_DIRECT_IO
 	}
-	if _, _, err := ioctl(loopFile.Fd(), ioctlSetStatus64, uintptr(unsafe.Pointer(&info))); err != nil {
+
+	_, _, err = ioctl(loop.Fd(), unix.LOOP_SET_STATUS64, uintptr(unsafe.Pointer(&info)))
+	if err == nil {
+		return nil
+	}
+
+	if param.Direct {
 		// Retry w/o direct IO flag in case kernel does not support it. The downside is that
 		// it will suffer from double cache problem.
-		info.flags &= ^(uint32(loFlagsDirectIO))
+		info.Flags &= ^(uint32(unix.LO_FLAGS_DIRECT_IO))
-		if _, _, err := ioctl(loopFile.Fd(), ioctlSetStatus64, uintptr(unsafe.Pointer(&info))); err != nil {
+		_, _, err = ioctl(loop.Fd(), unix.LOOP_SET_STATUS64, uintptr(unsafe.Pointer(&info)))
-			ioctl(loopFile.Fd(), ioctlClrFd, 0)
+		if err == nil {
-			return nil, errors.Errorf("cannot set loop info:%v", err)
+			return nil
 		}
 	}
 
-	return loopFile, nil
+	// Cleanup loop fd and return error
+	_, _, _ = ioctl(loop.Fd(), unix.LOOP_CLR_FD, 0)
+	return errors.Errorf("failed to set loop device info: %v", err)
 }
 
 // setupLoop looks for (and possibly creates) a free loop device, and
@@ -169,18 +142,15 @@ func setupLoopDev(backingFile, loopDev string, param LoopParams) (devFile *os.Fi
 // the loop device when done with it.
 //
 // Upon success, the file handle to the loop device is returned.
-func setupLoop(backingFile string, param LoopParams) (*os.File, error) {
+func setupLoop(backingFile string, param LoopParams) (string, error) {
-	var loopDev string
-
 	for retry := 1; retry < 200; retry++ {
 		num, err := getFreeLoopDev()
 		if err != nil {
-			return nil, err
+			return "", err
 		}
 
-		loopDev = fmt.Sprintf(loopDevFormat, num)
+		loopDev := fmt.Sprintf(loopDevFormat, num)
-		loopFile, err := setupLoopDev(backingFile, loopDev, param)
+		if err := setupLoopDev(backingFile, loopDev, param); err != nil {
-		if err != nil {
 			// Per util-linux/sys-utils/losetup.c:create_loop(),
 			// free loop device can race and we end up failing
 			// with EBUSY when trying to set it up.
@@ -189,40 +159,36 @@ func setupLoop(backingFile string, param LoopParams) (*os.File, error) {
 				time.Sleep(time.Millisecond * time.Duration(rand.Intn(retry*10)))
 				continue
 			}
-			return nil, err
+			return "", err
 		}
-		return loopFile, nil
+
+		return loopDev, nil
 	}
 
-	return nil, errors.New("Timeout creating new loopback device")
+	return "", errors.New("timeout creating new loopback device")
 }
 
 func removeLoop(loopdev string) error {
-	dev, err := os.Open(loopdev)
+	file, err := os.Open(loopdev)
 	if err != nil {
 		return err
 	}
-	_, _, err = ioctl(dev.Fd(), ioctlClrFd, 0)
+	defer file.Close()
-	dev.Close()
+
+	_, _, err = ioctl(file.Fd(), unix.LOOP_CLR_FD, 0)
 	return err
 }
 
 // Attach a specified backing file to a loop device
 func AttachLoopDevice(backingFile string) (string, error) {
-	dev, err := setupLoop(backingFile, LoopParams{})
+	return setupLoop(backingFile, LoopParams{})
-	if err != nil {
-		return "", err
-	}
-	dev.Close()
-
-	return dev.Name(), nil
 }
 
 // Detach a loop device
 func DetachLoopDevice(devices ...string) error {
 	for _, dev := range devices {
 		if err := removeLoop(dev); err != nil {
-			return err
+			return errors.Wrapf(err, "failed to remove loop device: %s", dev)
 		}
 	}
 

mount/losetup_linux_test.go

@@ -53,11 +53,11 @@ func TestSetupLoop(t *testing.T) {
 	}()
 
 	/* RO loop */
-	f, err = setupLoop(backingFile, LoopParams{Readonly: true, Autoclear: true})
+	path, err := setupLoop(backingFile, LoopParams{Readonly: true, Autoclear: true})
 	if err != nil {
 		t.Fatal(err)
 	}
-	ff, err := os.OpenFile(f.Name(), os.O_RDWR, 0)
+	ff, err := os.OpenFile(path, os.O_RDWR, 0)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -67,16 +67,13 @@ func TestSetupLoop(t *testing.T) {
 	if err = ff.Close(); err != nil {
 		t.Fatal(err)
 	}
-	if err = f.Close(); err != nil {
-		t.Fatal(err)
-	}
 
 	/* RW loop */
-	f, err = setupLoop(backingFile, LoopParams{Autoclear: true})
+	path, err = setupLoop(backingFile, LoopParams{Autoclear: true})
 	if err != nil {
 		t.Fatal(err)
 	}
-	ff, err = os.OpenFile(f.Name(), os.O_RDWR, 0)
+	ff, err = os.OpenFile(path, os.O_RDWR, 0)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -86,9 +83,6 @@ func TestSetupLoop(t *testing.T) {
 	if err = ff.Close(); err != nil {
 		t.Fatal(err)
 	}
-	if err = f.Close(); err != nil {
-		t.Fatal(err)
-	}
 }
 
 func TestAttachDetachLoopDevice(t *testing.T) {

mount/mount_linux.go

@@ -85,9 +85,8 @@ func (m *Mount) Mount(target string) (err error) {
 			if err != nil {
 				return err
 			}
-			defer devFile.Close()
 			// Mount the loop device instead
-			source = devFile.Name()
+			source = devFile
 		}
 		if err := mountAt(chdir, source, target, m.Type, uintptr(oflags), data); err != nil {
 			return err