[dpdk-dev,v3,2/8] net/failsafe: add "fd" parameter
Checks
Commit Message
From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
This parameter enables applications to provide device definitions through
an arbitrary file descriptor number.
Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
Cc: Gaetan Rivet <gaetan.rivet@6wind.com>
---
doc/guides/nics/fail_safe.rst | 9 ++++
drivers/net/failsafe/failsafe_args.c | 86 ++++++++++++++++++++++++++++++++-
drivers/net/failsafe/failsafe_private.h | 3 ++
3 files changed, 97 insertions(+), 1 deletion(-)
Comments
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.
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).
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
@@ -106,6 +106,15 @@ Fail-safe command line parameters
All commas within the ``shell command`` are replaced by spaces before
executing the command. This helps using scripts to specify devices.
+- **fd(<file descriptor number>)** parameter
+
+ This parameter reads a device definition from an arbitrary file descriptor
+ number in ``<iface>`` format as described above.
+
+ The file descriptor is read in non-blocking mode and is never closed in
+ order to take only the last line into account (unlike ``exec()``) at every
+ probe attempt.
+
- **mac** parameter [MAC address]
This parameter allows the user to set a default MAC address to the fail-safe
@@ -31,7 +31,11 @@
* OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*/
+#include <fcntl.h>
+#include <stdio.h>
+#include <stdlib.h>
#include <string.h>
+#include <unistd.h>
#include <errno.h>
#include <rte_debug.h>
@@ -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;
+
+ 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);
+ if (ret == -1)
+ goto error;
+ ret = fcntl(fd, F_SETFL, fd | O_NONBLOCK);
+ 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;
+ if (feof(fp)) {
+ if (!ret)
+ goto error;
+ } else if (ferror(fp)) {
+ if (errno != EAGAIN || !ret)
+ goto error;
+ } else if (!ret) {
+ 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;
+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) {
+ 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) {
ERROR("Unrecognized device type: %.*s", (int)b, param);
return -EINVAL;
}
@@ -463,6 +545,8 @@ typedef int (parse_cb)(struct rte_eth_dev *dev, const char *params,
continue;
if (sdev->cmdline)
ret = fs_execute_cmd(sdev, sdev->cmdline);
+ else if (sdev->fd_str)
+ ret = fs_read_fd(sdev, sdev->fd_str);
else
ret = fs_parse_sub_device(sdev);
if (ret == 0)
@@ -48,6 +48,7 @@
#define PMD_FAILSAFE_PARAM_STRING \
"dev(<ifc>)," \
"exec(<shell command>)," \
+ "fd(<fd number>)," \
"mac=mac_addr," \
"hotplug_poll=u64" \
""
@@ -111,6 +112,8 @@ struct sub_device {
struct fs_stats stats_snapshot;
/* Some device are defined as a command line */
char *cmdline;
+ /* Others are retrieved through a file descriptor */
+ char *fd_str;
/* fail-safe device backreference */
struct rte_eth_dev *fs_dev;
/* flag calling for recollection */