Re: [PATCH V1 for-next 3/7] IB/core: Add idr based standard types

[Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>]

On Wed, Feb 01, 2017 at 02:39:01PM +0200, Matan Barak wrote:

> +void uverbs_free_qp(struct ib_uobject *uobject)

This should be static...

Based on the last email I expect this to be

static int uverbs_destroy_qp(struct ib_uobject *uobject, enum rdma_remove_reason why)

> +{
> +	struct ib_qp *qp = uobject->object;
> +	struct ib_uqp_object *uqp =
> +		container_of(uobject, struct ib_uqp_object, uevent.uobject);
> +
> +	if (qp == qp->real_qp)
> +		ib_uverbs_detach_umcast(qp, uqp);
> +	ib_destroy_qp(qp);

This silently eats the error code from ib_destroy_qp, not OK. See
discussion in prior email. Lots of cases of this.

> +const struct uverbs_obj_idr_type uverbs_type_attrs_cq =
> +	UVERBS_TYPE_ALLOC_IDR_SZ(sizeof(struct ib_ucq_object), 0,
> +				 uverbs_free_cq);
>

It would be more readable if the struct decl was near the functions in
blocks.

Still don't like these macros. The open coded version would look like this:

const struct uverbs_obj_idr_type uverbs_type_attrs_cq = {
      .type = {
      	    .obj_size = sizeof(struct ib_ucq_object),
      	    .ops = &uverbs_idr_ops,
      },
      .destroy = uverbs_destroy_cq,
};

Whichs is more understandable and typical of the kernel style. If we
wish to add a new callback function for instance we don't need a new
macro or big churn.

You could do this I guess:

const struct uverbs_obj_idr_type uverbs_type_attrs_cq = {
      .type = UVERBS_IDR_TYPE(cq, 0),
      .destroy = uverbs_destroy_cq,
};

> +extern const struct uverbs_obj_type_ops uverbs_idr_ops;
> +
> +#define UVERBS_BUILD_BUG_ON(cond) (sizeof(char[1 - 2 * !!(cond)]) -	\
> +				   sizeof(char))

This probably doesn't add value, but you could make this a WARN_ON in
alloc_uobj().

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html