[dpdk-dev] [PATCH v3 2/8] net/failsafe: add "fd" parameter
Gaëtan Rivet
gaetan.rivet at 6wind.com
Tue Jan 16 12:19:08 CET 2018
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 at 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
More information about the dev
mailing list