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 17/23] mdrestore: Add open_device(), read_header() and show_info() functions
Date: Wed, 12 Jul 2023 10:46:09 -0700 [thread overview]
Message-ID: <20230712174609.GN108251@frogsfrogsfrogs> (raw)
In-Reply-To: <20230606092806.1604491-18-chandan.babu@oracle.com>
On Tue, Jun 06, 2023 at 02:58:00PM +0530, Chandan Babu R wrote:
> This commit moves functionality associated with opening the target device,
> reading metadump header information and printing information about the
> metadump into their respective functions. There are no functional changes made
> by this commit.
>
> Signed-off-by: Chandan Babu R <chandan.babu@oracle.com>
> ---
> mdrestore/xfs_mdrestore.c | 141 +++++++++++++++++++++++---------------
> 1 file changed, 85 insertions(+), 56 deletions(-)
>
> diff --git a/mdrestore/xfs_mdrestore.c b/mdrestore/xfs_mdrestore.c
> index 2a9527b9..61e06598 100644
> --- a/mdrestore/xfs_mdrestore.c
> +++ b/mdrestore/xfs_mdrestore.c
> @@ -6,6 +6,7 @@
>
> #include "libxfs.h"
> #include "xfs_metadump.h"
> +#include <libfrog/platform.h>
>
> static struct mdrestore {
> bool show_progress;
> @@ -40,8 +41,72 @@ print_progress(const char *fmt, ...)
> mdrestore.progress_since_warning = true;
> }
>
> +static int
> +open_device(
> + char *path,
> + bool *is_file)
> +{
> + struct stat statbuf;
> + int open_flags;
> + int fd;
> +
> + open_flags = O_RDWR;
> + *is_file = false;
> +
> + if (stat(path, &statbuf) < 0) {
> + /* ok, assume it's a file and create it */
> + open_flags |= O_CREAT;
> + *is_file = true;
> + } else if (S_ISREG(statbuf.st_mode)) {
> + open_flags |= O_TRUNC;
> + *is_file = true;
> + } else {
> + /*
> + * check to make sure a filesystem isn't mounted on the device
> + */
> + if (platform_check_ismounted(path, NULL, &statbuf, 0))
} else if (platform_check_ismounted(...)) ?
> + fatal("a filesystem is mounted on target device \"%s\","
> + " cannot restore to a mounted filesystem.\n",
> + path);
> + }
> +
> + fd = open(path, open_flags, 0644);
> + if (fd < 0)
> + fatal("couldn't open \"%s\"\n", path);
> +
> + return fd;
> +}
> +
> +static void
> +read_header(
> + struct xfs_metablock *mb,
> + 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)
> + fatal("error reading from metadump file\n");
> +}
> +
> +static void
> +show_info(
> + struct xfs_metablock *mb,
> + const char *md_file)
> +{
> + if (mb->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");
> + } else {
> + printf("%s: no informational flags present\n", md_file);
> + }
> +}
> +
> /*
> - * perform_restore() -- do the actual work to restore the metadump
> + * restore() -- do the actual work to restore the metadump
> *
> * @src_f: A FILE pointer to the source metadump
> * @dst_fd: the file descriptor for the target file
> @@ -51,9 +116,9 @@ print_progress(const char *fmt, ...)
> * src_f should be positioned just past a read the previously validated metablock
> */
> static void
> -perform_restore(
> - FILE *src_f,
> - int dst_fd,
> +restore(
> + FILE *md_fp,
> + int ddev_fd,
> int is_target_file,
> const struct xfs_metablock *mbp)
> {
> @@ -81,14 +146,15 @@ perform_restore(
> block_index = (__be64 *)((char *)metablock + sizeof(xfs_metablock_t));
> block_buffer = (char *)metablock + block_size;
>
> - if (fread(block_index, block_size - sizeof(struct xfs_metablock), 1, src_f) != 1)
> + if (fread(block_index, block_size - sizeof(struct xfs_metablock), 1,
> + md_fp) != 1)
Something I forgot to comment on in previous patches: Please don't
indent the second line of an if test at the same level as the if body.
It's much harder for me to distinguish the two. Compare:
if (fread(block_index, block_size - sizeof(struct xfs_metablock), 1,
md_fp) != 1)
fatal("error reading from metadump file\n");
vs:
if (fread(block_index, block_size - sizeof(struct xfs_metablock), 1,
md_fp) != 1)
fatal("error reading from metadump file\n");
Also, previous patches have done things like:
if (foocondition &&
barcondition &&
bazcondition)
dosomething();
vs.
if (foocondition &&
barcondition &&
bazcondition)
dosomething();
The code structure is easier to see at a glance, right? That's why the
xfs and kernel style guidelines ask for distinct indentation levels.
Please go back and fix all of that in the new code you're adding.
--D
> fatal("error reading from metadump file\n");
>
> if (block_index[0] != 0)
> fatal("first block is not the primary superblock\n");
>
>
> - if (fread(block_buffer, mb_count << mbp->mb_blocklog, 1, src_f) != 1)
> + if (fread(block_buffer, mb_count << mbp->mb_blocklog, 1, md_fp) != 1)
> fatal("error reading from metadump file\n");
>
> libxfs_sb_from_disk(&sb, (struct xfs_dsb *)block_buffer);
> @@ -111,7 +177,7 @@ perform_restore(
> if (is_target_file) {
> /* ensure regular files are correctly sized */
>
> - if (ftruncate(dst_fd, sb.sb_dblocks * sb.sb_blocksize))
> + if (ftruncate(ddev_fd, sb.sb_dblocks * sb.sb_blocksize))
> fatal("cannot set filesystem image size: %s\n",
> strerror(errno));
> } else {
> @@ -121,7 +187,7 @@ perform_restore(
> off64_t off;
>
> off = sb.sb_dblocks * sb.sb_blocksize - sizeof(lb);
> - if (pwrite(dst_fd, lb, sizeof(lb), off) < 0)
> + if (pwrite(ddev_fd, lb, sizeof(lb), off) < 0)
> fatal("failed to write last block, is target too "
> "small? (error: %s)\n", strerror(errno));
> }
> @@ -134,7 +200,7 @@ perform_restore(
> print_progress("%lld MB read", bytes_read >> 20);
>
> for (cur_index = 0; cur_index < mb_count; cur_index++) {
> - if (pwrite(dst_fd, &block_buffer[cur_index <<
> + if (pwrite(ddev_fd, &block_buffer[cur_index <<
> mbp->mb_blocklog], block_size,
> be64_to_cpu(block_index[cur_index]) <<
> BBSHIFT) < 0)
> @@ -145,7 +211,7 @@ perform_restore(
> if (mb_count < max_indices)
> break;
>
> - if (fread(metablock, block_size, 1, src_f) != 1)
> + if (fread(metablock, block_size, 1, md_fp) != 1)
> fatal("error reading from metadump file\n");
>
> mb_count = be16_to_cpu(metablock->mb_count);
> @@ -155,7 +221,7 @@ perform_restore(
> fatal("bad block count: %u\n", mb_count);
>
> if (fread(block_buffer, mb_count << mbp->mb_blocklog,
> - 1, src_f) != 1)
> + 1, md_fp) != 1)
> fatal("error reading from metadump file\n");
>
> bytes_read += block_size + (mb_count << mbp->mb_blocklog);
> @@ -172,7 +238,7 @@ perform_restore(
> offsetof(struct xfs_sb, sb_crc));
> }
>
> - if (pwrite(dst_fd, block_buffer, sb.sb_sectsize, 0) < 0)
> + if (pwrite(ddev_fd, block_buffer, sb.sb_sectsize, 0) < 0)
> fatal("error writing primary superblock: %s\n", strerror(errno));
>
> free(metablock);
> @@ -185,8 +251,6 @@ usage(void)
> exit(1);
> }
>
> -extern int platform_check_ismounted(char *, char *, struct stat *, int);
> -
> int
> main(
> int argc,
> @@ -195,9 +259,7 @@ main(
> FILE *src_f;
> int dst_fd;
> int c;
> - int open_flags;
> - struct stat statbuf;
> - int is_target_file;
> + bool is_target_file;
> uint32_t magic;
> struct xfs_metablock mb;
>
> @@ -231,8 +293,8 @@ main(
> usage();
>
> /*
> - * open source and test if this really is a dump. The first metadump block
> - * will be passed to perform_restore() which will continue to read the
> + * open source and test if this really is a dump. The first metadump
> + * block will be passed to restore() which will continue to read the
> * file from this point. This avoids rewind the stream, which causes
> * restore to fail when source was being read from stdin.
> */
> @@ -251,10 +313,7 @@ main(
>
> switch (be32_to_cpu(magic)) {
> case XFS_MD_MAGIC_V1:
> - 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, src_f) != 1)
> - fatal("error reading from metadump file\n");
> + read_header(&mb, src_f);
> break;
> default:
> fatal("specified file is not a metadata dump\n");
> @@ -262,16 +321,7 @@ main(
> }
>
> if (mdrestore.show_info) {
> - if (mb.mb_info & XFS_METADUMP_INFO_FLAGS) {
> - printf("%s: %sobfuscated, %s log, %s metadata blocks\n",
> - argv[optind],
> - mb.mb_info & XFS_METADUMP_OBFUSCATED ? "":"not ",
> - mb.mb_info & XFS_METADUMP_DIRTYLOG ? "dirty":"clean",
> - mb.mb_info & XFS_METADUMP_FULLBLOCKS ? "full":"zeroed");
> - } else {
> - printf("%s: no informational flags present\n",
> - argv[optind]);
> - }
> + show_info(&mb, argv[optind]);
>
> if (argc - optind == 1)
> exit(0);
> @@ -280,30 +330,9 @@ main(
> optind++;
>
> /* check and open target */
> - open_flags = O_RDWR;
> - is_target_file = 0;
> - if (stat(argv[optind], &statbuf) < 0) {
> - /* ok, assume it's a file and create it */
> - open_flags |= O_CREAT;
> - is_target_file = 1;
> - } else if (S_ISREG(statbuf.st_mode)) {
> - open_flags |= O_TRUNC;
> - is_target_file = 1;
> - } else {
> - /*
> - * check to make sure a filesystem isn't mounted on the device
> - */
> - if (platform_check_ismounted(argv[optind], NULL, &statbuf, 0))
> - fatal("a filesystem is mounted on target device \"%s\","
> - " cannot restore to a mounted filesystem.\n",
> - argv[optind]);
> - }
> -
> - dst_fd = open(argv[optind], open_flags, 0644);
> - if (dst_fd < 0)
> - fatal("couldn't open target \"%s\"\n", argv[optind]);
> + dst_fd = open_device(argv[optind], &is_target_file);
>
> - perform_restore(src_f, dst_fd, is_target_file, &mb);
> + restore(src_f, dst_fd, is_target_file, &mb);
>
> close(dst_fd);
> if (src_f != stdin)
> --
> 2.39.1
>
next prev parent reply other threads:[~2023-07-12 17:46 UTC|newest]
Thread overview: 54+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <2ExBvYxYPnyiGvOJzODNvNXx_bfn5RWAck_7UcV7OvxkF6fgAy6UZtI6d2-FdmxoIeBLPPX0vwW9uaJxBa5Rmg==@protonmail.internalid>
2023-06-06 9:27 ` [PATCH V2 00/23] Metadump v2 Chandan Babu R
2023-06-06 9:27 ` [PATCH V2 01/23] metadump: Use boolean values true/false instead of 1/0 Chandan Babu R
2023-06-06 9:27 ` [PATCH V2 02/23] mdrestore: Fix logic used to check if target device is large enough Chandan Babu R
2023-06-06 9:27 ` [PATCH V2 03/23] metadump: Declare boolean variables with bool type Chandan Babu R
2023-06-06 15:17 ` Darrick J. Wong
2023-06-06 9:27 ` [PATCH V2 04/23] metadump: Define and use struct metadump Chandan Babu R
2023-07-12 18:12 ` Darrick J. Wong
2023-07-20 13:13 ` Carlos Maiolino
2023-07-20 17:22 ` Darrick J. Wong
2023-06-06 9:27 ` [PATCH V2 05/23] metadump: Add initialization and release functions Chandan Babu R
2023-06-06 9:27 ` [PATCH V2 06/23] metadump: Postpone invocation of init_metadump() Chandan Babu R
2023-07-12 17:00 ` Darrick J. Wong
2023-06-06 9:27 ` [PATCH V2 07/23] metadump: Introduce struct metadump_ops Chandan Babu R
2023-07-12 17:06 ` Darrick J. Wong
2023-06-06 9:27 ` [PATCH V2 08/23] metadump: Introduce metadump v1 operations Chandan Babu R
2023-07-12 17:10 ` Darrick J. Wong
2023-07-13 4:36 ` Chandan Babu R
2023-06-06 9:27 ` [PATCH V2 09/23] metadump: Rename XFS_MD_MAGIC to XFS_MD_MAGIC_V1 Chandan Babu R
2023-06-06 9:27 ` [PATCH V2 10/23] metadump: Define metadump v2 ondisk format structures and macros Chandan Babu R
2023-07-12 17:18 ` Darrick J. Wong
2023-07-13 4:37 ` Chandan Babu R
2023-06-06 9:27 ` [PATCH V2 11/23] metadump: Define metadump ops for v2 format Chandan Babu R
2023-07-12 17:22 ` Darrick J. Wong
2023-07-13 4:45 ` Chandan Babu R
2023-06-06 9:27 ` [PATCH V2 12/23] xfs_db: Add support to read from external log device Chandan Babu R
2023-07-12 17:35 ` Darrick J. Wong
2023-07-13 5:24 ` Chandan Babu R
2023-06-06 9:27 ` [PATCH V2 13/23] metadump: Add support for passing version option Chandan Babu R
2023-06-06 9:27 ` [PATCH V2 14/23] mdrestore: Declare boolean variables with bool type Chandan Babu R
2023-07-12 17:35 ` Darrick J. Wong
2023-06-06 9:27 ` [PATCH V2 15/23] mdrestore: Define and use struct mdrestore Chandan Babu R
2023-07-12 18:12 ` Darrick J. Wong
2023-06-06 9:27 ` [PATCH V2 16/23] mdrestore: Detect metadump v1 magic before reading the header Chandan Babu R
2023-07-12 17:38 ` Darrick J. Wong
2023-06-06 9:28 ` [PATCH V2 17/23] mdrestore: Add open_device(), read_header() and show_info() functions Chandan Babu R
2023-07-12 17:46 ` Darrick J. Wong [this message]
2023-07-13 5:27 ` Chandan Babu R
2023-06-06 9:28 ` [PATCH V2 18/23] mdrestore: Introduce struct mdrestore_ops Chandan Babu R
2023-06-06 9:28 ` [PATCH V2 19/23] mdrestore: Replace metadump header pointer argument with generic pointer type Chandan Babu R
2023-07-12 17:55 ` Darrick J. Wong
2023-07-13 6:08 ` Chandan Babu R
2023-06-06 9:28 ` [PATCH V2 20/23] mdrestore: Introduce mdrestore v1 operations Chandan Babu R
2023-07-12 17:57 ` Darrick J. Wong
2023-06-06 9:28 ` [PATCH V2 21/23] mdrestore: Extract target device size verification into a function Chandan Babu R
2023-06-06 9:28 ` [PATCH V2 22/23] mdrestore: Define mdrestore ops for v2 format Chandan Babu R
2023-07-12 18:10 ` Darrick J. Wong
2023-07-13 6:27 ` Chandan Babu R
2023-07-13 14:22 ` Darrick J. Wong
2023-06-06 9:28 ` [PATCH V2 23/23] mdrestore: Add support for passing log device as an argument Chandan Babu R
2023-07-12 18:10 ` Darrick J. Wong
[not found] ` <181e7cbefa39e2dc59f2564c25966ac0d05aa6530483b7eb5d649de9a3d1cf7f@mu.id>
2023-07-13 6:48 ` Chandan Babu R
2023-06-06 12:10 ` [PATCH V2 00/23] Metadump v2 Carlos Maiolino
2023-06-06 12:38 ` Chandan Babu R
2023-06-06 16:04 ` Carlos Maiolino
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230712174609.GN108251@frogsfrogsfrogs \
--to=djwong@kernel.org \
--cc=cem@kernel.org \
--cc=chandan.babu@oracle.com \
--cc=linux-xfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).