[v7,1/4] ext2fs: opening filesystem code refactoring

There are similar opening filesystem code in different utilities.

The patch moves improved handling from try_open_fs()
into ext2fs_open(). This function make one of the action
based on parameters:
1) open filesystem with given superblock, superblock size
2) open filesystem with given superblock, but try to
find right block size

Signed-off-by: Artem Blagodarenko <c17828@cray.com>
---
 e2fsck/unix.c       | 28 +++-------------------------
 lib/ext2fs/openfs.c | 39 +++++++++++++++++++++++++++++++++++----
 misc/dumpe2fs.c     | 17 ++---------------
 3 files changed, 40 insertions(+), 44 deletions(-)

On Jan 29, 2019, at 10:51 AM, Artem Blagodarenko <artem.blagodarenko@gmail.com> wrote:
> 
> There are similar opening filesystem code in different utilities.
> 
> The patch moves improved handling from try_open_fs()
> into ext2fs_open(). This function make one of the action
> based on parameters:
> 1) open filesystem with given superblock, superblock size
> 2) open filesystem with given superblock, but try to
> find right block size
> 
> Signed-off-by: Artem Blagodarenko <c17828@cray.com>

Reviewed-by: Andreas Dilger <adilger@dilger.ca>

> ---
> e2fsck/unix.c       | 28 +++-------------------------
> lib/ext2fs/openfs.c | 39 +++++++++++++++++++++++++++++++++++----
> misc/dumpe2fs.c     | 17 ++---------------
> 3 files changed, 40 insertions(+), 44 deletions(-)
> 
> diff --git a/e2fsck/unix.c b/e2fsck/unix.c
> index 5b3552ec..ddcf52a4 100644
> --- a/e2fsck/unix.c
> +++ b/e2fsck/unix.c
> @@ -1151,31 +1151,9 @@ static errcode_t try_open_fs(e2fsck_t ctx, int flags, io_manager io_ptr,
> 			     ext2_filsys *ret_fs)
> {
> 	errcode_t retval;
> -
> -	*ret_fs = NULL;
> -	if (ctx->superblock && ctx->blocksize) {
> -		retval = ext2fs_open2(ctx->filesystem_name, ctx->io_options,
> -				      flags, ctx->superblock, ctx->blocksize,
> -				      io_ptr, ret_fs);
> -	} else if (ctx->superblock) {
> -		int blocksize;
> -		for (blocksize = EXT2_MIN_BLOCK_SIZE;
> -		     blocksize <= EXT2_MAX_BLOCK_SIZE; blocksize *= 2) {
> -			if (*ret_fs) {
> -				ext2fs_free(*ret_fs);
> -				*ret_fs = NULL;
> -			}
> -			retval = ext2fs_open2(ctx->filesystem_name,
> -					      ctx->io_options, flags,
> -					      ctx->superblock, blocksize,
> -					      io_ptr, ret_fs);
> -			if (!retval)
> -				break;
> -		}
> -	} else
> -		retval = ext2fs_open2(ctx->filesystem_name, ctx->io_options,
> -				      flags, 0, 0, io_ptr, ret_fs);
> -
> +	retval = ext2fs_open2(ctx->filesystem_name, ctx->io_options,
> +			   flags, ctx->superblock, ctx->blocksize,
> +			   io_ptr, ret_fs);
> 	if (retval == 0) {
> 		(*ret_fs)->priv_data = ctx;
> 		e2fsck_set_bitmap_type(*ret_fs, EXT2FS_BMAP64_RBTREE,
> diff --git a/lib/ext2fs/openfs.c b/lib/ext2fs/openfs.c
> index 85d73e2a..16d36200 100644
> --- a/lib/ext2fs/openfs.c
> +++ b/lib/ext2fs/openfs.c
> @@ -114,10 +114,10 @@ static void block_sha_map_free_entry(void *data)
>  *	EXT2_FLAG_64BITS - Allow 64-bit bitfields (needed for large
>  *				filesystems)
>  */
> -errcode_t ext2fs_open2(const char *name, const char *io_options,
> -		       int flags, int superblock,
> -		       unsigned int block_size, io_manager manager,
> -		       ext2_filsys *ret_fs)
> +static errcode_t __ext2fs_open2(const char *name, const char *io_options,
> +				int flags, int superblock,
> +				unsigned int block_size, io_manager manager,
> +				ext2_filsys *ret_fs)
> {
> 	ext2_filsys	fs;
> 	errcode_t	retval;
> @@ -515,6 +515,37 @@ cleanup:
> 	return retval;
> }
> 
> +errcode_t ext2fs_open2(const char *name, const char *io_options,
> +		       int flags, int superblock,
> +		       unsigned int block_size, io_manager manager,
> +		       ext2_filsys *ret_fs)
> +{
> +	errcode_t retval;
> +
> +	*ret_fs = NULL;
> +	if (superblock && block_size) {
> +		retval = __ext2fs_open2(name, io_options,
> +				      flags, superblock, block_size,
> +				      manager, ret_fs);
> +	} else {
> +		int size;
> +
> +		for (size = EXT2_MIN_BLOCK_SIZE;
> +		     size <= EXT2_MAX_BLOCK_SIZE; size *= 2) {
> +			if (*ret_fs) {
> +				ext2fs_free(*ret_fs);
> +				*ret_fs = NULL;
> +			}
> +			retval = __ext2fs_open2(name, io_options, flags,
> +						superblock, size,
> +						manager, ret_fs);
> +			if (!retval)
> +				break;
> +		}
> +	}
> +	return retval;
> +}
> +
> /*
>  * Set/get the filesystem data I/O channel.
>  *
> diff --git a/misc/dumpe2fs.c b/misc/dumpe2fs.c
> index 384ce925..183e2f6f 100644
> --- a/misc/dumpe2fs.c
> +++ b/misc/dumpe2fs.c
> @@ -667,21 +667,8 @@ int main (int argc, char ** argv)
> 	if (image_dump)
> 		flags |= EXT2_FLAG_IMAGE_FILE;
> try_open_again:
> -	if (use_superblock && !use_blocksize) {
> -		for (use_blocksize = EXT2_MIN_BLOCK_SIZE;
> -		     use_blocksize <= EXT2_MAX_BLOCK_SIZE;
> -		     use_blocksize *= 2) {
> -			retval = ext2fs_open (device_name, flags,
> -					      use_superblock,
> -					      use_blocksize, unix_io_manager,
> -					      &fs);
> -			if (!retval)
> -				break;
> -		}
> -	} else {
> -		retval = ext2fs_open(device_name, flags, use_superblock,
> -				     use_blocksize, unix_io_manager, &fs);
> -	}
> +	retval = ext2fs_open2(device_name, 0, flags, use_superblock,
> +			      use_blocksize, unix_io_manager, &fs);
> 	flags |= EXT2_FLAG_IGNORE_CSUM_ERRORS;
> 	if (retval && !retval_csum) {
> 		retval_csum = retval;
> --
> 2.14.3
> 


Cheers, Andreas

On Tue, Jan 29, 2019 at 08:51:31PM +0300, Artem Blagodarenko wrote:
> There are similar opening filesystem code in different utilities.
> 
> The patch moves improved handling from try_open_fs()
> into ext2fs_open(). This function make one of the action
> based on parameters:
> 1) open filesystem with given superblock, superblock size
> 2) open filesystem with given superblock, but try to
> find right block size

This changes the behavior of ext2fs_open() when the superblock and
blocksize parameters is zero.  Previously we read the primary
superblock at the known location, and a blocksize of zero means "don't
care" and so we just do the right thing.  With your patch, it's going
to loop for all possible block sizes, failing with
EXT2_ET_UNEXPECTED_BLOCK_SIZE, until we stumble on the correct file
system.  That's unfortunate, especially if the device is opened with
IO_FLAG_DIRECT_IO.

So the brute force searching should be done using if an explicit flag
is given.  Using a new flag to request this new behavior is safer, but
it's probably OK if we only do the blocksize search if the superblock
parameter is zero --- you'll note that's what the code you were
replacing in dumpe2fs and e2fsck was doing.

More generally, for this whole patch series, in the "happy case" where
the file system is fine, e2fsck and e2image should *not* do any extra
work.

						- Ted

On Tue, Jan 29, 2019 at 08:51:31PM +0300, Artem Blagodarenko wrote:
> There are similar opening filesystem code in different utilities.
> 
> The patch moves improved handling from try_open_fs()
> into ext2fs_open(). This function make one of the action
> based on parameters:
> 1) open filesystem with given superblock, superblock size
> 2) open filesystem with given superblock, but try to
> find right block size
> 
> Signed-off-by: Artem Blagodarenko <c17828@cray.com>

Thanks, applied.

					- Ted