netdev: Add 'errp' to set_config().

Since 55e075e("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 <[email protected]>
CC: Kevin Traynor <[email protected]>
Signed-off-by: Daniele Di Proietto <[email protected]>
Acked-by: Kevin Traynor <[email protected]>
Tested-by: Ciara Loftus <[email protected]>

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 {
                     int sid = rte_eth_dev_socket_id(new_port_id);
@@ -1251,7 +1252,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;
     }
 
@@ -1288,7 +1291,8 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args)
 }
 
 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);
 
@@ -1301,7 +1305,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;

lib/netdev-dummy.c

@@ -830,7 +830,8 @@ netdev_dummy_set_in6(struct netdev *netdev_, struct in6_addr *in6,
 #define DUMMY_MAX_QUEUES_PER_PORT 1024
 
 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;

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.

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,32 +501,42 @@ 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);
             }
 
             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,25 +717,27 @@ 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_);
     const char *peer;
 
     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;
     }
 

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)) {