Re: [PATCH V3 19/23] mdrestore: Replace metadump header pointer argument with a union pointer

["Darrick J. Wong" <djwong@kernel.org>]

On Mon, Jul 24, 2023 at 10:05:23AM +0530, Chandan Babu R wrote:
> We will need two variants of read_header(), show_info() and restore() helper
> functions to support two versions of metadump formats. To this end, A future
> commit will introduce a vector of function pointers to work with the two
> metadump formats. To have a common function signature for the function
> pointers, this commit replaces the first argument of the previously listed
> function pointers from "struct xfs_metablock *" with "union
> mdrestore_headers *".
> 
> Signed-off-by: Chandan Babu R <chandan.babu@oracle.com>

The overall code changes look fine to me, but I'm a little mystified why
the *previous* patch introduces union mdrestore_headers and the
mdrestore ops without using either of them.

IOWs, I think you could delete patch 18, move the union definition into
this patch, and move the mdrestore ops definitions that used to be in
patch 18 into patch 20.

--D

> ---
>  mdrestore/xfs_mdrestore.c | 61 +++++++++++++++++++--------------------
>  1 file changed, 29 insertions(+), 32 deletions(-)
> 
> diff --git a/mdrestore/xfs_mdrestore.c b/mdrestore/xfs_mdrestore.c
> index 53c5f68e..4d1bbf28 100644
> --- a/mdrestore/xfs_mdrestore.c
> +++ b/mdrestore/xfs_mdrestore.c
> @@ -91,27 +91,25 @@ open_device(
>  
>  static void
>  read_header(
> -	struct xfs_metablock	*mb,
> +	union mdrestore_headers	*h,
>  	FILE			*md_fp)
>  {
> -	mb->mb_magic = cpu_to_be32(XFS_MD_MAGIC_V1);
> -
> -	if (fread((uint8_t *)mb + sizeof(mb->mb_magic),
> -			sizeof(*mb) - sizeof(mb->mb_magic), 1, md_fp) != 1)
> +	if (fread((uint8_t *)&(h->v1.mb_count),
> +			sizeof(h->v1) - sizeof(h->magic), 1, md_fp) != 1)
>  		fatal("error reading from metadump file\n");
>  }
>  
>  static void
>  show_info(
> -	struct xfs_metablock	*mb,
> +	union mdrestore_headers	*h,
>  	const char		*md_file)
>  {
> -	if (mb->mb_info & XFS_METADUMP_INFO_FLAGS) {
> +	if (h->v1.mb_info & XFS_METADUMP_INFO_FLAGS) {
>  		printf("%s: %sobfuscated, %s log, %s metadata blocks\n",
>  			md_file,
> -			mb->mb_info & XFS_METADUMP_OBFUSCATED ? "":"not ",
> -			mb->mb_info & XFS_METADUMP_DIRTYLOG ? "dirty":"clean",
> -			mb->mb_info & XFS_METADUMP_FULLBLOCKS ? "full":"zeroed");
> +			h->v1.mb_info & XFS_METADUMP_OBFUSCATED ? "":"not ",
> +			h->v1.mb_info & XFS_METADUMP_DIRTYLOG ? "dirty":"clean",
> +			h->v1.mb_info & XFS_METADUMP_FULLBLOCKS ? "full":"zeroed");
>  	} else {
>  		printf("%s: no informational flags present\n", md_file);
>  	}
> @@ -129,10 +127,10 @@ show_info(
>   */
>  static void
>  restore(
> +	union mdrestore_headers *h,
>  	FILE			*md_fp,
>  	int			ddev_fd,
> -	int			is_target_file,
> -	const struct xfs_metablock	*mbp)
> +	int			is_target_file)
>  {
>  	struct xfs_metablock	*metablock;	/* header + index + blocks */
>  	__be64			*block_index;
> @@ -144,14 +142,14 @@ restore(
>  	xfs_sb_t		sb;
>  	int64_t			bytes_read;
>  
> -	block_size = 1 << mbp->mb_blocklog;
> +	block_size = 1 << h->v1.mb_blocklog;
>  	max_indices = (block_size - sizeof(xfs_metablock_t)) / sizeof(__be64);
>  
>  	metablock = (xfs_metablock_t *)calloc(max_indices + 1, block_size);
>  	if (metablock == NULL)
>  		fatal("memory allocation failure\n");
>  
> -	mb_count = be16_to_cpu(mbp->mb_count);
> +	mb_count = be16_to_cpu(h->v1.mb_count);
>  	if (mb_count == 0 || mb_count > max_indices)
>  		fatal("bad block count: %u\n", mb_count);
>  
> @@ -165,8 +163,7 @@ restore(
>  	if (block_index[0] != 0)
>  		fatal("first block is not the primary superblock\n");
>  
> -
> -	if (fread(block_buffer, mb_count << mbp->mb_blocklog, 1, md_fp) != 1)
> +	if (fread(block_buffer, mb_count << h->v1.mb_blocklog, 1, md_fp) != 1)
>  		fatal("error reading from metadump file\n");
>  
>  	libxfs_sb_from_disk(&sb, (struct xfs_dsb *)block_buffer);
> @@ -213,7 +210,7 @@ restore(
>  
>  		for (cur_index = 0; cur_index < mb_count; cur_index++) {
>  			if (pwrite(ddev_fd, &block_buffer[cur_index <<
> -					mbp->mb_blocklog], block_size,
> +					h->v1.mb_blocklog], block_size,
>  					be64_to_cpu(block_index[cur_index]) <<
>  						BBSHIFT) < 0)
>  				fatal("error writing block %llu: %s\n",
> @@ -232,11 +229,11 @@ restore(
>  		if (mb_count > max_indices)
>  			fatal("bad block count: %u\n", mb_count);
>  
> -		if (fread(block_buffer, mb_count << mbp->mb_blocklog,
> +		if (fread(block_buffer, mb_count << h->v1.mb_blocklog,
>  				1, md_fp) != 1)
>  			fatal("error reading from metadump file\n");
>  
> -		bytes_read += block_size + (mb_count << mbp->mb_blocklog);
> +		bytes_read += block_size + (mb_count << h->v1.mb_blocklog);
>  	}
>  
>  	if (mdrestore.progress_since_warning)
> @@ -265,15 +262,14 @@ usage(void)
>  
>  int
>  main(
> -	int 		argc,
> -	char 		**argv)
> +	int			argc,
> +	char			**argv)
>  {
> -	FILE		*src_f;
> -	int		dst_fd;
> -	int		c;
> -	bool		is_target_file;
> -	uint32_t	magic;
> -	struct xfs_metablock	mb;
> +	union mdrestore_headers headers;
> +	FILE			*src_f;
> +	int			dst_fd;
> +	int			c;
> +	bool			is_target_file;
>  
>  	mdrestore.show_progress = false;
>  	mdrestore.show_info = false;
> @@ -320,20 +316,21 @@ main(
>  			fatal("cannot open source dump file\n");
>  	}
>  
> -	if (fread(&magic, sizeof(magic), 1, src_f) != 1)
> +	if (fread(&headers.magic, sizeof(headers.magic), 1, src_f) != 1)
>  		fatal("Unable to read metadump magic from metadump file\n");
>  
> -	switch (be32_to_cpu(magic)) {
> +	switch (be32_to_cpu(headers.magic)) {
>  	case XFS_MD_MAGIC_V1:
> -		read_header(&mb, src_f);
>  		break;
>  	default:
>  		fatal("specified file is not a metadata dump\n");
>  		break;
>  	}
>  
> +	read_header(&headers, src_f);
> +
>  	if (mdrestore.show_info) {
> -		show_info(&mb, argv[optind]);
> +		show_info(&headers, argv[optind]);
>  
>  		if (argc - optind == 1)
>  			exit(0);
> @@ -344,7 +341,7 @@ main(
>  	/* check and open target */
>  	dst_fd = open_device(argv[optind], &is_target_file);
>  
> -	restore(src_f, dst_fd, is_target_file, &mb);
> +	restore(&headers, src_f, dst_fd, is_target_file);
>  
>  	close(dst_fd);
>  	if (src_f != stdin)
> -- 
> 2.39.1
>