Re: [dpdk-dev] [PATCH v3 2/8] net/failsafe: add "fd" parameter

[Matan Azrad <matan@mellanox.com>]

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.mazarguil@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