Re: [PATCH ib-next 2/3] IB/core: IB/core: Allow legacy verbs through extended interfaces

[Eli Cohen <eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>]

On Sun, Nov 08, 2015 at 05:04:55PM +0200, Matan Barak wrote:
> > +static int verify_command_mask(struct ib_device *ib_dev, __u32 command)
> > +{
> > +       u64 mask;
> > +
> > +       if (command <= IB_USER_VERBS_CMD_OPEN_QP)
> 
> I think using IB_USER_VERBS_CMD_THRESHOLD is clearer, but I don't
> think we need two uverbs_cmd_mask vendor variables.
> IMHO, a vendor shouldn't care if it's an extended/legacy uverb
> command. Maybe we should replace uverbs_cmd_mask with a bitmap.
> 
> > +               mask = ib_dev->uverbs_cmd_mask;
> > +       else
> > +               mask = ib_dev->uverbs_ex_cmd_mask;
> > +
> > +       if (mask & ((u64)1 << command))
> > +               return 0;
> > +
> > +       return -1;
> > +}
> > +

But IB_USER_VERBS_CMD_OPEN_QP is the last legacy verbs and I prefer a
more restrictive approach to avoid potentail issues in the future.

> >  static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
> >                              size_t count, loff_t *pos)
> >  {
> > @@ -704,6 +719,10 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
> >         }
> >
> >         command = hdr.command & IB_USER_VERBS_CMD_COMMAND_MASK;
> > +       if (verify_command_mask(ib_dev, command)) {
> > +               ret = -EINVAL;
> 
> Why did you replace ENOSYS with EINVAL?
> 

Like Haggai mentioned in the other response, checkpatch issues error
on this claiming that ENOSYS is reserved to unavailable system calls.
Is it applicable only for new implementations I am not sure. I don't
have clear preference for either ENOSYS or EINAVL.
> > +               goto out;
> > +       }
> >
--
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