linux-ext4 - Re: [PATCH v5 4/4] ext2fs: automaticlly open backup superblocks

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]
Message-Id: <660580DD-3745-4872-B8B3-FC6BEC9A386D@gmail.com>
Date:   Tue, 5 Jun 2018 16:23:49 +0300
From:   Artem Blagodarenko <artem.blagodarenko@...il.com>
To:     Andreas Dilger <adilger@...ger.ca>
Cc:     Ext4 Developers List <linux-ext4@...r.kernel.org>
Subject: Re: [PATCH v5 4/4] ext2fs: automaticlly open backup superblocks

Hello Andreas,

This issue was discussed on weekly ext4 developer concall (can not remain the date, months ago). If I understood Theodore right way , moving this code out of exported from ext2fs library was the main requirements for this new feature to be accepted.  Theodore, I am right?

Thanks.
Artem Blagodarenko.

> On 4 Jun 2018, at 20:58, Andreas Dilger <adilger@...ger.ca> wrote:
> 
> On May 26, 2018, at 4:46 PM, Artem Blagodarenko <artem.blagodarenko@...il.com> wrote:
>> 
>> e2image and e2fsck automatically try to open some backup superblocks,
>> if only blocksize is set or passed superblock can't be opened.
>> Try few backup superblocks (e.g. {1, 3, 5, 7, 9} * blocksize * 8).
>> 
>> This code is moved to lib/support/.
> 
> I don't recall seeing an explanation of why this is in lib/support/
> instead of lib/ext2fs?  Maybe as ext2fs_open_backups(), or is there
> some reason we don't want to export this function?
> 
>> Signed-off-by: Artem Blagodarenko <artem.blagodarenko@...il.com>
>> ---
>> 
>> diff --git a/e2fsck/message.c b/e2fsck/message.c
>> index 727f71d5..415d7609 100644
>> --- a/e2fsck/message.c
>> +++ b/e2fsck/message.c
>> @@ -465,7 +465,8 @@ static _INLINE_ void expand_percent_expression(FILE *f, ext2_filsys fs,
>> 		fprintf(f, "%*lld", width, (long long) ctx->blkcount);
>> 		break;
>> 	case 'S':
>> -		fprintf(f, "%llu", get_backup_sb(NULL, fs, NULL, NULL));
>> +		fprintf(f, "%llu", get_first_backup_sb(NULL, NULL, fs,
>> +							  NULL, NULL));
> 
> (style) align after '('
> 
>> diff --git a/e2fsck/unix.c b/e2fsck/unix.c
>> index 491e9eb6..1a5e556f 100644
>> --- a/e2fsck/unix.c
>> +++ b/e2fsck/unix.c
>> @@ -1479,11 +1479,19 @@ restart:
>> 				retval ? _("Superblock invalid,") :
>> 				_("Group descriptors look bad..."));
>> 			orig_superblock = ctx->superblock;
>> -			get_backup_sb(ctx, fs, ctx->filesystem_name, io_ptr);
>> -			if (fs)
>> -				ext2fs_close_free(&fs);
>> 			orig_retval = retval;
>> -			retval = try_open_fs(ctx, flags, io_ptr, &fs);
>> +			retval = try_backups(ctx->filesystem_name,
>> +						    ctx->io_options,
>> +						    flags,
>> +						    &ctx->superblock,
>> +						    &ctx->blocksize, io_ptr,
>> +						    &fs);
> 
> (style) align after '(', pack onto fewest lines possible
> 
>> diff --git a/lib/support/sb_backup.c b/lib/support/sb_backup.c
>> new file mode 100644
>> index 00000000..5660e237
>> --- /dev/null
>> +++ b/lib/support/sb_backup.c
> 
> (style) remove extra blank lines
> 
>> +blk64_t get_first_backup_sb(blk64_t *superblock, unsigned int *block_size,
>> +			       ext2_filsys fs, const char *name,
>> +			       io_manager manager)
> 
> (style) align after '('
> 
>> +{
>> +	struct ext2_super_block *sb;
>> +	io_channel		io = NULL;
>> +	void			*buf = NULL;
>> +	int			try_blocksize;
>> +	blk64_t			try_superblock, ret_sb = 8193;
>> +
>> +	/* superblock and block_size can be NULL if fs->super is passed */
>> +	if (fs && fs->super) {
>> +		ret_sb = (fs->super->s_blocks_per_group +
>> +			  fs->super->s_first_data_block);
> 
> (style) no need for parenthesis here
> 
>> +		if (superblock)
>> +			*superblock = ret_sb;
>> +		if(block_size)
> 
> (style) space after "if"
> 
>> +}
>> +
>> +
> 
> (style) remove extra blank line
> 
>> +errcode_t try_backups(const char *name, const char *io_options,
>> +			    int flags, blk64_t *superblock,
>> +			    unsigned int *block_size, io_manager manager,
>> +			    ext2_filsys *ret_fs)
> 
> (style) align after '('
> 
>> +{
>> +	errcode_t retval;
>> +	blk64_t try_block_number;
>> +	unsigned int i;
>> +
>> +	/*
>> +	 * Get first superblock location based on heuristic.
>> +	 * Blocksize is also returned and used to find next
>> +	 * superblock copy location.
>> +	 */
>> +	try_block_number = get_first_backup_sb(superblock, block_size,
>> +						  *ret_fs, name, manager);
> 
> (style) align after '('
> 
>> +
>> +}
>> +
>> +
> 
> (style) remove blank lines at end
> 
>> diff --git a/lib/support/sb_backup.h b/lib/support/sb_backup.h
>> new file mode 100644
>> index 00000000..f1d48bff
>> --- /dev/null
>> +++ b/lib/support/sb_backup.h
>> @@ -0,0 +1,20 @@
>> +blk64_t get_first_backup_sb(blk64_t *superblock, unsigned int *block_size,
>> +			       ext2_filsys fs, const char *name,
>> +			       io_manager manager);
>> +
>> +errcode_t try_backups(const char *name, const char *io_options,
>> +			    int flags, blk64_t *superblock,
>> +			    unsigned int *block_size, io_manager manager,
>> +			    ext2_filsys *ret_fs);
> 
> (style) align after '(
> 
>> diff --git a/misc/e2image.c b/misc/e2image.c
>> index 727e3876..d8569068 100644
>> --- a/misc/e2image.c
>> +++ b/misc/e2image.c
>> @@ -1612,6 +1612,13 @@ int main (int argc, char ** argv)
>> 	sprintf(offset_opt, "offset=%llu", source_offset);
>> 	retval = ext2fs_open2(device_name, offset_opt, open_flag,
>> 			      superblock, blocksize, unix_io_manager, &fs);
>> +	if (retval & (superblock | blocksize)) {
>> +		printf(_("Try backups in other location.\n"));
>> +		retval = try_backups(device_name, offset_opt, open_flag,
>> +					    &superblock, &blocksize,
>> +					    unix_io_manager, &fs);
> 
> (style) align after '('
> 
> 
> Cheers, Andreas
> 
> 
> 
> 
>