diff mbox series

[net-next,v3,2/2] net: vxlan: vxlan_core.c: Add extack support to vxlan_fdb_delete

Message ID 0d09ad611bb78b9113491007955f2211f3a06e82.1650800975.git.eng.alaamohamedsoliman.am@gmail.com
State Superseded
Headers show
Series propagate extack to vxlan_fdb_delete | expand

Commit Message

Alaa Mohamed April 24, 2022, 12:09 p.m. UTC 
Add extack to vxlan_fdb_delete and vxlan_fdb_parse

Signed-off-by: Alaa Mohamed <eng.alaamohamedsoliman.am@gmail.com>
---
changes in V2:
	- fix spelling vxlan_fdb_delete
	- add missing braces
	- edit error message
---
changes in V3:
	fix errors reported by checkpatch.pl
---
 drivers/net/vxlan/vxlan_core.c | 36 +++++++++++++++++++++++-----------
 1 file changed, 25 insertions(+), 11 deletions(-)

--
2.36.0

Comments

Julia Lawall April 24, 2022, 4:15 p.m. UTC | #1 
On Sun, 24 Apr 2022, Alaa Mohamed wrote:

> Add extack to vxlan_fdb_delete and vxlan_fdb_parse

It could be helpful to say more about what adding extack support involves.

>
> Signed-off-by: Alaa Mohamed <eng.alaamohamedsoliman.am@gmail.com>
> ---
> changes in V2:
> 	- fix spelling vxlan_fdb_delete
> 	- add missing braces
> 	- edit error message
> ---
> changes in V3:
> 	fix errors reported by checkpatch.pl

But your changes would seem to also be introducing more checkpatch errors,
because the Linux kernel coding style puts a space before an { at the
start of an if branch.

julia

> ---
>  drivers/net/vxlan/vxlan_core.c | 36 +++++++++++++++++++++++-----------
>  1 file changed, 25 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
> index cf2f60037340..4e1886655101 100644
> --- a/drivers/net/vxlan/vxlan_core.c
> +++ b/drivers/net/vxlan/vxlan_core.c
> @@ -1129,19 +1129,23 @@ static void vxlan_fdb_dst_destroy(struct vxlan_dev *vxlan, struct vxlan_fdb *f,
>
>  static int vxlan_fdb_parse(struct nlattr *tb[], struct vxlan_dev *vxlan,
>  			   union vxlan_addr *ip, __be16 *port, __be32 *src_vni,
> -			   __be32 *vni, u32 *ifindex, u32 *nhid)
> +			   __be32 *vni, u32 *ifindex, u32 *nhid, struct netlink_ext_ack *extack)
>  {
>  	struct net *net = dev_net(vxlan->dev);
>  	int err;
>
>  	if (tb[NDA_NH_ID] && (tb[NDA_DST] || tb[NDA_VNI] || tb[NDA_IFINDEX] ||
> -	    tb[NDA_PORT]))
> -		return -EINVAL;
> +	    tb[NDA_PORT])){
> +			NL_SET_ERR_MSG(extack, "DST, VNI, ifindex and port are mutually exclusive with NH_ID");
> +			return -EINVAL;
> +		}
>
>  	if (tb[NDA_DST]) {
>  		err = vxlan_nla_get_addr(ip, tb[NDA_DST]);
> -		if (err)
> +		if (err){
> +			NL_SET_ERR_MSG(extack, "Unsupported address family");
>  			return err;
> +		}
>  	} else {
>  		union vxlan_addr *remote = &vxlan->default_dst.remote_ip;
>
> @@ -1157,24 +1161,30 @@ static int vxlan_fdb_parse(struct nlattr *tb[], struct vxlan_dev *vxlan,
>  	}
>
>  	if (tb[NDA_PORT]) {
> -		if (nla_len(tb[NDA_PORT]) != sizeof(__be16))
> +		if (nla_len(tb[NDA_PORT]) != sizeof(__be16)){
> +			NL_SET_ERR_MSG(extack, "Invalid vxlan port");
>  			return -EINVAL;
> +		}
>  		*port = nla_get_be16(tb[NDA_PORT]);
>  	} else {
>  		*port = vxlan->cfg.dst_port;
>  	}
>
>  	if (tb[NDA_VNI]) {
> -		if (nla_len(tb[NDA_VNI]) != sizeof(u32))
> +		if (nla_len(tb[NDA_VNI]) != sizeof(u32)){
> +			NL_SET_ERR_MSG(extack, "Invalid vni");
>  			return -EINVAL;
> +		}
>  		*vni = cpu_to_be32(nla_get_u32(tb[NDA_VNI]));
>  	} else {
>  		*vni = vxlan->default_dst.remote_vni;
>  	}
>
>  	if (tb[NDA_SRC_VNI]) {
> -		if (nla_len(tb[NDA_SRC_VNI]) != sizeof(u32))
> +		if (nla_len(tb[NDA_SRC_VNI]) != sizeof(u32)){
> +			NL_SET_ERR_MSG(extack, "Invalid src vni");
>  			return -EINVAL;
> +		}
>  		*src_vni = cpu_to_be32(nla_get_u32(tb[NDA_SRC_VNI]));
>  	} else {
>  		*src_vni = vxlan->default_dst.remote_vni;
> @@ -1183,12 +1193,16 @@ static int vxlan_fdb_parse(struct nlattr *tb[], struct vxlan_dev *vxlan,
>  	if (tb[NDA_IFINDEX]) {
>  		struct net_device *tdev;
>
> -		if (nla_len(tb[NDA_IFINDEX]) != sizeof(u32))
> +		if (nla_len(tb[NDA_IFINDEX]) != sizeof(u32)){
> +			NL_SET_ERR_MSG(extack, "Invalid ifindex");
>  			return -EINVAL;
> +		}
>  		*ifindex = nla_get_u32(tb[NDA_IFINDEX]);
>  		tdev = __dev_get_by_index(net, *ifindex);
> -		if (!tdev)
> +		if (!tdev){
> +			NL_SET_ERR_MSG(extack,"Device not found");
>  			return -EADDRNOTAVAIL;
> +		}
>  	} else {
>  		*ifindex = 0;
>  	}
> @@ -1226,7 +1240,7 @@ static int vxlan_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
>  		return -EINVAL;
>
>  	err = vxlan_fdb_parse(tb, vxlan, &ip, &port, &src_vni, &vni, &ifindex,
> -			      &nhid);
> +			      &nhid, extack);
>  	if (err)
>  		return err;
>
> @@ -1291,7 +1305,7 @@ static int vxlan_fdb_delete(struct ndmsg *ndm, struct nlattr *tb[],
>  	int err;
>
>  	err = vxlan_fdb_parse(tb, vxlan, &ip, &port, &src_vni, &vni, &ifindex,
> -			      &nhid);
> +			      &nhid, extack);
>  	if (err)
>  		return err;
>
> --
> 2.36.0
>
>
>
Alaa Mohamed April 24, 2022, 6:52 p.m. UTC | #2 
On ٢٤‏/٤‏/٢٠٢٢ ١٨:١٥, Julia Lawall wrote:
>
> On Sun, 24 Apr 2022, Alaa Mohamed wrote:
>
>> Add extack to vxlan_fdb_delete and vxlan_fdb_parse
> It could be helpful to say more about what adding extack support involves.
>
>> Signed-off-by: Alaa Mohamed <eng.alaamohamedsoliman.am@gmail.com>
>> ---
>> changes in V2:
>> 	- fix spelling vxlan_fdb_delete
>> 	- add missing braces
>> 	- edit error message
>> ---
>> changes in V3:
>> 	fix errors reported by checkpatch.pl
> But your changes would seem to also be introducing more checkpatch errors,
> because the Linux kernel coding style puts a space before an { at the
> start of an if branch.
I ran checkpatch script before submitting this patch and got no error
>
> julia
>
>> ---
>>   drivers/net/vxlan/vxlan_core.c | 36 +++++++++++++++++++++++-----------
>>   1 file changed, 25 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
>> index cf2f60037340..4e1886655101 100644
>> --- a/drivers/net/vxlan/vxlan_core.c
>> +++ b/drivers/net/vxlan/vxlan_core.c
>> @@ -1129,19 +1129,23 @@ static void vxlan_fdb_dst_destroy(struct vxlan_dev *vxlan, struct vxlan_fdb *f,
>>
>>   static int vxlan_fdb_parse(struct nlattr *tb[], struct vxlan_dev *vxlan,
>>   			   union vxlan_addr *ip, __be16 *port, __be32 *src_vni,
>> -			   __be32 *vni, u32 *ifindex, u32 *nhid)
>> +			   __be32 *vni, u32 *ifindex, u32 *nhid, struct netlink_ext_ack *extack)
>>   {
>>   	struct net *net = dev_net(vxlan->dev);
>>   	int err;
>>
>>   	if (tb[NDA_NH_ID] && (tb[NDA_DST] || tb[NDA_VNI] || tb[NDA_IFINDEX] ||
>> -	    tb[NDA_PORT]))
>> -		return -EINVAL;
>> +	    tb[NDA_PORT])){
>> +			NL_SET_ERR_MSG(extack, "DST, VNI, ifindex and port are mutually exclusive with NH_ID");
>> +			return -EINVAL;
>> +		}
>>
>>   	if (tb[NDA_DST]) {
>>   		err = vxlan_nla_get_addr(ip, tb[NDA_DST]);
>> -		if (err)
>> +		if (err){
>> +			NL_SET_ERR_MSG(extack, "Unsupported address family");
>>   			return err;
>> +		}
>>   	} else {
>>   		union vxlan_addr *remote = &vxlan->default_dst.remote_ip;
>>
>> @@ -1157,24 +1161,30 @@ static int vxlan_fdb_parse(struct nlattr *tb[], struct vxlan_dev *vxlan,
>>   	}
>>
>>   	if (tb[NDA_PORT]) {
>> -		if (nla_len(tb[NDA_PORT]) != sizeof(__be16))
>> +		if (nla_len(tb[NDA_PORT]) != sizeof(__be16)){
>> +			NL_SET_ERR_MSG(extack, "Invalid vxlan port");
>>   			return -EINVAL;
>> +		}
>>   		*port = nla_get_be16(tb[NDA_PORT]);
>>   	} else {
>>   		*port = vxlan->cfg.dst_port;
>>   	}
>>
>>   	if (tb[NDA_VNI]) {
>> -		if (nla_len(tb[NDA_VNI]) != sizeof(u32))
>> +		if (nla_len(tb[NDA_VNI]) != sizeof(u32)){
>> +			NL_SET_ERR_MSG(extack, "Invalid vni");
>>   			return -EINVAL;
>> +		}
>>   		*vni = cpu_to_be32(nla_get_u32(tb[NDA_VNI]));
>>   	} else {
>>   		*vni = vxlan->default_dst.remote_vni;
>>   	}
>>
>>   	if (tb[NDA_SRC_VNI]) {
>> -		if (nla_len(tb[NDA_SRC_VNI]) != sizeof(u32))
>> +		if (nla_len(tb[NDA_SRC_VNI]) != sizeof(u32)){
>> +			NL_SET_ERR_MSG(extack, "Invalid src vni");
>>   			return -EINVAL;
>> +		}
>>   		*src_vni = cpu_to_be32(nla_get_u32(tb[NDA_SRC_VNI]));
>>   	} else {
>>   		*src_vni = vxlan->default_dst.remote_vni;
>> @@ -1183,12 +1193,16 @@ static int vxlan_fdb_parse(struct nlattr *tb[], struct vxlan_dev *vxlan,
>>   	if (tb[NDA_IFINDEX]) {
>>   		struct net_device *tdev;
>>
>> -		if (nla_len(tb[NDA_IFINDEX]) != sizeof(u32))
>> +		if (nla_len(tb[NDA_IFINDEX]) != sizeof(u32)){
>> +			NL_SET_ERR_MSG(extack, "Invalid ifindex");
>>   			return -EINVAL;
>> +		}
>>   		*ifindex = nla_get_u32(tb[NDA_IFINDEX]);
>>   		tdev = __dev_get_by_index(net, *ifindex);
>> -		if (!tdev)
>> +		if (!tdev){
>> +			NL_SET_ERR_MSG(extack,"Device not found");
>>   			return -EADDRNOTAVAIL;
>> +		}
>>   	} else {
>>   		*ifindex = 0;
>>   	}
>> @@ -1226,7 +1240,7 @@ static int vxlan_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
>>   		return -EINVAL;
>>
>>   	err = vxlan_fdb_parse(tb, vxlan, &ip, &port, &src_vni, &vni, &ifindex,
>> -			      &nhid);
>> +			      &nhid, extack);
>>   	if (err)
>>   		return err;
>>
>> @@ -1291,7 +1305,7 @@ static int vxlan_fdb_delete(struct ndmsg *ndm, struct nlattr *tb[],
>>   	int err;
>>
>>   	err = vxlan_fdb_parse(tb, vxlan, &ip, &port, &src_vni, &vni, &ifindex,
>> -			      &nhid);
>> +			      &nhid, extack);
>>   	if (err)
>>   		return err;
>>
>> --
>> 2.36.0
>>
>>
>>
Julia Lawall April 24, 2022, 6:56 p.m. UTC | #3 
On Sun, 24 Apr 2022, Alaa Mohamed wrote:

>
> On ٢٤/٤/٢٠٢٢ ١٨:١٥, Julia Lawall wrote:
> >
> > On Sun, 24 Apr 2022, Alaa Mohamed wrote:
> >
> > > Add extack to vxlan_fdb_delete and vxlan_fdb_parse
> > It could be helpful to say more about what adding extack support involves.
> >
> > > Signed-off-by: Alaa Mohamed <eng.alaamohamedsoliman.am@gmail.com>
> > > ---
> > > changes in V2:
> > > 	- fix spelling vxlan_fdb_delete
> > > 	- add missing braces
> > > 	- edit error message
> > > ---
> > > changes in V3:
> > > 	fix errors reported by checkpatch.pl
> > But your changes would seem to also be introducing more checkpatch errors,
> > because the Linux kernel coding style puts a space before an { at the
> > start of an if branch.
> I ran checkpatch script before submitting this patch and got no error

OK, whether checkpatch complains or not doesn't really matter.  The point
is that in the Linux kernel, one writes:

if (...) {

and not

if (...){

You can see other examples of ifs in the Linux kernel.

julia

> >
> > julia
> >
> > > ---
> > >   drivers/net/vxlan/vxlan_core.c | 36 +++++++++++++++++++++++-----------
> > >   1 file changed, 25 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/net/vxlan/vxlan_core.c
> > > b/drivers/net/vxlan/vxlan_core.c
> > > index cf2f60037340..4e1886655101 100644
> > > --- a/drivers/net/vxlan/vxlan_core.c
> > > +++ b/drivers/net/vxlan/vxlan_core.c
> > > @@ -1129,19 +1129,23 @@ static void vxlan_fdb_dst_destroy(struct vxlan_dev
> > > *vxlan, struct vxlan_fdb *f,
> > >
> > >   static int vxlan_fdb_parse(struct nlattr *tb[], struct vxlan_dev *vxlan,
> > >   			   union vxlan_addr *ip, __be16 *port, __be32
> > > *src_vni,
> > > -			   __be32 *vni, u32 *ifindex, u32 *nhid)
> > > +			   __be32 *vni, u32 *ifindex, u32 *nhid, struct
> > > netlink_ext_ack *extack)
> > >   {
> > >   	struct net *net = dev_net(vxlan->dev);
> > >   	int err;
> > >
> > >   	if (tb[NDA_NH_ID] && (tb[NDA_DST] || tb[NDA_VNI] || tb[NDA_IFINDEX] ||
> > > -	    tb[NDA_PORT]))
> > > -		return -EINVAL;
> > > +	    tb[NDA_PORT])){
> > > +			NL_SET_ERR_MSG(extack, "DST, VNI, ifindex and port are
> > > mutually exclusive with NH_ID");
> > > +			return -EINVAL;
> > > +		}
> > >
> > >   	if (tb[NDA_DST]) {
> > >   		err = vxlan_nla_get_addr(ip, tb[NDA_DST]);
> > > -		if (err)
> > > +		if (err){
> > > +			NL_SET_ERR_MSG(extack, "Unsupported address family");
> > >   			return err;
> > > +		}
> > >   	} else {
> > >   		union vxlan_addr *remote = &vxlan->default_dst.remote_ip;
> > >
> > > @@ -1157,24 +1161,30 @@ static int vxlan_fdb_parse(struct nlattr *tb[],
> > > struct vxlan_dev *vxlan,
> > >   	}
> > >
> > >   	if (tb[NDA_PORT]) {
> > > -		if (nla_len(tb[NDA_PORT]) != sizeof(__be16))
> > > +		if (nla_len(tb[NDA_PORT]) != sizeof(__be16)){
> > > +			NL_SET_ERR_MSG(extack, "Invalid vxlan port");
> > >   			return -EINVAL;
> > > +		}
> > >   		*port = nla_get_be16(tb[NDA_PORT]);
> > >   	} else {
> > >   		*port = vxlan->cfg.dst_port;
> > >   	}
> > >
> > >   	if (tb[NDA_VNI]) {
> > > -		if (nla_len(tb[NDA_VNI]) != sizeof(u32))
> > > +		if (nla_len(tb[NDA_VNI]) != sizeof(u32)){
> > > +			NL_SET_ERR_MSG(extack, "Invalid vni");
> > >   			return -EINVAL;
> > > +		}
> > >   		*vni = cpu_to_be32(nla_get_u32(tb[NDA_VNI]));
> > >   	} else {
> > >   		*vni = vxlan->default_dst.remote_vni;
> > >   	}
> > >
> > >   	if (tb[NDA_SRC_VNI]) {
> > > -		if (nla_len(tb[NDA_SRC_VNI]) != sizeof(u32))
> > > +		if (nla_len(tb[NDA_SRC_VNI]) != sizeof(u32)){
> > > +			NL_SET_ERR_MSG(extack, "Invalid src vni");
> > >   			return -EINVAL;
> > > +		}
> > >   		*src_vni = cpu_to_be32(nla_get_u32(tb[NDA_SRC_VNI]));
> > >   	} else {
> > >   		*src_vni = vxlan->default_dst.remote_vni;
> > > @@ -1183,12 +1193,16 @@ static int vxlan_fdb_parse(struct nlattr *tb[],
> > > struct vxlan_dev *vxlan,
> > >   	if (tb[NDA_IFINDEX]) {
> > >   		struct net_device *tdev;
> > >
> > > -		if (nla_len(tb[NDA_IFINDEX]) != sizeof(u32))
> > > +		if (nla_len(tb[NDA_IFINDEX]) != sizeof(u32)){
> > > +			NL_SET_ERR_MSG(extack, "Invalid ifindex");
> > >   			return -EINVAL;
> > > +		}
> > >   		*ifindex = nla_get_u32(tb[NDA_IFINDEX]);
> > >   		tdev = __dev_get_by_index(net, *ifindex);
> > > -		if (!tdev)
> > > +		if (!tdev){
> > > +			NL_SET_ERR_MSG(extack,"Device not found");
> > >   			return -EADDRNOTAVAIL;
> > > +		}
> > >   	} else {
> > >   		*ifindex = 0;
> > >   	}
> > > @@ -1226,7 +1240,7 @@ static int vxlan_fdb_add(struct ndmsg *ndm, struct
> > > nlattr *tb[],
> > >   		return -EINVAL;
> > >
> > >   	err = vxlan_fdb_parse(tb, vxlan, &ip, &port, &src_vni, &vni, &ifindex,
> > > -			      &nhid);
> > > +			      &nhid, extack);
> > >   	if (err)
> > >   		return err;
> > >
> > > @@ -1291,7 +1305,7 @@ static int vxlan_fdb_delete(struct ndmsg *ndm,
> > > struct nlattr *tb[],
> > >   	int err;
> > >
> > >   	err = vxlan_fdb_parse(tb, vxlan, &ip, &port, &src_vni, &vni, &ifindex,
> > > -			      &nhid);
> > > +			      &nhid, extack);
> > >   	if (err)
> > >   		return err;
> > >
> > > --
> > > 2.36.0
> > >
> > >
> > >
>
Alaa Mohamed April 24, 2022, 6:59 p.m. UTC | #4 
On ٢٤‏/٤‏/٢٠٢٢ ٢٠:٥٦, Julia Lawall wrote:
>
> On Sun, 24 Apr 2022, Alaa Mohamed wrote:
>
>> On ٢٤/٤/٢٠٢٢ ١٨:١٥, Julia Lawall wrote:
>>> On Sun, 24 Apr 2022, Alaa Mohamed wrote:
>>>
>>>> Add extack to vxlan_fdb_delete and vxlan_fdb_parse
>>> It could be helpful to say more about what adding extack support involves.
>>>
>>>> Signed-off-by: Alaa Mohamed <eng.alaamohamedsoliman.am@gmail.com>
>>>> ---
>>>> changes in V2:
>>>> 	- fix spelling vxlan_fdb_delete
>>>> 	- add missing braces
>>>> 	- edit error message
>>>> ---
>>>> changes in V3:
>>>> 	fix errors reported by checkpatch.pl
>>> But your changes would seem to also be introducing more checkpatch errors,
>>> because the Linux kernel coding style puts a space before an { at the
>>> start of an if branch.
>> I ran checkpatch script before submitting this patch and got no error
> OK, whether checkpatch complains or not doesn't really matter.  The point
> is that in the Linux kernel, one writes:
>
> if (...) {
>
> and not
>
> if (...){
>
> You can see other examples of ifs in the Linux kernel.


Yes, got it. I will fix it.


Thanks,

Alaa

>
> julia
>
>>> julia
>>>
>>>> ---
>>>>    drivers/net/vxlan/vxlan_core.c | 36 +++++++++++++++++++++++-----------
>>>>    1 file changed, 25 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/net/vxlan/vxlan_core.c
>>>> b/drivers/net/vxlan/vxlan_core.c
>>>> index cf2f60037340..4e1886655101 100644
>>>> --- a/drivers/net/vxlan/vxlan_core.c
>>>> +++ b/drivers/net/vxlan/vxlan_core.c
>>>> @@ -1129,19 +1129,23 @@ static void vxlan_fdb_dst_destroy(struct vxlan_dev
>>>> *vxlan, struct vxlan_fdb *f,
>>>>
>>>>    static int vxlan_fdb_parse(struct nlattr *tb[], struct vxlan_dev *vxlan,
>>>>    			   union vxlan_addr *ip, __be16 *port, __be32
>>>> *src_vni,
>>>> -			   __be32 *vni, u32 *ifindex, u32 *nhid)
>>>> +			   __be32 *vni, u32 *ifindex, u32 *nhid, struct
>>>> netlink_ext_ack *extack)
>>>>    {
>>>>    	struct net *net = dev_net(vxlan->dev);
>>>>    	int err;
>>>>
>>>>    	if (tb[NDA_NH_ID] && (tb[NDA_DST] || tb[NDA_VNI] || tb[NDA_IFINDEX] ||
>>>> -	    tb[NDA_PORT]))
>>>> -		return -EINVAL;
>>>> +	    tb[NDA_PORT])){
>>>> +			NL_SET_ERR_MSG(extack, "DST, VNI, ifindex and port are
>>>> mutually exclusive with NH_ID");
>>>> +			return -EINVAL;
>>>> +		}
>>>>
>>>>    	if (tb[NDA_DST]) {
>>>>    		err = vxlan_nla_get_addr(ip, tb[NDA_DST]);
>>>> -		if (err)
>>>> +		if (err){
>>>> +			NL_SET_ERR_MSG(extack, "Unsupported address family");
>>>>    			return err;
>>>> +		}
>>>>    	} else {
>>>>    		union vxlan_addr *remote = &vxlan->default_dst.remote_ip;
>>>>
>>>> @@ -1157,24 +1161,30 @@ static int vxlan_fdb_parse(struct nlattr *tb[],
>>>> struct vxlan_dev *vxlan,
>>>>    	}
>>>>
>>>>    	if (tb[NDA_PORT]) {
>>>> -		if (nla_len(tb[NDA_PORT]) != sizeof(__be16))
>>>> +		if (nla_len(tb[NDA_PORT]) != sizeof(__be16)){
>>>> +			NL_SET_ERR_MSG(extack, "Invalid vxlan port");
>>>>    			return -EINVAL;
>>>> +		}
>>>>    		*port = nla_get_be16(tb[NDA_PORT]);
>>>>    	} else {
>>>>    		*port = vxlan->cfg.dst_port;
>>>>    	}
>>>>
>>>>    	if (tb[NDA_VNI]) {
>>>> -		if (nla_len(tb[NDA_VNI]) != sizeof(u32))
>>>> +		if (nla_len(tb[NDA_VNI]) != sizeof(u32)){
>>>> +			NL_SET_ERR_MSG(extack, "Invalid vni");
>>>>    			return -EINVAL;
>>>> +		}
>>>>    		*vni = cpu_to_be32(nla_get_u32(tb[NDA_VNI]));
>>>>    	} else {
>>>>    		*vni = vxlan->default_dst.remote_vni;
>>>>    	}
>>>>
>>>>    	if (tb[NDA_SRC_VNI]) {
>>>> -		if (nla_len(tb[NDA_SRC_VNI]) != sizeof(u32))
>>>> +		if (nla_len(tb[NDA_SRC_VNI]) != sizeof(u32)){
>>>> +			NL_SET_ERR_MSG(extack, "Invalid src vni");
>>>>    			return -EINVAL;
>>>> +		}
>>>>    		*src_vni = cpu_to_be32(nla_get_u32(tb[NDA_SRC_VNI]));
>>>>    	} else {
>>>>    		*src_vni = vxlan->default_dst.remote_vni;
>>>> @@ -1183,12 +1193,16 @@ static int vxlan_fdb_parse(struct nlattr *tb[],
>>>> struct vxlan_dev *vxlan,
>>>>    	if (tb[NDA_IFINDEX]) {
>>>>    		struct net_device *tdev;
>>>>
>>>> -		if (nla_len(tb[NDA_IFINDEX]) != sizeof(u32))
>>>> +		if (nla_len(tb[NDA_IFINDEX]) != sizeof(u32)){
>>>> +			NL_SET_ERR_MSG(extack, "Invalid ifindex");
>>>>    			return -EINVAL;
>>>> +		}
>>>>    		*ifindex = nla_get_u32(tb[NDA_IFINDEX]);
>>>>    		tdev = __dev_get_by_index(net, *ifindex);
>>>> -		if (!tdev)
>>>> +		if (!tdev){
>>>> +			NL_SET_ERR_MSG(extack,"Device not found");
>>>>    			return -EADDRNOTAVAIL;
>>>> +		}
>>>>    	} else {
>>>>    		*ifindex = 0;
>>>>    	}
>>>> @@ -1226,7 +1240,7 @@ static int vxlan_fdb_add(struct ndmsg *ndm, struct
>>>> nlattr *tb[],
>>>>    		return -EINVAL;
>>>>
>>>>    	err = vxlan_fdb_parse(tb, vxlan, &ip, &port, &src_vni, &vni, &ifindex,
>>>> -			      &nhid);
>>>> +			      &nhid, extack);
>>>>    	if (err)
>>>>    		return err;
>>>>
>>>> @@ -1291,7 +1305,7 @@ static int vxlan_fdb_delete(struct ndmsg *ndm,
>>>> struct nlattr *tb[],
>>>>    	int err;
>>>>
>>>>    	err = vxlan_fdb_parse(tb, vxlan, &ip, &port, &src_vni, &vni, &ifindex,
>>>> -			      &nhid);
>>>> +			      &nhid, extack);
>>>>    	if (err)
>>>>    		return err;
>>>>
>>>> --
>>>> 2.36.0
>>>>
>>>>
>>>>
> >
Nikolay Aleksandrov April 24, 2022, 7:03 p.m. UTC | #5 
On 24/04/2022 21:52, Alaa Mohamed wrote:
> 
> On ٢٤‏/٤‏/٢٠٢٢ ١٨:١٥, Julia Lawall wrote:
>>
>> On Sun, 24 Apr 2022, Alaa Mohamed wrote:
>>
>>> Add extack to vxlan_fdb_delete and vxlan_fdb_parse
>> It could be helpful to say more about what adding extack support involves.
>>
>>> Signed-off-by: Alaa Mohamed <eng.alaamohamedsoliman.am@gmail.com>
>>> ---
>>> changes in V2:
>>>     - fix spelling vxlan_fdb_delete
>>>     - add missing braces
>>>     - edit error message
>>> ---
>>> changes in V3:
>>>     fix errors reported by checkpatch.pl
>> But your changes would seem to also be introducing more checkpatch errors,
>> because the Linux kernel coding style puts a space before an { at the
>> start of an if branch.
> I ran checkpatch script before submitting this patch and got no error

This is what I got:
WARNING: suspect code indent for conditional statements (8, 24)
#366: FILE: drivers/net/vxlan/vxlan_core.c:1137:
 	if (tb[NDA_NH_ID] && (tb[NDA_DST] || tb[NDA_VNI] || tb[NDA_IFINDEX] ||
[...]
+			NL_SET_ERR_MSG(extack, "DST, VNI, ifindex and port are mutually exclusive with NH_ID");

WARNING: line length of 111 exceeds 100 columns
#370: FILE: drivers/net/vxlan/vxlan_core.c:1139:
+			NL_SET_ERR_MSG(extack, "DST, VNI, ifindex and port are mutually exclusive with NH_ID");

ERROR: space required before the open brace '{'
#377: FILE: drivers/net/vxlan/vxlan_core.c:1145:
+		if (err){

ERROR: space required before the open brace '{'
#389: FILE: drivers/net/vxlan/vxlan_core.c:1164:
+		if (nla_len(tb[NDA_PORT]) != sizeof(__be16)){

ERROR: space required before the open brace '{'
#400: FILE: drivers/net/vxlan/vxlan_core.c:1174:
+		if (nla_len(tb[NDA_VNI]) != sizeof(u32)){

ERROR: space required before the open brace '{'
#411: FILE: drivers/net/vxlan/vxlan_core.c:1184:
+		if (nla_len(tb[NDA_SRC_VNI]) != sizeof(u32)){

ERROR: space required before the open brace '{'
#423: FILE: drivers/net/vxlan/vxlan_core.c:1196:
+		if (nla_len(tb[NDA_IFINDEX]) != sizeof(u32)){

ERROR: space required before the open brace '{'
#430: FILE: drivers/net/vxlan/vxlan_core.c:1202:
+		if (!tdev){

ERROR: space required after that ',' (ctx:VxV)
#431: FILE: drivers/net/vxlan/vxlan_core.c:1203:
+			NL_SET_ERR_MSG(extack,"Device not found");
Alaa Mohamed April 24, 2022, 7:20 p.m. UTC | #6 
On ٢٤‏/٤‏/٢٠٢٢ ٢١:٠٣, Nikolay Aleksandrov wrote:
> On 24/04/2022 21:52, Alaa Mohamed wrote:
>> On ٢٤‏/٤‏/٢٠٢٢ ١٨:١٥, Julia Lawall wrote:
>>> On Sun, 24 Apr 2022, Alaa Mohamed wrote:
>>>
>>>> Add extack to vxlan_fdb_delete and vxlan_fdb_parse
>>> It could be helpful to say more about what adding extack support involves.
>>>
>>>> Signed-off-by: Alaa Mohamed <eng.alaamohamedsoliman.am@gmail.com>
>>>> ---
>>>> changes in V2:
>>>>      - fix spelling vxlan_fdb_delete
>>>>      - add missing braces
>>>>      - edit error message
>>>> ---
>>>> changes in V3:
>>>>      fix errors reported by checkpatch.pl
>>> But your changes would seem to also be introducing more checkpatch errors,
>>> because the Linux kernel coding style puts a space before an { at the
>>> start of an if branch.
>> I ran checkpatch script before submitting this patch and got no error
> This is what I got:
> WARNING: suspect code indent for conditional statements (8, 24)
> #366: FILE: drivers/net/vxlan/vxlan_core.c:1137:
>   	if (tb[NDA_NH_ID] && (tb[NDA_DST] || tb[NDA_VNI] || tb[NDA_IFINDEX] ||
> [...]
> +			NL_SET_ERR_MSG(extack, "DST, VNI, ifindex and port are mutually exclusive with NH_ID");
>
> WARNING: line length of 111 exceeds 100 columns
> #370: FILE: drivers/net/vxlan/vxlan_core.c:1139:
> +			NL_SET_ERR_MSG(extack, "DST, VNI, ifindex and port are mutually exclusive with NH_ID");
>
> ERROR: space required before the open brace '{'
> #377: FILE: drivers/net/vxlan/vxlan_core.c:1145:
> +		if (err){
>
> ERROR: space required before the open brace '{'
> #389: FILE: drivers/net/vxlan/vxlan_core.c:1164:
> +		if (nla_len(tb[NDA_PORT]) != sizeof(__be16)){
>
> ERROR: space required before the open brace '{'
> #400: FILE: drivers/net/vxlan/vxlan_core.c:1174:
> +		if (nla_len(tb[NDA_VNI]) != sizeof(u32)){
>
> ERROR: space required before the open brace '{'
> #411: FILE: drivers/net/vxlan/vxlan_core.c:1184:
> +		if (nla_len(tb[NDA_SRC_VNI]) != sizeof(u32)){
>
> ERROR: space required before the open brace '{'
> #423: FILE: drivers/net/vxlan/vxlan_core.c:1196:
> +		if (nla_len(tb[NDA_IFINDEX]) != sizeof(u32)){
>
> ERROR: space required before the open brace '{'
> #430: FILE: drivers/net/vxlan/vxlan_core.c:1202:
> +		if (!tdev){
>
> ERROR: space required after that ',' (ctx:VxV)
> #431: FILE: drivers/net/vxlan/vxlan_core.c:1203:
> +			NL_SET_ERR_MSG(extack,"Device not found");
>
>
Thank you for sending that , but I don't know why I missed that when I 
ran the script. Anyway, I fixed all these errors as Julia said.

Thanks again,

Alaa
diff mbox series

Patch

diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
index cf2f60037340..4e1886655101 100644
--- a/drivers/net/vxlan/vxlan_core.c
+++ b/drivers/net/vxlan/vxlan_core.c
@@ -1129,19 +1129,23 @@  static void vxlan_fdb_dst_destroy(struct vxlan_dev *vxlan, struct vxlan_fdb *f,

 static int vxlan_fdb_parse(struct nlattr *tb[], struct vxlan_dev *vxlan,
 			   union vxlan_addr *ip, __be16 *port, __be32 *src_vni,
-			   __be32 *vni, u32 *ifindex, u32 *nhid)
+			   __be32 *vni, u32 *ifindex, u32 *nhid, struct netlink_ext_ack *extack)
 {
 	struct net *net = dev_net(vxlan->dev);
 	int err;

 	if (tb[NDA_NH_ID] && (tb[NDA_DST] || tb[NDA_VNI] || tb[NDA_IFINDEX] ||
-	    tb[NDA_PORT]))
-		return -EINVAL;
+	    tb[NDA_PORT])){
+			NL_SET_ERR_MSG(extack, "DST, VNI, ifindex and port are mutually exclusive with NH_ID");
+			return -EINVAL;
+		}

 	if (tb[NDA_DST]) {
 		err = vxlan_nla_get_addr(ip, tb[NDA_DST]);
-		if (err)
+		if (err){
+			NL_SET_ERR_MSG(extack, "Unsupported address family");
 			return err;
+		}
 	} else {
 		union vxlan_addr *remote = &vxlan->default_dst.remote_ip;

@@ -1157,24 +1161,30 @@  static int vxlan_fdb_parse(struct nlattr *tb[], struct vxlan_dev *vxlan,
 	}

 	if (tb[NDA_PORT]) {
-		if (nla_len(tb[NDA_PORT]) != sizeof(__be16))
+		if (nla_len(tb[NDA_PORT]) != sizeof(__be16)){
+			NL_SET_ERR_MSG(extack, "Invalid vxlan port");
 			return -EINVAL;
+		}
 		*port = nla_get_be16(tb[NDA_PORT]);
 	} else {
 		*port = vxlan->cfg.dst_port;
 	}

 	if (tb[NDA_VNI]) {
-		if (nla_len(tb[NDA_VNI]) != sizeof(u32))
+		if (nla_len(tb[NDA_VNI]) != sizeof(u32)){
+			NL_SET_ERR_MSG(extack, "Invalid vni");
 			return -EINVAL;
+		}
 		*vni = cpu_to_be32(nla_get_u32(tb[NDA_VNI]));
 	} else {
 		*vni = vxlan->default_dst.remote_vni;
 	}

 	if (tb[NDA_SRC_VNI]) {
-		if (nla_len(tb[NDA_SRC_VNI]) != sizeof(u32))
+		if (nla_len(tb[NDA_SRC_VNI]) != sizeof(u32)){
+			NL_SET_ERR_MSG(extack, "Invalid src vni");
 			return -EINVAL;
+		}
 		*src_vni = cpu_to_be32(nla_get_u32(tb[NDA_SRC_VNI]));
 	} else {
 		*src_vni = vxlan->default_dst.remote_vni;
@@ -1183,12 +1193,16 @@  static int vxlan_fdb_parse(struct nlattr *tb[], struct vxlan_dev *vxlan,
 	if (tb[NDA_IFINDEX]) {
 		struct net_device *tdev;

-		if (nla_len(tb[NDA_IFINDEX]) != sizeof(u32))
+		if (nla_len(tb[NDA_IFINDEX]) != sizeof(u32)){
+			NL_SET_ERR_MSG(extack, "Invalid ifindex");
 			return -EINVAL;
+		}
 		*ifindex = nla_get_u32(tb[NDA_IFINDEX]);
 		tdev = __dev_get_by_index(net, *ifindex);
-		if (!tdev)
+		if (!tdev){
+			NL_SET_ERR_MSG(extack,"Device not found");
 			return -EADDRNOTAVAIL;
+		}
 	} else {
 		*ifindex = 0;
 	}
@@ -1226,7 +1240,7 @@  static int vxlan_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
 		return -EINVAL;

 	err = vxlan_fdb_parse(tb, vxlan, &ip, &port, &src_vni, &vni, &ifindex,
-			      &nhid);
+			      &nhid, extack);
 	if (err)
 		return err;

@@ -1291,7 +1305,7 @@  static int vxlan_fdb_delete(struct ndmsg *ndm, struct nlattr *tb[],
 	int err;

 	err = vxlan_fdb_parse(tb, vxlan, &ip, &port, &src_vni, &vni, &ifindex,
-			      &nhid);
+			      &nhid, extack);
 	if (err)
 		return err;