utils: rm_offset_get_from_fd: Break early if not contiguous

Completing the current loop iteration if the current extent is not
contiguous with the last means the extent flags and file offset are
updated as if the extents *were* contiguous. In general, this causes
calling code to skip every second extent!

Also remove a redundant write to file_offset_next that is always
clobbered after the loop exits.

Fixes sahib#527

lib/utilities.c

@@ -1059,15 +1059,11 @@ RmOff rm_offset_get_from_fd(int fd, RmOff file_offset, RmOff *file_offset_next,
                 /* check if subsequent extents are contiguous */
                 if(fm_ext.fe_physical != expected)  {
                     /* current extent is not contiguous with previous, so we can stop */
-                    done = TRUE;
+                    g_free(fm);
+                    break;
                 }
             }
 
-            if (!done && file_offset_next != NULL) {
-                /* update logical offset of next fragment */
-                *file_offset_next = fm_ext.fe_logical + fm_ext.fe_length;
-            }
-
             if(fm_ext.fe_flags & FIEMAP_EXTENT_LAST) {
                 done = TRUE;
 

tests/test_mains/test_dedupe.py

@@ -2,68 +2,10 @@
 # encoding: utf-8
 
 from nose import with_setup
-from nose.tools import make_decorator
-from nose.plugins.skip import SkipTest
-from contextlib import contextmanager
-import psutil
 import re
 
 from tests.utils import *
 
-REFLINK_CAPABLE_FILESYSTEMS = {'btrfs', 'xfs', 'ocfs2'}
-
-@contextmanager
-def assert_exit_code(status_code):
-    """
-    Assert that the with block yields a subprocess.CalledProcessError
-    with a certain return code. If nothing is thrown, status_code
-    is required to be 0 to survive the test.
-    """
-    try:
-        yield
-    except subprocess.CalledProcessError as exc:
-        assert exc.returncode == status_code
-    else:
-        # No exception? status_code should be fine.
-        assert status_code == 0
-
-
-def up(path):
-    while path:
-        yield path
-        if path == "/":
-            break
-        path = os.path.dirname(path)
-
-def is_on_reflink_fs(path):
-    parts = psutil.disk_partitions(all=True)
-
-    # iterate up from `path` until mountpoint found
-    for up_path in up(path):
-        for part in parts:
-            if up_path == part.mountpoint:
-                print("{0} is {1} mounted at {2}".format(path, part.fstype, part.mountpoint))
-                return (part.fstype in REFLINK_CAPABLE_FILESYSTEMS)
-
-    print("No mountpoint found for {}".format(path))
-    return False
-
-
-# decorator for tests dependent on reflink-capable testdir
-def needs_reflink_fs(test):
-    def no_support(*args):
-        raise SkipTest("btrfs not supported")
-
-    def not_reflink_fs(*args):
-        raise SkipTest("testdir is not on reflink-capable filesystem")
-
-    if not has_feature('btrfs-support'):
-        return make_decorator(test)(no_support)
-    elif not is_on_reflink_fs(TESTDIR_NAME):
-        return make_decorator(test)(not_reflink_fs)
-    else:
-        return test
-
 
 @needs_reflink_fs
 @with_setup(usual_setup_func, usual_teardown_func)

tests/test_mains/test_is_reflink.py

@@ -2,27 +2,10 @@
 
 import os
 import subprocess
-from contextlib import contextmanager
 from nose import with_setup
 from tests.utils import *
 
 
-@contextmanager
-def assert_exit_code(status_code):
-    """
-    Assert that the with block yields a subprocess.CalledProcessError
-    with a certain return code. If nothing is thrown, status_code
-    is required to be 0 to survive the test.
-    """
-    try:
-        yield
-    except subprocess.CalledProcessError as exc:
-        assert exc.returncode == status_code
-    else:
-        # No exception? status_code should be fine.
-        assert status_code == 0
-
-
 def check_is_reflink_status(status_code, *paths):
     with assert_exit_code(status_code):
         run_rmlint_once(
@@ -104,3 +87,36 @@ def test_symlink():
         pass  # expected failure
     else:
         raise AssertionError('test was epxected to fail')
+
+
+def _run_dd_urandom(outfile, blocksize, count, extra=''):
+    fmt = 'dd status=none oflag=sync bs={bs} count={c} {e} if=/dev/urandom'
+    subprocess.run(
+        [*fmt.format(bs=blocksize, c=count, e=extra).split(), 'of=' + outfile],
+        check=True,
+    )
+
+
+def _make_reflink_testcase(extents, hole_extents=None, break_link=False):
+    path_a = os.path.join(TESTDIR_NAME, 'a')
+    path_b = os.path.join(TESTDIR_NAME, 'b')
+
+    _run_dd_urandom(path_a, '4K', extents)
+
+    if hole_extents is not None:
+        os.truncate(path_a, (extents + hole_extents) * 4 * 1024)
+
+    subprocess.run(['cp', '--reflink', path_a, path_b], check=True)
+
+    if break_link:
+        _run_dd_urandom(path_b, '4K', 1, 'seek=1 conv=notrunc')
+
+    # expect RM_LINK_NONE or RM_LINK_REFLINK
+    check_is_reflink_status(1 if break_link else 0, path_a, path_b)
+
+
+# GitHub issue #527: Make sure rmlint does not skip every other extent.
+@needs_reflink_fs
+@with_setup(usual_setup_func, usual_teardown_func)
+def test_second_extent_differs():
+    _make_reflink_testcase(extents=5, break_link=True)

tests/test_options/test_equal.py

@@ -9,22 +9,6 @@
 from tests.utils import *
 
 
-@contextmanager
-def assert_exit_code(status_code):
-    """
-    Assert that the with block yields a subprocess.CalledProcessError
-    with a certain return code. If nothing is thrown, status_code
-    is required to be 0 to survive the test.
-    """
-    try:
-        yield
-    except subprocess.CalledProcessError as exc:
-        assert exc.returncode == status_code
-    else:
-        # No exception? status_code should be fine.
-        assert status_code == 0
-
-
 @with_setup(usual_setup_func, usual_teardown_func)
 def test_equal_files():
     path_a = create_file('1234', 'a')

tests/utils.py

@@ -9,12 +9,15 @@
 import json
 import time
 import pprint
+import psutil
 import shutil
 import shlex
 import struct
 import subprocess
 import contextlib
 import xattr
+from nose.plugins.skip import SkipTest
+from nose.tools import make_decorator
 
 
 TESTDIR_NAME = os.getenv('RM_TS_DIR') or '/tmp/rmlint-unit-testdir'
@@ -43,6 +46,9 @@
     'paranoid',
 ]
 
+_REFLINK_CAPABLE_FILESYSTEMS = {'btrfs', 'xfs', 'ocfs2'}
+
+
 def runs_as_root():
     return os.geteuid() == 0
 
@@ -402,3 +408,57 @@ def must_read_xattr(path):
           See create_special_fs for a workaround.
     """
     return dict(xattr.xattr(os.path.join(TESTDIR_NAME, path)).items())
+
+
+@contextlib.contextmanager
+def assert_exit_code(status_code):
+    """
+    Assert that the with block yields a subprocess.CalledProcessError
+    with a certain return code. If nothing is thrown, status_code
+    is required to be 0 to survive the test.
+    """
+    try:
+        yield
+    except subprocess.CalledProcessError as exc:
+        assert exc.returncode == status_code
+    else:
+        # No exception? status_code should be fine.
+        assert status_code == 0
+
+
+def _up(path):
+    while path:
+        yield path
+        if path == "/":
+            break
+        path = os.path.dirname(path)
+
+
+def is_on_reflink_fs(path):
+    parts = psutil.disk_partitions(all=True)
+
+    # iterate up from `path` until mountpoint found
+    for up_path in _up(path):
+        for part in parts:
+            if up_path == part.mountpoint:
+                print("{0} is {1} mounted at {2}".format(path, part.fstype, part.mountpoint))
+                return (part.fstype in _REFLINK_CAPABLE_FILESYSTEMS)
+
+    print("No mountpoint found for {}".format(path))
+    return False
+
+
+# decorator for tests dependent on reflink-capable testdir
+def needs_reflink_fs(test):
+    def no_support(*args):
+        raise SkipTest("btrfs not supported")
+
+    def not_reflink_fs(*args):
+        raise SkipTest("testdir is not on reflink-capable filesystem")
+
+    if not has_feature('btrfs-support'):
+        return make_decorator(test)(no_support)
+    elif not is_on_reflink_fs(TESTDIR_NAME):
+        return make_decorator(test)(not_reflink_fs)
+    else:
+        return test