Re: [PATCH v3 3/4] mkfs.xfs: add configuration file parsing support using our own parser

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



I've applied all the recommendations for the man page updates you
provided. More below on the code questions.

On Thu, May 24, 2018 at 09:01:43PM -0700, Darrick J. Wong wrote:
> On Thu, May 24, 2018 at 08:19:42PM -0700, Luis R. Rodriguez wrote:
> > +static struct confopts *
> > +get_confopts(
> > +	const char	*section)
> > +{
> > +	unsigned int	i;
> > +	struct confopts	*opts;
> > +
> > +	for (i=0; i < ARRAY_SIZE(confopts_tab); i++) {
> > +		opts = &confopts_tab[i];
> > +		if (!opts)
> > +			return NULL;
> > +		if (strcmp(opts->name, section) == 0) {
> > +			if (opts->seen) {
> > +				fprintf(stderr, _("Section '%s' respecified\n"),
> > +						section);
> 
> If I have two [data] sections, will this resuilt in:
> 
> # mkfs.xfs -c foo /dev/sda1
> Section 'data' respecified
> Invalid section on line foo:1 [data]
> 
> ?

Yeah.

> The section isn't invalid, it's just double-specified, so...

I've fixed this, now we get:

ergon:~ # mkfs.xfs -f /dev/loop5 -c bar 
Section 'metadata' respecified
Error parsing command line config file: bar : Invalid argument

> > +				return NULL;
> > +			}
> > +			opts->seen = true;
> > +			return opts;
> > +		}
> > +	}
> 
> ...I'd print the 'Invalid section' error message here.

Or move the respecification error message check to the switch
statement handlig the section. I went with the later.

> > +static const char *conf_paths[] = {
> > +	".",
> > +	MKFS_XFS_CONF_DIR,
> > +};
> > +
> > +/*
> > + * If the file is not found -1 is returned and errno set. Otherwise
> > + * the file descriptor is returned.
> > + */
> > +int
> > +open_cli_config(
> > +	char			*cli_config_file,
> > +	char			**fpath)
> > +{
> > +	int			fd, len;
> > +	char			*final_path = NULL;
> > +	char			*relative_path= NULL;
> > +	unsigned int		i;
> > +
> > +	if (strlen(cli_config_file) > 2) {
> > +		if (cli_config_file[0] == '.' && cli_config_file[1] == '/')
> > +			final_path = cli_config_file;
> > +		else if (cli_config_file[0] == '.' && cli_config_file[1] == '.')
> > +			final_path = cli_config_file;
> > +		else if (cli_config_file[0] == '/')
> > +			final_path = cli_config_file;
> > +		else
> > +			relative_path = cli_config_file;
> > +	} else if (strlen(cli_config_file) == 1) {
> > +		if (cli_config_file[0] == '.' || cli_config_file[0] == '/') {
> > +			errno = EINVAL;
> > +			return -1;
> > +		} else
> > +			relative_path = cli_config_file;
> > +	}
> > +
> > +	if (final_path) {
> > +		fd = open(final_path, O_RDONLY);
> > +		if (fd >= 0)
> > +			memcpy(*fpath, final_path, strlen(final_path));
> > +		return fd;
> > +	}
> > +
> > +	/* We finally know the path is relative but just to be sure */
> > +	if (!relative_path) {
> > +		errno = ENXIO;
> > +		return -1;
> > +	}
> > +
> > +	for (i = 0; i < ARRAY_SIZE(conf_paths); i++) {
> > +		memset(*fpath, 0, PATH_MAX);
> > +		/*
> > +		 * For current directory evaluation, skip concatenating the
> > +		 * ./ from the file passed. We only concatenate for the other
> > +		 * paths we look up on.
> 
> If conf_paths[0] was "./" then you wouldn't have to special case this,
> I think.

No, I had tried it, it looks odd still, if the user specified -c foo,
the output should show ./foo, not just foo.

> > +	fp = fdopen(fd, "r");
> > +	if (!fp) {
> > +		ret = errno;
> > +		fprintf(stderr, _("Unable to open stream for config file: %s : %s\n"),
> > +				fpath, strerror(errno));
> 
> perror(fpath); ?

Sure.

> Looks good otherwise.

Groovy. I'll wait for others to comment otherwise I can spin up 4th
iteration after the weekend.

  Luis
--
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



[Index of Archives]     [XFS Filesystem Development (older mail)]     [Linux Filesystem Development]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux RAID]     [Linux SCSI]


  Powered by Linux