On 11/19/17 1:17 AM, Arkadi Sharshevsky wrote:
>
>
> On 11/18/2017 08:34 PM, David Ahern wrote:
>> On 11/14/17 9:18 AM, Jiri Pirko wrote:
>>> diff --git a/include/net/devlink.h b/include/net/devlink.h
>>> index 4d2c6fc..960e80a 100644
>>> --- a/include/net/devlink.h
>>> +++ b/include/net/devlink.h
>> ...
>>
>>> @@ -469,6 +523,32 @@ devlink_dpipe_match_put(struct sk_buff *skb,
>>> return 0;
>>> }
>>>
>>> +static inline int
>>> +devlink_resource_register(struct devlink *devlink,
>>> + const char *resource_name,
>>> + bool top_hierarchy,
>>> + bool reload_required,
>>> + u64 resource_size,
>>> + u64 resource_id,
>>> + u64 parent_resource_id,
>>> + struct devlink_resource_ops *resource_ops)
>>> +{
>>> + return 0;
>>> +}
>>> +
>>> +static inline void
>>> +devlink_resources_unregister(struct devlink *devlink,
>>> + struct devlink_resource *resource)
>>> +{
>>> +}
>>> +
>>> +static inline int
>>> +devlink_resource_size_get(struct devlink *devlink, u64 resource_id,
>>> + u64 *p_resource_size)
>>> +{
>>> + return -EINVAL;
>>
>> It's compiled out so -EOPNOTSUPP seems more appropriate.
>>
>
> will fix
>
>>
>>
>>> diff --git a/net/core/devlink.c b/net/core/devlink.c
>>> index 0114dfc..6ae644f 100644
>>> --- a/net/core/devlink.c
>>> +++ b/net/core/devlink.c
>>> +static int devlink_nl_cmd_resource_set(struct sk_buff *skb,
>>> + struct genl_info *info)
>>> +{
>>> + struct devlink *devlink = info->user_ptr[0];
>>> + struct devlink_resource *resource;
>>> + u64 resource_id;
>>> + u64 size;
>>> + int err;
>>> +
>>> + if (!info->attrs[DEVLINK_ATTR_RESOURCE_ID] ||
>>> + !info->attrs[DEVLINK_ATTR_RESOURCE_SIZE])
>>> + return -EINVAL;
>>
>> several of the of the DEVLINK_ATTR_RESOURCE attributes are kernel to
>> user only (e.g., DEVLINK_ATTR_RESOURCE_SIZE_NEW and
>> DEVLINK_ATTR_RESOURCE_RELOAD_REQUIRED), so if they are given by the user
>> that should be an error too right?
>>
>
> Not sure I understood. As you see I only check for the mandatory
> attributes, if the user provides not relevant data its ignored.
>
> We use one single nla_policy for all the commands (devlink_nl_policy)
>
>>
>>> + resource_id = nla_get_u64(info->attrs[DEVLINK_ATTR_RESOURCE_ID]);
>>
>> I don't see where these attributes are validated for proper size.
>>
>
> right, forgot to update the policy.
>
>>> +
>>> + resource = devlink_resource_find(devlink, NULL, resource_id);
>>> + if (!resource)
>>> + return -EINVAL;
>>> +
>>> + if (!resource->resource_ops->size_validate)
>>> + return -EINVAL;
>>
>> genl_info has extack; please add user messages for the above failures.
>>
>
> Isn't EOPNOTSUPP enough ?
No, I mean every failure above returns EINVAL. Add an extack message
telling the user what is wrong. e.g,
resource = devlink_resource_find(devlink, NULL, resource_id);
if (!resource) {
NL_SET_ERR_MSG(extack, "Invalid resource id");
return -EINVAL;
}
similarly for the rest.
