Since this patch was so big, I did my review as a diff against the source.
Overall the patch is good. Only some minor comments and questions.
Ethan
---
ofproto/ofproto-dpif.c | 19 +++++++++++++++++++
ofproto/ofproto.c | 12 +++++++++++-
ofproto/private.h | 25 +++++++++++++++++++------
3 files changed, 49 insertions(+), 7 deletions(-)
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index c2361f8..a31fd1b 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -174,6 +174,11 @@ struct action_xlate_ctx {
* its actions. If special_cb returns false on 'flow' rendered
* uninstallable and no actions will be executed. */
bool check_special;
+ /* This is probably not the patch to fix it, but I'm pretty sure that
+ * check_special is dead code now. Doing some research, it apparently has
+ * been since ofproto_send_packet() no longer had to call xlate_actions().
+ * I don't think this is worth fixing on master, but it may be worth adding
+ * a patch to the end of the next branch which cleans it up. */
/* xlate_actions() initializes and uses these members. The client might want
* to look at them after it returns. */
@@ -1400,6 +1405,9 @@ port_del(struct ofproto *ofproto_, uint16_t ofp_port)
* it from the bond before closing the netdev. The client will need to
* reconfigure everything after deleting ports, so then the slave will
* get re-added. */
+ /* I don't fully understand this comment. You don't ever close the
+ * netdev in this function. Do you intend to, or does it happen
+ * somewhere else? */
struct ofport_dpif *ofport = get_ofp_port(ofproto, ofp_port);
if (ofport) {
bundle_remove(&ofport->up);
@@ -1714,6 +1722,8 @@ update_stats(struct ofproto_dpif *p)
if (stats->n_packets >= facet->dp_packet_count) {
facet->packet_count += stats->n_packets -
facet->dp_packet_count;
+ /* The above line is too long. It was too long in the old
+ * version as well, but may as well fix it. */
} else {
VLOG_WARN_RL(&rl, "unexpected packet count from the datapath");
}
@@ -2472,6 +2482,10 @@ rule_destruct(struct rule *rule_)
LIST_FOR_EACH_SAFE (facet, next_facet, list_node, &rule->facets) {
facet_revalidate(ofproto, facet);
}
+ /* I may be thinking about this incorrectly. I think rule destruction
+ * requires ofproto->need_revalidate = true because of resubmits. Theres
+ * no way to predict which facets resubmit into 'rule_' and thus need their
+ * actions recalculation. */
}
static void
@@ -2545,6 +2559,10 @@ rule_execute(struct rule *rule_, struct flow *flow,
struct ofpbuf *packet)
ofpbuf_delete(odp_actions);
}
+/* The name of this function makes one think that it should actually be setting
+ * the ofp_actions in 'rule_'. Apparently, it only validates that the actions
+ * are reasonable. I can't tell if this is a mistake in the implementation, or
+ * just a case of insufficient documentation. */
static int
rule_modify_actions(struct rule *rule_,
const union ofp_action *actions, size_t n_actions)
@@ -2846,6 +2864,7 @@ xlate_autopath(struct action_xlate_ctx *ctx,
{
uint16_t ofp_port = ntohl(naa->id);
struct ofport_dpif *port = get_ofp_port(ctx->ofproto, ofp_port);
+
if (!port || !port->bundle) {
ofp_port = OFPP_NONE;
} else if (port->bundle->bond) {
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 57d4116..ec07895 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -200,6 +200,8 @@ ofproto_class_unregister(const struct ofproto_class *class)
{
size_t i;
+ /* Why do we need to maintaion the order of ofproto_classes? Seems fine,
+ * just curious. */
for (i = 0; i < n_ofproto_classes; i++) {
if (ofproto_classes[i] == class) {
for (i++; i < n_ofproto_classes; i++) {
@@ -541,7 +543,10 @@ ofproto_port_is_lacp_current(struct ofproto *ofproto,
uint16_t ofp_port)
* to 's'. Otherwise, this function registers a new bundle.
*
* Bundles affect only the treatment of packets output to the OFPP_NORMAL
- * port. */
+ * port.
+ *
+ * This isn't true. Bundles affect the autopath action as well.
+ */
int
ofproto_bundle_register(struct ofproto *ofproto, void *aux,
const struct ofproto_bundle_settings *s)
@@ -1271,6 +1276,10 @@ rule_create(struct ofproto *ofproto, const struct
cls_rule *cls_rule,
if (n_actions > 0) {
rule->n_actions = n_actions;
rule->actions = xmemdup(actions, n_actions * sizeof *actions);
+ } else {
+ /* I think this is required because rule_alloc() doesn't zero. */
+ rule->n_actions = 0;
+ rule->actions = NULL;
}
error = ofproto->ofproto_class->rule_construct(rule);
@@ -1960,6 +1969,7 @@ flow_stats_ds(struct rule *rule, struct ds *results)
ds_put_format(results, "duration=%llds, ",
(time_msec() - rule->created) / 1000);
+ /* I'm not sure if you intend to delete this line or uncomment it. */
//ds_put_format(results, "idle=%.3fs, ", (time_msec() - rule->used) /
1000.0);
ds_put_format(results, "priority=%u, ", rule->cr.priority);
ds_put_format(results, "n_packets=%"PRIu64", ", packet_count);
diff --git a/ofproto/private.h b/ofproto/private.h
index d79ed81..1ab8c6c 100644
--- a/ofproto/private.h
+++ b/ofproto/private.h
@@ -14,6 +14,11 @@
* limitations under the License.
*/
+/* I think it would be a good idea to have someone with hardware experience to
+ * read over this file in addition to me to make sure it's relatively easy to
+ * port. I don't see a reason why it wouldn't be, but I have no experience
+ * with hardware so it might be worth having another pair of eyes on it. */
+
#ifndef OFPROTO_PRIVATE_H
#define OFPROTO_PRIVATE_H 1
@@ -100,6 +105,7 @@ struct rule *ofproto_rule_lookup(struct ofproto *, const
struct flow *);
void ofproto_rule_expire(struct rule *, uint8_t reason);
void ofproto_rule_destroy(struct rule *);
+/* This comment is awesome. */
/* ofproto class structure, to be defined by each ofproto implementation.
*
*
@@ -168,7 +174,7 @@ void ofproto_rule_destroy(struct rule *);
* use of the new data structure, so it cannot perform much initialization.
* Its purpose is just to ensure that the new data structure has enough room
* for base and derived state. It may return a null pointer if memory is not
- * available, in which case none of the other functions is called.
+ * available, in which case none of the other functions are called.
*
* Each "construct" function initializes derived state in its respective data
* structure. When "construct" is called, all of the base state has already
@@ -246,7 +252,7 @@ struct ofproto_class {
* poll-loop.h. */
void (*wait)(struct ofproto *ofproto);
- /* Every "struct rule"s in 'ofproto' is about to be deleted, one by one.
+ /* Every "struct rule" in 'ofproto' is about to be deleted, one by one.
* This function may prepare for that, for example by clearing state in
* advance. It should *not* actually delete any "struct rule"s from
* 'ofproto', only prepare for it.
@@ -387,6 +393,10 @@ struct ofproto_class {
void (*rule_remove)(struct rule *rule);
+ /* It's not clear to me why the next couple of functions, and
+ * port_is_lacp_current() above don't get comments. When reading this, I
+ * would have found a comment on rule_execute() and rule_modify_actions()
+ * useful in particular. */
void (*rule_get_stats)(struct rule *rule, uint64_t *packet_count,
uint64_t *byte_count);
@@ -458,9 +468,12 @@ struct ofproto_class {
* has been registered, this has no effect.
*
* This function affects only the behavior of the OFPP_NORMAL action. An
- * implementation that does not to support it at all may set it to NULL or
+ * implementation that does not support it at all may set it to NULL or
* return EOPNOTSUPP. An implementation that supports only a subset of the
- * functionality should implement what it can and return 0. */
+ * functionality should implement what it can and return 0.
+ *
+ * It affects the behavior of the autopath action as well.
+ * */
int (*bundle_set)(struct ofproto *ofproto, void *aux,
const struct ofproto_bundle_settings *s);
@@ -480,7 +493,7 @@ struct ofproto_class {
* has been registered, this has no effect.
*
* This function affects only the behavior of the OFPP_NORMAL action. An
- * implementation that does not to support it at all may set it to NULL or
+ * implementation that does not support it at all may set it to NULL or
* return EOPNOTSUPP. An implementation that supports only a subset of the
* functionality should implement what it can and return 0. */
int (*mirror_set)(struct ofproto *ofproto, void *aux,
@@ -491,7 +504,7 @@ struct ofproto_class {
* 'flood_vlans' is NULL, then MAC learning applies to all VLANs.
*
* This function affects only the behavior of the OFPP_NORMAL action. An
- * implementation that does not to support it may set it to NULL or return
+ * implementation that does not support it may set it to NULL or return
* EOPNOTSUPP. */
int (*set_flood_vlans)(struct ofproto *ofproto,
unsigned long *flood_vlans);
--
1.7.4.4
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
