bug#6131: [PATCH]: fiemap support for efficient sparse file copy (original) (raw)

[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

From: jeff.liu
Subject: bug#6131: [PATCH]: fiemap support for efficient sparse file copy
Date: Tue, 25 May 2010 16:11:33 +0800
User-agent: Thunderbird 2.0.0.14 (X11/20080505)

Jim Meyering wrote:

jeff.liu wrote: ... > Please ignore above patch, I just found another issue in 'tests/cp/sparse', > the new created test > file also named to 'sparse', so when it running, the cp/sparse' will be_ _> truncated to expr 128 * > 1024 + 1._ _>_ _> Below patch fix it to create a sparse file 'sparse1' instead(I can not find_ _> out a better name for_ _> now), s/-le/=/ to compare the block count._ _>_ _> >From 0669ac6d0497a3c6abfc5d53202afc6bc47d0d07 Mon Sep 17 00:00:00 2001_ _> From: Jie Liu <address@hidden>_ _> Date: Mon, 24 May 2010 17:29:27 +0800_ _> Subject: [PATCH 1/1] cp: enhance the sparse file copy test_ _>_ _> * tests/cp/sparse: fix sparse file name to 'sparse1', improve_ _> the protability using shell constructs._ _>_ _> Signed-off-by: Jie Liu <address@hidden>_ _> ---_ _> tests/cp/sparse | 12 ++++++------_ _> 1 files changed, 6 insertions(+), 6 deletions(-)_ _>_ _> diff --git a/tests/cp/sparse b/tests/cp/sparse_ _> index 73c2924..cab8b9c 100755_ _> --- a/tests/cp/sparse_ _> +++ b/tests/cp/sparse_ _> @@ -28,17 +28,17 @@ requiresparsesupport_ _> # It has to be at least 128K in order to be sparse on some systems._ _> # Make its size one larger than 128K, in order to tickle the_ _> # bug in coreutils-6.0._ _> -size=expr 128 * 1024 + 1` > -dd bs=1 seek=$size of=sparse < /dev/null 2> /dev/null || frameworkfailure > +size=$((128 * 1024 + 1))

Thank you, but I will not use this patch.

First, $((...)) is not portable. Note that while $(...) is an improvement in coreutils tests, in most projects converting ... to $(...) would represent a portability regression. It happens to be acceptable in coreutils tests (and thus preferred by me) because coreutils ensures that tests are run using a shell that is modern enough to accept $(...). Thanks for the clarification.

Second, while I prefer $(...), it's not worth converting them one by one. There are over 500+ uses in tests/.

> +dd bs=1 seek=$size of=sparse1 < /dev/null 2> /dev/null || frameworkfailure

Are you worried about tests running in parallel, and thus this test's "sparse" file colliding with the one by the same name in the fiemap test? No. At first, I took a look at the file 'cp/sparse', I consider it could be truncated by the following DD(1) operation, because the target name also named as 'sparse' which is same as 'cp/sparse', if the test run at the same physical directory(i.e. tests/cp/)

++ expr 128 '*' 1024 + 1

but by checking through the run log file, looks all the tests are safely run at their own sub-dirs, Sorry for my take for granted for this point. +++ /home/jeff/opensource_dev/coreutils/src/mktemp -d --tmp=/home/jeff/opensource_dev/coreutils/tests cu-sparse.XXXXXXXXXX

That's not a problem, since each is run in its own separate subdirectory, via the machinery in test-lib.sh.

> -cp --sparse=always sparse copy || fail=1 > +cp --sparse=always sparse1 copy || fail=1

Thanks, -Jeff

-- With Windows 7, Microsoft is asserting legal control over your computer and is using this power to abuse computer users.