Re: [PATCH 6/7] mkfs: consolidate and streamline config opening function

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Tue, Jun 12, 2018 at 03:21:16PM -0700, Darrick J. Wong wrote:
> On Tue, Jun 12, 2018 at 02:31:22PM -0500, Eric Sandeen wrote:
> > There was a lot of duplication between open_cli_config and open_config_file
> > which was unnecessary.  Collapse all the logic into one function.
> > 
> > Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> > Signed-off-by: Eric Sandeen <sandeen@sandeen.net>
> > ---
> >  mkfs/config.c   | 148 +++++++++++++++++++++-----------------------------------
> >  mkfs/xfs_mkfs.c |   1 -
> >  2 files changed, 56 insertions(+), 93 deletions(-)
> > 
> > diff --git a/mkfs/config.c b/mkfs/config.c
> > index 940a055..173ab9a 100644
> > --- a/mkfs/config.c
> > +++ b/mkfs/config.c
> > @@ -523,60 +523,6 @@ config_stat_check(
> >  	return 0;
> >  }
> >  
> > -/*
> > - * If the file is not found -1 is returned and errno set. Otherwise
> > - * the file descriptor is returned.
> > - */
> > -int
> > -open_cli_config(
> > -	int		dirfd,
> > -	const char	*cli_config_file,
> > -	char		**fpath)
> > -{
> > -	int		fd = -1, len, ret;
> > -	struct stat	st;
> > -
> > -	fd = openat(AT_FDCWD, cli_config_file, O_NOFOLLOW, O_RDONLY);
> > -	if (fd < 0) {
> > -		len = snprintf(*fpath, PATH_MAX, "%s/%s", MKFS_XFS_CONF_DIR,
> > -			       cli_config_file);
> > -		/* Indicates truncation */
> > -		if (len >= PATH_MAX) {
> > -			errno = ENAMETOOLONG;
> > -			goto out;
> > -		}
> > -
> > -		fd = openat(dirfd, cli_config_file, O_NOFOLLOW, O_RDONLY);
> > -		if (fd < 0)
> > -			goto out;
> > -
> > -		ret = fstat(fd, &st);
> > -		if (ret != 0)
> > -			goto err_out_close;
> > -
> > -		ret = config_stat_check(&st);
> > -		if (ret != 0)
> > -			goto err_out_close;
> > -
> > -		goto out;
> > -	}
> > -
> > -	memcpy(*fpath, cli_config_file, strlen(cli_config_file));
> > -
> > -	ret = fstat(fd, &st);
> > -	if (ret != 0)
> > -		goto err_out_close;
> > -
> > -	ret = config_stat_check(&st);
> > -	if (ret != 0)
> > -		goto err_out_close;
> > -out:
> > -	return fd;
> > -err_out_close:
> > -	close(fd);
> > -	return -1;
> > -}
> > -
> >  #ifndef O_PATH
> >  #if defined __alpha__
> >  #define O_PATH		040000000
> > @@ -589,13 +535,26 @@ err_out_close:
> >  #endif
> >  #endif /* O_PATH */
> >  
> > +/*
> > + * Try to open a config file, either cli-specified, or default.
> > + *
> > + * If specified on commandline, search relative to pwd or absolute path.
> > + * If not specified or if above fails, try either cli-spec'd file or "default"
> > + * in MKFS_XFS_CONF_DIR.
> > + *
> > + * If any config file is successfully opened, dft->type is set to reflect the
> > + * source.
> > + *
> > + * If a cli-specified file is not found -1 is returned and errno set. Otherwise
> > + * the file descriptor is returned.
> > + */
> >  int
> >  open_config_file(
> > -	const char			*cli_config_file,
> > +	const char			*config_file,
> >  	struct mkfs_default_params	*dft,
> > -	char				**fpath)	/* path where config is found */
> > +	char				**fpath)	/* path where we found config */
> 
> Move the comment to the pre-function comment and who is expected to get
> rid of it?
> 
> "If a config file is found, its path is returned in **fpath.  This
> memory must be free()'d by the caller."
> 
> Otherwise seems fine so far...
> 
> --D
> 
> >  {
> > -	int				dirfd, fd = -1, len, ret;
> > +	int				dirfd = -1, fd = -1, len, ret = 0;
> >  	struct stat			st;
> >  
> >  	*fpath = malloc(PATH_MAX);
> > @@ -604,57 +563,62 @@ open_config_file(
> >  
> >  	memset(*fpath, 0, PATH_MAX);
> >  
> > -	dirfd = open(MKFS_XFS_CONF_DIR, O_PATH|O_NOFOLLOW|O_DIRECTORY);
> > -
> > -	if (cli_config_file) {
> > -		if (strlen(cli_config_file) > PATH_MAX) {
> > +	/* first try relative to pwd or absolute path to cli configfile */
> > +	if (config_file) {
> > +		dft->type = DEFAULTS_CLI_CONFIG;
> > +		if (strlen(config_file) > PATH_MAX) {
> >  			errno = ENAMETOOLONG;
> >  			goto out;
> >  		}
> > -		fd = open_cli_config(dirfd, cli_config_file, fpath);
> > -		goto out;
> > +		memcpy(*fpath, config_file, strlen(config_file));
> > +		fd = openat(AT_FDCWD, config_file, O_NOFOLLOW, O_RDONLY);
> >  	}
> >  
> > -	fd = openat(dirfd, "default", O_NOFOLLOW, O_RDONLY);
> > -	if (fd < 0)
> > -		goto out;
> > -
> > -	dft->type = DEFAULTS_CONFIG;
> > -
> > -	len = snprintf(*fpath, PATH_MAX, "%s/%s", MKFS_XFS_CONF_DIR, "default");
> > -	/* Indicates truncation */
> > -	if (len >= PATH_MAX) {
> > -		errno = ENAMETOOLONG;
> > -		goto err_out_close;
> > +	/* on failure search for cli config or default file in sysconfdir */
> > +	if (fd < 0) {
> > +		if (!config_file)
> > +			config_file = "default";
> 
> Use a symbolic constant instead of "default" here (and elsewhere)?

Fix this up, and
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> > +		len = snprintf(*fpath, PATH_MAX, "%s/%s", MKFS_XFS_CONF_DIR,
> > +				config_file);
> > +		/* Indicates truncation */
> > +		if (len >= PATH_MAX) {
> > +			errno = ENAMETOOLONG;
> > +			goto out;
> > +		}
> > +		dirfd = open(MKFS_XFS_CONF_DIR, O_PATH|O_NOFOLLOW|O_DIRECTORY);
> > +		if (dirfd < 0)
> > +			goto out;
> > +		fd = openat(dirfd, config_file, O_NOFOLLOW, O_RDONLY);
> > +		if (fd < 0)
> > +			goto out;
> > +		if (!strcmp(config_file, "default"))
> > +			dft->type = DEFAULTS_CONFIG;
> >  	}
> >  
> >  	ret = fstat(fd, &st);
> >  	if (ret != 0)
> > -		goto err_out_close;
> > -
> > +		goto out;
> >  	ret = config_stat_check(&st);
> >  	if (ret != 0)
> > -		goto err_out_close;
> > -
> > +		goto out;
> > +	
> >  out:
> > -	if (fd < 0) {
> > -		if (dft->type != DEFAULTS_BUILTIN) {
> > -			fprintf(stderr,
> > +	/* stat check is always fatal; missing is fatal only if cli-specified */
> > +	if (ret ||
> > +	    (fd < 0 && dft->type == DEFAULTS_CLI_CONFIG)) {
> > +		fprintf(stderr,
> >  _("Unable to open %s config file: %s : %s\n"),
> > -				default_type_str(dft->type), *fpath,
> > -				strerror(errno));
> > -			free(*fpath);
> > -			exit(1);
> > -		}
> > +			default_type_str(dft->type), *fpath,
> > +			strerror(errno));
> > +		free(*fpath);
> > +		exit(1);
> >  	}
> > +
> > +	if (ret && fd >= 0)
> > +		close(fd);
> >  	if (dirfd >= 0)
> >  		close(dirfd);
> > -	return fd;
> > -
> > -err_out_close:
> > -	close(fd);
> > -	fd = -1;
> > -	goto out;
> > +	return ret ? ret : fd;
> >  }
> >  
> >  /*
> > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> > index be3094b..acd98f6 100644
> > --- a/mkfs/xfs_mkfs.c
> > +++ b/mkfs/xfs_mkfs.c
> > @@ -3785,7 +3785,6 @@ _("respecification of configuration not allowed\n"));
> >  				exit(1);
> >  			}
> >  			cli_config_file = optarg;
> > -			dft.type = DEFAULTS_CLI_CONFIG;
> >  			break;
> >  		default:
> >  			continue;
> > -- 
> > 1.8.3.1
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html