Re: [ovs-dev] [PATCH] netdev: Add 'errp' to set_config().

[Loftus, Ciara]

> 
> Since 55e075e65ef9("netdev-dpdk: Arbitrary 'dpdk' port naming"),
> set_config() is used to identify a DPDK device, so it's better to report
> its detailed error message to the user.  Tunnel devices and patch ports
> rely a lot on set_config() as well.
> 
> This commit adds a param to set_config() that can be used to return
> an error message and makes use of that in netdev-dpdk and netdev-vport.
> 
> Before this patch:
> 
> $ ovs-vsctl add-port br0 dpdk0 -- set Interface dpdk0 type=dpdk
> ovs-vsctl: Error detected while setting up 'dpdk0': dpdk0: could not set
> configuration (Invalid argument).  See ovs-vswitchd log for details.
> ovs-vsctl: The default log directory is "/var/log/openvswitch/".
> 
> $ ovs-vsctl add-port br0 p+ -- set Interface p+ type=patch
> ovs-vsctl: Error detected while setting up 'p+': p+: could not set 
> configuration
> (Invalid argument).  See ovs-vswitchd log for details.
> ovs-vsctl: The default log directory is "/var/log/openvswitch/".
> 
> $ ovs-vsctl add-port br0 gnv0 -- set Interface gnv0 type=geneve
> ovs-vsctl: Error detected while setting up 'gnv0': gnv0: could not set
> configuration (Invalid argument).  See ovs-vswitchd log for details.
> ovs-vsctl: The default log directory is "/var/log/openvswitch/".
> 
> After this patch:
> 
> $ ovs-vsctl add-port br0 dpdk0 -- set Interface dpdk0 type=dpdk
> ovs-vsctl: Error detected while setting up 'dpdk0': 'dpdk0' is missing
> 'options:dpdk-devargs'. The old 'dpdk' names are not supported.
> See ovs-vswitchd log for details.
> ovs-vsctl: The default log directory is "/var/log/openvswitch/".
> 
> $ ovs-vsctl add-port br0 p+ -- set Interface p+ type=patch
> ovs-vsctl: Error detected while setting up 'p+': p+: patch type requires valid
> 'peer' argument.  See ovs-vswitchd log for details.
> ovs-vsctl: The default log directory is "/var/log/openvswitch/".
> 
> $ ovs-vsctl add-port br0 gnv0 -- set Interface gnv0 type=geneve
> ovs-vsctl: Error detected while setting up 'gnv0': gnv0: geneve type requires
> valid 'remote_ip' argument.  See ovs-vswitchd log for details.
> ovs-vsctl: The default log directory is "/var/log/openvswitch/".
> 
> CC: Ciara Loftus 
> CC: Kevin Traynor 
> Signed-off-by: Daniele Di Proietto 
> ---
>  lib/netdev-dpdk.c | 27 ++
>  lib/netdev-dummy.c|  3 +-
>  lib/netdev-provider.h |  9 --
>  lib/netdev-vport.c| 76 ++
> -
>  lib/netdev.c  | 10 +--
>  5 files changed, 84 insertions(+), 41 deletions(-)
> 
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 0f02c4d74..1bcc27a62 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -1132,7 +1132,7 @@ netdev_dpdk_lookup_by_port_id(int port_id)
>  }
> 
>  static int
> -netdev_dpdk_process_devargs(const char *devargs)
> +netdev_dpdk_process_devargs(const char *devargs, char **errp)
>  {
>  uint8_t new_port_id = UINT8_MAX;
> 
> @@ -1145,7 +1145,7 @@ netdev_dpdk_process_devargs(const char
> *devargs)
>  VLOG_INFO("Device '%s' attached to DPDK", devargs);
>  } else {
>  /* Attach unsuccessful */
> -VLOG_INFO("Error attaching device '%s' to DPDK", devargs);
> +VLOG_WARN_BUF(errp, "Error attaching device '%s' to DPDK",
> devargs);
>  return -1;
>  }
>  }
> @@ -1184,7 +1184,8 @@ dpdk_process_queue_size(struct netdev *netdev,
> const struct smap *args,
>  }
> 
>  static int
> -netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args)
> +netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args,
> +   char **errp)
>  {
>  struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
>  bool rx_fc_en, tx_fc_en, autoneg;
> @@ -1225,7 +1226,7 @@ netdev_dpdk_set_config(struct netdev *netdev,
> const struct smap *args)
>   * is valid */
>  if (!(dev->devargs && !strcmp(dev->devargs, new_devargs)
> && rte_eth_dev_is_valid_port(dev->port_id))) {
> -int new_port_id = netdev_dpdk_process_devargs(new_devargs);
> +int new_port_id = netdev_dpdk_process_devargs(new_devargs,
> errp);
>  if (!rte_eth_dev_is_valid_port(new_port_id)) {
>  err = EINVAL;
>  } else if (new_port_id == dev->port_id) {
> @@ -1235,10 +1236,10 @@ netdev_dpdk_set_config(struct netdev *netdev,
> const struct smap *args)
>  struct netdev_dpdk *dup_dev;
>  dup_dev = netdev_dpdk_lookup_by_port_id(new_port_id);
>  if (dup_dev) {
> -VLOG_WARN("'%s' is trying to use device '%s' which is "
> -  "already in use by '%s'.",
> -  netdev_get_name(netdev), new_devargs,
> -  netdev_get_name(_dev->up));
> +VLOG_WARN_BUF(errp, "'%s' is trying to use device '%s' "
> +  

Re: [ovs-dev] [PATCH] netdev: Add 'errp' to set_config().

[Kevin Traynor]

On 01/07/2017 01:24 AM, Daniele Di Proietto wrote:
> Since 55e075e65ef9("netdev-dpdk: Arbitrary 'dpdk' port naming"),
> set_config() is used to identify a DPDK device, so it's better to report
> its detailed error message to the user.  Tunnel devices and patch ports
> rely a lot on set_config() as well.
> 
> This commit adds a param to set_config() that can be used to return
> an error message and makes use of that in netdev-dpdk and netdev-vport.
> 
> Before this patch:
> 
> $ ovs-vsctl add-port br0 dpdk0 -- set Interface dpdk0 type=dpdk
> ovs-vsctl: Error detected while setting up 'dpdk0': dpdk0: could not set 
> configuration (Invalid argument).  See ovs-vswitchd log for details.
> ovs-vsctl: The default log directory is "/var/log/openvswitch/".
> 
> $ ovs-vsctl add-port br0 p+ -- set Interface p+ type=patch
> ovs-vsctl: Error detected while setting up 'p+': p+: could not set 
> configuration (Invalid argument).  See ovs-vswitchd log for details.
> ovs-vsctl: The default log directory is "/var/log/openvswitch/".
> 
> $ ovs-vsctl add-port br0 gnv0 -- set Interface gnv0 type=geneve
> ovs-vsctl: Error detected while setting up 'gnv0': gnv0: could not set 
> configuration (Invalid argument).  See ovs-vswitchd log for details.
> ovs-vsctl: The default log directory is "/var/log/openvswitch/".
> 
> After this patch:
> 
> $ ovs-vsctl add-port br0 dpdk0 -- set Interface dpdk0 type=dpdk
> ovs-vsctl: Error detected while setting up 'dpdk0': 'dpdk0' is missing 
> 'options:dpdk-devargs'. The old 'dpdk' names are not supported.  See 
> ovs-vswitchd log for details.
> ovs-vsctl: The default log directory is "/var/log/openvswitch/".
> 
> $ ovs-vsctl add-port br0 p+ -- set Interface p+ type=patch
> ovs-vsctl: Error detected while setting up 'p+': p+: patch type requires 
> valid 'peer' argument.  See ovs-vswitchd log for details.
> ovs-vsctl: The default log directory is "/var/log/openvswitch/".
> 
> $ ovs-vsctl add-port br0 gnv0 -- set Interface gnv0 type=geneve
> ovs-vsctl: Error detected while setting up 'gnv0': gnv0: geneve type requires 
> valid 'remote_ip' argument.  See ovs-vswitchd log for details.
> ovs-vsctl: The default log directory is "/var/log/openvswitch/".
> 
> CC: Ciara Loftus 
> CC: Kevin Traynor 
> Signed-off-by: Daniele Di Proietto 
> ---
>  lib/netdev-dpdk.c | 27 ++
>  lib/netdev-dummy.c|  3 +-
>  lib/netdev-provider.h |  9 --
>  lib/netdev-vport.c| 76 
> ++-
>  lib/netdev.c  | 10 +--
>  5 files changed, 84 insertions(+), 41 deletions(-)
> 

Note the commit message lines get truncated in git log, you may want to
wrap them. It's much more intuitive now, thanks.

Acked-by: Kevin Traynor 

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] netdev: Add 'errp' to set_config().

[Daniele Di Proietto]

Since 55e075e65ef9("netdev-dpdk: Arbitrary 'dpdk' port naming"),
set_config() is used to identify a DPDK device, so it's better to report
its detailed error message to the user.  Tunnel devices and patch ports
rely a lot on set_config() as well.

This commit adds a param to set_config() that can be used to return
an error message and makes use of that in netdev-dpdk and netdev-vport.

Before this patch:

$ ovs-vsctl add-port br0 dpdk0 -- set Interface dpdk0 type=dpdk
ovs-vsctl: Error detected while setting up 'dpdk0': dpdk0: could not set 
configuration (Invalid argument).  See ovs-vswitchd log for details.
ovs-vsctl: The default log directory is "/var/log/openvswitch/".

$ ovs-vsctl add-port br0 p+ -- set Interface p+ type=patch
ovs-vsctl: Error detected while setting up 'p+': p+: could not set 
configuration (Invalid argument).  See ovs-vswitchd log for details.
ovs-vsctl: The default log directory is "/var/log/openvswitch/".

$ ovs-vsctl add-port br0 gnv0 -- set Interface gnv0 type=geneve
ovs-vsctl: Error detected while setting up 'gnv0': gnv0: could not set 
configuration (Invalid argument).  See ovs-vswitchd log for details.
ovs-vsctl: The default log directory is "/var/log/openvswitch/".

After this patch:

$ ovs-vsctl add-port br0 dpdk0 -- set Interface dpdk0 type=dpdk
ovs-vsctl: Error detected while setting up 'dpdk0': 'dpdk0' is missing 
'options:dpdk-devargs'. The old 'dpdk' names are not supported.  See 
ovs-vswitchd log for details.
ovs-vsctl: The default log directory is "/var/log/openvswitch/".

$ ovs-vsctl add-port br0 p+ -- set Interface p+ type=patch
ovs-vsctl: Error detected while setting up 'p+': p+: patch type requires valid 
'peer' argument.  See ovs-vswitchd log for details.
ovs-vsctl: The default log directory is "/var/log/openvswitch/".

$ ovs-vsctl add-port br0 gnv0 -- set Interface gnv0 type=geneve
ovs-vsctl: Error detected while setting up 'gnv0': gnv0: geneve type requires 
valid 'remote_ip' argument.  See ovs-vswitchd log for details.
ovs-vsctl: The default log directory is "/var/log/openvswitch/".

CC: Ciara Loftus 
CC: Kevin Traynor 
Signed-off-by: Daniele Di Proietto 
---
 lib/netdev-dpdk.c | 27 ++
 lib/netdev-dummy.c|  3 +-
 lib/netdev-provider.h |  9 --
 lib/netdev-vport.c| 76 ++-
 lib/netdev.c  | 10 +--
 5 files changed, 84 insertions(+), 41 deletions(-)

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 0f02c4d74..1bcc27a62 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -1132,7 +1132,7 @@ netdev_dpdk_lookup_by_port_id(int port_id)
 }
 
 static int
-netdev_dpdk_process_devargs(const char *devargs)
+netdev_dpdk_process_devargs(const char *devargs, char **errp)
 {
 uint8_t new_port_id = UINT8_MAX;
 
@@ -1145,7 +1145,7 @@ netdev_dpdk_process_devargs(const char *devargs)
 VLOG_INFO("Device '%s' attached to DPDK", devargs);
 } else {
 /* Attach unsuccessful */
-VLOG_INFO("Error attaching device '%s' to DPDK", devargs);
+VLOG_WARN_BUF(errp, "Error attaching device '%s' to DPDK", 
devargs);
 return -1;
 }
 }
@@ -1184,7 +1184,8 @@ dpdk_process_queue_size(struct netdev *netdev, const 
struct smap *args,
 }
 
 static int
-netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args)
+netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args,
+   char **errp)
 {
 struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
 bool rx_fc_en, tx_fc_en, autoneg;
@@ -1225,7 +1226,7 @@ netdev_dpdk_set_config(struct netdev *netdev, const 
struct smap *args)
  * is valid */
 if (!(dev->devargs && !strcmp(dev->devargs, new_devargs)
&& rte_eth_dev_is_valid_port(dev->port_id))) {
-int new_port_id = netdev_dpdk_process_devargs(new_devargs);
+int new_port_id = netdev_dpdk_process_devargs(new_devargs, errp);
 if (!rte_eth_dev_is_valid_port(new_port_id)) {
 err = EINVAL;
 } else if (new_port_id == dev->port_id) {
@@ -1235,10 +1236,10 @@ netdev_dpdk_set_config(struct netdev *netdev, const 
struct smap *args)
 struct netdev_dpdk *dup_dev;
 dup_dev = netdev_dpdk_lookup_by_port_id(new_port_id);
 if (dup_dev) {
-VLOG_WARN("'%s' is trying to use device '%s' which is "
-  "already in use by '%s'.",
-  netdev_get_name(netdev), new_devargs,
-  netdev_get_name(_dev->up));
+VLOG_WARN_BUF(errp, "'%s' is trying to use device '%s' "
+"which is already in use by '%s'",
+  netdev_get_name(netdev), new_devargs,
+  netdev_get_name(_dev->up));