This looks great already. Some minor stuff.
On 03/08/2014 03:04 AM, Alex Wang wrote:
@@ -1459,7 +1459,7 @@ static const struct nla_policy
vport_policy[OVS_VPORT_ATTR_MAX + 1] = {
[OVS_VPORT_ATTR_STATS] = { .len = sizeof(struct ovs_vport_stats) },
[OVS_VPORT_ATTR_PORT_NO] = { .type = NLA_U32 },
[OVS_VPORT_ATTR_TYPE] = { .type = NLA_U32 },
- [OVS_VPORT_ATTR_UPCALL_PID] = { .type = NLA_U32 },
+ [OVS_VPORT_ATTR_UPCALL_PID] = { .type = NLA_UNSPEC },
[OVS_VPORT_ATTR_OPTIONS] = { .type = NLA_NESTED },
};
I can understand that you want to change the policy to reflect
the change in the extended format. Technically, staying at
NLA_U32 would result in the same as you have now: enforce a
minimal payload size of 4 bytes.
This change might raise eyebrows though in the push to netdev
as it looks like an ABI breakage without closer inspection.
+int ovs_vport_set_upcall_portids(struct vport *vport, struct nlattr *ids)
+{
+ struct vport_portids *old, *vport_portids;
+
+ if (!nla_len(ids) || nla_len(ids) % sizeof(u32))
+ return -EINVAL;
This may crash if OVS_VPORT_ATTR_UPCALL_PID is not provided and ids
becomes NULL since you removed the if (a[OVS_VPORT_ATTR_UPCALL_PID])
in ovs_vport_cmd_set().
+/**
+ * ovs_vport_get_upcall_portids - get the upcall_portids of @vport.
+ *
+ * @vport: vport from which to retrieve the portids.
+ * @skb: sk_buff where portids should be appended.
+ *
+ * Retrieves the configuration of the given vport, appending the
+ * %OVS_VPORT_ATTR_UPCALL_PID attribute which is the array of upcall
+ * portids to @skb.
+ *
+ * Returns 0 if successful, -EMSGSIZE if @skb has insufficient room.
+ * If an error occurs, @skb is left unmodified. Must be called with
+ * rcu_read_lock.
+ */
+int ovs_vport_get_upcall_portids(const struct vport *vport,
+ struct sk_buff *skb)
+{
+ struct vport_portids *ids;
+ int err = 0;
+
+ ids = rcu_dereference_ovsl(vport->upcall_portids);
+
+ if (vport->dp->user_features & OVS_DP_F_VPORT_PIDS) {
+ if (nla_put(skb, OVS_VPORT_ATTR_UPCALL_PID,
+ ids->n_ids * sizeof(u32), (void *) ids->ids))
+ err = -EMSGSIZE;
+
+ } else {
+ if (nla_put_u32(skb, OVS_VPORT_ATTR_UPCALL_PID,
+ *ids->ids))
+ err = -EMSGSIZE;
+ }
+
+ return err;
If you want to reduce code size here, you can simply return nla_put*()
and get rid of the additional branching since these functions already
return -EMSGSIZE.
+u32 ovs_vport_find_portid(const struct vport *p, struct sk_buff *skb)
+{
+ struct vport_portids *ids;
+
+ ids = rcu_dereference_ovsl(p->upcall_portids);
+
+ if (ids->n_ids == 1 && *ids->ids == 0)
+ return 0;
+
+ return ids->ids[skb_get_rxhash(skb) % ids->n_ids];
I have some dislike for the division here. I realize this is the
slow path but no point in adding it if it can be avoided.
Worst case, this is exploitable in a DoS scenario.
One idea here might be to look at RPS and see how it avoided
this using flow masks, see store_rps_dev_flow_table_cnt().
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
