Re: [PATCH V4 8/9] IB/core: Add RoCE IP based addressing extensions for rdma_ucm

[Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>]

On 17/9/2013 1:25 PM, Yann Droneaud wrote:
> Hi,
>
> Le 17.09.2013 12:02, Matan Barak a écrit :
>>
>> That's right - we're not checking anything here.
>> struct ib_uverbs_qp_attr_ex contains 2 user pointers:
>> +       /* represents: struct ib_uverbs_ah_attr_ex * __user */
>> +       void __user *ah_attr_ex;
>> +       /* represents: struct ib_uverbs_ah_attr_ex * __user */
>> +       void __user *alt_ah_attr_ex;
>>
>> Those pointers should be given to rdma_init_qp_attr in order to
>> initialize them.
>>
>> We are using pointers here in order to maintain future extendability
>> of the address handle structures.
>>
>
> First: you can't put pointer asis in public data structure. Look to all
> others "command" structures
> declared in include/uapi/rdma/ib_user_verbs.h

Thanks for the review. Looking at other commands, I see that pointers 
(such a the response) are passed as __u64 at the command structure. Is 
that what you mean ? I think it's a bit odd to pass those pointers as a 
part of the command, as they are output only attributes. Though, I'll 
change the code to use __u64 instead of the actual __user pointers.

>
> Second: if you're duplicating struct ib_uverbs_qp_attr, why not include
> it in struct ib_uverbs_qp_attr_ex
> it will reduce maintenance burden, clutter, etc.
> Or drop the unused fields in the _ex version while taking care of being
> at least equal size than
> the original version.

The extension verbs approach should be an evolution. Instead of using a 
cumbersome extended.basic.field notation, extended.field is more compact 
and readable. Using this notation will allow us to deprecate the basic 
structures when they won't be in use anymore.
Since the basic structures shouldn't change anymore as user applications 
relay on them, I don't see a burden in maintainability doing it this way.

>
> Same apply to struct ib_uverbs_modify_qp_ex, but struct
> ib_uverbs_modify_qp_ex has the comp_mask as first field (introducing a
> hole).

We'll add a reserved field to fix this hole. Thanks for that catch!

>
> Regards.
>
Regards
--
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