[openib-general] Re: [PATCH] libmthca: fix error handling in mthca_store_qp

[Michael S. Tsirkin]

Quoting Roland Dreier <rdreier at cisco.com>:
> Subject: Re: [PATCH] libmthca: fix error handling in mthca_store_qp
> 
> Thanks, good catch.  I decided it was clearer to split off the test of
> refcnt, and only increment refcnt if the allocation succeeds.
> 
>  - R.
> 
> --- libmthca/src/qp.c	(revision 4465)
> +++ libmthca/src/qp.c	(working copy)
> @@ -875,13 +875,15 @@ int mthca_store_qp(struct mthca_context 
>  
>  	pthread_mutex_lock(&ctx->qp_table_mutex);
>  
> -	if (!ctx->qp_table[tind].refcnt++) {
> +	if (!ctx->qp_table[tind].refcnt) {
>  		ctx->qp_table[tind].table = calloc(ctx->qp_table_mask + 1,
>  						   sizeof (struct mthca_qp *));
>  		if (!ctx->qp_table[tind].table) {
>  			ret = -1;
>  			goto out;
>  		}
> +
> +		++ctx->qp_table[tind].refcnt;
>  	}
>  
>  	ctx->qp_table[tind].table[qpn & ctx->qp_table_mask] = qp;


This does not look right: it seems you are incrementing the counter
from 0 to 1, but it never goes to 2.

How about this:

---

Only increment qp_table ref count if allocation succeeds.
Signed-off-by: Michael S. Tsirkin <mst at mellanox.co.il>

Index: openib/src/userspace/libmthca/src/qp.c
===================================================================
--- openib/src/userspace/libmthca/src/qp.c	(revision 4466)
+++ openib/src/userspace/libmthca/src/qp.c	(working copy)
@@ -875,7 +875,7 @@ int mthca_store_qp(struct mthca_context 
 
 	pthread_mutex_lock(&ctx->qp_table_mutex);
 
-	if (!ctx->qp_table[tind].refcnt++) {
+	if (!ctx->qp_table[tind].refcnt) {
 		ctx->qp_table[tind].table = calloc(ctx->qp_table_mask + 1,
 						   sizeof (struct mthca_qp *));
 		if (!ctx->qp_table[tind].table) {
@@ -884,6 +884,8 @@ int mthca_store_qp(struct mthca_context 
 		}
 	}
 
+	++ctx->qp_table[tind].refcnt;
+
 	ctx->qp_table[tind].table[qpn & ctx->qp_table_mask] = qp;
 
 out:

-- 
MST