Re: [PATCH V2 17/23] mdrestore: Add open_device(), read_header() and show_info() functions

From: Chandan Babu R <chandan.babu@oracle.com>
To: "Darrick J. Wong" <djwong@kernel.org>
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: Thu, 13 Jul 2023 10:57:28 +0530	[thread overview]
Message-ID: <875y6ojotb.fsf@debian-BULLSEYE-live-builder-AMD64> (raw)
In-Reply-To: <20230712174609.GN108251@frogsfrogsfrogs>

On Wed, Jul 12, 2023 at 10:46:09 AM -0700, Darrick J. Wong wrote:
> 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(...)) ?
>

Ok.

>> +			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.

Sorry, I will fix the indentation issues across the patchset.

-- 
chandan