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@xxxxxxxxxx>
>> ---
>> 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
