Message-Id: <39DA7EBB-8155-4557-83EC-BB6BFBBDB72E@gmail.com>
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