| 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
| ||
|
Date: Wed, 3 Aug 2022 20:00:50 +0300
From: Alexey Lyahkov <alexey.lyashkov@...il.com>
To: Lukas Czerner <lczerner@...hat.com>
Cc: linux-ext4 <linux-ext4@...r.kernel.org>,
Theodore Ts'o <tytso@....edu>,
Andreas Dilger <adilger@...ger.ca>,
Artem Blagodarenko <artem.blagodarenko@...il.com>
Subject: Re: [PATCH] e2fsprogs: avoid code duplication
Hi Lukas,
It don’t have any improvement. It have changes a prototype in the e2fsck to use a generic types,
instead of home coded as similar as debugfs does. Removing ctx->journal needs for same reason.
(as generic code have work with ext2fs
I started this work to make debugfs work fine with journal dump and modifications.
Originally, I found tag v3 isn’t work well with journal dump (large block numbers truncated),
checksums isn’t checked well with dump, … etc.
Loading journal have a lack init for structures related to the fast commit.
> On 3 Aug 2022, at 19:39, Lukas Czerner <lczerner@...hat.com> wrote:
>
> Hi Alexey,
>
> I assume this change is based on the maint branch?
>
> On Wed, Aug 03, 2022 at 10:54:07AM +0300, Alexey Lyashkov wrote:
>> debugfs and e2fsck have a so much code duplication in journal handing.
>> debugfs have lack a many journal features handing also.
>> Let's start code merging to avoid code duplication and lack features.
>>
>> userspace buffer head emulation moved into library.
>
> I can see that this is a little bit more involved than just moving the
> code, can you describe a little bit more what has to be done in order to
> move and deduplicate the code? I have not done a proper review but I can
> already see that the function prototypes are changing as well as some
> structures. I think it would be nice to get some idea from the commit
> description what to expect from this change.
>
> I've done some limited testing on this and I see no regression.
>
>>
>> Signed-off-by: Alexey Lyashkov <alexey.lyashkov@...il.com>
>> ---
>> debugfs/Makefile.in | 14 +-
>> debugfs/debugfs.c | 2 +-
>> debugfs/journal.c | 251 ---------------------------
>> debugfs/journal.h | 2 +-
>> debugfs/logdump.c | 2 +-
>> e2fsck/Makefile.in | 8 +-
>> e2fsck/e2fsck.c | 5 -
>> e2fsck/e2fsck.h | 1 -
>> e2fsck/journal.c | 278 ++----------------------------
>> e2fsck/logfile.c | 2 +-
>> e2fsck/recovery.c | 2 +-
>> e2fsck/revoke.c | 2 +-
>> e2fsck/unix.c | 4 +-
>> e2fsck/util.c | 2 +-
>> lib/ext2fs/Android.bp | 1 +
>> lib/ext2fs/Makefile.in | 23 +--
>> lib/ext2fs/jfs_user.c | 255 +++++++++++++++++++++++++++
>> {e2fsck => lib/ext2fs}/jfs_user.h | 55 +++---
>
> Can we perhaps take the opportunity to rename jfs_user to journal? I
> know it was historically this way, but it can we a bit confusing these
> days, especially when we actually have jfs file system.
I do it originally but… it conflicts with journal.c from e2fsck.
And this code handle just kernel API emulation now.
>
> More below...
>>
>> -
>> -}
>> -
>> /*
>> * This function makes sure that the superblock fields regarding the
>> * journal are consistent.
>> @@ -1525,7 +1285,7 @@ errcode_t e2fsck_check_ext3_journal(e2fsck_t ctx)
>> (!fix_problem(ctx, PR_0_JOURNAL_UNSUPP_VERSION, &pctx))))
>> retval = e2fsck_journal_fix_corrupt_super(ctx, journal,
>> &pctx);
>> - e2fsck_journal_release(ctx, journal, 0, 1);
>> + ext2fs_journal_release(ctx->fs, journal, 0, 1);
>> return retval;
>> }
>>
>> @@ -1552,7 +1312,7 @@ no_has_journal:
>> sb->s_journal_dev = 0;
>> memset(sb->s_journal_uuid, 0,
>> sizeof(sb->s_journal_uuid));
>> - e2fsck_clear_recover(ctx, force_fsck);
>> + ext2fs_clear_recover(ctx->fs, force_fsck);
>
> This is the kind of function prototype change I'd like to be mentioned
> in the description. Just to make it easier for reviewer today and for
> the future.
>
I may put a wrappers who just call ext2fs_* functions if it will be help with review.
It will have just ctx->fs call inside.
>>
>>
>> /* Command line options */
>> @@ -66,7 +66,7 @@ static int replace_bad_blocks;
>> static int keep_bad_blocks;
>> static char *bad_blocks_file;
>>
>> -e2fsck_t e2fsck_global_ctx; /* Try your very best not to use this! */
>> +struct e2fsck_struct *e2fsck_global_ctx; /* Try your very best not to use this! */
>
> Why is this necessary? I am just curious.
>
Using a pointer to structure make a full structure definition unnecessary.
So I can do
extern struct data *ptr;
some_call(ptr);
Without teach source about struct data itself.
It’s a specially for the jfs_user.h and J_ASSERT() implementation.
>> diff --git a/e2fsck/jfs_user.h b/lib/ext2fs/jfs_user.h
>> similarity index 89%
>> rename from e2fsck/jfs_user.h
>> rename to lib/ext2fs/jfs_user.h
>> index 4ad2005a..ed75c4a5 100644
>> --- a/e2fsck/jfs_user.h
>> +++ b/lib/ext2fs/jfs_user.h
>> @@ -11,7 +11,6 @@
>> #ifndef _JFS_USER_H
>> #define _JFS_USER_H
>>
>> -#ifdef DEBUGFS
>> #include <stdio.h>
>> #include <stdlib.h>
>> #if EXT2_FLAT_INCLUDES
>> @@ -23,13 +22,8 @@
>> #include "ext2fs/ext2fs.h"
>> #include "blkid/blkid.h"
>> #endif
>> -#else
>> -/*
>> - * Pull in the definition of the e2fsck context structure
>> - */
>> -#include "config.h"
>> -#include "e2fsck.h"
>> -#endif
>> +
>> +struct e2fsck_struct;
>>
>> #if __STDC_VERSION__ < 199901L
>> # if __GNUC__ >= 2 || _MSC_VER >= 1300
>> @@ -40,11 +34,8 @@
>> #endif
>>
>> struct buffer_head {
>> -#ifdef DEBUGFS
>> ext2_filsys b_fs;
>> -#else
>> - e2fsck_t b_ctx;
>> -#endif
>> + struct e2fsck_struct *b_ctx;
>
> Do we need to have both k_ctx and k_fs? Can we use union instead, or is
> not worth it?
>
I think better to have both, in some cases e2fsck looks to the own context attached to these structures.
I’m not a very understand this part - and probably it will be removed in future.
>
>>
>> # This nastiness is needed because of jfs_user.h hackery; when we finally
>> # clean up this mess, we should be able to drop it
>> -JOURNAL_CFLAGS = -I$(srcdir)/../e2fsck $(ALL_CFLAGS) -DDEBUGFS
>> -DEPEND_CFLAGS = -I$(top_srcdir)/e2fsck
>> +JOURNAL_CFLAGS = $(ALL_CFLAGS) -DDEBUGFS
>> +DEPEND_CFLAGS =
> ^
> You have a trailing whitespace here.
Thanks! Will fix it. It looks comment can removed also.
Alex
>
> Thanks!
> -Lukas
>
>>
>> .
>> --
>> 2.31.1
>>
>
Powered by blists - more mailing lists