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 <[email protected]>
> ---
> 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
>
>
>
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 <[email protected]>
>> ---
>> 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
>>
>>
>>
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 <[email protected]>
>>> ---
>>> 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");