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

[Chandan Babu R <chandan.babu@oracle.com>]

On Wed, Jul 12, 2023 at 10:55:24 AM -0700, Darrick J. Wong wrote:
> 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);
> 	...
> }
>

I agree with the above suggestions. I will apply them to the patchset.

-- 
chandan