Re: [PATCH 8/9] libfrog: create xfd_open function

[Allison Collins <allison.henderson@oracle.com>]

On 8/29/19 9:21 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Create a helper to open a file and initialize the xfd structure.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good
Reviewed-by: Allison Collins <allison.henderson@oracle.com>

> ---
>   fsr/xfs_fsr.c    |   26 ++++++--------------------
>   include/xfrog.h  |    1 +
>   libfrog/fsgeom.c |   22 ++++++++++++++++++++++
>   quota/quot.c     |    7 ++++---
>   scrub/phase1.c   |   25 ++++++++-----------------
>   5 files changed, 41 insertions(+), 40 deletions(-)
> 
> 
> diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c
> index 36402252..e8d41aaf 100644
> --- a/fsr/xfs_fsr.c
> +++ b/fsr/xfs_fsr.c
> @@ -592,18 +592,10 @@ fsrfs(char *mntdir, xfs_ino_t startino, int targetrange)
>   		return -1;
>   	}
>   
> -	if ((fsxfd.fd = open(mntdir, O_RDONLY)) < 0) {
> -		fsrprintf(_("unable to open: %s: %s\n"),
> -		          mntdir, strerror( errno ));
> -		free(fshandlep);
> -		return -1;
> -	}
> -
> -	ret = xfd_prepare_geometry(&fsxfd);
> +	ret = xfd_open(&fsxfd, mntdir, O_RDONLY);
>   	if (ret) {
> -		fsrprintf(_("Skipping %s: could not get XFS geometry\n"),
> -			  mntdir);
> -		xfd_close(&fsxfd);
> +		fsrprintf(_("unable to open XFS file: %s: %s\n"),
> +		          mntdir, strerror(ret));
>   		free(fshandlep);
>   		return -1;
>   	}
> @@ -725,16 +717,10 @@ fsrfile(
>   	 * Need to open something on the same filesystem as the
>   	 * file.  Open the parent.
>   	 */
> -	fsxfd.fd = open(getparent(fname), O_RDONLY);
> -	if (fsxfd.fd < 0) {
> -		fsrprintf(_("unable to open sys handle for %s: %s\n"),
> -			fname, strerror(errno));
> -		goto out;
> -	}
> -
> -	error = xfd_prepare_geometry(&fsxfd);
> +	error = xfd_open(&fsxfd, getparent(fname), O_RDONLY);
>   	if (error) {
> -		fsrprintf(_("Unable to get geom on fs for: %s\n"), fname);
> +		fsrprintf(_("unable to open sys handle for XFS file %s: %s\n"),
> +			fname, strerror(error));
>   		goto out;
>   	}
>   
> diff --git a/include/xfrog.h b/include/xfrog.h
> index 7bda9810..3a49acc3 100644
> --- a/include/xfrog.h
> +++ b/include/xfrog.h
> @@ -52,6 +52,7 @@ struct xfs_fd {
>   #define XFS_FD_INIT_EMPTY	XFS_FD_INIT(-1)
>   
>   int xfd_prepare_geometry(struct xfs_fd *xfd);
> +int xfd_open(struct xfs_fd *xfd, const char *pathname, int flags);
>   int xfd_close(struct xfs_fd *xfd);
>   
>   /* Convert AG number and AG inode number into fs inode number. */
> diff --git a/libfrog/fsgeom.c b/libfrog/fsgeom.c
> index cf9323c1..47644dc3 100644
> --- a/libfrog/fsgeom.c
> +++ b/libfrog/fsgeom.c
> @@ -117,6 +117,28 @@ xfd_prepare_geometry(
>   	return 0;
>   }
>   
> +/* Open a file on an XFS filesystem.  Returns zero or a positive error code. */
> +int
> +xfd_open(
> +	struct xfs_fd		*xfd,
> +	const char		*pathname,
> +	int			flags)
> +{
> +	int			ret;
> +
> +	xfd->fd = open(pathname, O_RDONLY);
> +	if (xfd->fd < 0)
> +		return errno;
> +
> +	ret = xfd_prepare_geometry(xfd);
> +	if (ret) {
> +		xfd_close(xfd);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
>   /*
>    * Release any resources associated with this xfs_fd structure.  Returns zero
>    * or a positive error code.
> diff --git a/quota/quot.c b/quota/quot.c
> index 1e970819..4614f684 100644
> --- a/quota/quot.c
> +++ b/quota/quot.c
> @@ -131,7 +131,7 @@ quot_bulkstat_mount(
>   	struct xfs_bstat	*buf;
>   	uint64_t		last = 0;
>   	uint32_t		count;
> -	int			i, sts;
> +	int			i, sts, ret;
>   	du_t			**dp;
>   
>   	/*
> @@ -146,8 +146,9 @@ quot_bulkstat_mount(
>   			*dp = NULL;
>   	ndu[0] = ndu[1] = ndu[2] = 0;
>   
> -	fsxfd.fd = open(fsdir, O_RDONLY);
> -	if (fsxfd.fd < 0) {
> +	ret = xfd_open(&fsxfd, fsdir, O_RDONLY);
> +	if (ret) {
> +		errno = ret;
>   		perror(fsdir);
>   		return;
>   	}
> diff --git a/scrub/phase1.c b/scrub/phase1.c
> index cbdbd010..15e67e37 100644
> --- a/scrub/phase1.c
> +++ b/scrub/phase1.c
> @@ -84,13 +84,17 @@ xfs_setup_fs(
>   	 * CAP_SYS_ADMIN, which we probably need to do anything fancy
>   	 * with the (XFS driver) kernel.
>   	 */
> -	ctx->mnt.fd = open(ctx->mntpoint, O_RDONLY | O_NOATIME | O_DIRECTORY);
> -	if (ctx->mnt.fd < 0) {
> -		if (errno == EPERM)
> +	error = xfd_open(&ctx->mnt, ctx->mntpoint,
> +			O_RDONLY | O_NOATIME | O_DIRECTORY);
> +	if (error) {
> +		if (error == EPERM)
>   			str_info(ctx, ctx->mntpoint,
>   _("Must be root to run scrub."));
> +		else if (error == ENOTTY)
> +			str_error(ctx, ctx->mntpoint,
> +_("Not an XFS filesystem."));
>   		else
> -			str_errno(ctx, ctx->mntpoint);
> +			str_liberror(ctx, error, ctx->mntpoint);
>   		return false;
>   	}
>   
> @@ -110,12 +114,6 @@ _("Must be root to run scrub."));
>   		return false;
>   	}
>   
> -	if (!platform_test_xfs_fd(ctx->mnt.fd)) {
> -		str_info(ctx, ctx->mntpoint,
> -_("Does not appear to be an XFS filesystem!"));
> -		return false;
> -	}
> -
>   	/*
>   	 * Flush everything out to disk before we start checking.
>   	 * This seems to reduce the incidence of stale file handle
> @@ -127,13 +125,6 @@ _("Does not appear to be an XFS filesystem!"));
>   		return false;
>   	}
>   
> -	/* Retrieve XFS geometry. */
> -	error = xfd_prepare_geometry(&ctx->mnt);
> -	if (error) {
> -		str_liberror(ctx, error, _("Retrieving XFS geometry"));
> -		return false;
> -	}
> -
>   	if (!xfs_action_lists_alloc(ctx->mnt.fsgeom.agcount,
>   				&ctx->action_lists)) {
>   		str_error(ctx, ctx->mntpoint, _("Not enough memory."));
>