From: Ben Pfaff Date: Mon, 26 Dec 2011 22:39:03 +0000 (-0800) Subject: dpif: Change provider interface to consistently use operation structs. X-Git-Url: https://pintos-os.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=89625d1efb32461071837b5e81c1238b4ca63387;p=openvswitch dpif: Change provider interface to consistently use operation structs. Until now, a "flow put" has represented its parameters in two different ways, depending on whether it was coming from dpif_flow_put() or from dpif_operate(), and similarly for an "execute" operation. This commit adopts the operation struct consistently within the dpif provider interface, which seems cleaner. This commit also factors out logging for flow puts and executes, which is useful in the following commit. This doesn't change the dpif client interface, since the two forms are more convenient for clients than always filling out an operation struct. Signed-off-by: Ben Pfaff --- diff --git a/lib/dpif-linux.c b/lib/dpif-linux.c index 36e93cc3..c06bb897 100644 --- a/lib/dpif-linux.c +++ b/lib/dpif-linux.c @@ -657,9 +657,7 @@ dpif_linux_flow_get(const struct dpif *dpif_, } static void -dpif_linux_init_flow_put(struct dpif *dpif_, enum dpif_flow_put_flags flags, - const struct nlattr *key, size_t key_len, - const struct nlattr *actions, size_t actions_len, +dpif_linux_init_flow_put(struct dpif *dpif_, const struct dpif_flow_put *put, struct dpif_linux_flow *request) { static struct nlattr dummy_action; @@ -667,37 +665,33 @@ dpif_linux_init_flow_put(struct dpif *dpif_, enum dpif_flow_put_flags flags, struct dpif_linux *dpif = dpif_linux_cast(dpif_); dpif_linux_flow_init(request); - request->cmd = (flags & DPIF_FP_CREATE + request->cmd = (put->flags & DPIF_FP_CREATE ? OVS_FLOW_CMD_NEW : OVS_FLOW_CMD_SET); request->dp_ifindex = dpif->dp_ifindex; - request->key = key; - request->key_len = key_len; + request->key = put->key; + request->key_len = put->key_len; /* Ensure that OVS_FLOW_ATTR_ACTIONS will always be included. */ - request->actions = actions ? actions : &dummy_action; - request->actions_len = actions_len; - if (flags & DPIF_FP_ZERO_STATS) { + request->actions = put->actions ? put->actions : &dummy_action; + request->actions_len = put->actions_len; + if (put->flags & DPIF_FP_ZERO_STATS) { request->clear = true; } - request->nlmsg_flags = flags & DPIF_FP_MODIFY ? 0 : NLM_F_CREATE; + request->nlmsg_flags = put->flags & DPIF_FP_MODIFY ? 0 : NLM_F_CREATE; } static int -dpif_linux_flow_put(struct dpif *dpif_, enum dpif_flow_put_flags flags, - const struct nlattr *key, size_t key_len, - const struct nlattr *actions, size_t actions_len, - struct dpif_flow_stats *stats) +dpif_linux_flow_put(struct dpif *dpif_, const struct dpif_flow_put *put) { struct dpif_linux_flow request, reply; struct ofpbuf *buf; int error; - dpif_linux_init_flow_put(dpif_, flags, key, key_len, actions, actions_len, - &request); + dpif_linux_init_flow_put(dpif_, put, &request); error = dpif_linux_flow_transact(&request, - stats ? &reply : NULL, - stats ? &buf : NULL); - if (!error && stats) { - dpif_linux_flow_get_stats(&reply, stats); + put->stats ? &reply : NULL, + put->stats ? &buf : NULL); + if (!error && put->stats) { + dpif_linux_flow_get_stats(&reply, put->stats); ofpbuf_delete(buf); } return error; @@ -821,39 +815,35 @@ dpif_linux_flow_dump_done(const struct dpif *dpif OVS_UNUSED, void *state_) static struct ofpbuf * dpif_linux_encode_execute(int dp_ifindex, - const struct nlattr *key, size_t key_len, - const struct nlattr *actions, size_t actions_len, - const struct ofpbuf *packet) + const struct dpif_execute *d_exec) { - struct ovs_header *execute; + struct ovs_header *k_exec; struct ofpbuf *buf; - buf = ofpbuf_new(128 + actions_len + packet->size); + buf = ofpbuf_new(128 + d_exec->actions_len + d_exec->packet->size); nl_msg_put_genlmsghdr(buf, 0, ovs_packet_family, NLM_F_REQUEST, OVS_PACKET_CMD_EXECUTE, OVS_PACKET_VERSION); - execute = ofpbuf_put_uninit(buf, sizeof *execute); - execute->dp_ifindex = dp_ifindex; + k_exec = ofpbuf_put_uninit(buf, sizeof *k_exec); + k_exec->dp_ifindex = dp_ifindex; - nl_msg_put_unspec(buf, OVS_PACKET_ATTR_PACKET, packet->data, packet->size); - nl_msg_put_unspec(buf, OVS_PACKET_ATTR_KEY, key, key_len); - nl_msg_put_unspec(buf, OVS_PACKET_ATTR_ACTIONS, actions, actions_len); + nl_msg_put_unspec(buf, OVS_PACKET_ATTR_PACKET, + d_exec->packet->data, d_exec->packet->size); + nl_msg_put_unspec(buf, OVS_PACKET_ATTR_KEY, d_exec->key, d_exec->key_len); + nl_msg_put_unspec(buf, OVS_PACKET_ATTR_ACTIONS, + d_exec->actions, d_exec->actions_len); return buf; } static int -dpif_linux_execute__(int dp_ifindex, const struct nlattr *key, size_t key_len, - const struct nlattr *actions, size_t actions_len, - const struct ofpbuf *packet) +dpif_linux_execute__(int dp_ifindex, const struct dpif_execute *execute) { struct ofpbuf *request; int error; - request = dpif_linux_encode_execute(dp_ifindex, - key, key_len, actions, actions_len, - packet); + request = dpif_linux_encode_execute(dp_ifindex, execute); error = nl_sock_transact(genl_sock, request, NULL); ofpbuf_delete(request); @@ -861,15 +851,11 @@ dpif_linux_execute__(int dp_ifindex, const struct nlattr *key, size_t key_len, } static int -dpif_linux_execute(struct dpif *dpif_, - const struct nlattr *key, size_t key_len, - const struct nlattr *actions, size_t actions_len, - const struct ofpbuf *packet) +dpif_linux_execute(struct dpif *dpif_, const struct dpif_execute *execute) { struct dpif_linux *dpif = dpif_linux_cast(dpif_); - return dpif_linux_execute__(dpif->dp_ifindex, key, key_len, - actions, actions_len, packet); + return dpif_linux_execute__(dpif->dp_ifindex, execute); } static void @@ -889,9 +875,7 @@ dpif_linux_operate(struct dpif *dpif_, struct dpif_op **ops, size_t n_ops) struct dpif_flow_put *put = &op->u.flow_put; struct dpif_linux_flow request; - dpif_linux_init_flow_put(dpif_, put->flags, put->key, put->key_len, - put->actions, put->actions_len, - &request); + dpif_linux_init_flow_put(dpif_, put, &request); if (put->stats) { request.nlmsg_flags |= NLM_F_ECHO; } @@ -900,9 +884,8 @@ dpif_linux_operate(struct dpif *dpif_, struct dpif_op **ops, size_t n_ops) } else if (op->type == DPIF_OP_EXECUTE) { struct dpif_execute *execute = &op->u.execute; - txn->request = dpif_linux_encode_execute( - dpif->dp_ifindex, execute->key, execute->key_len, - execute->actions, execute->actions_len, execute->packet); + txn->request = dpif_linux_encode_execute(dpif->dp_ifindex, + execute); } else { NOT_REACHED(); } @@ -1285,6 +1268,7 @@ dpif_linux_vport_send(int dp_ifindex, uint32_t port_no, { struct ofpbuf actions, key, packet; struct odputil_keybuf keybuf; + struct dpif_execute execute; struct flow flow; uint64_t action; @@ -1297,8 +1281,12 @@ dpif_linux_vport_send(int dp_ifindex, uint32_t port_no, ofpbuf_use_stack(&actions, &action, sizeof action); nl_msg_put_u32(&actions, OVS_ACTION_ATTR_OUTPUT, port_no); - return dpif_linux_execute__(dp_ifindex, key.data, key.size, - actions.data, actions.size, &packet); + execute.key = key.data; + execute.key_len = key.size; + execute.actions = actions.data; + execute.actions_len = actions.size; + execute.packet = &packet; + return dpif_linux_execute__(dp_ifindex, &execute); } static bool diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index e2fb816b..d7c3da02 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -745,29 +745,26 @@ clear_stats(struct dp_netdev_flow *flow) } static int -dpif_netdev_flow_put(struct dpif *dpif, enum dpif_flow_put_flags flags, - const struct nlattr *nl_key, size_t nl_key_len, - const struct nlattr *actions, size_t actions_len, - struct dpif_flow_stats *stats) +dpif_netdev_flow_put(struct dpif *dpif, const struct dpif_flow_put *put) { struct dp_netdev *dp = get_dp_netdev(dpif); struct dp_netdev_flow *flow; struct flow key; int error; - error = dpif_netdev_flow_from_nlattrs(nl_key, nl_key_len, &key); + error = dpif_netdev_flow_from_nlattrs(put->key, put->key_len, &key); if (error) { return error; } flow = dp_netdev_lookup_flow(dp, &key); if (!flow) { - if (flags & DPIF_FP_CREATE) { + if (put->flags & DPIF_FP_CREATE) { if (hmap_count(&dp->flow_table) < MAX_FLOWS) { - if (stats) { - memset(stats, 0, sizeof *stats); + if (put->stats) { + memset(put->stats, 0, sizeof *put->stats); } - return add_flow(dpif, &key, actions, actions_len); + return add_flow(dpif, &key, put->actions, put->actions_len); } else { return EFBIG; } @@ -775,13 +772,13 @@ dpif_netdev_flow_put(struct dpif *dpif, enum dpif_flow_put_flags flags, return ENOENT; } } else { - if (flags & DPIF_FP_MODIFY) { - int error = set_flow_actions(flow, actions, actions_len); + if (put->flags & DPIF_FP_MODIFY) { + int error = set_flow_actions(flow, put->actions, put->actions_len); if (!error) { - if (stats) { - get_dpif_flow_stats(flow, stats); + if (put->stats) { + get_dpif_flow_stats(flow, put->stats); } - if (flags & DPIF_FP_ZERO_STATS) { + if (put->flags & DPIF_FP_ZERO_STATS) { clear_stats(flow); } } @@ -894,30 +891,29 @@ dpif_netdev_flow_dump_done(const struct dpif *dpif OVS_UNUSED, void *state_) } static int -dpif_netdev_execute(struct dpif *dpif, - const struct nlattr *key_attrs, size_t key_len, - const struct nlattr *actions, size_t actions_len, - const struct ofpbuf *packet) +dpif_netdev_execute(struct dpif *dpif, const struct dpif_execute *execute) { struct dp_netdev *dp = get_dp_netdev(dpif); struct ofpbuf copy; struct flow key; int error; - if (packet->size < ETH_HEADER_LEN || packet->size > UINT16_MAX) { + if (execute->packet->size < ETH_HEADER_LEN || + execute->packet->size > UINT16_MAX) { return EINVAL; } /* Make a deep copy of 'packet', because we might modify its data. */ - ofpbuf_init(©, DP_NETDEV_HEADROOM + packet->size); + ofpbuf_init(©, DP_NETDEV_HEADROOM + execute->packet->size); ofpbuf_reserve(©, DP_NETDEV_HEADROOM); - ofpbuf_put(©, packet->data, packet->size); + ofpbuf_put(©, execute->packet->data, execute->packet->size); flow_extract(©, 0, 0, -1, &key); - error = dpif_netdev_flow_from_nlattrs(key_attrs, key_len, &key); + error = dpif_netdev_flow_from_nlattrs(execute->key, execute->key_len, + &key); if (!error) { dp_netdev_execute_actions(dp, ©, &key, - actions, actions_len); + execute->actions, execute->actions_len); } ofpbuf_uninit(©); diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h index 1b32a79d..5ef4c991 100644 --- a/lib/dpif-provider.h +++ b/lib/dpif-provider.h @@ -210,30 +210,28 @@ struct dpif_class { struct ofpbuf **actionsp, struct dpif_flow_stats *stats); /* Adds or modifies a flow in 'dpif'. The flow is specified by the Netlink - * attributes with types OVS_KEY_ATTR_* in the 'key_len' bytes starting at - * 'key'. The associated actions are specified by the Netlink attributes - * with types OVS_ACTION_ATTR_* in the 'actions_len' bytes starting at - * 'actions'. + * attributes with types OVS_KEY_ATTR_* in the 'put->key_len' bytes + * starting at 'put->key'. The associated actions are specified by the + * Netlink attributes with types OVS_ACTION_ATTR_* in the + * 'put->actions_len' bytes starting at 'put->actions'. * * - If the flow's key does not exist in 'dpif', then the flow will be - * added if 'flags' includes DPIF_FP_CREATE. Otherwise the operation - * will fail with ENOENT. + * added if 'put->flags' includes DPIF_FP_CREATE. Otherwise the + * operation will fail with ENOENT. * - * If the operation succeeds, then 'stats', if nonnull, must be zeroed. + * If the operation succeeds, then 'put->stats', if nonnull, must be + * zeroed. * * - If the flow's key does exist in 'dpif', then the flow's actions will - * be updated if 'flags' includes DPIF_FP_MODIFY. Otherwise the + * be updated if 'put->flags' includes DPIF_FP_MODIFY. Otherwise the * operation will fail with EEXIST. If the flow's actions are updated, - * then its statistics will be zeroed if 'flags' includes + * then its statistics will be zeroed if 'put->flags' includes * DPIF_FP_ZERO_STATS, and left as-is otherwise. * - * If the operation succeeds, then 'stats', if nonnull, must be set to - * the flow's statistics before the update. + * If the operation succeeds, then 'put->stats', if nonnull, must be set + * to the flow's statistics before the update. */ - int (*flow_put)(struct dpif *dpif, enum dpif_flow_put_flags flags, - const struct nlattr *key, size_t key_len, - const struct nlattr *actions, size_t actions_len, - struct dpif_flow_stats *stats); + int (*flow_put)(struct dpif *dpif, const struct dpif_flow_put *put); /* Deletes a flow from 'dpif' and returns 0, or returns ENOENT if 'dpif' * does not contain such a flow. The flow is specified by the Netlink @@ -283,15 +281,13 @@ struct dpif_class { * successful call to the 'flow_dump_start' function for 'dpif'. */ int (*flow_dump_done)(const struct dpif *dpif, void *state); - /* Performs the 'actions_len' bytes of actions in 'actions' on the Ethernet - * frame specified in 'packet' taken from the flow specified in the - * 'key_len' bytes of 'key'. ('key' is mostly redundant with 'packet', but - * it contains some metadata that cannot be recovered from 'packet', such - * as tun_id and in_port.) */ - int (*execute)(struct dpif *dpif, - const struct nlattr *key, size_t key_len, - const struct nlattr *actions, size_t actions_len, - const struct ofpbuf *packet); + /* Performs the 'execute->actions_len' bytes of actions in + * 'execute->actions' on the Ethernet frame specified in 'execute->packet' + * taken from the flow specified in the 'execute->key_len' bytes of + * 'execute->key'. ('execute->key' is mostly redundant with + * 'execute->packet', but it contains some metadata that cannot be + * recovered from 'execute->packet', such as tun_id and in_port.) */ + int (*execute)(struct dpif *dpif, const struct dpif_execute *execute); /* Executes each of the 'n_ops' operations in 'ops' on 'dpif', in the order * in which they are specified, placing each operation's results in the diff --git a/lib/dpif.c b/lib/dpif.c index 328ffa39..d8134698 100644 --- a/lib/dpif.c +++ b/lib/dpif.c @@ -1,5 +1,5 @@ /* - * Copyright (c) 2008, 2009, 2010, 2011 Nicira Networks. + * Copyright (c) 2008, 2009, 2010, 2011, 2012 Nicira Networks. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -87,6 +87,10 @@ static void log_flow_message(const struct dpif *dpif, int error, static void log_operation(const struct dpif *, const char *operation, int error); static bool should_log_flow_message(int error); +static void log_flow_put_message(struct dpif *, const struct dpif_flow_put *, + int error); +static void log_execute_message(struct dpif *, const struct dpif_execute *, + int error); static void dp_initialize(void) @@ -764,6 +768,23 @@ dpif_flow_get(const struct dpif *dpif, return error; } +static int +dpif_flow_put__(struct dpif *dpif, const struct dpif_flow_put *put) +{ + int error; + + COVERAGE_INC(dpif_flow_put); + assert(!(put->flags & ~(DPIF_FP_CREATE | DPIF_FP_MODIFY + | DPIF_FP_ZERO_STATS))); + + error = dpif->dpif_class->flow_put(dpif, put); + if (error && put->stats) { + memset(put->stats, 0, sizeof *put->stats); + } + log_flow_put_message(dpif, put, error); + return error; +} + /* Adds or modifies a flow in 'dpif'. The flow is specified by the Netlink * attributes with types OVS_KEY_ATTR_* in the 'key_len' bytes starting at * 'key'. The associated actions are specified by the Netlink attributes with @@ -790,35 +811,15 @@ dpif_flow_put(struct dpif *dpif, enum dpif_flow_put_flags flags, const struct nlattr *actions, size_t actions_len, struct dpif_flow_stats *stats) { - int error; - - COVERAGE_INC(dpif_flow_put); - assert(!(flags & ~(DPIF_FP_CREATE | DPIF_FP_MODIFY | DPIF_FP_ZERO_STATS))); - - error = dpif->dpif_class->flow_put(dpif, flags, key, key_len, - actions, actions_len, stats); - if (error && stats) { - memset(stats, 0, sizeof *stats); - } - if (should_log_flow_message(error)) { - struct ds s; - - ds_init(&s); - ds_put_cstr(&s, "put"); - if (flags & DPIF_FP_CREATE) { - ds_put_cstr(&s, "[create]"); - } - if (flags & DPIF_FP_MODIFY) { - ds_put_cstr(&s, "[modify]"); - } - if (flags & DPIF_FP_ZERO_STATS) { - ds_put_cstr(&s, "[zero]"); - } - log_flow_message(dpif, error, ds_cstr(&s), key, key_len, stats, - actions, actions_len); - ds_destroy(&s); - } - return error; + struct dpif_flow_put put; + + put.flags = flags; + put.key = key; + put.key_len = key_len; + put.actions = actions; + put.actions_len = actions_len; + put.stats = stats; + return dpif_flow_put__(dpif, &put); } /* Deletes a flow from 'dpif' and returns 0, or returns ENOENT if 'dpif' does @@ -938,6 +939,23 @@ dpif_flow_dump_done(struct dpif_flow_dump *dump) return dump->error == EOF ? 0 : dump->error; } +static int +dpif_execute__(struct dpif *dpif, const struct dpif_execute *execute) +{ + int error; + + COVERAGE_INC(dpif_execute); + if (execute->actions_len > 0) { + error = dpif->dpif_class->execute(dpif, execute); + } else { + error = 0; + } + + log_execute_message(dpif, execute, error); + + return error; +} + /* Causes 'dpif' to perform the 'actions_len' bytes of actions in 'actions' on * the Ethernet frame specified in 'packet' taken from the flow specified in * the 'key_len' bytes of 'key'. ('key' is mostly redundant with 'packet', but @@ -951,30 +969,14 @@ dpif_execute(struct dpif *dpif, const struct nlattr *actions, size_t actions_len, const struct ofpbuf *buf) { - int error; - - COVERAGE_INC(dpif_execute); - if (actions_len > 0) { - error = dpif->dpif_class->execute(dpif, key, key_len, - actions, actions_len, buf); - } else { - error = 0; - } - - if (!(error ? VLOG_DROP_WARN(&error_rl) : VLOG_DROP_DBG(&dpmsg_rl))) { - struct ds ds = DS_EMPTY_INITIALIZER; - char *packet = ofp_packet_to_string(buf->data, buf->size); - ds_put_format(&ds, "%s: execute ", dpif_name(dpif)); - format_odp_actions(&ds, actions, actions_len); - if (error) { - ds_put_format(&ds, " failed (%s)", strerror(error)); - } - ds_put_format(&ds, " on packet %s", packet); - vlog(THIS_MODULE, error ? VLL_WARN : VLL_DBG, "%s", ds_cstr(&ds)); - ds_destroy(&ds); - free(packet); - } - return error; + struct dpif_execute execute; + + execute.key = key; + execute.key_len = key_len; + execute.actions = actions; + execute.actions_len = actions_len; + execute.packet = buf; + return dpif_execute__(dpif, &execute); } /* Executes each of the 'n_ops' operations in 'ops' on 'dpif', in the order in @@ -995,24 +997,14 @@ dpif_operate(struct dpif *dpif, struct dpif_op **ops, size_t n_ops) for (i = 0; i < n_ops; i++) { struct dpif_op *op = ops[i]; - struct dpif_flow_put *put; - struct dpif_execute *execute; switch (op->type) { case DPIF_OP_FLOW_PUT: - put = &op->u.flow_put; - op->error = dpif_flow_put(dpif, put->flags, - put->key, put->key_len, - put->actions, put->actions_len, - put->stats); + op->error = dpif_flow_put__(dpif, &op->u.flow_put); break; case DPIF_OP_EXECUTE: - execute = &op->u.execute; - op->error = dpif_execute(dpif, execute->key, execute->key_len, - execute->actions, - execute->actions_len, - execute->packet); + op->error = dpif_execute__(dpif, &op->u.execute); break; default: @@ -1220,3 +1212,50 @@ log_flow_message(const struct dpif *dpif, int error, const char *operation, vlog(THIS_MODULE, flow_message_log_level(error), "%s", ds_cstr(&ds)); ds_destroy(&ds); } + +static void +log_flow_put_message(struct dpif *dpif, const struct dpif_flow_put *put, + int error) +{ + if (should_log_flow_message(error)) { + struct ds s; + + ds_init(&s); + ds_put_cstr(&s, "put"); + if (put->flags & DPIF_FP_CREATE) { + ds_put_cstr(&s, "[create]"); + } + if (put->flags & DPIF_FP_MODIFY) { + ds_put_cstr(&s, "[modify]"); + } + if (put->flags & DPIF_FP_ZERO_STATS) { + ds_put_cstr(&s, "[zero]"); + } + log_flow_message(dpif, error, ds_cstr(&s), + put->key, put->key_len, put->stats, + put->actions, put->actions_len); + ds_destroy(&s); + } +} + +static void +log_execute_message(struct dpif *dpif, const struct dpif_execute *execute, + int error) +{ + if (!(error ? VLOG_DROP_WARN(&error_rl) : VLOG_DROP_DBG(&dpmsg_rl))) { + struct ds ds = DS_EMPTY_INITIALIZER; + char *packet; + + packet = ofp_packet_to_string(execute->packet->data, + execute->packet->size); + ds_put_format(&ds, "%s: execute ", dpif_name(dpif)); + format_odp_actions(&ds, execute->actions, execute->actions_len); + if (error) { + ds_put_format(&ds, " failed (%s)", strerror(error)); + } + ds_put_format(&ds, " on packet %s", packet); + vlog(THIS_MODULE, error ? VLL_WARN : VLL_DBG, "%s", ds_cstr(&ds)); + ds_destroy(&ds); + free(packet); + } +}