On Apr 22, 2020, at 11:54 AM, Artem Blagodarenko <artem.blagodarenko@xxxxxxxxx> wrote:
> Subject: LUS-1922 e2image: add option to ignore fs errors
Probably "LUS-1922" shouldn't be in the patch description, only linked
via "Cray-bug-id: LUS-1922" below.
> From: Alexey Lyashkov <alexey.lyashkov@xxxxxxx>
>
> Add extended "-E ignore_error" option to be more tolerant
> to fs errors while scanning inode extents.
>
> Signed-off-by: Alexey Lyashkov <alexey.lyashkov@xxxxxxx>
> Signed-off-by: Artem Blagodarenko <artem.blagodarenko@xxxxxxx>
Mostly OK. I think the commit message change can be done by Ted at
landing time, and my other comments are mostly style issues that
Ted may have other opinions about. So you can add:
Reviewed-by: Andreas Dilger <adilger@xxxxxxxxx>
if the patch lands as-is or if we decide to fix those issues.
> Cray-bug-id: LUS-1922
> Change-Id: Ib79300656726839b1d3b7ee1dd0793c60679d296
>
> diff --git a/lib/support/mvstring.c b/lib/support/mvstring.c
> new file mode 100644
> index 00000000..1ed2fd67
> --- /dev/null
> +++ b/lib/support/mvstring.c
> +char *string_copy(const char *s)
> +{
> + char *ret;
> +
> + if (!s)
> + return 0;
> + ret = malloc(strlen(s)+1);
> + if (ret)
> + strcpy(ret, s);
> + return ret;
> +}
Why not use "strdup()" for this? It isn't really a problem with
this patch, since it was in e2initrd_helper.c previously and just
moved into the helper library, but seems strange. The strdup()
function has existed for a very long time already, so there should
not be any compatibility issues, but Ted added a patch using this
function only a year ago, so maybe I'm missing something? It dates
back to:
2001-01-05 Use string_copy() instead of strdup() for portability's sake
It would probably make sense to remove the duplicate copies that
still exist in e2fsck/util.c and misc/fsck.c, and add a comment
why it is better than strdup()?
> diff --git a/misc/e2initrd_helper.c b/misc/e2initrd_helper.c
> index 436aab8c..ab5991a4 100644
> --- a/misc/e2initrd_helper.c
> +++ b/misc/e2initrd_helper.c
> @@ -151,21 +152,6 @@ static int mem_file_eof(struct mem_file *file)
> return (file->ptr >= file->size);
> }
>
> -/*
> - * fstab parsing code
> - */
> -static char *string_copy(const char *s)
> -{
> - char *ret;
> -
> - if (!s)
> - return 0;
> - ret = malloc(strlen(s)+1);
> - if (ret)
> - strcpy(ret, s);
> - return ret;
> -}
> -
>
> diff --git a/tests/i_error_tolerance/script b/tests/i_error_tolerance/script
> new file mode 100644
> index 00000000..9cdec475
> --- /dev/null
> +++ b/tests/i_error_tolerance/script
> @@ -0,0 +1,38 @@
> +if test -x $E2IMAGE_EXE; then
> +if test -x $DEBUGFS_EXE; then
Having the nested "if" blocks is confusing at the end. I was
wondering how "else #if test -x ..." was doing anything, or
why there were two seemingly-duplicate "else" blocks.
This should just check for both files at the same time, and
exit early if they are not found, like:
if ! test -x $E2IMAGE_EXE || ! test -x $DEBUGFS_EXE; then
echo "$test_name: $test_description: skipped"
return 0
fi
or maybe:
if ! test -x $E2IMAGE_EXE; then
echo "$test_name: $test_description: skipped (no e2image)"
return 0
fi
if ! test -x $DEBUGFS_EXE; then
echo "$test_name: $test_description: skipped (no debugfs)"
return 0
fi
> +
> +SKIP_GUNZIP="true"
> +
> +TEST_DATA="$test_name.tmp"
> +dd if=/dev/urandom of=$TEST_DATA bs=1k count=16 > /dev/null 2>&1
> +
> +dd if=/dev/zero of=$TMPFILE bs=1k count=100 > /dev/null 2>&1
> +$MKE2FS -Ft ext4 -O ^extents $TMPFILE > /dev/null 2>&1
> +$DEBUGFS -w $TMPFILE << EOF > /dev/null 2>&1
> +write $TEST_DATA testfile
> +set_inode_field testfile block[IND] 1000000
> +q
> +EOF
> +
> +$E2IMAGE -r $TMPFILE $TMPFILE.back
> +
> +ls -l $TMPFILE.back
In this case, it isn't clear whether there should be an error
or not (e.g. if "ignore_error" was the default), so I don't
think it should be checked, but...
> +$E2IMAGE -r -E ignore_error $TMPFILE $TMPFILE.back
> +
> +ls -l $TMPFILE.back
... should this return an error if $TMPFILE.back doesn't exist?
> +
> +mv $TMPFILE.back $TMPFILE
> +
> +. $cmd_dir/run_e2fsck
> +
> +rm -f $TEST_DATA
> +
> +unset E2FSCK_TIME TEST_DATA
> +
> +else #if test -x $DEBUGFS_EXE; then
> + echo "$test_name: $test_description: skipped"
> +fi
> +else #if test -x $E2IMAGE_EXE; then
> + echo "$test_name: $test_description: skipped"
> +fi
> --
> 2.21.1 (Apple Git-122.3)
>
Cheers, Andreas
Attachment:
signature.asc
Description: Message signed with OpenPGP
