linux-ext4 - Re: [PATCH v2] LUS-1922 e2image: add option to ignore fs errors

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 22 Apr 2020 14:01:16 -0600
From:   Andreas Dilger <adilger@...ger.ca>
To:     Artem Blagodarenko <artem.blagodarenko@...il.com>
Cc:     Ext4 Developers List <linux-ext4@...r.kernel.org>,
        Alexey Lyashkov <alexey.lyashkov@....com>
Subject: Re: [PATCH v2] LUS-1922 e2image: add option to ignore fs errors

On Apr 22, 2020, at 11:54 AM, Artem Blagodarenko <artem.blagodarenko@...il.com> 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@....com>
> 
> Add extended "-E ignore_error" option to be more tolerant
> to fs errors while scanning inode extents.
> 
> Signed-off-by: Alexey Lyashkov <alexey.lyashkov@....com>
> Signed-off-by: Artem Blagodarenko <artem.blagodarenko@....com>

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@...ger.ca>

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






Download attachment "signature.asc" of type "application/pgp-signature" (874 bytes)