[ovs-dev] [PATCH] netdev: Add 'errp' to set_config().
Loftus, Ciara
ciara.loftus at intel.com
Wed Jan 11 15:15:03 UTC 2017
>
> 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<port_id>' 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 <ciara.loftus at intel.com>
> CC: Kevin Traynor <ktraynor at redhat.com>
> Signed-off-by: Daniele Di Proietto <diproiettod at vmware.com>
> ---
> 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(&dup_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(&dup_dev->up));
> err = EADDRINUSE;
> } else {
> dev->devargs = xstrdup(new_devargs);
> @@ -1249,7 +1250,9 @@ netdev_dpdk_set_config(struct netdev *netdev,
> const struct smap *args)
> }
> }
> } else {
> - /* dpdk-devargs unspecified - can't configure device */
> + VLOG_WARN_BUF(errp, "'%s' is missing 'options:dpdk-devargs'. "
> + "The old 'dpdk<port_id>' names are not supported",
> + netdev_get_name(netdev));
> err = EINVAL;
> }
>
> @@ -1286,7 +1289,8 @@ out:
> }
>
> static int
> -netdev_dpdk_ring_set_config(struct netdev *netdev, const struct smap
> *args)
> +netdev_dpdk_ring_set_config(struct netdev *netdev, const struct smap
> *args,
> + char **errp OVS_UNUSED)
> {
> struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
>
> @@ -1299,7 +1303,8 @@ netdev_dpdk_ring_set_config(struct netdev
> *netdev, const struct smap *args)
>
> static int
> netdev_dpdk_vhost_client_set_config(struct netdev *netdev,
> - const struct smap *args)
> + const struct smap *args,
> + char **errp OVS_UNUSED)
> {
> struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
> const char *path;
> diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c
> index d406cbcaf..92fdf4902 100644
> --- a/lib/netdev-dummy.c
> +++ b/lib/netdev-dummy.c
> @@ -828,7 +828,8 @@ netdev_dummy_set_in6(struct netdev *netdev_,
> struct in6_addr *in6,
> }
>
> static int
> -netdev_dummy_set_config(struct netdev *netdev_, const struct smap
> *args)
> +netdev_dummy_set_config(struct netdev *netdev_, const struct smap
> *args,
> + char **errp OVS_UNUSED)
> {
> struct netdev_dummy *netdev = netdev_dummy_cast(netdev_);
> const char *pcap;
> diff --git a/lib/netdev-provider.h b/lib/netdev-provider.h
> index c8507a56b..8346fc4bb 100644
> --- a/lib/netdev-provider.h
> +++ b/lib/netdev-provider.h
> @@ -277,8 +277,13 @@ struct netdev_class {
> /* Changes the device 'netdev''s configuration to 'args'.
> *
> * If this netdev class does not support configuration, this may be a null
> - * pointer. */
> - int (*set_config)(struct netdev *netdev, const struct smap *args);
> + * pointer.
> + *
> + * If the return value is not zero (meaning that an error occurred),
> + * the provider can allocate a string with an error message in '*errp'.
> + * The caller has to call free on it. */
> + int (*set_config)(struct netdev *netdev, const struct smap *args,
> + char **errp);
>
> /* Returns the tunnel configuration of 'netdev'. If 'netdev' is
> * not a tunnel, returns null.
> diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c
> index 02a246aff..ad5ffcc81 100644
> --- a/lib/netdev-vport.c
> +++ b/lib/netdev-vport.c
> @@ -35,6 +35,7 @@
> #include "netdev-native-tnl.h"
> #include "netdev-provider.h"
> #include "netdev-vport-private.h"
> +#include "openvswitch/dynamic-string.h"
> #include "ovs-router.h"
> #include "packets.h"
> #include "poll-loop.h"
> @@ -397,15 +398,17 @@ parse_tunnel_ip(const char *value, bool
> accept_mcast, bool *flow,
> }
>
> static int
> -set_tunnel_config(struct netdev *dev_, const struct smap *args)
> +set_tunnel_config(struct netdev *dev_, const struct smap *args, char
> **errp)
> {
> struct netdev_vport *dev = netdev_vport_cast(dev_);
> const char *name = netdev_get_name(dev_);
> const char *type = netdev_get_type(dev_);
> + struct ds errors = DS_EMPTY_INITIALIZER;
> bool needs_dst_port, has_csum;
> uint16_t dst_proto = 0, src_proto = 0;
> struct netdev_tunnel_config tnl_cfg;
> struct smap_node *node;
> + int err;
>
> has_csum = strstr(type, "gre") || strstr(type, "geneve") ||
> strstr(type, "stt") || strstr(type, "vxlan");
> @@ -433,25 +436,24 @@ set_tunnel_config(struct netdev *dev_, const
> struct smap *args)
>
> SMAP_FOR_EACH (node, args) {
> if (!strcmp(node->key, "remote_ip")) {
> - int err;
> err = parse_tunnel_ip(node->value, false, &tnl_cfg.ip_dst_flow,
> &tnl_cfg.ipv6_dst, &dst_proto);
> switch (err) {
> case ENOENT:
> - VLOG_WARN("%s: bad %s 'remote_ip'", name, type);
> + ds_put_format(&errors, "%s: bad %s 'remote_ip'\n", name, type);
> break;
> case EINVAL:
> - VLOG_WARN("%s: multicast remote_ip=%s not allowed",
> - name, node->value);
> - return EINVAL;
> + ds_put_format(&errors,
> + "%s: multicast remote_ip=%s not allowed\n",
> + name, node->value);
> + goto out;
> }
> } else if (!strcmp(node->key, "local_ip")) {
> - int err;
> err = parse_tunnel_ip(node->value, true, &tnl_cfg.ip_src_flow,
> &tnl_cfg.ipv6_src, &src_proto);
> switch (err) {
> case ENOENT:
> - VLOG_WARN("%s: bad %s 'local_ip'", name, type);
> + ds_put_format(&errors, "%s: bad %s 'local_ip'\n", name, type);
> break;
> }
> } else if (!strcmp(node->key, "tos")) {
> @@ -464,7 +466,8 @@ set_tunnel_config(struct netdev *dev_, const struct
> smap *args)
> if (*endptr == '\0' && tos == (tos & IP_DSCP_MASK)) {
> tnl_cfg.tos = tos;
> } else {
> - VLOG_WARN("%s: invalid TOS %s", name, node->value);
> + ds_put_format(&errors, "%s: invalid TOS %s\n", name,
> + node->value);
> }
> }
> } else if (!strcmp(node->key, "ttl")) {
> @@ -498,7 +501,8 @@ set_tunnel_config(struct netdev *dev_, const struct
> smap *args)
> if (!strcmp(type, "vxlan") && !strcmp(ext, "gbp")) {
> tnl_cfg.exts |= (1 << OVS_VXLAN_EXT_GBP);
> } else {
> - VLOG_WARN("%s: unknown extension '%s'", name, ext);
> + ds_put_format(&errors, "%s: unknown extension '%s'\n",
> + name, ext);
> }
>
> ext = strtok_r(NULL, ",", &save_ptr);
> @@ -506,24 +510,33 @@ set_tunnel_config(struct netdev *dev_, const
> struct smap *args)
>
> free(str);
> } else {
> - VLOG_WARN("%s: unknown %s argument '%s'", name, type, node-
> >key);
> + ds_put_format(&errors, "%s: unknown %s argument '%s'\n",
> + name, type, node->key);
> }
> }
>
> if (!ipv6_addr_is_set(&tnl_cfg.ipv6_dst) && !tnl_cfg.ip_dst_flow) {
> - VLOG_ERR("%s: %s type requires valid 'remote_ip' argument",
> - name, type);
> - return EINVAL;
> + ds_put_format(&errors,
> + "%s: %s type requires valid 'remote_ip' argument\n",
> + name, type);
> + err = EINVAL;
> + goto out;
> }
> if (tnl_cfg.ip_src_flow && !tnl_cfg.ip_dst_flow) {
> - VLOG_ERR("%s: %s type requires 'remote_ip=flow' with
> 'local_ip=flow'",
> - name, type);
> - return EINVAL;
> + ds_put_format(&errors,
> + "%s: %s type requires 'remote_ip=flow' "
> + "with 'local_ip=flow'\n",
> + name, type);
> + err = EINVAL;
> + goto out;
> }
> if (src_proto && dst_proto && src_proto != dst_proto) {
> - VLOG_ERR("%s: 'remote_ip' and 'local_ip' has to be of the same address
> family",
> - name);
> - return EINVAL;
> + ds_put_format(&errors,
> + "%s: 'remote_ip' and 'local_ip' "
> + "has to be of the same address family\n",
> + name);
> + err = EINVAL;
> + goto out;
> }
> if (!tnl_cfg.ttl) {
> tnl_cfg.ttl = DEFAULT_TTL;
> @@ -545,7 +558,18 @@ set_tunnel_config(struct netdev *dev_, const struct
> smap *args)
> }
> ovs_mutex_unlock(&dev->mutex);
>
> - return 0;
> + err = 0;
> +
> +out:
> + ds_chomp(&errors, '\n');
> + VLOG_WARN("%s", ds_cstr(&errors));
> + if (err) {
> + *errp = ds_steal_cstr(&errors);
> + }
> +
> + ds_destroy(&errors);
> +
> + return err;
> }
>
> static int
> @@ -693,7 +717,7 @@ get_patch_config(const struct netdev *dev_, struct
> smap *args)
> }
>
> static int
> -set_patch_config(struct netdev *dev_, const struct smap *args)
> +set_patch_config(struct netdev *dev_, const struct smap *args, char
> **errp)
> {
> struct netdev_vport *dev = netdev_vport_cast(dev_);
> const char *name = netdev_get_name(dev_);
> @@ -701,17 +725,19 @@ set_patch_config(struct netdev *dev_, const struct
> smap *args)
>
> peer = smap_get(args, "peer");
> if (!peer) {
> - VLOG_ERR("%s: patch type requires valid 'peer' argument", name);
> + VLOG_ERR_BUF(errp, "%s: patch type requires valid 'peer' argument",
> + name);
> return EINVAL;
> }
>
> if (smap_count(args) > 1) {
> - VLOG_ERR("%s: patch type takes only a 'peer' argument", name);
> + VLOG_ERR_BUF(errp, "%s: patch type takes only a 'peer' argument",
> + name);
> return EINVAL;
> }
>
> if (!strcmp(name, peer)) {
> - VLOG_ERR("%s: patch peer must not be self", name);
> + VLOG_ERR_BUF(errp, "%s: patch peer must not be self", name);
> return EINVAL;
> }
>
> diff --git a/lib/netdev.c b/lib/netdev.c
> index f7a1001f2..afc3818d4 100644
> --- a/lib/netdev.c
> +++ b/lib/netdev.c
> @@ -417,13 +417,19 @@ netdev_set_config(struct netdev *netdev, const
> struct smap *args, char **errp)
> {
> if (netdev->netdev_class->set_config) {
> const struct smap no_args = SMAP_INITIALIZER(&no_args);
> + char *verbose_error = NULL;
> int error;
>
> error = netdev->netdev_class->set_config(netdev,
> - args ? args : &no_args);
> + args ? args : &no_args,
> + &verbose_error);
> if (error) {
> - VLOG_WARN_BUF(errp, "%s: could not set configuration (%s)",
> + VLOG_WARN_BUF(verbose_error ? NULL : errp,
> + "%s: could not set configuration (%s)",
> netdev_get_name(netdev), ovs_strerror(error));
> + if (verbose_error) {
> + *errp = verbose_error;
> + }
> }
> return error;
> } else if (args && !smap_is_empty(args)) {
> --
> 2.11.0
Thanks for this patch. From the dpdk-devargs perspective the verbose log message is a lot more useful than what was in place previous & should help with anybody upgrading 2.6 -> 2.7 when the time comes. I tested with & without dpdk-devargs and it worked as expected. Just needs a rebase.
Tested-by: Ciara Loftus <ciara.loftus at intel.com>
More information about the dev
mailing list