On Tue, May 29, 2018 at 04:31:46PM -0700, Darrick J. Wong wrote:
> On Tue, May 29, 2018 at 03:06:02PM -0700, Luis R. Rodriguez wrote:
> > +int
> > +parse_defaults_file(
> > + struct mkfs_default_params *dft,
> > + int default_fd,
> > + char *config_file)
> > +{
> > + char *fpath;
> > + int fd;
> > + FILE *fp;
> > + int ret;
> > + struct stat sp;
> > +
> > + if (strlen(config_file) > PATH_MAX)
> > + return ENAMETOOLONG;
> > +
> > + fpath = malloc(PATH_MAX);
> > + if (!fpath)
> > + return ENOMEM;
> > + memset(fpath, 0, PATH_MAX);
> > +
> > + if (default_fd < 0) {
> > + fd = open_cli_config(config_file, &fpath);
> > + if (fd < 0) {
> > + free(fpath);
> > + return errno;
> > + }
> > + } else {
> > + fd = default_fd;
> > + memcpy(fpath, config_file, strlen(config_file));
> > + }
> > +
> > + /*
> > + * At this point we know we have a valid file descriptor and have
> > + * figured out the path to the file used on fpath. Get the file stream
> > + * and do a bit of sanity checks before parsing the file.
> > + */
> > +
> > + fp = fdopen(fd, "r");
> > + if (!fp) {
> > + perror(fpath);
>
> It occurred to me just now (sorry...) that the caller of this function
> prints out a string with strerror() contents, so the perror here is
> unnecessary
Alright.
> since the caller will cough up an error anyway. Then all of
> these constructions here become:
>
> fp = fdopen(...);
> if (!fp)
> return -1;
>
> ret = fstat(...);
> if (ret)
> goto out;
>
> if (S_ISDIR(...)) {
> errno = EISDIR;
> goto out;
> }
> ...
> return 0;
> out:
> return -1;
Sure.
>
> and the call site now becomes:
>
> if (parse_defaults_file(...)) {
> fprintf(stderr, "%s: file is bad: %s\n", path, strerror(errno));
> ...
> }
Works with me.
> Granted maybe we should just merge this and do all those cleanups
> separately.
;)
> > + if (S_ISDIR(sp.st_mode)) {
> > + ret = EBADF;
>
> ret = EISDIR?
Chinner asked we always make a regular file out of the config,
so now a regular file check will suffice, and EISDIR would not
be as descriptive.
So I'll wait to hear back on some other minor things, hopefully next
week we can wrap up this series for good.
--
Do not panic
--
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
