[openib-general] [PATCH] rdma_bind_addr() leaks a cma_dev reference count

[Krishna Kumar2]

> look up Documentation/CodingStyle Chapter 7: Centralized exiting of 
functions
> which says unconditional statements are easier to understand and follow,
> and note that with this style nesting is reduced.

Hmmm, OK, I will re-phrase this patch to reduce nesting.

thanks,

- KK

"Michael S. Tsirkin" <mst at mellanox.co.il> wrote on 10/17/2006 10:53:03 AM:

> Quoting r. Krishna Kumar <krkumar2 at in.ibm.com>:
> > Subject: [PATCH] rdma_bind_addr() leaks a cma_dev reference count
> > 
> > rdma_bind_addr leaks a cma_dev reference count in failure
> > case.
> > 
> > Signed-off-by: Krishna Kumar <krkumar2 at in.ibm.com>
> > --------
> > diff -ruNp org/drivers/infiniband/core/cma.c 
new/drivers/infiniband/core/cma.c
> > --- org/drivers/infiniband/core/cma.c   2006-10-09 17:13:41.000000000 
+0530
> > +++ new/drivers/infiniband/core/cma.c   2006-10-09 19:42:31.000000000 
+0530
> > @@ -1749,6 +1749,7 @@ static int cma_get_port(struct rdma_id_p
> >  int rdma_bind_addr(struct rdma_cm_id *id, struct sockaddr *addr)
> >  {
> >     struct rdma_id_private *id_priv;
> > +   int did_acquire_dev = 0;
> >     int ret;
> > 
> >     if (addr->sa_family != AF_INET)
> > @@ -1765,18 +1766,20 @@ int rdma_bind_addr(struct rdma_cm_id *id
> >           ret = cma_acquire_dev(id_priv);
> >           mutex_unlock(&lock);
> >        }
> > -      if (ret)
> > -         goto err;
> > +      if (!ret)
> > +         did_acquire_dev = 1;
> > +      else
> > +         goto out;
> >     }
> > -
> >     memcpy(&id->route.addr.src_addr, addr, ip_addr_size(addr));
> >     ret = cma_get_port(id_priv);
> > -   if (ret)
> > -      goto err;
> > 
> > -   return 0;
> > -err:
> > -   cma_comp_exch(id_priv, CMA_ADDR_BOUND, CMA_IDLE);
> > +out:
> > +   if (ret) {
> > +      if (did_acquire_dev)
> > +         cma_detach_from_dev(id_priv);
> > +      cma_comp_exch(id_priv, CMA_ADDR_BOUND, CMA_IDLE);
> > +   }
> >     return ret;
> >  }
> >  EXPORT_SYMBOL(rdma_bind_addr);
> 
> Ugh, replacing two labels with an if statement is uglifying code: look 
how
> nesting got 2-deep already.  Please do the error handling the usual 
linux way
> without testing flags:
> 
>    if (ret)
>       goto err
> 
>    ....
> err:
>    cma_comp_exch(id_priv, CMA_ADDR_BOUND, CMA_IDLE);
> out:
>    return ret;
> 
> is better than
>    if (ret)
>       goto out;
> 
>    ....
> 
> out:
>    if (ret)
>       cma_comp_exch(id_priv, CMA_ADDR_BOUND, CMA_IDLE);
> 
> look up Documentation/CodingStyle Chapter 7: Centralized exiting of 
functions
> which says unconditional statements are easier to understand and follow,
> and note that with this style nesting is reduced.
> 
> -- 
> MST