Add GLib/GIO dependency; replace several internal utility functions by andy5995 · Pull Request #529 · theimpossibleastronaut/rmw

andy5995 · 2026-04-28T17:57:21Z

Add glib-2.0 and gio-2.0 as required dependencies (>= 2.50)
Use GLib equivalents for path handling, URL encoding, and directory creation; use FICLONE-based recursive directory move via GIO for moves across btrfs/bcachefs subvolumes
Update CI configs, build-env Dockerfiles, Debian control, and AppImage script to include the new dependency

mlawren

Well, it seems to compile and work, but I don't think I'll be trusting much of my code to AI anytime soon :-)

Anyhow, comments inline. Thanks for responding.

mlawren · 2026-04-28T18:56:24Z

+  if (!dir)
+  {
+    *save_errno = errno;
+    rmdir(dst);


If the src directory was opened before creating dst above, then dst wouldn't have to be rmdir'd here.

mlawren · 2026-04-28T19:15:04Z

-/* returns 0 on success, non-zero on failure.
-   On error sets *out_errno when out_errno != NULL. */
+/* Move src to dst. For regular files uses g_file_move (rename or copy_file_range).
+   For directories, falls back to FICLONE per-file (fails with EXDEV if the


I don't understand the logic of using g_file_move here, when do_fclone_dir uses do_fclone for regular files.

mlawren · 2026-04-28T19:17:08Z

+   Returns 0 on success, -1 on failure; sets errno on failure. */
 int
-safe_mv_via_exec(const char *src, const char *dst, int *out_errno)
+rmw_move(const char *src, const char *dst)


I think this function is only called for FICLONE filesystems? Perhaps that should be reflected in the name.

mlawren · 2026-04-28T19:19:14Z

+  g_object_unref(gsrc);
+  g_object_unref(gdst);
+
+  if (!ok && g_error_matches(error, G_IO_ERROR, G_IO_ERROR_WOULD_RECURSE))


This appear from the block contents, to be branching if src is not a directory. Surely lstat would be a better initial test, before using do_ficlone?

andy5995 · 2026-04-28T19:56:54Z

Well, it seems to compile and work, but I don't think I'll be trusting much of my code to AI anytime soon :-)

@mlawren I understand. That's one reason I have tests and haven't merged this yet. ;) I tested the bcachefs changes using qemu and Fedora image. But I know AI is not favored by a lot of folks, and I appreciate you reviewing anyway. I gave your comments to @claude and tried to address them all in 6d540e7 I'll be testing and reviewing this more myself before its merged.

I use images formatted with btrfs and bcachefs for testing. The tests will skip if they're not present in repo root/src/test. They're downloaded during the CI workflow runs, but none of the runners have bcachefs support so only the btrfs test is run atm.

The images are in my Dropbox at https://www.dropbox.com/scl/fi/57g3ixd3w3tuz4qoc2zp1/rmw-btrfs-test.img?rlkey=yc7krtntswsa1bwz0sbugy4gi&st=hkgrht05&dl=0 and https://www.dropbox.com/scl/fi/key20cj08ca9engqt5kzi/rmw-bcachefs-test.img?rlkey=th6zw1ar7t0dy9m7qzah5wvq9&st=nrwczcqn&dl=0 if you ever feel like trying out the test suite.

mlawren · 2026-04-28T20:05:51Z

          if (waste_curr->dev_num != st_target.dev_num)
          {
-            /* cross-device: different device numbers */
-            if (!S_ISDIR(st_file_arg.st_mode))


ficlone_move already checks for dir or file. Just call it without duplicating that here.

mlawren · 2026-04-28T20:07:08Z

@@ -93,8 +93,8 @@ move_back(const char *src, const char *dest, bool want_dry_run)

    if (S_ISDIR(st_src.st_mode))


Same here - ficlone_move does a directory check, so much of this can be removed.

- Add glib-2.0 and gio-2.0 as required dependencies (>= 2.50) - Use GLib equivalents for path handling, URL encoding, and directory creation; use FICLONE-based recursive directory move via GIO for moves across btrfs/bcachefs subvolumes - Update CI configs, build-env Dockerfiles, Debian control, and AppImage script to include the new dependency Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

- Add bcachefs detection to is_ficlone_fs() so bcachefs waste folders are selected for cross-subvolume moves - Fail with ENOTSUP on special files (FIFOs, sockets, devices) instead of passing them to open() where FIFOs would block indefinitely - Set *save_errno on all early failure paths in do_ficlone() - Preserve timestamps and ownership after successful clone - Fix #else stub: return EXDEV/-1 instead of silent false success Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

@mlawren

…LONE path When confronted by the comments by @mlawren , @claude responded to each of them ;) - Rename rmw_move() to ficlone_move() and move it from utils.c to ficlone.c to reflect that it is only called on FICLONE filesystems - Replace g_file_move() in ficlone_move() with direct FICLONE calls: directories via do_ficlone_dir(), symlinks via readlink/symlink/unlink, other types fail with ENOTSUP - Dispatch on file type before calling do_ficlone() vs ficlone_move() in both the cross-device and same-device EXDEV paths in main.c, making regular files consistently use do_ficlone() at all call sites - Open src directory before creating dst in do_ficlone_dir() to avoid needing to rmdir(dst) on opendir() failure - Drop <gio/gio.h> and <glib.h> from ficlone.c/ficlone.h Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

restore.c: add S_ISLNK branch so top-level symlinks are restored via readlink/symlink/unlink instead of do_ficlone() which would follow the link test_btrfs_clone.sh: - use explicit losetup + mount -t btrfs (like bcachefs test) - add stale mount/loop device cleanup at startup - add trap cleanup EXIT so filesystem is always unmounted on failure - add /proc/filesystems btrfs kernel check - use exit $SKIP for all skip paths - add symlink-in-directory and top-level symlink move+restore tests test_bcachefs.sh: - add trap cleanup EXIT - add deeply nested directory test (parity with btrfs) - add top-level symlink and symlink-in-directory move+restore tests Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Guard sym_link cleanup with [ -L ] before ln -s to avoid silently clobbering unexpected files on test re-runs. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

… safeguard tests - Add stderr messages for previously silent failure paths in do_ficlone_dir: lstat, readlink, symlink, and unlink of src_child all now print the filename and error string. - Add files_moved counter; emit a partial-move warning (naming both src and dst) only when at least one entry was already transferred before the failure, so a clean early failure stays silent. - Factor the two read-only-dir safeguard test cases out of test_btrfs_clone.sh and test_bcachefs.sh into test/test_ficlone_safeguards.sh; each script now sets FS_MOUNTPOINT/FS_RMW_CMD/FS_WASTE_DIR and sources the shared file. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

ficlone_move handles both S_ISDIR and S_ISLNK internally, so the separate branches in restore.c are replaced with a single ficlone_move call for non-regular files. In main.c's cross-device block, the redundant lstat is dropped in favour of st_file_arg (already populated earlier), matching the same-device EXDEV fallback. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

… via ficlone_move do_ficlone and do_ficlone_dir now follow standard C errno semantics -- they save errno locally around close() calls and set errno before each return -1, removing the int *save_errno out-parameter entirely. Both functions are now static (internal to ficlone.c); only is_ficlone_fs and ficlone_move are exported from ficlone.h. ficlone_move gains an S_ISREG branch so it handles all file types. Call sites in main.c and restore.c now call ficlone_move(src, dst) unconditionally and check errno == EXDEV on failure, removing all local save_errno/clone_errno variables and the S_ISREG dispatch. move_back() in restore.c also drops its redundant lstat and st_src. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…FILES Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

- do_ficlone_dir: save errno before closedir() and restore on the failure path, since closedir can clobber it on error. - do_ficlone_dir symlink branch: save errno from unlink(src_child) before the dst_child cleanup unlink, then restore it. - ficlone_move S_ISLNK branch: unlink(dst) and restore errno when unlink(src) fails after symlink(dst) succeeded, preventing an orphaned symlink in the waste folder. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

mlawren

After a cursory look (i.e. I'm not handing out guarantees :) it now seems reasonable.

Works well for me, and for this particular case rmw does a better job than mv :-)

andy5995 added this to the v0.10.0 milestone Apr 28, 2026

andy5995 force-pushed the use-glib branch from 703a616 to d85db0d Compare April 28, 2026 18:12

andy5995 mentioned this pull request Apr 28, 2026

bugfix: fix cross-subvolume rename failure on bcachefs #527

Merged

andy5995 force-pushed the use-glib branch from d85db0d to 3d0a4f5 Compare April 28, 2026 18:25

mlawren suggested changes Apr 28, 2026

View reviewed changes

andy5995 and others added 3 commits April 28, 2026 15:28

andy5995 force-pushed the use-glib branch from 6d540e7 to 51dba4d Compare April 28, 2026 20:36

andy5995 force-pushed the use-glib branch from 51dba4d to 201574b Compare April 28, 2026 20:39

andy5995 and others added 2 commits April 28, 2026 22:57

refactor: defer dst_child construction until after lstat succeeds

05f3a8e

Guard sym_link cleanup with [ -L ] before ln -s to avoid silently clobbering unexpected files on test re-runs. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

andy5995 force-pushed the use-glib branch from 247cb92 to 60f922c Compare April 29, 2026 04:23

andy5995 and others added 4 commits April 28, 2026 23:28

i18n: mark partial-move warning for translation; add ficlone.c to POT…

4105683

…FILES Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

mlawren approved these changes Apr 29, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add GLib/GIO dependency; replace several internal utility functions#529

Add GLib/GIO dependency; replace several internal utility functions#529
andy5995 wants to merge 10 commits intomasterfrom
use-glib

andy5995 commented Apr 28, 2026

Uh oh!

mlawren left a comment

Uh oh!

mlawren Apr 28, 2026

Uh oh!

mlawren Apr 28, 2026

Uh oh!

mlawren Apr 28, 2026

Uh oh!

mlawren Apr 28, 2026

Uh oh!

andy5995 commented Apr 28, 2026

Uh oh!

mlawren Apr 28, 2026

Uh oh!

mlawren Apr 28, 2026

Uh oh!

mlawren left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		@@ -93,8 +93,8 @@ move_back(const char src, const char dest, bool want_dry_run)

		if (S_ISDIR(st_src.st_mode))

Conversation

andy5995 commented Apr 28, 2026

Uh oh!

mlawren left a comment

Choose a reason for hiding this comment

Uh oh!

mlawren Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

mlawren Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

mlawren Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

mlawren Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

andy5995 commented Apr 28, 2026

Uh oh!

mlawren Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

mlawren Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

mlawren left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants