Return meaningful errors for brctl modification commands.

When datapaths and interfaces are modified, we now do more thorough
checks to see whether they will succeed or not.  When datapaths are
added, we now block until they are created, so that follow-on ioctl
calls to attach interfaces will immediately work.

diff --git a/datapath/brcompat.c b/datapath/brcompat.c
index 29526c575412d6cdc45328564e9c5b7bddaea027..b86e8e0c9d98874faca71c749ba05c9fd50e1575 100644 (file)
--- a/datapath/brcompat.c
+++ b/datapath/brcompat.c
@@ -1,8 +1,9 @@
 #include <linux/kernel.h>
 #include <asm/uaccess.h>
-#include <linux/netdevice.h>
+#include <linux/completion.h>
 #include <linux/etherdevice.h>
 #include <linux/if_bridge.h>
+#include <linux/netdevice.h>
 #include <net/genetlink.h>
 
 #include "compat.h"
@@ -14,8 +15,21 @@
 static struct genl_family brc_genl_family;
 static struct genl_multicast_group brc_mc_group;
 
+/* Completion for vswitchd to notify the ioctl that the operation
+ * completed. */
+DECLARE_COMPLETION(dp_act_done);
+ 
+/* Time to wait for vswitchd to respond to a datapath action (in
+ * milliseconds) */
+#define DP_ACT_TIMEOUT 5000 
+
+/* Positive errno as a result of a datapath action.  Calls that make
+ * use of this variable are serialized by the br_ioctl_mutex. */
+int dp_act_err;
+
 int brc_send_dp_add_del(const char *dp_name, int add);
-int brc_send_port_add_del(const char *dp_name, const char *port_name, int add);
+int brc_send_port_add_del(struct net_device *dev, struct net_device *port, 
+               int add);
 
 /* Called with RTNL lock. */
 static int
@@ -49,8 +63,12 @@ get_port_ifindices(struct datapath *dp, int *ifindices, int num)
        rcu_read_unlock();
 }
 
-/* Legacy deviceless bridge ioctl's.  We only use this to provide
- * support for "brctl show". */
+/* Legacy deviceless bridge ioctl's.  The primary reason to do this 
+ * is to provide support for 'brctl show' without having to implement
+ * the bridge's sysfs system.  Also, 'brctl' will fall back on the
+ * legacy device ioctl's if an attempt to add or delete a bridge results in
+ * an error.  By re-attempting the failed add or delete call, the error 
+ * messages from 'brctl' are meaningful. */
 static int
 old_deviceless(void __user *uarg)
 {
@@ -79,11 +97,26 @@ old_deviceless(void __user *uarg)
                kfree(indices);
                return ret;
        }
+
+       case BRCTL_DEL_BRIDGE: 
+       case BRCTL_ADD_BRIDGE: { 
+               char dp_name[IFNAMSIZ];
+
+               if (copy_from_user(dp_name, (void __user *)args[1], IFNAMSIZ))
+                       return -EFAULT;
+
+               dp_name[IFNAMSIZ-1] = 0;
+               if (args[0] == BRCTL_ADD_BRIDGE)
+                       return brc_send_dp_add_del(dp_name, 1);
+
+               return brc_send_dp_add_del(dp_name, 0);
+       }
        }
 
        return -EOPNOTSUPP;
 }
 
+/* Called with the br_ioctl_mutex. */
 static int
 #if LINUX_VERSION_CODE <= KERNEL_VERSION(2,6,23)
 brc_ioctl_deviceless_stub(unsigned int cmd, void __user *uarg)
@@ -93,6 +126,7 @@ brc_ioctl_deviceless_stub(struct net *net, unsigned int cmd, void __user *uarg)
 {
        switch (cmd) {
        case SIOCGIFBR:
+       case SIOCSIFBR:
                return old_deviceless(uarg);
 
        case SIOCBRADDBR:
@@ -113,8 +147,12 @@ brc_ioctl_deviceless_stub(struct net *net, unsigned int cmd, void __user *uarg)
        return -EOPNOTSUPP;
 }
 
-/* Legacy ioctl's through SIOCDEVPRIVATE.  We only use this to provide
- * support for "brctl show". */
+/* Legacy ioctl's through SIOCDEVPRIVATE.  The primary reason to do this 
+ * is to provide support for 'brctl show' without having to implement
+ * the bridge's sysfs system.  Also, 'brctl' will fall back on the
+ * legacy device ioctl's if an attempt to modify an interface results in
+ * an error.  By re-attempting the failed modify call, the error messages 
+ * from 'brctl' are meaningful. */
 static int
 old_dev_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
 {
@@ -126,6 +164,19 @@ old_dev_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
                return -EFAULT;
 
        switch (args[0]) {
+       case BRCTL_ADD_IF:
+       case BRCTL_DEL_IF: {
+               struct net_device *port;
+               int err;
+
+               port = dev_get_by_index(&init_net, args[1]);
+               if (!port) 
+                       return -EINVAL;
+               err = brc_send_port_add_del(dev, port, args[0] == BRCTL_ADD_IF);
+               dev_put(port);
+               return err;
+       }
+
        case BRCTL_GET_BRIDGE_INFO: {
                struct __bridge_info b;
                uint64_t id = 0;
@@ -133,7 +184,6 @@ old_dev_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
 
                memset(&b, 0, sizeof(struct __bridge_info));
 
-               /* xxx Mutex Locking? */
                rcu_read_lock();
                for (i=0; i<ETH_ALEN; i++) 
                        id |= (uint64_t)dev->dev_addr[i] << (8*(ETH_ALEN-1 - i));
@@ -191,8 +241,7 @@ brc_dev_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
                        port = dev_get_by_index(&init_net, rq->ifr_ifindex);
                        if (!port) 
                                return -EINVAL;
-                       err = brc_send_port_add_del(dev->name, port->name, 
-                                               cmd == SIOCBRADDIF);
+                       err = brc_send_port_add_del(dev, port, cmd == SIOCBRADDIF);
                        dev_put(port);
                        break;
                }
@@ -249,10 +298,33 @@ static struct genl_ops brc_genl_ops_query_dp = {
        .dumpit = NULL
 };
 
+/* Attribute policy: what each attribute may contain.  */
+static struct nla_policy brc_genl_policy[BRC_GENL_A_MAX + 1] = {
+       [BRC_GENL_A_ERR_CODE] = { .type = NLA_U32 }
+};
+
+static int
+brc_genl_dp_result(struct sk_buff *skb, struct genl_info *info)
+{
+       dp_act_err = nla_get_u32(info->attrs[BRC_GENL_A_ERR_CODE]);
+       complete(&dp_act_done);
+
+       return 0;
+}
+
+static struct genl_ops brc_genl_ops_dp_result = {
+       .cmd = BRC_GENL_C_DP_RESULT,
+       .flags = GENL_ADMIN_PERM, /* Requires CAP_NET_ADMIN privelege. */
+       .policy = brc_genl_policy,
+       .doit = brc_genl_dp_result,
+       .dumpit = NULL
+};
+
 int brc_send_dp_add_del(const char *dp_name, int add)
 {
        struct sk_buff *skb;
        void *data;
+       int retval;
 
        skb = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
        if (skb == NULL)
@@ -269,8 +341,22 @@ int brc_send_dp_add_del(const char *dp_name, int add)
 
        NLA_PUT_STRING(skb, BRC_GENL_A_DP_NAME, dp_name);
 
+       init_completion(&dp_act_done);
+
        genlmsg_end(skb, data);
-       return genlmsg_multicast(skb, 0, brc_mc_group.id, GFP_ATOMIC);
+       retval = genlmsg_multicast(skb, 0, brc_mc_group.id, GFP_ATOMIC);
+       if (retval < 0) 
+               return retval;
+
+       if (!wait_for_completion_timeout(&dp_act_done, 
+                               msecs_to_jiffies(DP_ACT_TIMEOUT))) 
+               return -EIO;
+
+       /* The value is returned as a positive errno, so make it negative */
+       if (dp_act_err) 
+               return -dp_act_err;
+
+       return 0;
 
 nla_put_failure:
 err:
@@ -278,8 +364,11 @@ err:
        return -EINVAL;
 }
 
-int brc_send_port_add_del(const char *dp_name, const char *port_name, int add)
+int brc_send_port_add_del(struct net_device *dev, struct net_device *port, 
+               int add)
 {
+       struct dp_dev *dp_dev = netdev_priv(dev);
+       struct datapath *dp = dp_dev->dp;
        struct sk_buff *skb;
        void *data;
 
@@ -287,17 +376,28 @@ int brc_send_port_add_del(const char *dp_name, const char *port_name, int add)
        if (skb == NULL)
                return -ENOMEM;
 
-       if (add)
+       if (add) {
+               /* Only add the port if it's not attached to a datapath. */
+               if (port->br_port != NULL) {
+                       kfree_skb(skb);
+                       return -EBUSY;
+               }
                data = genlmsg_put(skb, 0, 0, &brc_genl_family, 0, 
                                   BRC_GENL_C_PORT_ADD);
-       else
+       } else {
+               /* Only delete the port if it's attached to this datapath. */
+               if (port->br_port == NULL || port->br_port->dp != dp) {
+                       kfree_skb(skb);
+                       return -ENOENT;
+               }
                data = genlmsg_put(skb, 0, 0, &brc_genl_family, 0, 
                                   BRC_GENL_C_PORT_DEL);
+       }
        if (!data)
                goto err;
 
-       NLA_PUT_STRING(skb, BRC_GENL_A_DP_NAME, dp_name);
-       NLA_PUT_STRING(skb, BRC_GENL_A_PORT_NAME, port_name);
+       NLA_PUT_STRING(skb, BRC_GENL_A_DP_NAME, dev->name);
+       NLA_PUT_STRING(skb, BRC_GENL_A_PORT_NAME, port->name);
 
        genlmsg_end(skb, data);
        return genlmsg_multicast(skb, 0, brc_mc_group.id, GFP_ATOMIC);
@@ -331,6 +431,10 @@ __init brc_init(void)
        if (err != 0) 
                goto err_unregister;
 
+       err = genl_register_ops(&brc_genl_family, &brc_genl_ops_dp_result);
+       if (err != 0) 
+               goto err_unregister;
+
        strcpy(brc_mc_group.name, "brcompat");
        err = genl_register_mc_group(&brc_genl_family, &brc_mc_group);
        if (err < 0)

diff --git a/include/openflow/brcompat-netlink.h b/include/openflow/brcompat-netlink.h
index 1a276df41e8a1756480e8d42e85c4ece3a048cb3..34560a3ab4bb9200e68e065bc99e41605ca4f360 100644 (file)
--- a/include/openflow/brcompat-netlink.h
+++ b/include/openflow/brcompat-netlink.h
@@ -39,9 +39,10 @@
 /* Attributes that can be attached to the datapath's netlink messages. */
 enum {
        BRC_GENL_A_UNSPEC,
-       BRC_GENL_A_DP_NAME,       /* Datapath name. */
-       BRC_GENL_A_PORT_NAME, /* Interface name. */
-       BRC_GENL_A_MC_GROUP,  /* Generic netlink multicast group. */
+       BRC_GENL_A_DP_NAME,         /* Datapath name. */
+       BRC_GENL_A_PORT_NAME,   /* Interface name. */
+       BRC_GENL_A_ERR_CODE,    /* Positive error code. */
+       BRC_GENL_A_MC_GROUP,    /* Generic netlink multicast group. */
 
        __BRC_GENL_A_MAX,
        BRC_GENL_A_MAX = __BRC_GENL_A_MAX - 1
@@ -50,11 +51,12 @@ enum {
 /* Commands that can be executed on the datapath's netlink interface. */
 enum brc_genl_command {
        BRC_GENL_C_UNSPEC,
-       BRC_GENL_C_DP_ADD,       /* Datapath created. */
-       BRC_GENL_C_DP_DEL,       /* Datapath destroyed. */
-       BRC_GENL_C_PORT_ADD, /* Port added to datapath. */
-       BRC_GENL_C_PORT_DEL, /* Port removed from datapath. */
-       BRC_GENL_C_QUERY_MC, /* Get multicast group for brcompat. */
+       BRC_GENL_C_DP_ADD,      /* Datapath created. */
+       BRC_GENL_C_DP_DEL,      /* Datapath destroyed. */
+       BRC_GENL_C_DP_RESULT,   /* Result of datapath command from vswitchd. */
+       BRC_GENL_C_PORT_ADD,    /* Port added to datapath. */
+       BRC_GENL_C_PORT_DEL,    /* Port removed from datapath. */
+       BRC_GENL_C_QUERY_MC,    /* Get multicast group for brcompat. */
 
        __BRC_GENL_C_MAX,
        BRC_GENL_C_MAX = __BRC_GENL_C_MAX - 1

diff --git a/vswitchd/brcompat.c b/vswitchd/brcompat.c
index 9fe89720cb752bdeb9ce77ffaaffdd94d9f651d9..0151d1ff2fa361b47c119687197fba5e0591ead3 100644 (file)
--- a/vswitchd/brcompat.c
+++ b/vswitchd/brcompat.c
@@ -275,7 +275,7 @@ static int
 brc_add_dp(const char *dp_name)
 {
     if (bridge_exists(dp_name)) {
-        return EINVAL;
+        return EEXIST;
     }
 
     brc_modify_config(dp_name, NULL, BMC_ADD_DP);
@@ -294,7 +294,7 @@ static int
 brc_del_dp(const char *dp_name)
 {
     if (!bridge_exists(dp_name)) {
-        return EINVAL;
+        return ENXIO;
     }
 
     brc_modify_config(dp_name, NULL, BMC_DEL_DP);
@@ -312,8 +312,11 @@ brc_del_dp(const char *dp_name)
 int 
 brc_handle_dp_cmd(struct ofpbuf *buffer, bool add)
 {
+    int dp_act_err;
+    int retval;
     struct nlattr *attrs[ARRAY_SIZE(brc_dp_policy)];
     const char *dp_name;
+    struct ofpbuf msg;
 
     if (!nl_policy_parse(buffer, brc_dp_policy, attrs, 
                 ARRAY_SIZE(brc_dp_policy))) {
@@ -323,10 +326,23 @@ brc_handle_dp_cmd(struct ofpbuf *buffer, bool add)
     dp_name = nl_attr_get(attrs[BRC_GENL_A_DP_NAME]);
 
     if (add) {
-        return brc_add_dp(dp_name);
+        dp_act_err = brc_add_dp(dp_name);
     } else {
-        return brc_del_dp(dp_name);
+        dp_act_err = brc_del_dp(dp_name);
     }
+
+    /* Notify the brcompat kernel module of the result. */
+    ofpbuf_init(&msg, 0);
+    nl_msg_put_genlmsghdr(&msg, nl_sock, 32, brc_family, NLM_F_REQUEST, 
+            BRC_GENL_C_DP_RESULT, 1);
+    nl_msg_put_u32(&msg, BRC_GENL_A_ERR_CODE, dp_act_err);
+    retval = nl_sock_send(nl_sock, &msg, false);
+    if (retval) {
+        VLOG_WARN_RL(&rl, "brc_handle_dp_cmd: %s", strerror(retval));
+    }
+    ofpbuf_uninit(&msg);
+
+    return dp_act_err;
 }
 
 static const struct nl_policy brc_port_policy[] = {