[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openvswitch-dev
Subject:    [ovs-dev] [loops 4/6] datapath: Propagate packet transmit errors to execute_actions() caller.
From:       blp () nicira ! com (Ben Pfaff)
Date:       2010-07-27 17:12:32
Message-ID: 20100727171232.GC8777 () nicira ! com
[Download RAW message or body]

On Mon, Jul 26, 2010 at 04:11:03PM -0700, Jesse Gross wrote:
> On Fri, Jul 23, 2010 at 4:44 PM, Ben Pfaff <blp at nicira.com> wrote:
> >
> > +static struct dp_port *output_group(struct datapath *dp, __u16 group,
> > +                                   struct sk_buff *skb, gfp_t gfp)
> >  {
> > -       struct dp_port_group *g = rcu_dereference(dp->groups[group]);
> > -       int prev_port = -1;
> > +       struct dp_port_group *g;
> > +       struct dp_port *prev_port;
> 
> I think you want to initialize prev_port to NULL.

Ouch.  Usually I expect GCC to warn about this kind of egregious
problem, but it didn't.

> 
> >                case ODPAT_OUTPUT_GROUP:
> >                        prev_port = output_group(dp, a->output_group.group,
> >                                                 skb, gfp);
> > +                       if (IS_ERR(prev_port))
> > +                               return PTR_ERR(prev_port);
> >
> 
> Isn't this going to leak the original skb in the event of an error?

Good catch, thank you.  I added a "kfree_skb(skb);" before the return.

> > @@ -488,10 +499,10 @@ int execute_actions(struct datapath *dp, struct
> > sk_buff *skb,
> >                }
> >                if (!skb)
> >                        return -ENOMEM;
> >
> 
> I think there's a missing closing brace for the for loop here.

Oops.

> > +       if (prev_port) {
> > +               err = vport_send(prev_port->vport, skb);
> > +               if (unlikely(err < 0))
> > +                       return err;
> >        }
> > -       if (prev_port != -1)
> > -               do_output(dp, skb, prev_port);
> > -       else
> > -               kfree_skb(skb);
> 
> 
> Don't we need to free the skb if !prev_port?

Yes.

Boy, I really screwed up on this patch.  I must not have even compiled it,
and yet I remember doing that.



[prev in list] [next in list] [prev in thread] [next in thread]