[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
>>
>