On Tue, Nov 10, 2015 at 1:15 AM, Jason Gunthorpe
<jgunthorpe@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Tue, Nov 10, 2015 at 01:05:31AM +0200, Eli Cohen wrote:
>> On Mon, Nov 09, 2015 at 03:35:31PM -0700, Jason Gunthorpe wrote:
>> > On Mon, Nov 09, 2015 at 08:48:36AM +0200, Haggai Eran wrote:
>> > > On 08/11/2015 17:04, Matan Barak wrote:
>> > > >> @@ -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?
>> > >
>> > > I think ENOSYS is meant only to represent a system call number that
>> > > isn't supported. There was a change in checkpatch that now warns about
>> > > it [1]. I'm not sure the intention was to fix existing uses though.
>> >
>> > Within verbs we should have two kinds of return - not supported
>> > request, and supported request with invalid parameters.
>> >
>> > Maybe use EOPNOTSUPP ?
>> >
>>
>> What about Matan's concern regarding legacy code? Maybe we should
>> leave ENOSYS as that's how it is all over the IB stack code.
>>
>> quote:
>> 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.
>
> Since the change is to make the kernel do the above fall back
> internally, this specific example doesn't make alot of sense to worry
> about. Ie the extended verb won't fail anymore, and if it does the
> legacy one won't work anyhow.
>
The kernel will do the above fallback if the command is a legacy
command wrapped in an extended command (i.e - no extra flags).
If an application uses one of the extended values, and fall back to
legacy verb on ENOSYS, it'll behave differently after this change:
Re-posting this example:
err = extended_verb(*the_first_extension_we_added*); /* Kernel won't
fall back to legacy verbs here, as we use an actual extended request
*/
if (err == ENOSYS)
err = legacy_verb();
if (err)
return err;
> But if there is something out there that does care about ENOSYS we
> should try to keep it, but don't convert ENOSYS to EINVAL.
>
> Also, when the driver tests the ex flags for support it should be
> returning EOPNOTSUPP or such not EINVAL.. Return codes for the ex
> stuff could stand a good sanity audit.
>
> Jason
Matan
--
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
