On 1/16/18 4:19 PM, Jamal Hadi Salim wrote:
> On 18-01-16 06:58 PM, David Ahern wrote:
>> On 1/16/18 9:20 AM, Alexander Aring wrote:
>
>
>>> }
>>> if (n->nlmsg_type != RTM_NEWTFILTER ||
>>> !(n->nlmsg_flags & NLM_F_CREATE)) {
>>> + NL_SET_ERR_MSG(extack, "Need both RTM_NEWTFILTER and
>>> NLM_F_CREATE to create a new filter");
>>
>> that does not seem the right message. tc_ctl_tfilter is overloaded for
>> new, delete and get so the response here needs to reflect that. I
>> believe in this case the user did not specify a valid chain.
>>
>
> Are you sure you are looking at the correct code?
tp = tcf_chain_tp_find(chain, &chain_info, protocol,
prio, prio_allocate);
if (IS_ERR(tp)) {
err = PTR_ERR(tp);
goto errout;
}
if (tp == NULL) {
/* Proto-tcf does not exist, create new one */
if (tca[TCA_KIND] == NULL || !protocol) {
err = -EINVAL;
goto errout;
}
if (n->nlmsg_type != RTM_NEWTFILTER ||
!(n->nlmsg_flags & NLM_F_CREATE)) {
err = -ENOENT;
goto errout;
}
Seems like that code path is run for other than RTM_NEWTFILTER. Even the
check there says != is ok -- just error out with an ENOENT.
> It is a create message that is at stake here.
> A create has to have RTM_NEWTFILTER and NLM_F_CREATE
>
>> Also, the messages are targeted at users not developers, so no code
>> jargon / API references.
>
> Generally true, but should this rule really be scripture?
> The main user here is tc in user space and it doesnt make mistakes
> in this case i.e we will never see this error with tc because a
> create will always have those two set correctly; OTOH, a developer
> writing some new app is more likely to make this mistake (in which
> case this message is very helpful).
argumentative. I have focused on adding specific error messages that
help a user understand why a command failed. It can be done with
referencing API names.
