On Mon, Nov 9, 2015 at 5:55 PM, Eli Cohen <eli@xxxxxxxxxxxx> wrote:
> 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.
>
So maybe it's worth adding a IB_USER_VERBS_CMD_LAST_LEGACY_VERB.
It looks a bit weird why you explicitly check for IB_USER_VERBS_CMD_OPEN_QP.
>> > 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.
I think it could break old applications:
err = extended_verb(the_first_extension_we_added);
if (err == ENOSYS)
err = legacy_verb();
if (err)
return err;
Such applications used the first extension (that was added during the
addition of the extended verb) and when they realized it's not
supported, they dropped to the legacy verb. This change can now cause
the return of -EINVAL an early termination with an error.
>> > + goto out;
>> > + }
>> >
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
