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

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

Hi Matan,

You forgot to fix the fcntl call, see below,

On Thu, Jan 18, 2018 at 08:43:40AM +0000, Matan Azrad wrote:
> This parameter enables applications to provide device definitions through
> an arbitrary file descriptor number.
> 
> Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Signed-off-by: Matan Azrad <matan@mellanox.com>

with the relevant fixes:
Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>

> ---
>  doc/guides/nics/fail_safe.rst           |  9 ++++
>  drivers/net/failsafe/failsafe_args.c    | 80 ++++++++++++++++++++++++++++++++-
>  drivers/net/failsafe/failsafe_private.h |  3 ++
>  3 files changed, 91 insertions(+), 1 deletion(-)
> 
> diff --git a/doc/guides/nics/fail_safe.rst b/doc/guides/nics/fail_safe.rst
> index c4e3d2e..5b1b47e 100644
> --- a/doc/guides/nics/fail_safe.rst
> +++ b/doc/guides/nics/fail_safe.rst
> @@ -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
> diff --git a/drivers/net/failsafe/failsafe_args.c b/drivers/net/failsafe/failsafe_args.c
> index ec63ac9..db5235b 100644
> --- a/drivers/net/failsafe/failsafe_args.c
> +++ b/drivers/net/failsafe/failsafe_args.c
> @@ -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,67 @@ 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 oflags;
> +	int lcount;
> +
> +	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;
> +	oflags = fcntl(fd, F_GETFL);
> +	if (oflags == -1)
> +		goto error;
> +	if (fcntl(fd, F_SETFL, fd | O_NONBLOCK) == -1)

fcntl(fd, F_SETFL, oflags | O_NONBLOCK); here

> +		goto error;
> +	fp = fdopen(fd, "r");
> +	if (!fp)

While you're at it, here please use

    if (fp != NULL)

instead.

Regards,
-- 
Gaëtan Rivet
6WIND