Hi Gaetan
OK for all, will change it.
From: Gaëtan Rivet, Tuesday, January 16, 2018 1:19 PM
> 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
