[dpdk-dev] [PATCH V1 0/2] examples/ethtool: fix MTU set and add MTU query

[dpdk-dev] [PATCH V1 0/2] examples/ethtool: fix MTU set and add MTU query

[Min Hu (Connor)]

From: Huisong Li <lihuisong@huawei.com>

This patchset fixes MTU data type when set MTU, and supports the query
of MTU.

Huisong Li (2):
  examples/ethtool: fix data type of MTU
  examples/ethtool: support the query of MTU

 examples/ethtool/ethtool-app/ethapp.c | 54 ++++++++++++++++++----------
 examples/ethtool/lib/rte_ethtool.c    | 16 ++++++++---
 examples/ethtool/lib/rte_ethtool.h    | 16 ++++++++++-
 3 files changed, 63 insertions(+), 23 deletions(-)

-- 
2.8.1

[dpdk-dev] [PATCH V1 1/2] examples/ethtool: fix data type of MTU

[Min Hu (Connor)]

From: Huisong Li <lihuisong@huawei.com>

This patch changes the data type of 'mtu' in rte_ethtool_net_change_mtu()
from 'int' to 'uint16_t'.

Fixes: bda68ab9d1e7 ("examples/ethtool: add user-space ethtool sample application")
Cc: stable@dpdk.org

Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
 examples/ethtool/ethtool-app/ethapp.c | 6 +-----
 examples/ethtool/lib/rte_ethtool.c    | 9 +++++----
 examples/ethtool/lib/rte_ethtool.h    | 2 +-
 3 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/examples/ethtool/ethtool-app/ethapp.c b/examples/ethtool/ethtool-app/ethapp.c
index 36a1c37..fc743ce 100644
--- a/examples/ethtool/ethtool-app/ethapp.c
+++ b/examples/ethtool/ethtool-app/ethapp.c
@@ -521,13 +521,9 @@ pcmd_mtu_callback(void *ptr_params,
 {
 	struct pcmd_intstr_params *params = ptr_params;
 	int stat;
-	int new_mtu;
+	uint16_t new_mtu;
 	char *ptr_parse_end;
 
-	if (!rte_eth_dev_is_valid_port(params->port)) {
-		printf("Error: Invalid port number %i\n", params->port);
-		return;
-	}
 	new_mtu = strtoul(params->opt, &ptr_parse_end, 10);
 	if (*ptr_parse_end != '\0' ||
 			new_mtu < RTE_ETHER_MIN_MTU ||
diff --git a/examples/ethtool/lib/rte_ethtool.c b/examples/ethtool/lib/rte_ethtool.c
index 4132516..73193ed 100644
--- a/examples/ethtool/lib/rte_ethtool.c
+++ b/examples/ethtool/lib/rte_ethtool.c
@@ -345,12 +345,13 @@ rte_ethtool_net_validate_addr(uint16_t port_id __rte_unused,
 	return rte_is_valid_assigned_ether_addr(addr);
 }
 
+
 int
-rte_ethtool_net_change_mtu(uint16_t port_id, int mtu)
+rte_ethtool_net_change_mtu(uint16_t port_id, uint16_t mtu)
 {
-	if (mtu < 0 || mtu > UINT16_MAX)
-		return -EINVAL;
-	return rte_eth_dev_set_mtu(port_id, (uint16_t)mtu);
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
+
+	return rte_eth_dev_set_mtu(port_id, mtu);
 }
 
 int
diff --git a/examples/ethtool/lib/rte_ethtool.h b/examples/ethtool/lib/rte_ethtool.h
index f177096..fe3250e 100644
--- a/examples/ethtool/lib/rte_ethtool.h
+++ b/examples/ethtool/lib/rte_ethtool.h
@@ -309,7 +309,7 @@ int rte_ethtool_net_validate_addr(uint16_t port_id,
  *   - (-EINVAL) if parameters invalid.
  *   - others depends on the specific operations implementation.
  */
-int rte_ethtool_net_change_mtu(uint16_t port_id, int mtu);
+int rte_ethtool_net_change_mtu(uint16_t port_id, uint16_t mtu);
 
 /**
  * Retrieve the Ethernet device traffic statistics
-- 
2.8.1

[dpdk-dev] [PATCH V1 2/2] examples/ethtool: support the query of MTU

[Min Hu (Connor)]

From: Huisong Li <lihuisong@huawei.com>

This patch supports the query of MTU.

Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
 examples/ethtool/ethtool-app/ethapp.c | 48 +++++++++++++++++++++++++----------
 examples/ethtool/lib/rte_ethtool.c    |  7 +++++
 examples/ethtool/lib/rte_ethtool.h    | 14 ++++++++++
 3 files changed, 56 insertions(+), 13 deletions(-)

diff --git a/examples/ethtool/ethtool-app/ethapp.c b/examples/ethtool/ethtool-app/ethapp.c
index fc743ce..aed4bc5 100644
--- a/examples/ethtool/ethtool-app/ethapp.c
+++ b/examples/ethtool/ethtool-app/ethapp.c
@@ -517,31 +517,41 @@ pcmd_macaddr_callback(void *ptr_params,
 static void
 pcmd_mtu_callback(void *ptr_params,
 	__rte_unused struct cmdline *ctx,
-	__rte_unused void *ptr_data)
+	void *ptr_data)
 {
 	struct pcmd_intstr_params *params = ptr_params;
 	int stat;
 	uint16_t new_mtu;
 	char *ptr_parse_end;
 
-	new_mtu = strtoul(params->opt, &ptr_parse_end, 10);
-	if (*ptr_parse_end != '\0' ||
-			new_mtu < RTE_ETHER_MIN_MTU ||
-			new_mtu > RTE_ETHER_MAX_JUMBO_FRAME_LEN) {
-		printf("Port %i: Invalid MTU value\n", params->port);
+	if (ptr_data == NULL) {
+		new_mtu = strtoul(params->opt, &ptr_parse_end, 10);
+		if (*ptr_parse_end != '\0' ||
+				new_mtu < RTE_ETHER_MIN_MTU ||
+				new_mtu > RTE_ETHER_MAX_JUMBO_FRAME_LEN) {
+			printf("Port %i: Invalid MTU value\n", params->port);
+			return;
+		}
+		stat = rte_ethtool_net_change_mtu(params->port, new_mtu);
+		if (stat == 0)
+			printf("Port %i: MTU set to %i\n", params->port,
+				new_mtu);
+		else if (stat == -ENOTSUP)
+			printf("Port %i: Operation not supported\n",
+				params->port);
+		else
+			printf("Port %i: Error setting MTU\n", params->port);
+
 		return;
 	}
-	stat = rte_ethtool_net_change_mtu(params->port, new_mtu);
-	if (stat == 0)
-		printf("Port %i: MTU set to %i\n", params->port, new_mtu);
-	else if (stat == -ENOTSUP)
+
+	stat = rte_ethtool_net_get_mtu(params->port, &new_mtu);
+	if (stat)
 		printf("Port %i: Operation not supported\n", params->port);
 	else
-		printf("Port %i: Error setting MTU\n", params->port);
+		printf("Port %i: Current MTU: %i\n", params->port, new_mtu);
 }
 
-
-
 static void pcmd_portstats_callback(__rte_unused void *ptr_params,
 	__rte_unused struct cmdline *ctx,
 	__rte_unused void *ptr_data)
@@ -799,6 +809,17 @@ cmdline_parse_inst_t pcmd_macaddr = {
 		NULL
 	},
 };
+cmdline_parse_inst_t pcmd_mtu_get = {
+	.f = pcmd_mtu_callback,
+	.data = (void *)0x01,
+	.help_str = "mtu <port_id>\n"
+		"     Get MTU",
+	.tokens = {
+		(void *)&pcmd_mtu_token_cmd,
+		(void *)&pcmd_intstr_token_port,
+		NULL
+	},
+};
 cmdline_parse_inst_t pcmd_mtu = {
 	.f = pcmd_mtu_callback,
 	.data = NULL,
@@ -879,6 +900,7 @@ cmdline_parse_ctx_t list_prompt_commands[] = {
 	(cmdline_parse_inst_t *)&pcmd_link,
 	(cmdline_parse_inst_t *)&pcmd_macaddr_get,
 	(cmdline_parse_inst_t *)&pcmd_macaddr,
+	(cmdline_parse_inst_t *)&pcmd_mtu_get,
 	(cmdline_parse_inst_t *)&pcmd_mtu,
 	(cmdline_parse_inst_t *)&pcmd_open,
 	(cmdline_parse_inst_t *)&pcmd_pause_noopt,
diff --git a/examples/ethtool/lib/rte_ethtool.c b/examples/ethtool/lib/rte_ethtool.c
index 73193ed..e69b2c1 100644
--- a/examples/ethtool/lib/rte_ethtool.c
+++ b/examples/ethtool/lib/rte_ethtool.c
@@ -345,6 +345,13 @@ rte_ethtool_net_validate_addr(uint16_t port_id __rte_unused,
 	return rte_is_valid_assigned_ether_addr(addr);
 }
 
+int
+rte_ethtool_net_get_mtu(uint16_t port_id, uint16_t *mtu)
+{
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
+
+	return rte_eth_dev_get_mtu(port_id, mtu);
+}
 
 int
 rte_ethtool_net_change_mtu(uint16_t port_id, uint16_t mtu)
diff --git a/examples/ethtool/lib/rte_ethtool.h b/examples/ethtool/lib/rte_ethtool.h
index fe3250e..434ad1d 100644
--- a/examples/ethtool/lib/rte_ethtool.h
+++ b/examples/ethtool/lib/rte_ethtool.h
@@ -23,6 +23,7 @@
  * rte_ethtool_net_stop:            net_device_ops::ndo_stop
  * rte_ethtool_net_set_mac_addr:    net_device_ops::ndo_set_mac_address
  * rte_ethtool_net_validate_addr:   net_device_ops::ndo_validate_addr
+ * rte_ethtool_net_get_mtu:         net_device_ops::ndo_get_mtu
  * rte_ethtool_net_change_mtu:      net_device_ops::ndo_change_mtu
  * rte_ethtool_net_get_stats64:     net_device_ops::ndo_get_stats64
  * rte_ethtool_net_vlan_rx_add_vid  net_device_ops::ndo_vlan_rx_add_vid
@@ -296,6 +297,19 @@ int rte_ethtool_net_validate_addr(uint16_t port_id,
 				struct rte_ether_addr *addr);
 
 /**
+ * Retrieve the MTU of an Ethernet device.
+ *
+ * @param port_id
+ *   The port identifier of the Ethernet device.
+ * @param mtu
+ *   A pointer to a uint16_t where the retrieved MTU is to be stored.
+ * @return
+ *   - (0) if successful.
+ *   - (-ENODEV) if *port_id* invalid.
+ */
+int rte_ethtool_net_get_mtu(uint16_t port_id, uint16_t *mtu);
+
+/**
  * Setting the Ethernet device maximum Tx unit.
  *
  * @param port_id
-- 
2.8.1

Re: [dpdk-dev] [PATCH V1 0/2] examples/ethtool: fix MTU set and add MTU query

[Min Hu (Connor)]

Hi, all,
	any comments?

在 2021/4/29 18:53, Min Hu (Connor) 写道:
> From: Huisong Li <lihuisong@huawei.com>
> 
> This patchset fixes MTU data type when set MTU, and supports the query
> of MTU.
> 
> Huisong Li (2):
>    examples/ethtool: fix data type of MTU
>    examples/ethtool: support the query of MTU
> 
>   examples/ethtool/ethtool-app/ethapp.c | 54 ++++++++++++++++++----------
>   examples/ethtool/lib/rte_ethtool.c    | 16 ++++++++---
>   examples/ethtool/lib/rte_ethtool.h    | 16 ++++++++++-
>   3 files changed, 63 insertions(+), 23 deletions(-)
> 

Re: [dpdk-dev] [PATCH V1 1/2] examples/ethtool: fix data type of MTU

[David Marchand]

On Thu, Apr 29, 2021 at 12:53 PM Min Hu (Connor) <humin29@huawei.com> wrote:
>
> From: Huisong Li <lihuisong@huawei.com>
>
> This patch changes the data type of 'mtu' in rte_ethtool_net_change_mtu()
> from 'int' to 'uint16_t'.

You did not describe why this change is needed.


>
> Fixes: bda68ab9d1e7 ("examples/ethtool: add user-space ethtool sample application")
> Cc: stable@dpdk.org
>
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> ---
>  examples/ethtool/ethtool-app/ethapp.c | 6 +-----
>  examples/ethtool/lib/rte_ethtool.c    | 9 +++++----
>  examples/ethtool/lib/rte_ethtool.h    | 2 +-
>  3 files changed, 7 insertions(+), 10 deletions(-)
>
> diff --git a/examples/ethtool/ethtool-app/ethapp.c b/examples/ethtool/ethtool-app/ethapp.c
> index 36a1c37..fc743ce 100644
> --- a/examples/ethtool/ethtool-app/ethapp.c
> +++ b/examples/ethtool/ethtool-app/ethapp.c
> @@ -521,13 +521,9 @@ pcmd_mtu_callback(void *ptr_params,
>  {
>         struct pcmd_intstr_params *params = ptr_params;
>         int stat;
> -       int new_mtu;
> +       uint16_t new_mtu;

strtoul can overflow the stack, when storing to new_mtu some lines below.
You should either change new_mtu to unsigned long int or switch
strtoul to some other integer format helper.


>         char *ptr_parse_end;
>
> -       if (!rte_eth_dev_is_valid_port(params->port)) {
> -               printf("Error: Invalid port number %i\n", params->port);
> -               return;
> -       }

Adding the check in lib/rte_ethtool.c is ok, but why not keep the
check in the application?


>         new_mtu = strtoul(params->opt, &ptr_parse_end, 10);
>         if (*ptr_parse_end != '\0' ||
>                         new_mtu < RTE_ETHER_MIN_MTU ||

With this patch, rte_ethtool_net_change_mtu() now takes a uint16_t.
You should add a check on new_mtu < UINT16_MAX.


> diff --git a/examples/ethtool/lib/rte_ethtool.c b/examples/ethtool/lib/rte_ethtool.c
> index 4132516..73193ed 100644
> --- a/examples/ethtool/lib/rte_ethtool.c
> +++ b/examples/ethtool/lib/rte_ethtool.c
> @@ -345,12 +345,13 @@ rte_ethtool_net_validate_addr(uint16_t port_id __rte_unused,
>         return rte_is_valid_assigned_ether_addr(addr);
>  }
>
> +
>  int
> -rte_ethtool_net_change_mtu(uint16_t port_id, int mtu)
> +rte_ethtool_net_change_mtu(uint16_t port_id, uint16_t mtu)
>  {
> -       if (mtu < 0 || mtu > UINT16_MAX)
> -               return -EINVAL;
> -       return rte_eth_dev_set_mtu(port_id, (uint16_t)mtu);
> +       RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +
> +       return rte_eth_dev_set_mtu(port_id, mtu);
>  }
>
>  int
> diff --git a/examples/ethtool/lib/rte_ethtool.h b/examples/ethtool/lib/rte_ethtool.h
> index f177096..fe3250e 100644
> --- a/examples/ethtool/lib/rte_ethtool.h
> +++ b/examples/ethtool/lib/rte_ethtool.h
> @@ -309,7 +309,7 @@ int rte_ethtool_net_validate_addr(uint16_t port_id,
>   *   - (-EINVAL) if parameters invalid.
>   *   - others depends on the specific operations implementation.
>   */
> -int rte_ethtool_net_change_mtu(uint16_t port_id, int mtu);
> +int rte_ethtool_net_change_mtu(uint16_t port_id, uint16_t mtu);
>
>  /**
>   * Retrieve the Ethernet device traffic statistics
> --
> 2.8.1
>


-- 
David Marchand

Re: [dpdk-dev] [PATCH V1 2/2] examples/ethtool: support the query of MTU

[David Marchand]

On Thu, Apr 29, 2021 at 12:53 PM Min Hu (Connor) <humin29@huawei.com> wrote:
>
> From: Huisong Li <lihuisong@huawei.com>
>
> This patch supports the query of MTU.
>
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> ---
>  examples/ethtool/ethtool-app/ethapp.c | 48 +++++++++++++++++++++++++----------
>  examples/ethtool/lib/rte_ethtool.c    |  7 +++++
>  examples/ethtool/lib/rte_ethtool.h    | 14 ++++++++++
>  3 files changed, 56 insertions(+), 13 deletions(-)
>
> diff --git a/examples/ethtool/ethtool-app/ethapp.c b/examples/ethtool/ethtool-app/ethapp.c
> index fc743ce..aed4bc5 100644
> --- a/examples/ethtool/ethtool-app/ethapp.c
> +++ b/examples/ethtool/ethtool-app/ethapp.c
> @@ -517,31 +517,41 @@ pcmd_macaddr_callback(void *ptr_params,
>  static void
>  pcmd_mtu_callback(void *ptr_params,
>         __rte_unused struct cmdline *ctx,
> -       __rte_unused void *ptr_data)
> +       void *ptr_data)
>  {
>         struct pcmd_intstr_params *params = ptr_params;
>         int stat;
>         uint16_t new_mtu;
>         char *ptr_parse_end;
>
> -       new_mtu = strtoul(params->opt, &ptr_parse_end, 10);
> -       if (*ptr_parse_end != '\0' ||
> -                       new_mtu < RTE_ETHER_MIN_MTU ||
> -                       new_mtu > RTE_ETHER_MAX_JUMBO_FRAME_LEN) {
> -               printf("Port %i: Invalid MTU value\n", params->port);
> +       if (ptr_data == NULL) {
> +               new_mtu = strtoul(params->opt, &ptr_parse_end, 10);
> +               if (*ptr_parse_end != '\0' ||
> +                               new_mtu < RTE_ETHER_MIN_MTU ||
> +                               new_mtu > RTE_ETHER_MAX_JUMBO_FRAME_LEN) {
> +                       printf("Port %i: Invalid MTU value\n", params->port);
> +                       return;
> +               }
> +               stat = rte_ethtool_net_change_mtu(params->port, new_mtu);
> +               if (stat == 0)
> +                       printf("Port %i: MTU set to %i\n", params->port,
> +                               new_mtu);
> +               else if (stat == -ENOTSUP)
> +                       printf("Port %i: Operation not supported\n",
> +                               params->port);
> +               else
> +                       printf("Port %i: Error setting MTU\n", params->port);
> +
>                 return;
>         }
> -       stat = rte_ethtool_net_change_mtu(params->port, new_mtu);
> -       if (stat == 0)
> -               printf("Port %i: MTU set to %i\n", params->port, new_mtu);
> -       else if (stat == -ENOTSUP)
> +
> +       stat = rte_ethtool_net_get_mtu(params->port, &new_mtu);
> +       if (stat)
>                 printf("Port %i: Operation not supported\n", params->port);
>         else
> -               printf("Port %i: Error setting MTU\n", params->port);
> +               printf("Port %i: Current MTU: %i\n", params->port, new_mtu);
>  }


Please separate as two helpers, there is no code shared between set
and get, afaics.


>
> -
> -
>  static void pcmd_portstats_callback(__rte_unused void *ptr_params,
>         __rte_unused struct cmdline *ctx,
>         __rte_unused void *ptr_data)
> @@ -799,6 +809,17 @@ cmdline_parse_inst_t pcmd_macaddr = {
>                 NULL
>         },
>  };
> +cmdline_parse_inst_t pcmd_mtu_get = {
> +       .f = pcmd_mtu_callback,
> +       .data = (void *)0x01,
> +       .help_str = "mtu <port_id>\n"
> +               "     Get MTU",
> +       .tokens = {
> +               (void *)&pcmd_mtu_token_cmd,
> +               (void *)&pcmd_intstr_token_port,
> +               NULL
> +       },
> +};
>  cmdline_parse_inst_t pcmd_mtu = {
>         .f = pcmd_mtu_callback,
>         .data = NULL,
> @@ -879,6 +900,7 @@ cmdline_parse_ctx_t list_prompt_commands[] = {
>         (cmdline_parse_inst_t *)&pcmd_link,
>         (cmdline_parse_inst_t *)&pcmd_macaddr_get,
>         (cmdline_parse_inst_t *)&pcmd_macaddr,
> +       (cmdline_parse_inst_t *)&pcmd_mtu_get,
>         (cmdline_parse_inst_t *)&pcmd_mtu,
>         (cmdline_parse_inst_t *)&pcmd_open,
>         (cmdline_parse_inst_t *)&pcmd_pause_noopt,
> diff --git a/examples/ethtool/lib/rte_ethtool.c b/examples/ethtool/lib/rte_ethtool.c
> index 73193ed..e69b2c1 100644
> --- a/examples/ethtool/lib/rte_ethtool.c
> +++ b/examples/ethtool/lib/rte_ethtool.c
> @@ -345,6 +345,13 @@ rte_ethtool_net_validate_addr(uint16_t port_id __rte_unused,
>         return rte_is_valid_assigned_ether_addr(addr);
>  }
>
> +int
> +rte_ethtool_net_get_mtu(uint16_t port_id, uint16_t *mtu)
> +{
> +       RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +
> +       return rte_eth_dev_get_mtu(port_id, mtu);
> +}
>
>  int
>  rte_ethtool_net_change_mtu(uint16_t port_id, uint16_t mtu)
> diff --git a/examples/ethtool/lib/rte_ethtool.h b/examples/ethtool/lib/rte_ethtool.h
> index fe3250e..434ad1d 100644
> --- a/examples/ethtool/lib/rte_ethtool.h
> +++ b/examples/ethtool/lib/rte_ethtool.h
> @@ -23,6 +23,7 @@
>   * rte_ethtool_net_stop:            net_device_ops::ndo_stop
>   * rte_ethtool_net_set_mac_addr:    net_device_ops::ndo_set_mac_address
>   * rte_ethtool_net_validate_addr:   net_device_ops::ndo_validate_addr
> + * rte_ethtool_net_get_mtu:         net_device_ops::ndo_get_mtu
>   * rte_ethtool_net_change_mtu:      net_device_ops::ndo_change_mtu
>   * rte_ethtool_net_get_stats64:     net_device_ops::ndo_get_stats64
>   * rte_ethtool_net_vlan_rx_add_vid  net_device_ops::ndo_vlan_rx_add_vid
> @@ -296,6 +297,19 @@ int rte_ethtool_net_validate_addr(uint16_t port_id,
>                                 struct rte_ether_addr *addr);
>
>  /**
> + * Retrieve the MTU of an Ethernet device.
> + *
> + * @param port_id
> + *   The port identifier of the Ethernet device.
> + * @param mtu
> + *   A pointer to a uint16_t where the retrieved MTU is to be stored.
> + * @return
> + *   - (0) if successful.
> + *   - (-ENODEV) if *port_id* invalid.
> + */
> +int rte_ethtool_net_get_mtu(uint16_t port_id, uint16_t *mtu);
> +
> +/**
>   * Setting the Ethernet device maximum Tx unit.
>   *
>   * @param port_id
> --
> 2.8.1
>


-- 
David Marchand

Re: [dpdk-dev] [PATCH V1 1/2] examples/ethtool: fix data type of MTU

[lihuisong (C)]

在 2021/11/18 1:49, David Marchand 写道:
> On Thu, Apr 29, 2021 at 12:53 PM Min Hu (Connor) <humin29@huawei.com> wrote:
>> From: Huisong Li <lihuisong@huawei.com>
>>
>> This patch changes the data type of 'mtu' in rte_ethtool_net_change_mtu()
>> from 'int' to 'uint16_t'.
> You did not describe why this change is needed.

Ensure that the input parameter of rte_ethtool_net_change_mtu() API in lib/rte_ethtool.c
re consistent with rte_eth_dev_set_mtu() in ethdev layer.
I will fix commit log.

>
>
>> Fixes: bda68ab9d1e7 ("examples/ethtool: add user-space ethtool sample application")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> ---
>>   examples/ethtool/ethtool-app/ethapp.c | 6 +-----
>>   examples/ethtool/lib/rte_ethtool.c    | 9 +++++----
>>   examples/ethtool/lib/rte_ethtool.h    | 2 +-
>>   3 files changed, 7 insertions(+), 10 deletions(-)
>>
>> diff --git a/examples/ethtool/ethtool-app/ethapp.c b/examples/ethtool/ethtool-app/ethapp.c
>> index 36a1c37..fc743ce 100644
>> --- a/examples/ethtool/ethtool-app/ethapp.c
>> +++ b/examples/ethtool/ethtool-app/ethapp.c
>> @@ -521,13 +521,9 @@ pcmd_mtu_callback(void *ptr_params,
>>   {
>>          struct pcmd_intstr_params *params = ptr_params;
>>          int stat;
>> -       int new_mtu;
>> +       uint16_t new_mtu;
> strtoul can overflow the stack, when storing to new_mtu some lines below.
> You should either change new_mtu to unsigned long int or switch
> strtoul to some other integer format helper.

You are right. I want add an mtu specific cmd parameter structure:

struct pcmd_mtu_params {
     cmdline_fixed_string_t cmd;
     uint16_t port;
     uint16_t mtu_value;
};

MTU size is limited when users enter the value.

Like:

cmdline_parse_token_num_t pcmd_mtu_token_port =
     TOKEN_NUM_INITIALIZER(struct pcmd_mtu_params, mtu_value, RTE_UINT16);

What do you think, David?

>
>>          char *ptr_parse_end;
>>
>> -       if (!rte_eth_dev_is_valid_port(params->port)) {
>> -               printf("Error: Invalid port number %i\n", params->port);
>> -               return;
>> -       }
> Adding the check in lib/rte_ethtool.c is ok, but why not keep the
> check in the application?
All right, let it stay here.
>
>
>>          new_mtu = strtoul(params->opt, &ptr_parse_end, 10);
>>          if (*ptr_parse_end != '\0' ||
>>                          new_mtu < RTE_ETHER_MIN_MTU ||
> With this patch, rte_ethtool_net_change_mtu() now takes a uint16_t.
> You should add a check on new_mtu < UINT16_MAX.
>
If we use above new method, this check can be removed.
>> diff --git a/examples/ethtool/lib/rte_ethtool.c b/examples/ethtool/lib/rte_ethtool.c
>> index 4132516..73193ed 100644
>> --- a/examples/ethtool/lib/rte_ethtool.c
>> +++ b/examples/ethtool/lib/rte_ethtool.c
>> @@ -345,12 +345,13 @@ rte_ethtool_net_validate_addr(uint16_t port_id __rte_unused,
>>          return rte_is_valid_assigned_ether_addr(addr);
>>   }
>>
>> +
>>   int
>> -rte_ethtool_net_change_mtu(uint16_t port_id, int mtu)
>> +rte_ethtool_net_change_mtu(uint16_t port_id, uint16_t mtu)
>>   {
>> -       if (mtu < 0 || mtu > UINT16_MAX)
>> -               return -EINVAL;
>> -       return rte_eth_dev_set_mtu(port_id, (uint16_t)mtu);
>> +       RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> +       return rte_eth_dev_set_mtu(port_id, mtu);
>>   }
>>
>>   int
>> diff --git a/examples/ethtool/lib/rte_ethtool.h b/examples/ethtool/lib/rte_ethtool.h
>> index f177096..fe3250e 100644
>> --- a/examples/ethtool/lib/rte_ethtool.h
>> +++ b/examples/ethtool/lib/rte_ethtool.h
>> @@ -309,7 +309,7 @@ int rte_ethtool_net_validate_addr(uint16_t port_id,
>>    *   - (-EINVAL) if parameters invalid.
>>    *   - others depends on the specific operations implementation.
>>    */
>> -int rte_ethtool_net_change_mtu(uint16_t port_id, int mtu);
>> +int rte_ethtool_net_change_mtu(uint16_t port_id, uint16_t mtu);
>>
>>   /**
>>    * Retrieve the Ethernet device traffic statistics
>> --
>> 2.8.1
>>
>

Re: [dpdk-dev] [PATCH V1 2/2] examples/ethtool: support the query of MTU

[lihuisong (C)]

在 2021/11/18 1:51, David Marchand 写道:
> On Thu, Apr 29, 2021 at 12:53 PM Min Hu (Connor) <humin29@huawei.com> wrote:
>> From: Huisong Li <lihuisong@huawei.com>
>>
>> This patch supports the query of MTU.
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> ---
>>   examples/ethtool/ethtool-app/ethapp.c | 48 +++++++++++++++++++++++++----------
>>   examples/ethtool/lib/rte_ethtool.c    |  7 +++++
>>   examples/ethtool/lib/rte_ethtool.h    | 14 ++++++++++
>>   3 files changed, 56 insertions(+), 13 deletions(-)
>>
>> diff --git a/examples/ethtool/ethtool-app/ethapp.c b/examples/ethtool/ethtool-app/ethapp.c
>> index fc743ce..aed4bc5 100644
>> --- a/examples/ethtool/ethtool-app/ethapp.c
>> +++ b/examples/ethtool/ethtool-app/ethapp.c
>> @@ -517,31 +517,41 @@ pcmd_macaddr_callback(void *ptr_params,
>>   static void
>>   pcmd_mtu_callback(void *ptr_params,
>>          __rte_unused struct cmdline *ctx,
>> -       __rte_unused void *ptr_data)
>> +       void *ptr_data)
>>   {
>>          struct pcmd_intstr_params *params = ptr_params;
>>          int stat;
>>          uint16_t new_mtu;
>>          char *ptr_parse_end;
>>
>> -       new_mtu = strtoul(params->opt, &ptr_parse_end, 10);
>> -       if (*ptr_parse_end != '\0' ||
>> -                       new_mtu < RTE_ETHER_MIN_MTU ||
>> -                       new_mtu > RTE_ETHER_MAX_JUMBO_FRAME_LEN) {
>> -               printf("Port %i: Invalid MTU value\n", params->port);
>> +       if (ptr_data == NULL) {
>> +               new_mtu = strtoul(params->opt, &ptr_parse_end, 10);
>> +               if (*ptr_parse_end != '\0' ||
>> +                               new_mtu < RTE_ETHER_MIN_MTU ||
>> +                               new_mtu > RTE_ETHER_MAX_JUMBO_FRAME_LEN) {
>> +                       printf("Port %i: Invalid MTU value\n", params->port);
>> +                       return;
>> +               }
>> +               stat = rte_ethtool_net_change_mtu(params->port, new_mtu);
>> +               if (stat == 0)
>> +                       printf("Port %i: MTU set to %i\n", params->port,
>> +                               new_mtu);
>> +               else if (stat == -ENOTSUP)
>> +                       printf("Port %i: Operation not supported\n",
>> +                               params->port);
>> +               else
>> +                       printf("Port %i: Error setting MTU\n", params->port);
>> +
>>                  return;
>>          }
>> -       stat = rte_ethtool_net_change_mtu(params->port, new_mtu);
>> -       if (stat == 0)
>> -               printf("Port %i: MTU set to %i\n", params->port, new_mtu);
>> -       else if (stat == -ENOTSUP)
>> +
>> +       stat = rte_ethtool_net_get_mtu(params->port, &new_mtu);
>> +       if (stat)
>>                  printf("Port %i: Operation not supported\n", params->port);
>>          else
>> -               printf("Port %i: Error setting MTU\n", params->port);
>> +               printf("Port %i: Current MTU: %i\n", params->port, new_mtu);
>>   }
>
> Please separate as two helpers, there is no code shared between set
> and get, afaics.
>
The main purpose of my modification is to be consistent with other 
command styles.

Like: pcmd_ringparam & pcmd_ringparam_set and pcmd_macaddr & 
pcmd_macaddr_get.

Do you mean to use a separate interface to implement MTU query?

>> -
>> -
>>   static void pcmd_portstats_callback(__rte_unused void *ptr_params,
>>          __rte_unused struct cmdline *ctx,
>>          __rte_unused void *ptr_data)
>> @@ -799,6 +809,17 @@ cmdline_parse_inst_t pcmd_macaddr = {
>>                  NULL
>>          },
>>   };
>> +cmdline_parse_inst_t pcmd_mtu_get = {
>> +       .f = pcmd_mtu_callback,
>> +       .data = (void *)0x01,
>> +       .help_str = "mtu <port_id>\n"
>> +               "     Get MTU",
>> +       .tokens = {
>> +               (void *)&pcmd_mtu_token_cmd,
>> +               (void *)&pcmd_intstr_token_port,
>> +               NULL
>> +       },
>> +};
>>   cmdline_parse_inst_t pcmd_mtu = {
>>          .f = pcmd_mtu_callback,
>>          .data = NULL,
>> @@ -879,6 +900,7 @@ cmdline_parse_ctx_t list_prompt_commands[] = {
>>          (cmdline_parse_inst_t *)&pcmd_link,
>>          (cmdline_parse_inst_t *)&pcmd_macaddr_get,
>>          (cmdline_parse_inst_t *)&pcmd_macaddr,
>> +       (cmdline_parse_inst_t *)&pcmd_mtu_get,
>>          (cmdline_parse_inst_t *)&pcmd_mtu,
>>          (cmdline_parse_inst_t *)&pcmd_open,
>>          (cmdline_parse_inst_t *)&pcmd_pause_noopt,
>> diff --git a/examples/ethtool/lib/rte_ethtool.c b/examples/ethtool/lib/rte_ethtool.c
>> index 73193ed..e69b2c1 100644
>> --- a/examples/ethtool/lib/rte_ethtool.c
>> +++ b/examples/ethtool/lib/rte_ethtool.c
>> @@ -345,6 +345,13 @@ rte_ethtool_net_validate_addr(uint16_t port_id __rte_unused,
>>          return rte_is_valid_assigned_ether_addr(addr);
>>   }
>>
>> +int
>> +rte_ethtool_net_get_mtu(uint16_t port_id, uint16_t *mtu)
>> +{
>> +       RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> +       return rte_eth_dev_get_mtu(port_id, mtu);
>> +}
>>
>>   int
>>   rte_ethtool_net_change_mtu(uint16_t port_id, uint16_t mtu)
>> diff --git a/examples/ethtool/lib/rte_ethtool.h b/examples/ethtool/lib/rte_ethtool.h
>> index fe3250e..434ad1d 100644
>> --- a/examples/ethtool/lib/rte_ethtool.h
>> +++ b/examples/ethtool/lib/rte_ethtool.h
>> @@ -23,6 +23,7 @@
>>    * rte_ethtool_net_stop:            net_device_ops::ndo_stop
>>    * rte_ethtool_net_set_mac_addr:    net_device_ops::ndo_set_mac_address
>>    * rte_ethtool_net_validate_addr:   net_device_ops::ndo_validate_addr
>> + * rte_ethtool_net_get_mtu:         net_device_ops::ndo_get_mtu
>>    * rte_ethtool_net_change_mtu:      net_device_ops::ndo_change_mtu
>>    * rte_ethtool_net_get_stats64:     net_device_ops::ndo_get_stats64
>>    * rte_ethtool_net_vlan_rx_add_vid  net_device_ops::ndo_vlan_rx_add_vid
>> @@ -296,6 +297,19 @@ int rte_ethtool_net_validate_addr(uint16_t port_id,
>>                                  struct rte_ether_addr *addr);
>>
>>   /**
>> + * Retrieve the MTU of an Ethernet device.
>> + *
>> + * @param port_id
>> + *   The port identifier of the Ethernet device.
>> + * @param mtu
>> + *   A pointer to a uint16_t where the retrieved MTU is to be stored.
>> + * @return
>> + *   - (0) if successful.
>> + *   - (-ENODEV) if *port_id* invalid.
>> + */
>> +int rte_ethtool_net_get_mtu(uint16_t port_id, uint16_t *mtu);
>> +
>> +/**
>>    * Setting the Ethernet device maximum Tx unit.
>>    *
>>    * @param port_id
>> --
>> 2.8.1
>>
>

Re: [dpdk-dev] [PATCH V1 0/2] examples/ethtool: fix MTU set and add MTU query

[Stephen Hemminger]

On Mon, 28 Jun 2021 11:23:12 +0800
"Min Hu (Connor)" <humin29@huawei.com> wrote:

> Hi, all,
> 	any comments?
> 
> 在 2021/4/29 18:53, Min Hu (Connor) 写道:
> > From: Huisong Li <lihuisong@huawei.com>
> > 
> > This patchset fixes MTU data type when set MTU, and supports the query
> > of MTU.
> > 
> > Huisong Li (2):
> >    examples/ethtool: fix data type of MTU
> >    examples/ethtool: support the query of MTU
> > 
> >   examples/ethtool/ethtool-app/ethapp.c | 54 ++++++++++++++++++----------
> >   examples/ethtool/lib/rte_ethtool.c    | 16 ++++++++---
> >   examples/ethtool/lib/rte_ethtool.h    | 16 ++++++++++-
> >   3 files changed, 63 insertions(+), 23 deletions(-)
> >   

There were several review comments.
In patch 1, the comment was that strtoul will return unsigned long.
Therefore new_mtu should be of type unsigned long, and check that is
is less than UINT16_MAX.

For the second patch, David's comment was that get and set should
logically be separate functions. The overlap was poor design in original
ethtool program, and lets not repeat that.

Overall, not many people looked at this patch because the ethtool
example is one of those "throw it over the wall and forget it" applications
that originally came from Cisco, and has seen little interest since then.