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

["Gaëtan Rivet" <gaetan.rivet@6wind.com>]

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