[2/3] xfs_mdrestore: refactor open-coded fd/is_file into a structure

Commit Message

Darrick J. Wong Dec. 19, 2024, 7:44 p.m. UTC

From: Darrick J. Wong <[e-mail]>

Create an explicit object to track the fd and flags associated with a
device onto which we are restoring metadata, and use it to reduce the
amount of open-coded arguments to ->restore.  This avoids some grossness
in the next patch.

Signed-off-by: "Darrick J. Wong" <[e-mail]>
---
 mdrestore/xfs_mdrestore.c |  123 +++++++++++++++++++++++----------------------
 1 file changed, 63 insertions(+), 60 deletions(-)

Comments

Christoph Hellwig Dec. 21, 2024, 8:02 a.m. UTC | #1

Looks good:

Reviewed-by: Christoph Hellwig <[e-mail]>

Patch

diff --git a/mdrestore/xfs_mdrestore.c b/mdrestore/xfs_mdrestore.c
index c6c00270234442..c5584fec68813e 100644
--- a/mdrestore/xfs_mdrestore.c
+++ b/mdrestore/xfs_mdrestore.c
@@ -15,12 +15,20 @@  union mdrestore_headers {
 	struct xfs_metadump_header	v2;
 };
 
+struct mdrestore_dev {
+	int		fd;
+	bool		is_file;
+};
+
+#define DEFINE_MDRESTORE_DEV(name) \
+	struct mdrestore_dev name = { .fd = -1 }
+
 struct mdrestore_ops {
 	void (*read_header)(union mdrestore_headers *header, FILE *md_fp);
 	void (*show_info)(union mdrestore_headers *header, const char *md_file);
 	void (*restore)(union mdrestore_headers *header, FILE *md_fp,
-			int ddev_fd, bool is_data_target_file, int logdev_fd,
-			bool is_log_target_file);
+			const struct mdrestore_dev *ddev,
+			const struct mdrestore_dev *logdev);
 };
 
 static struct mdrestore {
@@ -108,25 +116,24 @@  fixup_superblock(
 		fatal("error writing primary superblock: %s\n", strerror(errno));
 }
 
-static int
+static void
 open_device(
-	char		*path,
-	bool		*is_file)
+	struct mdrestore_dev	*dev,
+	char			*path)
 {
-	struct stat	statbuf;
-	int		open_flags;
-	int		fd;
+	struct stat		statbuf;
+	int			open_flags;
 
 	open_flags = O_RDWR;
-	*is_file = false;
+	dev->is_file = false;
 
 	if (stat(path, &statbuf) < 0)  {
 		/* ok, assume it's a file and create it */
 		open_flags |= O_CREAT;
-		*is_file = true;
+		dev->is_file = true;
 	} else if (S_ISREG(statbuf.st_mode))  {
 		open_flags |= O_TRUNC;
-		*is_file = true;
+		dev->is_file = true;
 	} else if (platform_check_ismounted(path, NULL, &statbuf, 0)) {
 		/*
 		 * check to make sure a filesystem isn't mounted on the device
@@ -136,23 +143,30 @@  open_device(
 				path);
 	}
 
-	fd = open(path, open_flags, 0644);
-	if (fd < 0)
+	dev->fd = open(path, open_flags, 0644);
+	if (dev->fd < 0)
 		fatal("couldn't open \"%s\"\n", path);
+}
 
-	return fd;
+static void
+close_device(
+	struct mdrestore_dev	*dev)
+{
+	if (dev->fd >= 0)
+		close(dev->fd);
+	dev->fd = -1;
+	dev->is_file = false;
 }
 
 static void
 verify_device_size(
-	int		dev_fd,
-	bool		is_file,
-	xfs_rfsblock_t	nr_blocks,
-	uint32_t	blocksize)
+	const struct mdrestore_dev	*dev,
+	xfs_rfsblock_t			nr_blocks,
+	uint32_t			blocksize)
 {
-	if (is_file) {
+	if (dev->is_file) {
 		/* ensure regular files are correctly sized */
-		if (ftruncate(dev_fd, nr_blocks * blocksize))
+		if (ftruncate(dev->fd, nr_blocks * blocksize))
 			fatal("cannot set filesystem image size: %s\n",
 				strerror(errno));
 	} else {
@@ -161,7 +175,7 @@  verify_device_size(
 		off_t		off;
 
 		off = nr_blocks * blocksize - sizeof(lb);
-		if (pwrite(dev_fd, lb, sizeof(lb), off) < 0)
+		if (pwrite(dev->fd, lb, sizeof(lb), off) < 0)
 			fatal("failed to write last block, is target too "
 				"small? (error: %s)\n", strerror(errno));
 	}
@@ -195,12 +209,10 @@  show_info_v1(
 
 static void
 restore_v1(
-	union mdrestore_headers *h,
-	FILE			*md_fp,
-	int			ddev_fd,
-	bool			is_data_target_file,
-	int			logdev_fd,
-	bool			is_log_target_file)
+	union mdrestore_headers		*h,
+	FILE				*md_fp,
+	const struct mdrestore_dev	*ddev,
+	const struct mdrestore_dev	*logdev)
 {
 	struct xfs_metablock	*metablock;	/* header + index + blocks */
 	__be64			*block_index;
@@ -254,8 +266,7 @@  restore_v1(
 
 	((struct xfs_dsb*)block_buffer)->sb_inprogress = 1;
 
-	verify_device_size(ddev_fd, is_data_target_file, sb.sb_dblocks,
-			sb.sb_blocksize);
+	verify_device_size(ddev, sb.sb_dblocks, sb.sb_blocksize);
 
 	bytes_read = 0;
 
@@ -263,7 +274,7 @@  restore_v1(
 		maybe_print_progress(&mb_read, bytes_read);
 
 		for (cur_index = 0; cur_index < mb_count; cur_index++) {
-			if (pwrite(ddev_fd, &block_buffer[cur_index <<
+			if (pwrite(ddev->fd, &block_buffer[cur_index <<
 					h->v1.mb_blocklog], block_size,
 					be64_to_cpu(block_index[cur_index]) <<
 						BBSHIFT) < 0)
@@ -292,7 +303,7 @@  restore_v1(
 
 	final_print_progress(&mb_read, bytes_read);
 
-	fixup_superblock(ddev_fd, block_buffer, &sb);
+	fixup_superblock(ddev->fd, block_buffer, &sb);
 
 	free(metablock);
 }
@@ -376,12 +387,10 @@  restore_meta_extent(
 
 static void
 restore_v2(
-	union mdrestore_headers	*h,
-	FILE			*md_fp,
-	int			ddev_fd,
-	bool			is_data_target_file,
-	int			logdev_fd,
-	bool			is_log_target_file)
+	union mdrestore_headers		*h,
+	FILE				*md_fp,
+	const struct mdrestore_dev	*ddev,
+	const struct mdrestore_dev	*logdev)
 {
 	struct xfs_sb		sb;
 	struct xfs_meta_extent	xme;
@@ -415,16 +424,14 @@  restore_v2(
 
 	((struct xfs_dsb *)block_buffer)->sb_inprogress = 1;
 
-	verify_device_size(ddev_fd, is_data_target_file, sb.sb_dblocks,
-			sb.sb_blocksize);
+	verify_device_size(ddev, sb.sb_dblocks, sb.sb_blocksize);
 
 	if (sb.sb_logstart == 0) {
 		ASSERT(mdrestore.external_log == true);
-		verify_device_size(logdev_fd, is_log_target_file, sb.sb_logblocks,
-				sb.sb_blocksize);
+		verify_device_size(logdev, sb.sb_logblocks, sb.sb_blocksize);
 	}
 
-	if (pwrite(ddev_fd, block_buffer, len, 0) < 0)
+	if (pwrite(ddev->fd, block_buffer, len, 0) < 0)
 		fatal("error writing primary superblock: %s\n",
 			strerror(errno));
 
@@ -446,11 +453,11 @@  restore_v2(
 		switch (be64_to_cpu(xme.xme_addr) & XME_ADDR_DEVICE_MASK) {
 		case XME_ADDR_DATA_DEVICE:
 			device = "data";
-			fd = ddev_fd;
+			fd = ddev->fd;
 			break;
 		case XME_ADDR_LOG_DEVICE:
 			device = "log";
-			fd = logdev_fd;
+			fd = logdev->fd;
 			break;
 		default:
 			fatal("Invalid device found in metadump\n");
@@ -467,7 +474,7 @@  restore_v2(
 
 	final_print_progress(&mb_read, bytes_read);
 
-	fixup_superblock(ddev_fd, block_buffer, &sb);
+	fixup_superblock(ddev->fd, block_buffer, &sb);
 
 	free(block_buffer);
 }
@@ -492,13 +499,11 @@  main(
 	char			**argv)
 {
 	union mdrestore_headers	headers;
+	DEFINE_MDRESTORE_DEV(ddev);
+	DEFINE_MDRESTORE_DEV(logdev);
 	FILE			*src_f;
-	char			*logdev = NULL;
-	int			data_dev_fd = -1;
-	int			log_dev_fd = -1;
+	char			*logdev_path = NULL;
 	int			c;
-	bool			is_data_dev_file = false;
-	bool			is_log_dev_file = false;
 
 	mdrestore.show_progress = false;
 	mdrestore.show_info = false;
@@ -516,7 +521,7 @@  main(
 				mdrestore.show_info = true;
 				break;
 			case 'l':
-				logdev = optarg;
+				logdev_path = optarg;
 				mdrestore.external_log = true;
 				break;
 			case 'V':
@@ -555,7 +560,7 @@  main(
 
 	switch (be32_to_cpu(headers.magic)) {
 	case XFS_MD_MAGIC_V1:
-		if (logdev != NULL)
+		if (logdev_path != NULL)
 			usage();
 		mdrestore.mdrops = &mdrestore_ops_v1;
 		break;
@@ -581,18 +586,16 @@  main(
 	optind++;
 
 	/* check and open data device */
-	data_dev_fd = open_device(argv[optind], &is_data_dev_file);
+	open_device(&ddev, argv[optind]);
 
+	/* check and open log device */
 	if (mdrestore.external_log)
-		/* check and open log device */
-		log_dev_fd = open_device(logdev, &is_log_dev_file);
+		open_device(&logdev, logdev_path);
 
-	mdrestore.mdrops->restore(&headers, src_f, data_dev_fd,
-			is_data_dev_file, log_dev_fd, is_log_dev_file);
+	mdrestore.mdrops->restore(&headers, src_f, &ddev, &logdev);
 
-	close(data_dev_fd);
-	if (mdrestore.external_log)
-		close(log_dev_fd);
+	close_device(&ddev);
+	close_device(&logdev);
 
 	if (src_f != stdin)
 		fclose(src_f);