On Fri, May 25, 2018 at 04:33:07PM -0700, Luis R. Rodriguez wrote:
> 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
Ok, thanks!
> > > + 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.
Ah. Good point.
> > > + 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.
<nod>
--D
> 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
