capi: change *link and rename arguments to match syscall API

CHANGELOG.md

@@ -6,6 +6,30 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 
 ## [Unreleased] ##
 
+### Breaking ###
+* `pathrs_inroot_hardlink` and `pathrs_inroot_symlink` have been switched to
+  using the standard argument order from their respective system calls
+  (previously the order was swapped, which lead to possible confusion).
+  - Previously compiled programs will continue to work (thanks to symbol
+    versioning) but rebuilt programs will need to adjust their argument order.
+  - Rust users are not affected by this change.
+  - For the Go and Python bindings, the wrappers have also had their argument
+    orders swapped to match the C API and so will also need to be updated when
+    rebuilding.
+  - For the sake of future extensions (and to ease the migration),
+    `pathrs_inroot_hardlink` now accepts both an `old_root_fd` and
+    `new_root_fd`. At the moment, callers must pass *the same value* to both
+    arguments (this means the same numeric file descriptor value, not just a
+    reference to the same underlying file).
+* `pathrs_inroot_rename` also now accepts both an `old_root_fd` and
+  `new_root_fd`, with the same caveats as `pathrs_inroot_hardlink` above.
+* `RenameFlags` is now backed by a `u64` (instead of `libc::c_uint`) so that we
+  can accommodate future extension bits beyond the kernel's current 32-bit ABI.
+  Rust callers using `RenameFlags::bits()` or storing the raw value will need
+  to adjust their types.
+  - As part of this, `pathrs_inroot_rename` now also takes a `uint64_t`, and
+    thus the the Go `(*Root).Rename` wrapper takes a `uint64` as well.
+
 ### Added ###
 - capi: We now have a new `pathrs_version` API that provides runtime version
   information, which loosely matches other libraries like `libseccomp`. The API

contrib/bindings/python/pathrs/_libpathrs_cffi/lib.pyi

@@ -90,7 +90,11 @@ def pathrs_inroot_creat(
     rootfd: RawFd, path: CString, flags: int, filemode: int
 ) -> Union[RawFd, ErrorId]: ...
 def pathrs_inroot_rename(
-    rootfd: RawFd, src: CString, dst: CString, flags: int
+    old_rootfd: RawFd,
+    old_path: CString,
+    new_rootfd: RawFd,
+    new_path: CString,
+    flags: int,
 ) -> Union[Literal[0], ErrorId]: ...
 def pathrs_inroot_rmdir(rootfd: RawFd, path: CString) -> Union[Literal[0], ErrorId]: ...
 def pathrs_inroot_unlink(
@@ -107,10 +111,16 @@ def pathrs_inroot_mknod(
     rootfd: RawFd, path: CString, mode: int, dev: int
 ) -> Union[Literal[0], ErrorId]: ...
 def pathrs_inroot_hardlink(
-    rootfd: RawFd, path: CString, target: CString
+    old_rootfd: RawFd,
+    old_path: CString,
+    new_rootfd: RawFd,
+    new_path: CString,
+    flags: int,
 ) -> Union[Literal[0], ErrorId]: ...
 def pathrs_inroot_symlink(
-    rootfd: RawFd, path: CString, target: CString
+    target: CString,
+    rootfd: RawFd,
+    linkpath: CString,
 ) -> Union[Literal[0], ErrorId]: ...
 def pathrs_inroot_readlink(
     rootfd: RawFd, path: CString, linkbuf: CBuffer, linkbuf_size: int

contrib/bindings/python/pathrs/_pathrs.py

@@ -305,7 +305,7 @@ def rename(self, src: str, dst: str, flags: int = 0, /) -> None:
         """
         # TODO: Should we have a separate Root.swap() operation?
         err = libpathrs_so.pathrs_inroot_rename(
-            self.fileno(), _cstr(src), _cstr(dst), flags
+            self.fileno(), _cstr(src), self.fileno(), _cstr(dst), flags
         )
         if _is_pathrs_err(err):
             raise PathrsError._fetch(err) or INTERNAL_ERROR
@@ -398,35 +398,35 @@ def mknod(self, path: str, mode: int, device: int = 0, /) -> None:
         if _is_pathrs_err(err):
             raise PathrsError._fetch(err) or INTERNAL_ERROR
 
-    def hardlink(self, path: str, target: str, /) -> None:
+    def hardlink(self, target: str, linkname: str, /) -> None:
         """
         Create a hardlink between two paths inside the Root.
 
-        path is the path to the *new* hardlink, and target is a path to the
-        *existing* file.
+        linkname is the path to the *new* hardlink, and target is a path to
+        the *existing* file.
 
         A pathrs.Error is raised if the path for the new hardlink already
         exists.
         """
         err = libpathrs_so.pathrs_inroot_hardlink(
-            self.fileno(), _cstr(path), _cstr(target)
+            self.fileno(), _cstr(target), self.fileno(), _cstr(linkname), 0
         )
         if _is_pathrs_err(err):
             raise PathrsError._fetch(err) or INTERNAL_ERROR
 
-    def symlink(self, path: str, target: str, /) -> None:
+    def symlink(self, target: str, linkname: str, /) -> None:
         """
         Create a symlink at the given path in the Root.
 
-        path is the path to the *new* symlink, and target is what the symink
-        will point to. Note that symlinks contents are not verified on Linux,
-        so there are no restrictions on what target you put.
+        linkname is the path to the *new* symlink, and target is what the
+        symlink will point to. Note that symlink contents are not verified on
+        Linux, so there are no restrictions on what target you put.
 
         A pathrs.Error is raised if the path for the new symlink already
         exists.
         """
         err = libpathrs_so.pathrs_inroot_symlink(
-            self.fileno(), _cstr(path), _cstr(target)
+            _cstr(target), self.fileno(), _cstr(linkname)
         )
         if _is_pathrs_err(err):
             raise PathrsError._fetch(err) or INTERNAL_ERROR

e2e-tests/cmd/go/root.go

@@ -314,8 +314,7 @@ var rootHardlinkCmd = &cli.Command{
 		target := cmd.StringArg("target")
 		linkname := cmd.StringArg("linkname")
 
-		// TODO: These arguments need to get swapped.
-		return root.Hardlink(linkname, target)
+		return root.Hardlink(target, linkname)
 	},
 }
 
@@ -336,8 +335,7 @@ var rootSymlinkCmd = &cli.Command{
 		target := cmd.StringArg("target")
 		linkname := cmd.StringArg("linkname")
 
-		// TODO: These arguments need to get swapped.
-		return root.Symlink(linkname, target)
+		return root.Symlink(target, linkname)
 	},
 }
 
@@ -443,7 +441,7 @@ var rootRenameCmd = &cli.Command{
 		src := cmd.StringArg("source")
 		dst := cmd.StringArg("destination")
 
-		var renameArgs uint
+		var renameArgs uint64
 		if !cmd.Bool("clobber") {
 			renameArgs |= unix.RENAME_NOREPLACE
 		}

e2e-tests/cmd/python/pathrs-cmd.py

@@ -104,17 +104,15 @@ def root_hardlink(args: argparse.Namespace):
     target: str = args.target
     linkname: str = args.linkname
 
-    # TODO: These arguments need to get swapped.
-    root.hardlink(linkname, target)
+    root.hardlink(target, linkname)
 
 
 def root_symlink(args: argparse.Namespace):
     root: pathrs.Root = args.root
     target: str = args.target
     linkname: str = args.linkname
 
-    # TODO: These arguments need to get swapped.
-    root.symlink(linkname, target)
+    root.symlink(target, linkname)
 
 
 def root_readlink(args: argparse.Namespace):

go-pathrs/internal/libpathrs/libpathrs_linux.go

@@ -154,14 +154,14 @@ func InRootCreat(rootFd uintptr, path string, flags int, mode uint32) (uintptr,
 }
 
 // InRootRename wraps pathrs_inroot_rename.
-func InRootRename(rootFd uintptr, src, dst string, flags uint) error {
-	cSrc := C.CString(src)
-	defer C.free(unsafe.Pointer(cSrc))
+func InRootRename(oldRootFd uintptr, oldPath string, newRootFd uintptr, newPath string, flags uint64) error {
+	cOldPath := C.CString(oldPath)
+	defer C.free(unsafe.Pointer(cOldPath))
 
-	cDst := C.CString(dst)
-	defer C.free(unsafe.Pointer(cDst))
+	cNewPath := C.CString(newPath)
+	defer C.free(unsafe.Pointer(cNewPath))
 
-	err := C.pathrs_inroot_rename(C.int(rootFd), cSrc, cDst, C.uint(flags))
+	err := C.pathrs_inroot_rename(C.int(oldRootFd), cOldPath, C.int(newRootFd), cNewPath, C.uint64_t(flags))
 	return fetchError(err)
 }
 
@@ -193,26 +193,26 @@ func InRootMknod(rootFd uintptr, path string, mode uint32, dev uint64) error {
 }
 
 // InRootSymlink wraps pathrs_inroot_symlink.
-func InRootSymlink(rootFd uintptr, path, target string) error {
-	cPath := C.CString(path)
-	defer C.free(unsafe.Pointer(cPath))
+func InRootSymlink(target string, rootFd uintptr, linkpath string) error {
+	cLinkpath := C.CString(linkpath)
+	defer C.free(unsafe.Pointer(cLinkpath))
 
 	cTarget := C.CString(target)
 	defer C.free(unsafe.Pointer(cTarget))
 
-	err := C.pathrs_inroot_symlink(C.int(rootFd), cPath, cTarget)
+	err := C.pathrs_inroot_symlink(cTarget, C.int(rootFd), cLinkpath)
 	return fetchError(err)
 }
 
 // InRootHardlink wraps pathrs_inroot_hardlink.
-func InRootHardlink(rootFd uintptr, path, target string) error {
-	cPath := C.CString(path)
-	defer C.free(unsafe.Pointer(cPath))
+func InRootHardlink(oldRootFd uintptr, oldPath string, newRootFd uintptr, newPath string, flags uint64) error {
+	cNewPath := C.CString(newPath)
+	defer C.free(unsafe.Pointer(cNewPath))
 
-	cTarget := C.CString(target)
-	defer C.free(unsafe.Pointer(cTarget))
+	cOldPath := C.CString(oldPath)
+	defer C.free(unsafe.Pointer(cOldPath))
 
-	err := C.pathrs_inroot_hardlink(C.int(rootFd), cPath, cTarget)
+	err := C.pathrs_inroot_hardlink(C.int(oldRootFd), cOldPath, C.int(newRootFd), cNewPath, C.uint64_t(flags))
 	return fetchError(err)
 }
 

go-pathrs/root_linux.go

@@ -155,9 +155,9 @@ func (r *Root) Create(path string, flags int, mode os.FileMode) (*os.File, error
 
 // Rename two paths within a [Root]'s directory tree. The flags argument is
 // identical to the RENAME_* flags to the renameat2(2) system call.
-func (r *Root) Rename(src, dst string, flags uint) error {
+func (r *Root) Rename(src, dst string, flags uint64) error {
 	_, err := fdutils.WithFileFd(r.inner, func(rootFd uintptr) (struct{}, error) {
-		err := libpathrs.InRootRename(rootFd, src, dst, flags)
+		err := libpathrs.InRootRename(rootFd, src, rootFd, dst, flags)
 		return struct{}{}, err
 	})
 	return err
@@ -277,26 +277,26 @@ func (r *Root) Mknod(path string, mode os.FileMode, dev uint64) error {
 }
 
 // Symlink creates a symlink within a [Root]'s directory tree. The symlink is
-// created at path and is a link to target.
+// created at newname and is a link to oldname.
 //
 // This is effectively equivalent to [os.Symlink].
-func (r *Root) Symlink(path, target string) error {
+func (r *Root) Symlink(oldname, newname string) error {
 	_, err := fdutils.WithFileFd(r.inner, func(rootFd uintptr) (struct{}, error) {
-		err := libpathrs.InRootSymlink(rootFd, path, target)
+		err := libpathrs.InRootSymlink(oldname, rootFd, newname)
 		return struct{}{}, err
 	})
 	return err
 }
 
 // Hardlink creates a hardlink within a [Root]'s directory tree. The hardlink
-// is created at path and is a link to target. Both paths are within the
+// is created at newname and is a link to oldname. Both paths are within the
 // [Root]'s directory tree (you cannot hardlink to a different [Root] or the
 // host).
 //
 // This is effectively equivalent to [os.Link].
-func (r *Root) Hardlink(path, target string) error {
+func (r *Root) Hardlink(oldname, newname string) error {
 	_, err := fdutils.WithFileFd(r.inner, func(rootFd uintptr) (struct{}, error) {
-		err := libpathrs.InRootHardlink(rootFd, path, target)
+		err := libpathrs.InRootHardlink(rootFd, oldname, rootFd, newname, 0)
 		return struct{}{}, err
 	})
 	return err

hack/check-elf-symbols.sh

@@ -93,7 +93,13 @@ unversioned="$(awk '!($(NF-1) ~ /^LIBPATHRS_|^\(LIBPATHRS_.*\)/)' <<<"$SYMBOL_OB
 [ -z "$unversioned" ] || {
 	echo "UNVERSIONED SYMBOLS ($(wc -l <<<"$unversioned") total):"
 	echo "$unversioned"
-	bail "$ELF_FILE contains unversioned symbols"
+	# FIXME: lld incorrectly keeps the compatibility shim functions visible and
+	# there doesn't appear to be a way to fix this. For a longer discussion of
+	# these problems, see the upstream discussion -- in particular:
+	# <https://internals.rust-lang.org/t/support-symbol-versioning-with-export-name/23626/10>.
+	if grep -Evwq "__pathrs_.*_v[[:digit:]]+" <<<"$unversioned"; then
+		bail "$ELF_FILE contains non-compat unversioned symbols"
+	fi
 }
 
 echo "++ ALL SYMBOLS ARE VERSIONED! ++"

include/pathrs.h

@@ -367,6 +367,13 @@ int pathrs_inroot_readlink(int root_fd,
  * Rename a path within the rootfs referenced by root_fd. The flags argument is
  * identical to the renameat2(2) flags that are supported on the system.
  *
+ * # Future-proofing
+ *
+ * This function takes two `root_fd` arguments in order for future extensions
+ * to be able to use them, but callers must specify the same *value* for both
+ * arguments. (NOTE: This is not referring to the same underlying file, the
+ * actual file descriptor number must be identical.)
+ *
  * # Return Value
  *
  * On success, this function returns 0.
@@ -376,10 +383,11 @@ int pathrs_inroot_readlink(int root_fd,
  * the system errno(7) value associated with the error, etc), use
  * pathrs_errorinfo().
  */
-int pathrs_inroot_rename(int root_fd,
-                         const char *src,
-                         const char *dst,
-                         uint32_t flags);
+int pathrs_inroot_rename(int old_root_fd,
+                         const char *old_path,
+                         int new_root_fd,
+                         const char *new_path,
+                         uint64_t flags);
 
 /**
  * Remove the empty directory at path within the rootfs referenced by root_fd.
@@ -530,12 +538,23 @@ int pathrs_inroot_mknod(int root_fd,
  * the system errno(7) value associated with the error, etc), use
  * pathrs_errorinfo().
  */
-int pathrs_inroot_symlink(int root_fd, const char *path, const char *target);
+int pathrs_inroot_symlink(const char *target,
+                          int root_fd,
+                          const char *linkpath);
 
 /**
  * Create a hardlink within the rootfs referenced by root_fd. Both the hardlink
  * path and target are resolved within the rootfs.
  *
+ * # Future-proofing
+ *
+ * This function takes two `root_fd` arguments in order for future extensions
+ * to be able to use them, but callers must specify the same *value* for both
+ * arguments. (NOTE: This is not referring to the same underlying file, the
+ * actual file descriptor number must be identical.)
+ *
+ * The `flags` argument is included for future extensions and must be 0.
+ *
  * # Return Value
  *
  * On success, this function returns 0.
@@ -545,7 +564,11 @@ int pathrs_inroot_symlink(int root_fd, const char *path, const char *target);
  * the system errno(7) value associated with the error, etc), use
  * pathrs_errorinfo().
  */
-int pathrs_inroot_hardlink(int root_fd, const char *path, const char *target);
+int pathrs_inroot_hardlink(int old_root_fd,
+                           const char *old_path,
+                           int new_root_fd,
+                           const char *new_path,
+                           uint64_t flags);
 
 /**
  * Create a new (custom) procfs root handle.