From 0d8ab296d70b092d0eac035dfc72fd3c06b46371 Mon Sep 17 00:00:00 2001 From: Cebtenzzre Date: Wed, 15 Sep 2021 14:22:46 -0400 Subject: [PATCH] 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 #527 --- lib/utilities.c | 8 +--- tests/test_mains/test_dedupe.py | 58 ---------------------------- tests/test_mains/test_is_reflink.py | 50 ++++++++++++++++-------- tests/test_options/test_equal.py | 16 -------- tests/utils.py | 60 +++++++++++++++++++++++++++++ 5 files changed, 95 insertions(+), 97 deletions(-) diff --git a/lib/utilities.c b/lib/utilities.c index 5eeadbf99..4c757f0ca 100644 --- a/lib/utilities.c +++ b/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; diff --git a/tests/test_mains/test_dedupe.py b/tests/test_mains/test_dedupe.py index 3a8e2a94f..c43dd2820 100644 --- a/tests/test_mains/test_dedupe.py +++ b/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) diff --git a/tests/test_mains/test_is_reflink.py b/tests/test_mains/test_is_reflink.py index 5a18bd8e4..077b99a2e 100644 --- a/tests/test_mains/test_is_reflink.py +++ b/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) diff --git a/tests/test_options/test_equal.py b/tests/test_options/test_equal.py index 6b899e06c..38bb739fc 100644 --- a/tests/test_options/test_equal.py +++ b/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') diff --git a/tests/utils.py b/tests/utils.py index 33487f4db..becb2196c 100644 --- a/tests/utils.py +++ b/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