This patch seems fine to me. I would think that documentation of the functions unchanged in this patch would probably be better placed in the previous patch where their prototypes are added. Not a big deal though.
Ethan On Tue, Apr 26, 2011 at 13:51, Ben Pfaff <[email protected]> wrote: > Also, make a few minor adjustments to the interface so that it makes a > little more sense. > --- > ofproto/ofproto-dpif.c | 15 +++- > ofproto/ofproto.c | 21 +++-- > ofproto/private.h | 190 > +++++++++++++++++++++++++++++++++++++++++++++--- > 3 files changed, 203 insertions(+), 23 deletions(-) > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index d83ed6a..4b34ff9 100644 > --- a/ofproto/ofproto-dpif.c > +++ b/ofproto/ofproto-dpif.c > @@ -2443,6 +2443,13 @@ rule_construct(struct rule *rule_) > struct rule_dpif *rule = rule_dpif_cast(rule_); > struct ofproto_dpif *ofproto = ofproto_dpif_cast(rule->up.ofproto); > struct cls_rule *displaced_rule; > + int error; > + > + error = validate_actions(rule->up.actions, rule->up.n_actions, > + &rule->up.cr.flow, ofproto->max_ports); > + if (error) { > + return error; > + } > > rule->used = rule->up.created; > rule->packet_count = 0; > @@ -2500,7 +2507,7 @@ rule_get_stats(struct rule *rule_, uint64_t *packets, > uint64_t *bytes) > } > } > > -static void > +static int > rule_execute(struct rule *rule_, struct flow *flow, struct ofpbuf *packet) > { > struct rule_dpif *rule = rule_dpif_cast(rule_); > @@ -2514,7 +2521,7 @@ rule_execute(struct rule *rule_, struct flow *flow, > struct ofpbuf *packet) > facet = facet_lookup_valid(ofproto, flow); > if (facet && facet->rule == rule) { > facet_execute(ofproto, facet, packet); > - return; > + return 0; > } > > /* Otherwise, if 'rule' is in fact the correct rule for 'packet', then > @@ -2523,7 +2530,7 @@ rule_execute(struct rule *rule_, struct flow *flow, > struct ofpbuf *packet) > facet = facet_create(rule, flow, packet); > facet_execute(ofproto, facet, packet); > facet_install(ofproto, facet, true); > - return; > + return 0; > } > > /* We can't account anything to a facet. If we were to try, then that > @@ -2539,6 +2546,8 @@ rule_execute(struct rule *rule_, struct flow *flow, > struct ofpbuf *packet) > flow_push_stats(rule, flow, 1, size, rule->used); > } > ofpbuf_delete(odp_actions); > + > + return 0; > } > > static int > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c > index 0e76666..a22efe3 100644 > --- a/ofproto/ofproto.c > +++ b/ofproto/ofproto.c > @@ -695,8 +695,10 @@ ofproto_run(struct ofproto *p) > return ENODEV; > } > > - while ((error = p->ofproto_class->port_poll(p, &devname)) != EAGAIN) { > - process_port_change(p, error, devname); > + if (p->ofproto_class->port_poll) { > + while ((error = p->ofproto_class->port_poll(p, &devname)) != EAGAIN) > { > + process_port_change(p, error, devname); > + } > } > while ((error = netdev_monitor_poll(p->netdev_monitor, > &devname)) != EAGAIN) { > @@ -712,7 +714,9 @@ void > ofproto_wait(struct ofproto *p) > { > p->ofproto_class->wait(p); > - p->ofproto_class->port_poll_wait(p); > + if (p->ofproto_class->port_poll_wait) { > + p->ofproto_class->port_poll_wait(p); > + } > netdev_monitor_poll_wait(p->netdev_monitor); > connmgr_wait(p->connmgr); > } > @@ -1336,7 +1340,7 @@ ofproto_rule_lookup(struct ofproto *ofproto, const > struct flow *flow) > * with statistics for 'packet' either way. > * > * Takes ownership of 'packet'. */ > -static void > +static int > rule_execute(struct rule *rule, uint16_t in_port, struct ofpbuf *packet) > { > struct flow flow; > @@ -1344,7 +1348,7 @@ rule_execute(struct rule *rule, uint16_t in_port, > struct ofpbuf *packet) > assert(ofpbuf_headroom(packet) >= sizeof(struct ofp_packet_in)); > > flow_extract(packet, 0, in_port, &flow); > - rule->ofproto->ofproto_class->rule_execute(rule, &flow, packet); > + return rule->ofproto->ofproto_class->rule_execute(rule, &flow, packet); > } > > /* Remove 'rule' from 'ofproto' and free up the associated memory: > @@ -2205,7 +2209,8 @@ add_flow(struct ofconn *ofconn, struct flow_mod *fm) > } > > if (packet) { > - rule_execute(rule, in_port, packet); > + assert(!buf_err); > + return rule_execute(rule, in_port, packet); > } > return buf_err; > } > @@ -2233,9 +2238,7 @@ send_buffered_packet(struct ofconn *ofconn, > return error; > } > > - rule_execute(rule, in_port, packet); > - > - return 0; > + return rule_execute(rule, in_port, packet); > } > > /* OFPFC_MODIFY and OFPFC_MODIFY_STRICT. */ > diff --git a/ofproto/private.h b/ofproto/private.h > index d79ed81..cbf2c2e 100644 > --- a/ofproto/private.h > +++ b/ofproto/private.h > @@ -204,8 +204,30 @@ struct ofproto_class { > /* ## Factory Functions ## */ > /* ## ----------------- ## */ > > + /* Enumerates the types of all support ofproto types into 'types'. The > + * caller has already initialized 'types' and other ofproto classes might > + * already have added names to it. */ > void (*enumerate_types)(struct sset *types); > + > + /* Enumerates the names of all existing datapath of the specified 'type' > + * into 'names' 'all_dps'. The caller has already initialized 'names' as > + * an empty sset. > + * > + * 'type' is one of the types enumerated by ->enumerate_types(). > + * > + * Returns 0 if successful, otherwise a positive errno value. > + */ > int (*enumerate_names)(const char *type, struct sset *names); > + > + /* Deletes the datapath with the specified 'type' and 'name'. The caller > + * should have closed any open ofproto with this 'type' and 'name'; this > + * function is allowed to fail if that is not the case. > + * > + * 'type' is one of the types enumerated by ->enumerate_types(). > + * 'name' is one of the names enumerated by ->enumerate_names() for > 'type'. > + * > + * Returns 0 if successful, otherwise a positive errno value. > + */ > int (*del)(const char *type, const char *name); > > /* ## --------------------------- ## */ > @@ -224,7 +246,10 @@ struct ofproto_class { > * Only one ofproto instance needs to be supported for any given datapath. > * If a datapath is already open as part of one "ofproto", then another > * attempt to "construct" the same datapath as part of another ofproto is > - * allowed to fail with an error. */ > + * allowed to fail with an error. > + * > + * ->construct() returns 0 if successful, otherwise a positive errno > + * value. */ > struct ofproto *(*alloc)(void); > int (*construct)(struct ofproto *ofproto); > void (*destruct)(struct ofproto *ofproto); > @@ -238,6 +263,10 @@ struct ofproto_class { > * > * - Call ofproto_rule_expire() for each OpenFlow flow that has reached > * its hard_timeout or idle_timeout, to expire the flow. > + * > + * Returns 0 if successful, otherwise a positive errno value. The ENODEV > + * return value specifically means that the datapath underlying 'ofproto' > + * has been destroyed (externally, e.g. by an admin running ovs-dpctl). > */ > int (*run)(struct ofproto *ofproto); > > @@ -284,6 +313,9 @@ struct ofproto_class { > * implementation's ports, in the same way as at ofproto > * initialization, and construct and destruct ofports to reflect all > of > * the changes. > + * > + * ->port_construct() returns 0 if successful, otherwise a positive errno > + * value. > */ > struct ofport *(*port_alloc)(void); > int (*port_construct)(struct ofport *ofport); > @@ -316,12 +348,22 @@ struct ofproto_class { > int (*port_query_by_name)(const struct ofproto *ofproto, > const char *devname, struct ofproto_port *port); > > - /* Attempts to add 'netdev' as a port on 'ofproto'. If successful, sets > - * '*ofp_portp' to the new port's port number. */ > + /* Attempts to add 'netdev' as a port on 'ofproto'. Returns 0 if > + * successful, otherwise a positive errno value. If successful, sets > + * '*ofp_portp' to the new port's port number. > + * > + * It doesn't matter whether the new port will be returned by a later > call > + * to ->port_poll(); the implementation may do whatever is more > + * convenient. */ > int (*port_add)(struct ofproto *ofproto, struct netdev *netdev, > uint16_t *ofp_portp); > > - /* Deletes port number 'ofp_port' from the datapath for 'ofproto'. */ > + /* Deletes port number 'ofp_port' from the datapath for 'ofproto'. > Returns > + * 0 if successful, otherwise a positive errno value. > + * > + * It doesn't matter whether the new port will be returned by a later > call > + * to ->port_poll(); the implementation may do whatever is more > + * convenient. */ > int (*port_del)(struct ofproto *ofproto, uint16_t ofp_port); > > /* Attempts to begin dumping the ports in 'ofproto'. On success, returns > 0 > @@ -371,39 +413,165 @@ struct ofproto_class { > * > * If the set of ports in 'ofproto' has not changed, returns EAGAIN. May > * also return other positive errno values to indicate that something has > - * gone wrong. */ > + * gone wrong. > + * > + * If the set of ports in a datapath is fixed, or if the only way that > the > + * set of ports in a datapath can change is through ->port_add() and > + * ->port_del(), then this function may be a null pointer. > + */ > int (*port_poll)(const struct ofproto *ofproto, char **devnamep); > > - /* Arranges for the poll loop to wake up when 'port_poll' will return a > - * value other than EAGAIN. */ > + /* Arranges for the poll loop to wake up when ->port_poll() will return a > + * value other than EAGAIN. > + * > + * If the set of ports in a datapath is fixed, or if the only way that > the > + * set of ports in a datapath can change is through ->port_add() and > + * ->port_del(), or if the poll loop will always wake up anyway when > + * ->port_poll() will return a value other than EAGAIN, then this > function > + * may be a null pointer. > + */ > void (*port_poll_wait)(const struct ofproto *ofproto); > > + /* Checks the status of LACP negotiation for 'port'. Returns 1 if LACP > + * partner information for 'port' is up-to-date, 0 if LACP partner > + * information is not current (generally indicating a connectivity > + * problem), or -1 if LACP is not enabled on 'port'. > + * > + * This function may be a null pointer if the ofproto implementation does > + * not support LACP. */ > int (*port_is_lacp_current)(const struct ofport *port); > > +/* ## ----------------------- ## */ > +/* ## OpenFlow Rule Functions ## */ > +/* ## ----------------------- ## */ > + > + /* Life-cycle functions for a "struct rule" (see "Life Cycle" above). > + * > + * ->rule_construct() should first check whether the rule is acceptable: > + * > + * - Validate that the matching rule in 'rule->cr' is supported by the > + * datapath. If not, then return an OpenFlow error code (as returned > + * by ofp_mkerr()). > + * > + * For example, if the datapath does not support registers, then it > + * should return an error if 'rule->cr' does not wildcard all > + * registers. > + * > + * - Validate that 'rule->actions' and 'rule->n_actions' are > well-formed > + * OpenFlow actions that can be correctly implemented by the > datapath. > + * If not, then return an OpenFlow error code (as returned by > + * ofp_mkerr()). > + * > + * The validate_actions() function (in ofp-util.c) can be useful as a > + * model for action validation, but it accepts all of the OpenFlow > + * actions that OVS understands. If your ofproto implementation only > + * implements a subset of those, then you should implement your own > + * action validation. > + * > + * If the rule is acceptable, then ->rule_construct() should modify the > + * flow table: > + * > + * - If there was already a rule with exactly the same matching > criteria > + * and priority in the classifier, then it should remove that rule > from > + * the classifier and destroy it (with ofproto_rule_destroy()). > + * > + * - Insert the new rule into the ofproto's 'cls' classifier, and into > + * the datapath flow table. > + * > + * (The function classifier_insert() both inserts a rule into the > + * classifier and removes any rule with identical matching criteria, > so > + * this single call implements parts of both steps above.) > + * > + * Other than inserting 'rule->cr' into the classifier, > ->rule_construct() > + * should not modify any base members of struct rule. > + * > + * When ->rule_destruct() is called, 'rule' has already been removed from > + * the classifier and the datapath flow table (by calling > ->rule_remove()), > + * so ->rule_destruct() should not duplicate that behavior. */ > struct rule *(*rule_alloc)(void); > int (*rule_construct)(struct rule *rule); > void (*rule_destruct)(struct rule *rule); > void (*rule_dealloc)(struct rule *rule); > > + /* Removes 'rule' from 'rule->ofproto->cls' and from the datapath. > + * > + * 'rule' will be destructed, with ->rule_destruct(), soon after. */ > void (*rule_remove)(struct rule *rule); > > + /* Obtains statistics for 'rule', storing the number of packets that have > + * matched it in '*packet_count' and the number of bytes in those packets > + * in '*byte_count'. */ > void (*rule_get_stats)(struct rule *rule, uint64_t *packet_count, > uint64_t *byte_count); > > - void (*rule_execute)(struct rule *rule, struct flow *flow, > - struct ofpbuf *packet); > - > + /* Applies the actions in 'rule' to 'packet'. (This implements sending > + * buffered packets for OpenFlow OFPT_FLOW_MOD commands.) > + * > + * Takes ownership of 'packet' (so it should eventually free it, with > + * ofpbuf_delete()). > + * > + * 'flow' reflects the flow information for 'packet'. All of the > + * information in 'flow' is extracted from 'packet', except for > + * flow->tun_id and flow->in_port, which are assigned the correct values > + * for the incoming packet. The register values are zeroed. > + * > + * The statistics for 'packet' should be included in 'rule'. > + * > + * Returns 0 if successful, otherwise an OpenFlow error code (as returned > + * by ofp_mkerr()). */ > + int (*rule_execute)(struct rule *rule, struct flow *flow, > + struct ofpbuf *packet); > + > + /* Validates that the 'n' elements in 'actions' are well-formed OpenFlow > + * actions that can be correctly implemented by the datapath. If not, > then > + * return an OpenFlow error code (as returned by ofp_mkerr()). If so, > + * then update the datapath to implement the new actions and return 0. > + * > + * When this function runs, 'rule' still has its original actions. If > this > + * function returns 0, then the caller will update 'rule' with the new > + * actions and free the old ones. */ > int (*rule_modify_actions)(struct rule *rule, > const union ofp_action *actions, size_t n); > > + /* These functions implement the OpenFlow IP fragment handling policy. > By > + * default ('drop_frags' == false), an OpenFlow switch should treat IP > + * fragments the same way as other packets (although TCP and UDP port > + * numbers cannot be determined). With 'drop_frags' == true, the switch > + * should drop all IP fragments without passing them through the flow > + * table. */ > bool (*get_drop_frags)(struct ofproto *ofproto); > void (*set_drop_frags)(struct ofproto *ofproto, bool drop_frags); > > + /* Implements the OpenFlow OFPT_PACKET_OUT command. The datapath should > + * execute the 'n_actions' in the 'actions' array on 'packet'. > + * > + * The caller retains ownership of 'packet', so ->packet_out() should not > + * modify or free it. > + * > + * This function must validate that the 'n_actions' elements in 'actions' > + * are well-formed OpenFlow actions that can be correctly implemented by > + * the datapath. If not, then it should return an OpenFlow error code > (as > + * returned by ofp_mkerr()). > + * > + * 'flow' reflects the flow information for 'packet'. All of the > + * information in 'flow' is extracted from 'packet', except for > + * flow->in_port, which is taken from the OFPT_PACKET_OUT message. > + * flow->tun_id and its register values are zeroed. > + * > + * 'packet' is not matched against the OpenFlow flow table, so its > + * statistics should not be included in OpenFlow flow statistics. > + * > + * Returns 0 if successful, otherwise an OpenFlow error code (as returned > + * by ofp_mkerr()). */ > int (*packet_out)(struct ofproto *ofproto, struct ofpbuf *packet, > const struct flow *flow, > const union ofp_action *actions, > size_t n_actions); > - > + > +/* ## ------------------------- ## */ > +/* ## OFPP_NORMAL configuration ## */ > +/* ## ------------------------- ## */ > + > /* Configures NetFlow on 'ofproto' according to the options in > * 'netflow_options', or turns off NetFlow if 'netflow_options' is NULL. > * > -- > 1.7.4.4 > > _______________________________________________ > dev mailing list > [email protected] > http://openvswitch.org/mailman/listinfo/dev > _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
