Re: [iproute PATCH v2 2/7] Use C99 style initializers everywhere

[Jakub Sitnicki]

On Wed, Jun 22, 2016 at 11:29 AM CEST, Phil Sutter  wrote:
> On Wed, Jun 22, 2016 at 11:12:14AM +0200, Jakub Sitnicki wrote:
>> On Tue, Jun 21, 2016 at 06:18 PM CEST, Phil Sutter  wrote:
>> > This big patch was compiled by vimgrepping for memset calls and changing
>> > to C99 initializer if applicable. One notable exception is the
>> > initialization of union bpf_attr in tc/tc_bpf.c: changing it would break
>> > for older gcc versions (at least <=3.4.6).
>> >
>> > Calls to memset for struct rtattr pointer fields for parse_rtattr*()
>> > were just dropped since they are not needed.
>> >
>> > The changes here allowed the compiler to discover some unused variables,
>> > so get rid of them, too.
>> >
>> > Signed-off-by: Phil Sutter 
>> > ---
>> 
>> [...]
>> 
>> > diff --git a/bridge/fdb.c b/bridge/fdb.c
>> > index be849f980a802..a59d6a9c13018 100644
>> > --- a/bridge/fdb.c
>> > +++ b/bridge/fdb.c
>> > @@ -177,16 +177,15 @@ static int fdb_show(int argc, char **argv)
>> >struct nlmsghdr n;
>> >struct ifinfomsgifm;
>> >charbuf[256];
>> > -  } req;
>> > +  } req = {
>> > +  .n.nlmsg_len = NLMSG_LENGTH(sizeof(struct ifinfomsg)),
>> > +  .ifm.ifi_family = PF_BRIDGE
>> > +  };
>> >  
>> 
>> A comma is allowed after a list of initializers (IOW, after the last
>> initializer). Having it would make for smaller/cleaner diffs in the
>> future if new structure members get added and need to be initialized.
>
> Good point! I knew about that already, but that's good reasoning why one
> would actually want to do that intentionally.

It also helps us all with the compulsive need for closure ;-)

Thanks,
Jakub

Re: [iproute PATCH v2 2/7] Use C99 style initializers everywhere

[Phil Sutter]

Hi Jakub,

On Wed, Jun 22, 2016 at 11:12:14AM +0200, Jakub Sitnicki wrote:
> On Tue, Jun 21, 2016 at 06:18 PM CEST, Phil Sutter  wrote:
> > This big patch was compiled by vimgrepping for memset calls and changing
> > to C99 initializer if applicable. One notable exception is the
> > initialization of union bpf_attr in tc/tc_bpf.c: changing it would break
> > for older gcc versions (at least <=3.4.6).
> >
> > Calls to memset for struct rtattr pointer fields for parse_rtattr*()
> > were just dropped since they are not needed.
> >
> > The changes here allowed the compiler to discover some unused variables,
> > so get rid of them, too.
> >
> > Signed-off-by: Phil Sutter 
> > ---
> 
> [...]
> 
> > diff --git a/bridge/fdb.c b/bridge/fdb.c
> > index be849f980a802..a59d6a9c13018 100644
> > --- a/bridge/fdb.c
> > +++ b/bridge/fdb.c
> > @@ -177,16 +177,15 @@ static int fdb_show(int argc, char **argv)
> > struct nlmsghdr n;
> > struct ifinfomsgifm;
> > charbuf[256];
> > -   } req;
> > +   } req = {
> > +   .n.nlmsg_len = NLMSG_LENGTH(sizeof(struct ifinfomsg)),
> > +   .ifm.ifi_family = PF_BRIDGE
> > +   };
> >  
> 
> A comma is allowed after a list of initializers (IOW, after the last
> initializer). Having it would make for smaller/cleaner diffs in the
> future if new structure members get added and need to be initialized.

Good point! I knew about that already, but that's good reasoning why one
would actually want to do that intentionally.

Thanks, Phil

Re: [iproute PATCH v2 2/7] Use C99 style initializers everywhere

[Jakub Sitnicki]

Hi Phil,

On Tue, Jun 21, 2016 at 06:18 PM CEST, Phil Sutter  wrote:
> This big patch was compiled by vimgrepping for memset calls and changing
> to C99 initializer if applicable. One notable exception is the
> initialization of union bpf_attr in tc/tc_bpf.c: changing it would break
> for older gcc versions (at least <=3.4.6).
>
> Calls to memset for struct rtattr pointer fields for parse_rtattr*()
> were just dropped since they are not needed.
>
> The changes here allowed the compiler to discover some unused variables,
> so get rid of them, too.
>
> Signed-off-by: Phil Sutter 
> ---

[...]

> diff --git a/bridge/fdb.c b/bridge/fdb.c
> index be849f980a802..a59d6a9c13018 100644
> --- a/bridge/fdb.c
> +++ b/bridge/fdb.c
> @@ -177,16 +177,15 @@ static int fdb_show(int argc, char **argv)
>   struct nlmsghdr n;
>   struct ifinfomsgifm;
>   charbuf[256];
> - } req;
> + } req = {
> + .n.nlmsg_len = NLMSG_LENGTH(sizeof(struct ifinfomsg)),
> + .ifm.ifi_family = PF_BRIDGE
> + };
>  

A comma is allowed after a list of initializers (IOW, after the last
initializer). Having it would make for smaller/cleaner diffs in the
future if new structure members get added and need to be initialized.

Thanks,
Jakub

Re: [iproute PATCH v2 2/7] Use C99 style initializers everywhere

[David Ahern]

On 6/21/16 11:03 AM, Phil Sutter wrote:

I downloaded CentOS 5 and 6. iproute2 fails to compile on CentOS 5.11;
ip command builds on 6.8 but with a flurry of redefinition errors
(BUILD_BUG_ON), but fails at tc.


What's the exact error message please? Maybe some incompatibility in
kernel headers? Although that shouldn't be ...


lib
CC   libgenl.o
CC   ll_map.o
CC   libnetlink.o
AR   libnetlink.a
CC   utils.o
In file included from utils.c:35:
../include/utils.h:212:1: warning: "BUILD_BUG_ON" redefined
In file included from ../include/linux/netlink.h:4,
 from ../include/linux/if_link.h:5,
 from ../include/linux/netdevice.h:31,
 from ../include/linux/if_arp.h:26,
 from utils.c:28:
/usr/include/linux/kernel.h:29:1: warning: this is the location of the 
previous definition

...


CC   tc_bpf.o
tc_bpf.c:41:26: error: linux/if_alg.h: No such file or directory
In file included from tc_bpf.c:45:
../include/utils.h:212:1: warning: "BUILD_BUG_ON" redefined
In file included from ../include/linux/netlink.h:4,
 from ../include/libnetlink.h:7,
 from ../include/utils.h:10,
 from tc_bpf.c:45:
/usr/include/linux/kernel.h:29:1: warning: this is the location of the 
previous definition

make[1]: *** [tc_bpf.o] Error 1
make: *** [all] Error 2

Re: [iproute PATCH v2 2/7] Use C99 style initializers everywhere

[Stephen Hemminger]

On Tue, 21 Jun 2016 19:17:31 +0200
Phil Sutter  wrote:

> On Tue, Jun 21, 2016 at 11:13:11AM -0600, David Ahern wrote:
> > On 6/21/16 11:03 AM, Phil Sutter wrote:
> > >> I downloaded CentOS 5 and 6. iproute2 fails to compile on CentOS 5.11;
> > >> ip command builds on 6.8 but with a flurry of redefinition errors
> > >> (BUILD_BUG_ON), but fails at tc.
> > >
> > > What's the exact error message please? Maybe some incompatibility in
> > > kernel headers? Although that shouldn't be ...
> [...]
> >  CC   tc_bpf.o
> > tc_bpf.c:41:26: error: linux/if_alg.h: No such file or directory
> 
> Ah! Looks like this header is missing in iproute2's copy of kernel
> headers.
> 
> Stephen, would you import the missing one?

I ran a script and imported all the headers that were linux/if_*.h and used
by current source.

Re: [iproute PATCH v2 2/7] Use C99 style initializers everywhere

[Phil Sutter]

On Tue, Jun 21, 2016 at 11:13:11AM -0600, David Ahern wrote:
> On 6/21/16 11:03 AM, Phil Sutter wrote:
> >> I downloaded CentOS 5 and 6. iproute2 fails to compile on CentOS 5.11;
> >> ip command builds on 6.8 but with a flurry of redefinition errors
> >> (BUILD_BUG_ON), but fails at tc.
> >
> > What's the exact error message please? Maybe some incompatibility in
> > kernel headers? Although that shouldn't be ...
[...]
>  CC   tc_bpf.o
> tc_bpf.c:41:26: error: linux/if_alg.h: No such file or directory

Ah! Looks like this header is missing in iproute2's copy of kernel
headers.

Stephen, would you import the missing one?

Thanks, Phil

Re: [iproute PATCH v2 2/7] Use C99 style initializers everywhere

[Phil Sutter]

On Tue, Jun 21, 2016 at 10:24:37AM -0600, David Ahern wrote:
> On 6/21/16 10:18 AM, Phil Sutter wrote:
> > This big patch was compiled by vimgrepping for memset calls and changing
> > to C99 initializer if applicable. One notable exception is the
> > initialization of union bpf_attr in tc/tc_bpf.c: changing it would break
> > for older gcc versions (at least <=3.4.6).
> >
> > Calls to memset for struct rtattr pointer fields for parse_rtattr*()
> > were just dropped since they are not needed.
> >
> > The changes here allowed the compiler to discover some unused variables,
> > so get rid of them, too.
> >
> > Signed-off-by: Phil Sutter 
> > ---
> > Changes since v1:
> > - Dropped former changes to tc/tc_bpf.c as they are incompatible to older
> >   gcc versions (at least <=3.4.6).
> 
> 
> What OS versions have you compiled iproute2 against?

Tested on Gentoo with old compiler installed, so apart from gcc the
system is up to date.

> I downloaded CentOS 5 and 6. iproute2 fails to compile on CentOS 5.11; 
> ip command builds on 6.8 but with a flurry of redefinition errors 
> (BUILD_BUG_ON), but fails at tc.

What's the exact error message please? Maybe some incompatibility in
kernel headers? Although that shouldn't be ...

Cheers, Phil

[iproute PATCH v2 2/7] Use C99 style initializers everywhere

[Phil Sutter]

This big patch was compiled by vimgrepping for memset calls and changing
to C99 initializer if applicable. One notable exception is the
initialization of union bpf_attr in tc/tc_bpf.c: changing it would break
for older gcc versions (at least <=3.4.6).

Calls to memset for struct rtattr pointer fields for parse_rtattr*()
were just dropped since they are not needed.

The changes here allowed the compiler to discover some unused variables,
so get rid of them, too.

Signed-off-by: Phil Sutter 
---
Changes since v1:
- Dropped former changes to tc/tc_bpf.c as they are incompatible to older
  gcc versions (at least <=3.4.6).
---
 bridge/fdb.c |  29 +++--
 bridge/link.c|  16 +++
 bridge/mdb.c |  19 -
 bridge/vlan.c|  19 -
 genl/ctrl.c  |  48 +
 ip/ip6tunnel.c   |  10 ++---
 ip/ipaddress.c   |  31 ++
 ip/ipaddrlabel.c |  23 +-
 ip/iplink.c  |  67 ++---
 ip/iplink_can.c  |   4 +-
 ip/ipmaddr.c |  27 +---
 ip/ipmroute.c|   8 +---
 ip/ipneigh.c |  36 
 ip/ipnetconf.c   |  12 +++---
 ip/ipnetns.c |  45 ++-
 ip/ipntable.c|  27 +---
 ip/iproute.c |  85 
 ip/iprule.c  |  26 +--
 ip/iptoken.c |  21 -
 ip/iptunnel.c|  31 --
 ip/ipxfrm.c  |  26 +++
 ip/link_gre.c|  22 +-
 ip/link_gre6.c   |  22 +-
 ip/link_ip6tnl.c |  29 ++---
 ip/link_iptnl.c  |  26 +--
 ip/link_vti.c|  22 +-
 ip/link_vti6.c   |  22 +-
 ip/xfrm_policy.c | 110 ++-
 ip/xfrm_state.c  | 128 +++
 lib/libnetlink.c |  74 ++--
 lib/ll_map.c |   1 -
 misc/arpd.c  |  68 ++---
 misc/ss.c|  41 --
 tc/e_bpf.c   |   7 +--
 tc/em_cmp.c  |   4 +-
 tc/em_ipset.c|   4 +-
 tc/em_meta.c |   4 +-
 tc/em_nbyte.c|   4 +-
 tc/em_u32.c  |   4 +-
 tc/f_flow.c  |   3 --
 tc/f_flower.c|   3 +-
 tc/f_fw.c|   6 +--
 tc/f_route.c |   3 --
 tc/f_rsvp.c  |   6 +--
 tc/f_u32.c   |  12 ++
 tc/m_bpf.c   |   5 +--
 tc/m_csum.c  |   4 +-
 tc/m_ematch.c|   4 +-
 tc/m_gact.c  |   5 +--
 tc/m_ife.c   |   5 +--
 tc/m_mirred.c|   7 +--
 tc/m_nat.c   |   4 +-
 tc/m_pedit.c |   8 +---
 tc/m_police.c|   5 +--
 tc/q_atm.c   |   3 +-
 tc/q_cbq.c   |  22 +++---
 tc/q_choke.c |   4 +-
 tc/q_codel.c |   3 +-
 tc/q_dsmark.c|   1 -
 tc/q_fifo.c  |   4 +-
 tc/q_fq_codel.c  |   3 +-
 tc/q_hfsc.c  |  13 ++
 tc/q_htb.c   |  15 +++
 tc/q_netem.c |  16 +++
 tc/q_red.c   |   4 +-
 tc/q_sfb.c   |  17 
 tc/q_sfq.c   |   4 +-
 tc/q_tbf.c   |   4 +-
 tc/tc_bpf.c  |  47 +---
 tc/tc_class.c|  33 ++
 tc/tc_exec.c |   3 +-
 tc/tc_filter.c   |  35 ++-
 tc/tc_qdisc.c|  35 ++-
 tc/tc_stab.c |   4 +-
 tc/tc_util.c |   3 +-
 75 files changed, 657 insertions(+), 898 deletions(-)

diff --git a/bridge/fdb.c b/bridge/fdb.c
index be849f980a802..a59d6a9c13018 100644
--- a/bridge/fdb.c
+++ b/bridge/fdb.c
@@ -177,16 +177,15 @@ static int fdb_show(int argc, char **argv)
struct nlmsghdr n;
struct ifinfomsgifm;
charbuf[256];
-   } req;
+   } req = {
+   .n.nlmsg_len = NLMSG_LENGTH(sizeof(struct ifinfomsg)),
+   .ifm.ifi_family = PF_BRIDGE
+   };
 
char *filter_dev = NULL;
char *br = NULL;
int msg_size = sizeof(struct ifinfomsg);
 
-   memset(, 0, sizeof(req));
-   req.n.nlmsg_len = NLMSG_LENGTH(sizeof(struct ifinfomsg));
-   req.ifm.ifi_family = PF_BRIDGE;
-
while (argc > 0) {
if ((strcmp(*argv, "brport") == 0) || strcmp(*argv, "dev") == 
0) {
NEXT_ARG();
@@ -247,7 +246,17 @@ static int fdb_modify(int cmd, int flags, int argc, char 
**argv)
struct nlmsghdr n;
struct ndmsgndm;
charbuf[256];
-   } req;
+   } req = {
+   .n = {
+   .nlmsg_len = NLMSG_LENGTH(sizeof(struct ndmsg)),
+   .nlmsg_flags = NLM_F_REQUEST | flags,
+   .nlmsg_type = cmd
+   },
+   .ndm = {
+   .ndm_family = PF_BRIDGE,
+   .ndm_state = NUD_NOARP
+   }
+   };
char *addr = NULL;
char *d = NULL;
char abuf[ETH_ALEN];
@@ -259,14 +268,6 @@ static int fdb_modify(int cmd, int flags, int argc, char 
**argv)
char *endptr;
short vid = -1;
 
-   memset(, 0, 

Re: [iproute PATCH v2 2/7] Use C99 style initializers everywhere

[David Ahern]

On 6/21/16 10:18 AM, Phil Sutter wrote:

This big patch was compiled by vimgrepping for memset calls and changing
to C99 initializer if applicable. One notable exception is the
initialization of union bpf_attr in tc/tc_bpf.c: changing it would break
for older gcc versions (at least <=3.4.6).

Calls to memset for struct rtattr pointer fields for parse_rtattr*()
were just dropped since they are not needed.

The changes here allowed the compiler to discover some unused variables,
so get rid of them, too.

Signed-off-by: Phil Sutter 
---
Changes since v1:
- Dropped former changes to tc/tc_bpf.c as they are incompatible to older
  gcc versions (at least <=3.4.6).



What OS versions have you compiled iproute2 against?

I downloaded CentOS 5 and 6. iproute2 fails to compile on CentOS 5.11; 
ip command builds on 6.8 but with a flurry of redefinition errors 
(BUILD_BUG_ON), but fails at tc.