Re: [PATCH V2 19/23] mdrestore: Replace metadump header pointer argument with generic pointer type

From: "Darrick J. Wong" <djwong@kernel.org>
To: Chandan Babu R <chandan.babu@oracle.com>
Cc: linux-xfs@vger.kernel.org, cem@kernel.org
Subject: Re: [PATCH V2 19/23] mdrestore: Replace metadump header pointer argument with generic pointer type
Date: Wed, 12 Jul 2023 10:55:24 -0700	[thread overview]
Message-ID: <20230712175524.GO108251@frogsfrogsfrogs> (raw)
In-Reply-To: <20230606092806.1604491-20-chandan.babu@oracle.com>

On Tue, Jun 06, 2023 at 02:58:02PM +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 "void *".
> 
> Signed-off-by: Chandan Babu R <chandan.babu@oracle.com>
> ---
>  mdrestore/xfs_mdrestore.c | 24 +++++++++++++++++-------
>  1 file changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/mdrestore/xfs_mdrestore.c b/mdrestore/xfs_mdrestore.c
> index 08f52527..5451a58b 100644
> --- a/mdrestore/xfs_mdrestore.c
> +++ b/mdrestore/xfs_mdrestore.c
> @@ -87,9 +87,11 @@ open_device(
>  
>  static void
>  read_header(
> -	struct xfs_metablock	*mb,
> +	void			*header,

Should we be using a union here instead of a generic void pointer?

union xfs_mdrestore_headers {
	__be32				magic;
	struct xfs_metablock		v1;
	struct xfs_metadump_header	v2;
};

Then you can do:

	union xfs_mdrestore_headers	headers;

	fread(&headers.magic, sizeof(headers.magic), 1, md_fp));

	switch (be32_to_cpu(headers.magic)) {
	case XFS_MD_MAGIC_V1:
		ret = dosomething_v1(&headers, ...);
		break;
	case XFS_MD_MAGIC_V2:
		ret = dosomething_v2(&headers, ...);
		break;

And there'll be at least *some* typechecking going on here.

>  	FILE			*md_fp)
>  {
> +	struct xfs_metablock	*mb = header;
> +
>  	mb->mb_magic = cpu_to_be32(XFS_MD_MAGIC_V1);

And no need for the casting:

static void
read_header_v1(
	union xfs_mdrestore_headers	*h,
	FILE				*md_fp)
{
	fread(&h->v1.mb_count, sizeof(h->v1.mb_count), 1, md_fp);
	...
}

static void
read_header_v2(
	union xfs_mdrestore_headers	*h,
	FILE				*md_fp)
{
	fread(&h->v2.xmh_version,
			sizeof(struct xfs_metadump_header) - offsetof(struct xfs_metadump_header, xmh_version),
			1, md_fp);
	...
}

--D

>  
>  	if (fread((uint8_t *)mb + sizeof(mb->mb_magic),
> @@ -99,9 +101,11 @@ read_header(
>  
>  static void
>  show_info(
> -	struct xfs_metablock	*mb,
> +	void			*header,
>  	const char		*md_file)
>  {
> +	struct xfs_metablock	*mb = header;
> +
>  	if (mb->mb_info & XFS_METADUMP_INFO_FLAGS) {
>  		printf("%s: %sobfuscated, %s log, %s metadata blocks\n",
>  			md_file,
> @@ -125,12 +129,13 @@ show_info(
>   */
>  static void
>  restore(
> +	void			*header,
>  	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 */
> +	struct xfs_metablock	*mbp;
>  	__be64			*block_index;
>  	char			*block_buffer;
>  	int			block_size;
> @@ -140,6 +145,8 @@ restore(
>  	xfs_sb_t		sb;
>  	int64_t			bytes_read;
>  
> +	mbp = header;
> +
>  	block_size = 1 << mbp->mb_blocklog;
>  	max_indices = (block_size - sizeof(xfs_metablock_t)) / sizeof(__be64);
>  
> @@ -269,6 +276,7 @@ main(
>  	int		c;
>  	bool		is_target_file;
>  	uint32_t	magic;
> +	void		*header;
>  	struct xfs_metablock	mb;
>  
>  	mdrestore.show_progress = false;
> @@ -321,15 +329,17 @@ main(
>  
>  	switch (be32_to_cpu(magic)) {
>  	case XFS_MD_MAGIC_V1:
> -		read_header(&mb, src_f);
> +		header = &mb;
>  		break;
>  	default:
>  		fatal("specified file is not a metadata dump\n");
>  		break;
>  	}
>  
> +	read_header(header, src_f);
> +
>  	if (mdrestore.show_info) {
> -		show_info(&mb, argv[optind]);
> +		show_info(header, argv[optind]);
>  
>  		if (argc - optind == 1)
>  			exit(0);
> @@ -340,7 +350,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(header, src_f, dst_fd, is_target_file);
>  
>  	close(dst_fd);
>  	if (src_f != stdin)
> -- 
> 2.39.1
> 