Re: [PATCH 3/8] libibverbs: Introduce XRC domains

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

On Mon, Sep 24, 2012 at 08:45:10PM +0000, Hefty, Sean wrote:
> > > +struct ibv_xrcd *__ibv_open_xrcd(struct ibv_context *context, int fd, int
> > oflags)
> > > +{
> > > +	struct verbs_context *context_ex = verbs_get_ctx(context);
> > > +	struct ibv_xrcd *xrcd;
> > 
> > This patch series seems to be missing the checks against a null return
> > of verbs_get_ctx - if the driver is not extended these functions
> > should return ENOSYS, not crash..
> 
> Patch 2 extends ibv_context for providers that don't by adding a
> translation layer for older providers.  This seemed to keep things
> simpler, since the core verbs code could rely on extension support.

Well, that patch is pretty complex and it slows down common case high
speed calls (eg ibv_poll_cq) unless the provider is (fully?)
upgraded. That is something I wanted to see avoided.

Plus I don't see that we should be trying to make extended structures
for many of the things in patch 2 (ibv_qp, ibv_srq, ibv_ah, ibv_mr,
ibv_cq, ibv_pd, ibv_mw). Additional functionality for those objects is
better served through new, optional, function entry points than by
allowing the consumer to muck about directl in those structures. Those
structs, in particular, should have been opaque from day 1, IMHO.

So at first blush, adding null checks in _ex functions seems like a
lessor evil, IHMO...

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