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

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

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.

> +        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;

> +        /* Line must end with a newline character */
> +        fs_sanitize_cmdline(output);
> +        if (output[0] == '\0')
> +                goto error;
> +        ret = fs_parse_device(sdev, output);
> +        if (ret)
> +                ERROR("Parsing device '%s' failed", output);
> +        err = ret;

no need to use ret instead of err here?

           err = fs_parse_device(sdev, output);
           if (err)
                   ERROR("Parsing device '%s' failed", output);

Thus allowing to remove the "ret" variable completely.

> +error:
> +        if (fp)
> +                fclose(fp);
> +        if (fd != -1)
> +                close(fd);
> +        return err;
> +}
> +
> +static int
>  fs_parse_device_param(struct rte_eth_dev *dev, const char *param,
>                  uint8_t head)
>  {
> @@ -202,6 +273,14 @@ typedef int (parse_cb)(struct rte_eth_dev *dev, const char *params,
>                  }
>                  if (ret)
>                          goto free_args;
> +        } else if (strncmp(param, "fd", 2) == 0) {

How about strncmp(param, "fd(", 3) == 0 here?
I think I made a mistake for dev and exec device types, no reason at
this point to reiterate for fd as well.

> +                ret = fs_read_fd(sdev, args);
> +                if (ret == -ENODEV) {
> +                        DEBUG("Reading device info from FD failed");
> +                        ret = 0;
> +                }
> +                if (ret)
> +                        goto free_args;
>          } else {
>                  ERROR("Unrecognized device type: %.*s", (int)b, param);
>                  return -EINVAL;
> @@ -409,6 +488,8 @@ typedef int (parse_cb)(struct rte_eth_dev *dev, const char *params,
>          FOREACH_SUBDEV(sdev, i, dev) {
>                  free(sdev->cmdline);
>                  sdev->cmdline = NULL;
> +                free(sdev->fd_str);
> +                sdev->fd_str = NULL;
>                  free(sdev->devargs.args);
>                  sdev->devargs.args = NULL;
>          }
> @@ -424,7 +505,8 @@ typedef int (parse_cb)(struct rte_eth_dev *dev, const char *params,
>                  param[b] != '\0')
>                  b++;
>          if (strncmp(param, "dev", b) != 0 &&
> -            strncmp(param, "exec", b) != 0) {
> +            strncmp(param, "exec", b) != 0 &&
> +            strncmp(param, "fd", b) != 0) {

If the strncmp above is modified, this one should be as well for
consistency.

-- 
Gaëtan Rivet
6WIND