Deduplicate code

Signed-off-by: Jan Rodák <hony.com@seznam.cz>

internal/rawfilelock/rawfilelock.go

@@ -0,0 +1,64 @@
+package rawfilelock
+
+import (
+	"os"
+)
+
+type LockType byte
+
+const (
+	ReadLock LockType = iota
+	WriteLock
+)
+
+type FileHandle = fileHandle
+
+// OpenLock opens a file for locking
+// WARNING: This is the underlying file locking primitive of the OS;
+// because closing FileHandle releases the lock, it is not suitable for use
+// if there is any chance of two concurrent goroutines attempting to use the same lock.
+// Most users should use the higher-level operations from internal/staging_lockfile or pkg/lockfile.
+func OpenLock(path string, readOnly bool) (FileHandle, error) {
+	flags := os.O_CREATE
+	if readOnly {
+		flags |= os.O_RDONLY
+	} else {
+		flags |= os.O_RDWR
+	}
+
+	fd, err := openHandle(path, flags)
+	if err == nil {
+		return fd, nil
+	}
+
+	return fd, &os.PathError{Op: "open", Path: path, Err: err}
+}
+
+// TryLockFile attempts to lock a file handle
+func TryLockFile(fd FileHandle, lockType LockType) error {
+	return lockHandle(fd, lockType, true)
+}
+
+// LockFile locks a file handle
+func LockFile(fd FileHandle, lockType LockType) error {
+	return lockHandle(fd, lockType, false)
+}
+
+// UnlockAndClose unlocks and closes a file handle
+func UnlockAndCloseHandle(fd FileHandle) {
+	unlockAndCloseHandle(fd)
+}
+
+// CloseHandle closes a file handle without unlocking
+//
+// WARNING: This is a last-resort function for error handling only!
+// On Unix systems, closing a file descriptor automatically releases any locks,
+// so "closing without unlocking" is impossible. This function will release
+// the lock as a side effect of closing the file.
+//
+// This function should only be used in error paths where the lock state
+// is already corrupted or when giving up on lock management entirely.
+// Normal code should use UnlockAndCloseHandle instead.
+func CloseHandle(fd FileHandle) {
+	closeHandle(fd)
+}

internal/rawfilelock/rawfilelock_test.go

@@ -0,0 +1,86 @@
+package rawfilelock
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestOpenLock(t *testing.T) {
+	t.Parallel()
+
+	for _, tc := range []struct {
+		name    string
+		prepare func() (path string, readOnly bool)
+	}{
+		{
+			name: "file exists (read/write)",
+			prepare: func() (string, bool) {
+				tempFile, err := os.CreateTemp(t.TempDir(), "lock-")
+				require.NoError(t, err)
+				tempFile.Close()
+				return tempFile.Name(), false
+			},
+		},
+		{
+			name: "file exists readonly (readonly)",
+			prepare: func() (string, bool) {
+				tempFile, err := os.CreateTemp(t.TempDir(), "lock-")
+				require.NoError(t, err)
+				tempFile.Close()
+				return tempFile.Name(), true
+			},
+		},
+		{
+			name: "base dir exists (read/write)",
+			prepare: func() (string, bool) {
+				tempDir := os.TempDir()
+				require.DirExists(t, tempDir)
+				return filepath.Join(tempDir, "test-1.lock"), false
+			},
+		},
+	} {
+		path, readOnly := tc.prepare()
+
+		fd, err := OpenLock(path, readOnly)
+		require.NoError(t, err, tc.name)
+		UnlockAndCloseHandle(fd)
+
+		fd, err = OpenLock(path, readOnly)
+		require.NoError(t, err)
+		UnlockAndCloseHandle(fd)
+
+		require.Nil(t, os.RemoveAll(path))
+	}
+}
+
+func TestOpenLockNotCreateParentDir(t *testing.T) {
+	tmpDir := t.TempDir()
+	lockPath := filepath.Join(tmpDir, "lockfile")
+	fd, err := OpenLock(lockPath, false)
+	require.NoError(t, err)
+	UnlockAndCloseHandle(fd)
+
+	lockPath = filepath.Join(tmpDir, "subdir", "lockfile")
+	_, err = OpenLock(lockPath, false)
+	require.Error(t, err)
+}
+
+func TestTryLockFileAndLockFile(t *testing.T) {
+	tmpFile, err := os.CreateTemp(t.TempDir(), "lockfile")
+	require.NoError(t, err)
+	defer os.Remove(tmpFile.Name())
+	fd, err := OpenLock(tmpFile.Name(), false)
+	require.NoError(t, err)
+
+	require.NoError(t, TryLockFile(fd, WriteLock))
+	UnlockAndCloseHandle(fd)
+
+	fd2, err := OpenLock(tmpFile.Name(), false)
+	require.NoError(t, err)
+
+	require.NoError(t, LockFile(fd2, WriteLock))
+	UnlockAndCloseHandle(fd2)
+}

internal/rawfilelock/rawfilelock_unix.go

@@ -1,6 +1,6 @@
 //go:build !windows
 
-package staging_lockfile
+package rawfilelock
 
 import (
 	"time"
@@ -16,9 +16,9 @@ func openHandle(path string, mode int) (fileHandle, error) {
 	return fileHandle(fd), err
 }
 
-func lockHandle(fd fileHandle, lType lockType, nonblocking bool) error {
+func lockHandle(fd fileHandle, lType LockType, nonblocking bool) error {
 	fType := unix.F_RDLCK
-	if lType != readLock {
+	if lType != ReadLock {
 		fType = unix.F_WRLCK
 	}
 	lk := unix.Flock_t{

internal/rawfilelock/rawfilelock_windows.go

@@ -1,6 +1,6 @@
 //go:build windows
 
-package staging_lockfile
+package rawfilelock
 
 import (
 	"golang.org/x/sys/windows"
@@ -19,9 +19,9 @@ func openHandle(path string, mode int) (fileHandle, error) {
 	return fileHandle(fd), err
 }
 
-func lockHandle(fd fileHandle, lType lockType, nonblocking bool) error {
+func lockHandle(fd fileHandle, lType LockType, nonblocking bool) error {
 	flags := 0
-	if lType != readLock {
+	if lType != ReadLock {
 		flags = windows.LOCKFILE_EXCLUSIVE_LOCK
 	}
 	if nonblocking {

internal/staging_lockfile/staging_lockfile.go

@@ -2,16 +2,10 @@ package staging_lockfile
 
 import (
 	"fmt"
-	"os"
 	"path/filepath"
 	"sync"
-)
 
-type lockType byte
+	"github.com/containers/storage/internal/rawfilelock"
-
-const (
-	readLock lockType = iota
-	writeLock
 )
 
 // StagingLockFile represents a file lock used to coordinate access to staging areas.
@@ -30,7 +24,7 @@ type StagingLockFile struct {
 	// stateMutex is used to synchronize concurrent accesses to the state below
 	stateMutex *sync.Mutex
 	locked     bool
-	fd         fileHandle
+	fd         rawfilelock.FileHandle
 }
 
 var (
@@ -86,11 +80,11 @@ func getLockfile(path string) (*StagingLockFile, error) {
 // This function will be called at most once for each unique path within a process.
 func createStagingLockFileForPath(path string) (*StagingLockFile, error) {
 	// Check if we can open the lock.
-	fd, err := openLock(path)
+	fd, err := rawfilelock.OpenLock(path, false)
 	if err != nil {
 		return nil, err
 	}
-	unlockAndCloseHandle(fd)
+	rawfilelock.UnlockAndCloseHandle(fd)
 
 	return &StagingLockFile{
 		file:       path,
@@ -100,30 +94,6 @@ func createStagingLockFileForPath(path string) (*StagingLockFile, error) {
 	}, nil
 }
 
-// openLock opens the file at the specified path with read-write access.
-// It creates the file and any necessary parent directories if they don't exist.
-// Returns a file handle that can be used for locking.
-func openLock(path string) (fd fileHandle, err error) {
-	flags := os.O_CREATE
-	flags |= os.O_RDWR
-
-	fd, err = openHandle(path, flags)
-	if err == nil {
-		return fd, nil
-	}
-
-	// the directory of the lockfile seems to be removed, try to create it
-	if os.IsNotExist(err) {
-		if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil {
-			return fd, fmt.Errorf("creating lock file directory: %w", err)
-		}
-
-		return openLock(path)
-	}
-
-	return fd, &os.PathError{Op: "open", Path: path, Err: err}
-}
-
 // tryLock attempts to acquire an exclusive lock on the StagingLockFile without blocking.
 // It first tries to acquire the internal rwMutex, then opens and tries to lock the file.
 // Returns nil on success or an error if any step fails.
@@ -136,15 +106,15 @@ func (l *StagingLockFile) tryLock() error {
 	}
 	l.stateMutex.Lock()
 	defer l.stateMutex.Unlock()
-	fd, err := openLock(l.file)
+	fd, err := rawfilelock.OpenLock(l.file, false)
 	if err != nil {
 		rwMutexUnlocker()
 		return err
 	}
 	l.fd = fd
 
-	if err = lockHandle(l.fd, writeLock, true); err != nil {
+	if err = rawfilelock.TryLockFile(l.fd, rawfilelock.WriteLock); err != nil {
-		closeHandle(fd)
+		rawfilelock.CloseHandle(fd)
 		rwMutexUnlocker()
 		return err
 	}

internal/staging_lockfile/staging_lockfile_test.go

@@ -4,7 +4,6 @@ import (
 	"io"
 	"os"
 	"os/exec"
-	"path/filepath"
 	"runtime"
 	"sync"
 	"sync/atomic"
@@ -229,50 +228,3 @@ func TestLockfileMultiProcess(t *testing.T) {
 	wg.Wait()
 	assert.True(t, whighest == 1, "expected to have no more than one writer lock active at a time, had %d", whighest)
 }
-
-func TestOpenLock(t *testing.T) {
-	t.Parallel()
-
-	for _, tc := range []struct {
-		name    string
-		prepare func() (path string)
-	}{
-		{
-			name: "file exists (read/write)",
-			prepare: func() string {
-				tempFile, err := os.CreateTemp("", "lock-")
-				require.NoError(t, err)
-				tempFile.Close()
-				return tempFile.Name()
-			},
-		},
-		{
-			name: "base dir exists (read/write)",
-			prepare: func() string {
-				tempDir := os.TempDir()
-				require.DirExists(t, tempDir)
-				return filepath.Join(tempDir, "test-1.lock")
-			},
-		},
-		{
-			name: "base dir not exists (read/write)",
-			prepare: func() string {
-				tempDir, err := os.MkdirTemp("", "lock-")
-				require.NoError(t, err)
-				return filepath.Join(tempDir, "subdir", "test-1.lock")
-			},
-		},
-	} {
-		path := tc.prepare()
-
-		fd, err := openLock(path)
-		require.NoError(t, err, tc.name)
-		unlockAndCloseHandle(fd)
-
-		fd, err = openLock(path)
-		require.NoError(t, err)
-		unlockAndCloseHandle(fd)
-
-		require.Nil(t, os.RemoveAll(path))
-	}
-}

pkg/lockfile/lockfile.go

@@ -6,6 +6,8 @@ import (
 	"path/filepath"
 	"sync"
 	"time"
+
+	"github.com/containers/storage/internal/rawfilelock"
 )
 
 // A Locker represents a file lock where the file is used to cache an
@@ -55,13 +57,6 @@ type Locker interface {
 	AssertLockedForWriting()
 }
 
-type lockType byte
-
-const (
-	readLock lockType = iota
-	writeLock
-)
-
 // LockFile represents a file lock where the file is used to cache an
 // identifier of the last party that made changes to whatever's being protected
 // by the lock.
@@ -79,12 +74,12 @@ type LockFile struct {
 	stateMutex *sync.Mutex
 	counter    int64
 	lw         LastWrite // A global value valid as of the last .Touch() or .Modified()
-	lockType   lockType
+	lockType   rawfilelock.LockType
 	locked     bool
 	// The following fields are only modified on transitions between counter == 0 / counter != 0.
 	// Thus, they can be safely accessed by users _that currently hold the LockFile_ without locking.
 	// In other cases, they need to be protected using stateMutex.
-	fd fileHandle
+	fd rawfilelock.FileHandle
 }
 
 var (
@@ -129,12 +124,12 @@ func (l *LockFile) Lock() {
 	if l.ro {
 		panic("can't take write lock on read-only lock file")
 	}
-	l.lock(writeLock)
+	l.lock(rawfilelock.WriteLock)
 }
 
 // RLock locks the lockfile as a reader.
 func (l *LockFile) RLock() {
-	l.lock(readLock)
+	l.lock(rawfilelock.ReadLock)
 }
 
 // TryLock attempts to lock the lockfile as a writer.  Panic if the lock is a read-only one.
@@ -142,12 +137,12 @@ func (l *LockFile) TryLock() error {
 	if l.ro {
 		panic("can't take write lock on read-only lock file")
 	}
-	return l.tryLock(writeLock)
+	return l.tryLock(rawfilelock.WriteLock)
 }
 
 // TryRLock attempts to lock the lockfile as a reader.
 func (l *LockFile) TryRLock() error {
-	return l.tryLock(readLock)
+	return l.tryLock(rawfilelock.ReadLock)
 }
 
 // Unlock unlocks the lockfile.
@@ -172,9 +167,9 @@ func (l *LockFile) Unlock() {
 		l.locked = false
 		// Close the file descriptor on the last unlock, releasing the
 		// file lock.
-		unlockAndCloseHandle(l.fd)
+		rawfilelock.UnlockAndCloseHandle(l.fd)
 	}
-	if l.lockType == readLock {
+	if l.lockType == rawfilelock.ReadLock {
 		l.rwMutex.RUnlock()
 	} else {
 		l.rwMutex.Unlock()
@@ -206,7 +201,7 @@ func (l *LockFile) AssertLockedForWriting() {
 
 	l.AssertLocked()
 	// Like AssertLocked, don’t even bother with l.stateMutex.
-	if l.lockType == readLock {
+	if l.lockType == rawfilelock.ReadLock {
 		panic("internal error: lock is not held for writing")
 	}
 }
@@ -273,7 +268,7 @@ func (l *LockFile) Touch() error {
 		return err
 	}
 	l.stateMutex.Lock()
-	if !l.locked || (l.lockType == readLock) {
+	if !l.locked || (l.lockType == rawfilelock.ReadLock) {
 		panic("attempted to update last-writer in lockfile without the write lock")
 	}
 	defer l.stateMutex.Unlock()
@@ -324,6 +319,24 @@ func getLockfile(path string, ro bool) (*LockFile, error) {
 	return lockFile, nil
 }
 
+// openLock opens a lock file at the specified path, creating the parent directory if it does not exist.
+func openLock(path string, readOnly bool) (rawfilelock.FileHandle, error) {
+	fd, err := rawfilelock.OpenLock(path, readOnly)
+	if err == nil {
+		return fd, nil
+	}
+
+	// the directory of the lockfile seems to be removed, try to create it
+	if os.IsNotExist(err) {
+		if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil {
+			return fd, fmt.Errorf("creating lock file directory: %w", err)
+		}
+
+		return openLock(path, readOnly)
+	}
+	return fd, &os.PathError{Op: "open", Path: path, Err: err}
+}
+
 // createLockFileForPath returns new *LockFile object, possibly (depending on the platform)
 // working inter-process and associated with the specified path.
 //
@@ -343,11 +356,11 @@ func createLockFileForPath(path string, ro bool) (*LockFile, error) {
 	if err != nil {
 		return nil, err
 	}
-	unlockAndCloseHandle(fd)
+	rawfilelock.UnlockAndCloseHandle(fd)
 
-	lType := writeLock
+	lType := rawfilelock.WriteLock
 	if ro {
-		lType = readLock
+		lType = rawfilelock.ReadLock
 	}
 
 	return &LockFile{
@@ -362,40 +375,10 @@ func createLockFileForPath(path string, ro bool) (*LockFile, error) {
 	}, nil
 }
 
-// openLock opens the file at path and returns the corresponding file
-// descriptor. The path is opened either read-only or read-write,
-// depending on the value of ro argument.
-//
-// openLock will create the file and its parent directories,
-// if necessary.
-func openLock(path string, ro bool) (fd fileHandle, err error) {
-	flags := os.O_CREATE
-	if ro {
-		flags |= os.O_RDONLY
-	} else {
-		flags |= os.O_RDWR
-	}
-	fd, err = openHandle(path, flags)
-	if err == nil {
-		return fd, nil
-	}
-
-	// the directory of the lockfile seems to be removed, try to create it
-	if os.IsNotExist(err) {
-		if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil {
-			return fd, fmt.Errorf("creating lock file directory: %w", err)
-		}
-
-		return openLock(path, ro)
-	}
-
-	return fd, &os.PathError{Op: "open", Path: path, Err: err}
-}
-
 // lock locks the lockfile via syscall based on the specified type and
 // command.
-func (l *LockFile) lock(lType lockType) {
+func (l *LockFile) lock(lType rawfilelock.LockType) {
-	if lType == readLock {
+	if lType == rawfilelock.ReadLock {
 		l.rwMutex.RLock()
 	} else {
 		l.rwMutex.Lock()
@@ -413,7 +396,7 @@ func (l *LockFile) lock(lType lockType) {
 		// Optimization: only use the (expensive) syscall when
 		// the counter is 0.  In this case, we're either the first
 		// reader lock or a writer lock.
-		if err := lockHandle(l.fd, lType, false); err != nil {
+		if err := rawfilelock.LockFile(l.fd, lType); err != nil {
 			panic(err)
 		}
 	}
@@ -424,10 +407,10 @@ func (l *LockFile) lock(lType lockType) {
 
 // lock locks the lockfile via syscall based on the specified type and
 // command.
-func (l *LockFile) tryLock(lType lockType) error {
+func (l *LockFile) tryLock(lType rawfilelock.LockType) error {
 	var success bool
 	var rwMutexUnlocker func()
-	if lType == readLock {
+	if lType == rawfilelock.ReadLock {
 		success = l.rwMutex.TryRLock()
 		rwMutexUnlocker = l.rwMutex.RUnlock
 	} else {
@@ -451,8 +434,8 @@ func (l *LockFile) tryLock(lType lockType) error {
 		// Optimization: only use the (expensive) syscall when
 		// the counter is 0.  In this case, we're either the first
 		// reader lock or a writer lock.
-		if err = lockHandle(l.fd, lType, true); err != nil {
+		if err = rawfilelock.TryLockFile(l.fd, lType); err != nil {
-			closeHandle(fd)
+			rawfilelock.CloseHandle(fd)
 			rwMutexUnlocker()
 			return err
 		}

pkg/lockfile/lockfile_test.go

@@ -4,7 +4,6 @@ import (
 	"io"
 	"os"
 	"os/exec"
-	"path/filepath"
 	"runtime"
 	"sync"
 	"sync/atomic"
@@ -844,59 +843,3 @@ func TestLockfileMultiprocessModifiedSince(t *testing.T) {
 	require.NoError(t, err)
 	assert.False(t, modified)
 }
-
-func TestOpenLock(t *testing.T) {
-	t.Parallel()
-
-	for _, tc := range []struct {
-		name    string
-		prepare func() (path string, readOnly bool)
-	}{
-		{
-			name: "file exists (read/write)",
-			prepare: func() (string, bool) {
-				tempFile, err := os.CreateTemp("", "lock-")
-				require.NoError(t, err)
-				tempFile.Close()
-				return tempFile.Name(), false
-			},
-		},
-		{
-			name: "file exists readonly (readonly)",
-			prepare: func() (string, bool) {
-				tempFile, err := os.CreateTemp("", "lock-")
-				require.NoError(t, err)
-				tempFile.Close()
-				return tempFile.Name(), true
-			},
-		},
-		{
-			name: "base dir exists (read/write)",
-			prepare: func() (string, bool) {
-				tempDir := os.TempDir()
-				require.DirExists(t, tempDir)
-				return filepath.Join(tempDir, "test-1.lock"), false
-			},
-		},
-		{
-			name: "base dir not exists (read/write)",
-			prepare: func() (string, bool) {
-				tempDir, err := os.MkdirTemp("", "lock-")
-				require.NoError(t, err)
-				return filepath.Join(tempDir, "subdir", "test-1.lock"), false
-			},
-		},
-	} {
-		path, readOnly := tc.prepare()
-
-		fd, err := openLock(path, readOnly)
-		require.NoError(t, err, tc.name)
-		unlockAndCloseHandle(fd)
-
-		fd, err = openLock(path, readOnly)
-		require.NoError(t, err)
-		unlockAndCloseHandle(fd)
-
-		require.Nil(t, os.RemoveAll(path))
-	}
-}

pkg/lockfile/lockfile_unix.go

@@ -9,8 +9,6 @@ import (
 	"golang.org/x/sys/unix"
 )
 
-type fileHandle uintptr
-
 // GetLastWrite returns a LastWrite value corresponding to current state of the lock.
 // This is typically called before (_not after_) loading the state when initializing a consumer
 // of the data protected by the lock.
@@ -66,41 +64,3 @@ func (l *LockFile) TouchedSince(when time.Time) bool {
 	touched := time.Unix(mtim.Unix())
 	return when.Before(touched)
 }
-
-func openHandle(path string, mode int) (fileHandle, error) {
-	mode |= unix.O_CLOEXEC
-	fd, err := unix.Open(path, mode, 0o644)
-	return fileHandle(fd), err
-}
-
-func lockHandle(fd fileHandle, lType lockType, nonblocking bool) error {
-	fType := unix.F_RDLCK
-	if lType != readLock {
-		fType = unix.F_WRLCK
-	}
-	lk := unix.Flock_t{
-		Type:   int16(fType),
-		Whence: int16(unix.SEEK_SET),
-		Start:  0,
-		Len:    0,
-	}
-	cmd := unix.F_SETLKW
-	if nonblocking {
-		cmd = unix.F_SETLK
-	}
-	for {
-		err := unix.FcntlFlock(uintptr(fd), cmd, &lk)
-		if err == nil || nonblocking {
-			return err
-		}
-		time.Sleep(10 * time.Millisecond)
-	}
-}
-
-func unlockAndCloseHandle(fd fileHandle) {
-	unix.Close(int(fd))
-}
-
-func closeHandle(fd fileHandle) {
-	unix.Close(int(fd))
-}

pkg/lockfile/lockfile_windows.go

@@ -14,8 +14,6 @@ const (
 	allBytes = ^uint32(0)
 )
 
-type fileHandle windows.Handle
-
 // GetLastWrite returns a LastWrite value corresponding to current state of the lock.
 // This is typically called before (_not after_) loading the state when initializing a consumer
 // of the data protected by the lock.
@@ -73,37 +71,3 @@ func (l *LockFile) TouchedSince(when time.Time) bool {
 	}
 	return when.Before(stat.ModTime())
 }
-
-func openHandle(path string, mode int) (fileHandle, error) {
-	mode |= windows.O_CLOEXEC
-	fd, err := windows.Open(path, mode, windows.S_IWRITE)
-	return fileHandle(fd), err
-}
-
-func lockHandle(fd fileHandle, lType lockType, nonblocking bool) error {
-	flags := 0
-	if lType != readLock {
-		flags = windows.LOCKFILE_EXCLUSIVE_LOCK
-	}
-	if nonblocking {
-		flags |= windows.LOCKFILE_FAIL_IMMEDIATELY
-	}
-	ol := new(windows.Overlapped)
-	if err := windows.LockFileEx(windows.Handle(fd), uint32(flags), reserved, allBytes, allBytes, ol); err != nil {
-		if nonblocking {
-			return err
-		}
-		panic(err)
-	}
-	return nil
-}
-
-func unlockAndCloseHandle(fd fileHandle) {
-	ol := new(windows.Overlapped)
-	windows.UnlockFileEx(windows.Handle(fd), reserved, allBytes, allBytes, ol)
-	closeHandle(fd)
-}
-
-func closeHandle(fd fileHandle) {
-	windows.Close(windows.Handle(fd))
-}