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@xxxxxxxxxx>
> Signed-off-by: Eric Sandeen <sandeen@xxxxxxxxxxx>
> ---
> 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)?
> + 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@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
