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