Re: [PATCH iproute2 net-next] nstat: add sctp snmp support

2016-09-04 Thread Hangbin Liu 
2016-09-02 18:09 GMT+08:00 Phil Sutter :
> Did you forget to add the load call to update_db(), or am I missing
> something?

Opps, forgot to add it there. I will send patchv2 for this issue.

Thanks
Hangbin

> Apart from that, looks nice and clean.
>
> Cheers, Phil

Re: [PATCH iproute2 net-next] nstat: add sctp snmp support

2016-09-02 Thread Stephen Hemminger 
On Fri, 2 Sep 2016 12:46:49 -0700
Stephen Hemminger  wrote:

> On Fri,  2 Sep 2016 15:12:38 +0800
> Hangbin Liu  wrote:
> 
> > SCTP module was not load by default. But this should be OK since we will not
> > load table if fdopen() failed.
> > 
> > Signed-off-by: Hangbin Liu   
> 
> This seems like a bad idea for the normal distro user.
> If they run nstat command the SCTP kernel module would get loaded even
> if never used.
> 
> Makes more sense not to display SCTP statistics if it isn't loaded.
> 

Never mind, opening the proc file won't load SCTP kernel module.

Re: [PATCH iproute2 net-next] nstat: add sctp snmp support

2016-09-02 Thread Stephen Hemminger 
On Fri,  2 Sep 2016 15:12:38 +0800
Hangbin Liu  wrote:

> SCTP module was not load by default. But this should be OK since we will not
> load table if fdopen() failed.
> 
> Signed-off-by: Hangbin Liu 

This seems like a bad idea for the normal distro user.
If they run nstat command the SCTP kernel module would get loaded even
if never used.

Makes more sense not to display SCTP statistics if it isn't loaded.

Re: [PATCH iproute2 net-next] nstat: add sctp snmp support

2016-09-02 Thread Phil Sutter 
On Fri, Sep 02, 2016 at 03:12:38PM +0800, Hangbin Liu wrote:
> SCTP module was not load by default. But this should be OK since we will not
> load table if fdopen() failed.
> 
> Signed-off-by: Hangbin Liu 
> ---
>  misc/nstat.c | 17 +
>  1 file changed, 17 insertions(+)
> 
> diff --git a/misc/nstat.c b/misc/nstat.c
> index 6143719..8035be4 100644
> --- a/misc/nstat.c
> +++ b/misc/nstat.c
> @@ -76,6 +76,11 @@ static int net_snmp6_open(void)
>   return generic_proc_open("PROC_NET_SNMP6", "net/snmp6");
>  }
>  
> +static int net_sctp_snmp_open(void)
> +{
> + return generic_proc_open("PROC_NET_SCTP_SNMP", "net/sctp/snmp");
> +}
> +
>  struct nstat_ent {
>   struct nstat_ent *next;
>   char *id;
> @@ -247,6 +252,16 @@ static void load_ugly_table(FILE *fp)
>   }
>  }
>  
> +static void load_sctp_snmp(void)
> +{
> + FILE *fp = fdopen(net_sctp_snmp_open(), "r");
> +
> + if (fp) {
> + load_good_table(fp);
> + fclose(fp);
> + }
> +}
> +
>  static void load_snmp(void)
>  {
>   FILE *fp = fdopen(net_snmp_open(), "r");
> @@ -450,6 +465,7 @@ static void server_loop(int fd)
>   load_netstat();
>   load_snmp6();
>   load_snmp();
> + load_sctp_snmp();
>  
>   for (;;) {
>   int status;
> @@ -706,6 +722,7 @@ int main(int argc, char *argv[])
>   load_netstat();
>   load_snmp6();
>   load_snmp();
> + load_sctp_snmp();
>   if (info_source[0] == 0)
>   strcpy(info_source, "kernel");
>   }

Did you forget to add the load call to update_db(), or am I missing
something?

Apart from that, looks nice and clean.

Cheers, Phil