diff mbox series

[34/50] xfs_mdrestore: refactor open-coded fd/is_file into a structure

Message ID 173405323705.1228937.13659584501492804544.stgit@frogsfrogsfrogs (mailing list archive)
State New
Headers show
Series None | expand

Commit Message

Darrick J. Wong Dec. 13, 2024, 1:29 a.m. UTC 
From: Darrick J. Wong <djwong@kernel.org>

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" <djwong@kernel.org>
---
 mdrestore/xfs_mdrestore.c |  123 +++++++++++++++++++++++----------------------
 1 file changed, 63 insertions(+), 60 deletions(-)

Comments

Andrey Albershteyn Dec. 20, 2024, 2:37 p.m. UTC | #1 
On 2024-12-12 17:29:06, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> 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" <djwong@kernel.org>

LGTM
Reviewed-by: Andrey Albershteyn <aalbersh@kernel.org>

> ---
>  mdrestore/xfs_mdrestore.c |  123 +++++++++++++++++++++++----------------------
>  1 file changed, 63 insertions(+), 60 deletions(-)
> 
> 
> 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);
>
diff mbox series

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);