Hi again, made a mistake in reviewing, see below.
On Tue, Jan 16, 2018 at 11:54:43AM +0100, Gaëtan Rivet wrote: > Hi Matam, Adrien, > > On Tue, Jan 09, 2018 at 02:47:27PM +0000, Matan Azrad wrote: > > From: Adrien Mazarguil <adrien.mazarg...@6wind.com> > > > > This parameter enables applications to provide device definitions through > > an arbitrary file descriptor number. > > Ok on the principle, > > <snip> > > > @@ -161,6 +165,73 @@ typedef int (parse_cb)(struct rte_eth_dev *dev, const > > char *params, > > } > > > > static int > > +fs_read_fd(struct sub_device *sdev, char *fd_str) > > +{ > > + FILE *fp = NULL; > > + int fd = -1; > > + /* store possible newline as well */ > > + char output[DEVARGS_MAXLEN + 1]; > > + int err = -ENODEV; > > + int ret; > > ret is used as flag older, line counter and then error reporting. > err should be the only variable used for reading errors from function > and reporting it. > > It would be clearer to use descriptive names, such as "oflags" and "nl" > or "lcount". I don't really care about one additional variable in this > function, for the sake of expressiveness. > > > + > > + RTE_ASSERT(fd_str != NULL || sdev->fd_str != NULL); > > + if (sdev->fd_str == NULL) { > > + sdev->fd_str = strdup(fd_str); > > + if (sdev->fd_str == NULL) { > > + ERROR("Command line allocation failed"); > > + return -ENOMEM; > > + } > > + } > > + errno = 0; > > + fd = strtol(fd_str, &fd_str, 0); > > + if (errno || *fd_str || fd < 0) { > > + ERROR("Parsing FD number failed"); > > + goto error; > > + } > > + /* Fiddle with copy of file descriptor */ > > + fd = dup(fd); > > + if (fd == -1) > > + goto error; > > + ret = fcntl(fd, F_GETFL); > > oflags = fcntl(...); > > > + if (ret == -1) > > + goto error; > > + ret = fcntl(fd, F_SETFL, fd | O_NONBLOCK); > > err = fcntl(fd, F_SETFL, oflags | O_NONBLOCK); > Using (fd | O_NONBLOCK) is probably a mistake. > This is sneaky. err is -ENODEV and would change to -1 on error, losing some meaning. > > + if (ret == -1) > > + goto error; > > + fp = fdopen(fd, "r"); > > + if (!fp) > > + goto error; > > + fd = -1; > > + /* Only take the last line into account */ > > + ret = 0; > > + while (fgets(output, sizeof(output), fp)) > > + ++ret; > > lcount = 0; > while (fgets(output, sizeof(output), fp)) > ++lcount; > > > > + if (feof(fp)) { > > + if (!ret) > > + goto error; > > + } else if (ferror(fp)) { > > + if (errno != EAGAIN || !ret) > > + goto error; > > + } else if (!ret) { > > + goto error; > > + } > > These branches seems needlessly complicated: > > if (lcount == 0) > goto error; > else if (ferror(fp) && errno != EAGAIN) > goto error; > Here err would have been set to 0 previously with the fcntl call, meaning that jumping to error would return 0 as well. I know Adrien wanted to avoid the usual ugly if (error) { err = -ENODEV; goto error; } But this kind of sneakiness is not easy to parse and maintain. If someone adds a new path of error later, this kind of subtlety *will* be lost. So between ugliness and maintainability, I choose maintainability (being the maintainer, of course). -- Gaëtan Rivet 6WIND