diff mbox

[-next] bonding: bond_get_size() returns wrong size

Message ID 20131101101844.GA29795@longonot.mountain
State Accepted, archived
Delegated to: David Miller
Headers show

Commit Message

Dan Carpenter Nov. 1, 2013, 10:18 a.m. UTC 
There is an extra semi-colon so bond_get_size() doesn't return the
correct value.

Fixes: ec76aa49855f ('bonding: add Netlink support active_slave option')
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Veaceslav Falico Nov. 1, 2013, 10:41 a.m. UTC | #1 
On Fri, Nov 01, 2013 at 01:18:44PM +0300, Dan Carpenter wrote:
>There is an extra semi-colon so bond_get_size() doesn't return the
>correct value.
>
>Fixes: ec76aa49855f ('bonding: add Netlink support active_slave option')
>Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Hrm, how does it even build. Good catch, thanks.

Acked-by: Veaceslav Falico <vfalico@redhat.com>

>
>diff --git a/drivers/net/bonding/bond_netlink.c b/drivers/net/bonding/bond_netlink.c
>index 7661261..40e7b1c 100644
>--- a/drivers/net/bonding/bond_netlink.c
>+++ b/drivers/net/bonding/bond_netlink.c
>@@ -82,8 +82,8 @@ static int bond_newlink(struct net *src_net, struct net_device *bond_dev,
>
> static size_t bond_get_size(const struct net_device *bond_dev)
> {
>-	return nla_total_size(sizeof(u8));	/* IFLA_BOND_MODE */
>-		+ nla_total_size(sizeof(u32));	/* IFLA_BOND_ACTIVE_SLAVE */
>+	return nla_total_size(sizeof(u8)) +	/* IFLA_BOND_MODE */
>+		nla_total_size(sizeof(u32));	/* IFLA_BOND_ACTIVE_SLAVE */
> }
>
> static int bond_fill_info(struct sk_buff *skb,
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet Nov. 1, 2013, 10:52 a.m. UTC | #2 
On Fri, 2013-11-01 at 11:41 +0100, Veaceslav Falico wrote:
> On Fri, Nov 01, 2013 at 01:18:44PM +0300, Dan Carpenter wrote:
> >There is an extra semi-colon so bond_get_size() doesn't return the
> >correct value.
> >
> >Fixes: ec76aa49855f ('bonding: add Netlink support active_slave option')
> >Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> Hrm, how does it even build. Good catch, thanks.
> 
> Acked-by: Veaceslav Falico <vfalico@redhat.com>

Yep, an alternative is to use following format :

static inline size_t br_port_info_size(void)
{
        return nla_total_size(1)        /* IFLA_BRPORT_STATE  */
                + nla_total_size(2)     /* IFLA_BRPORT_PRIORITY */
                + nla_total_size(4)     /* IFLA_BRPORT_COST */
                + nla_total_size(1)     /* IFLA_BRPORT_MODE */
                + nla_total_size(1)     /* IFLA_BRPORT_GUARD */
                + nla_total_size(1)     /* IFLA_BRPORT_PROTECT */
                + nla_total_size(1)     /* IFLA_BRPORT_FAST_LEAVE */
                + nla_total_size(1)     /* IFLA_BRPORT_LEARNING */
                + nla_total_size(1)     /* IFLA_BRPORT_UNICAST_FLOOD */
                + 0;
}

So that a patch adding a new attribute doesn't have to change the 'last
line'

 

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jiri Pirko Nov. 1, 2013, 11:35 a.m. UTC | #3 
Fri, Nov 01, 2013 at 11:41:00AM CET, vfalico@redhat.com wrote:
>On Fri, Nov 01, 2013 at 01:18:44PM +0300, Dan Carpenter wrote:
>>There is an extra semi-colon so bond_get_size() doesn't return the
>>correct value.
>>
>>Fixes: ec76aa49855f ('bonding: add Netlink support active_slave option')
>>Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
>Hrm, how does it even build. Good catch, thanks.

I wonder about the same thing.


Reviewed-by: Jiri Pirko <jiri@resnulli.us>


>
>Acked-by: Veaceslav Falico <vfalico@redhat.com>
>
>>
>>diff --git a/drivers/net/bonding/bond_netlink.c b/drivers/net/bonding/bond_netlink.c
>>index 7661261..40e7b1c 100644
>>--- a/drivers/net/bonding/bond_netlink.c
>>+++ b/drivers/net/bonding/bond_netlink.c
>>@@ -82,8 +82,8 @@ static int bond_newlink(struct net *src_net, struct net_device *bond_dev,
>>
>>static size_t bond_get_size(const struct net_device *bond_dev)
>>{
>>-	return nla_total_size(sizeof(u8));	/* IFLA_BOND_MODE */
>>-		+ nla_total_size(sizeof(u32));	/* IFLA_BOND_ACTIVE_SLAVE */
>>+	return nla_total_size(sizeof(u8)) +	/* IFLA_BOND_MODE */
>>+		nla_total_size(sizeof(u32));	/* IFLA_BOND_ACTIVE_SLAVE */
>>}
>>
>>static int bond_fill_info(struct sk_buff *skb,
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nikolay Aleksandrov Nov. 1, 2013, 12:25 p.m. UTC | #4 
On 11/01/2013 12:35 PM, Jiri Pirko wrote:
> Fri, Nov 01, 2013 at 11:41:00AM CET, vfalico@redhat.com wrote:
>> On Fri, Nov 01, 2013 at 01:18:44PM +0300, Dan Carpenter wrote:
>>> There is an extra semi-colon so bond_get_size() doesn't return the
>>> correct value.
>>>
>>> Fixes: ec76aa49855f ('bonding: add Netlink support active_slave option')
>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>
>> Hrm, how does it even build. Good catch, thanks.
> 
> I wonder about the same thing.
> 
+ and - can be unary operators so it's a valid expression :-)
Nice catch

Nik

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Walter Harms Nov. 1, 2013, 12:30 p.m. UTC | #5 
Am 01.11.2013 13:25, schrieb Nikolay Aleksandrov:
> On 11/01/2013 12:35 PM, Jiri Pirko wrote:
>> Fri, Nov 01, 2013 at 11:41:00AM CET, vfalico@redhat.com wrote:
>>> On Fri, Nov 01, 2013 at 01:18:44PM +0300, Dan Carpenter wrote:
>>>> There is an extra semi-colon so bond_get_size() doesn't return the
>>>> correct value.
>>>>
>>>> Fixes: ec76aa49855f ('bonding: add Netlink support active_slave option')
>>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>>
>>> Hrm, how does it even build. Good catch, thanks.
>>
>> I wonder about the same thing.
>>
> + and - can be unary operators so it's a valid expression :-)
> Nice catch
> 

NTL my gcc warns (-Wall)
  warning: statement with no effect

this should come up.

re,
 wh

> Nik
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller Nov. 2, 2013, 6:09 a.m. UTC | #6 
From: Dan Carpenter <dan.carpenter@oracle.com>
Date: Fri, 1 Nov 2013 13:18:44 +0300

> There is an extra semi-colon so bond_get_size() doesn't return the
> correct value.
> 
> Fixes: ec76aa49855f ('bonding: add Netlink support active_slave option')
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Applied, thanks a lot Dan.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/net/bonding/bond_netlink.c b/drivers/net/bonding/bond_netlink.c
index 7661261..40e7b1c 100644
--- a/drivers/net/bonding/bond_netlink.c
+++ b/drivers/net/bonding/bond_netlink.c
@@ -82,8 +82,8 @@  static int bond_newlink(struct net *src_net, struct net_device *bond_dev,
 
 static size_t bond_get_size(const struct net_device *bond_dev)
 {
-	return nla_total_size(sizeof(u8));	/* IFLA_BOND_MODE */
-		+ nla_total_size(sizeof(u32));	/* IFLA_BOND_ACTIVE_SLAVE */
+	return nla_total_size(sizeof(u8)) +	/* IFLA_BOND_MODE */
+		nla_total_size(sizeof(u32));	/* IFLA_BOND_ACTIVE_SLAVE */
 }
 
 static int bond_fill_info(struct sk_buff *skb,