[ovs-dev] [nxm 30/42] ofproto: Refactor handle_flow_mod().

Ben Pfaff blp at nicira.com
Mon Nov 8 18:18:42 UTC 2010


On Sat, Nov 06, 2010 at 12:08:39AM -0700, Justin Pettit wrote:
> On Oct 28, 2010, at 10:28 AM, Ben Pfaff wrote:
> 
> > + * If successful, the first action is stored in '*actionsp' and the number of
> > + * "union ofp_action" size elements into '*n_actionsp'.  Otherwise NULL and 0
> > + * are stored, respectively.
> > + *
> > + * This function not check that the actions are valid (the caller should do so,
> 
> This sentence not have verb, Ock.

Isn't "check" a verb?

Fixed.

> > + * with validate_actions()).  The caller is also responsible for making sure
> > + * that 'b->data' is initially aligned appropriately for "union ofp_action". */
> > +int
> > +ofputil_pull_actions(struct ofpbuf *b, unsigned int actions_len,
> > +                     union ofp_action **actionsp, size_t *n_actionsp)
> 
> The function check_ofp_packet_out() does a lot of these same checks.
> It may want to use this function, too.

That function gets deleted later in this series, because it doesn't fit
well into the refactored code.

> > +static int
> > +handle_flow_mod(struct ofproto *p, struct ofconn *ofconn,
> > +                struct ofp_header *oh)
> > +{
> > +    struct ofp_match orig_match;
> > +    struct ofp_flow_mod *ofm;
> > +    struct flow_mod fm;
> > +    struct ofpbuf b;
> > +    int error;
> > +
> > +    b.data = oh;
> > +    b.size = ntohs(oh->length);
> 
> Even though it's fine here, it seems like it would be safer to
> initialize "b" in case any of the functions used on it change later.

We don't really have a function for doing that gracefully.  We'd have to
initialize it and then do a ofpbuf_put_uninit().  It might be worth
creating a function for doing this, since it's not the only place we do
similar things.




More information about the dev mailing list