Re: [PATCH iproute2 v2 1/3] ifstat: Add extended statistics to ifstat

2016-12-18 Thread Or Gerlitz
On Thu, Dec 15, 2016 at 3:00 PM, Nogah Frankel  wrote:

> +/* Note: if one xstat name in subset of another, it should be before it in 
> this
> + * list.
> + * Name length must be under 64 chars.
> + */

nit "in subset" --> "is subset" ?


RE: [PATCH iproute2 v2 1/3] ifstat: Add extended statistics to ifstat

2016-12-18 Thread Nogah Frankel
> -Original Message-
> From: Rami Rosen [mailto:roszenr...@gmail.com]
> Sent: Friday, December 16, 2016 5:21 PM
> To: Nogah Frankel 
> Cc: Stephen Hemminger ; netdev@vger.kernel.org;
> ro...@cumulusnetworks.com; Jiri Pirko ; Elad Raz
> ; Yotam Gigi ; Ido Schimmel
> ; Or Gerlitz 
> Subject: Re: [PATCH iproute2 v2 1/3] ifstat: Add extended statistics to ifstat
> 
> Hi,
> 
> >Thanks, I'll fix it.
> Another minor nit, on this occasion:
> 
> bool is_extanded should be: bool is_extended
> 
> Regards,
> Rami Rosen

Thank you, I will fix it.

Nogah


Re: [PATCH iproute2 v2 1/3] ifstat: Add extended statistics to ifstat

2016-12-16 Thread Rami Rosen
Hi,

>Thanks, I'll fix it.
Another minor nit, on this occasion:

bool is_extanded should be: bool is_extended

Regards,
Rami Rosen


RE: [PATCH iproute2 v2 1/3] ifstat: Add extended statistics to ifstat

2016-12-16 Thread Nogah Frankel

> -Original Message-
> From: Stephen Hemminger [mailto:step...@networkplumber.org]
> Sent: Thursday, December 15, 2016 7:33 PM
> To: Nogah Frankel 
> Cc: netdev@vger.kernel.org; ro...@cumulusnetworks.com; Jiri Pirko
> ; Elad Raz ; Yotam Gigi
> ; Ido Schimmel ; Or Gerlitz
> 
> Subject: Re: [PATCH iproute2 v2 1/3] ifstat: Add extended statistics to ifstat
> 
> On Thu, 15 Dec 2016 15:00:43 +0200
> Nogah Frankel  wrote:
> 
> > Extended stats are part of the RTM_GETSTATS method. This patch adds them
> > to ifstat.
> > While extended stats can come in many forms, we support only the
> > rtnl_link_stats64 struct for them (which is the 64 bits version of struct
> > rtnl_link_stats).
> > We support stats in the main nesting level, or one lower.
> > The extension can be called by its name or any shorten of it. If there is
> > more than one matched, the first one will be picked.
> >
> > To get the extended stats the flag -x  is used.
> >
> > Signed-off-by: Nogah Frankel 
> > Reviewed-by: Jiri Pirko 
> > ---
> >  misc/ifstat.c | 161
> --
> >  1 file changed, 146 insertions(+), 15 deletions(-)
> >
> > diff --git a/misc/ifstat.c b/misc/ifstat.c
> > index 92d67b0..d17ae21 100644
> > --- a/misc/ifstat.c
> > +++ b/misc/ifstat.c
> > @@ -35,6 +35,7 @@
> >
> >  #include 
> >
> > +#include "utils.h"
> >  int dump_zeros;
> >  int reset_history;
> >  int ignore_history;
> 
> Minor nit, please cleanup include order here (original code was wrong).
> 
> Standard practice is:
>  #include system headers (like stdio.h etc)
>  #include "xxx.h" local headers.
> 
> Should be:
> #include 
> 
> #include 
> #include 
> 
> #include "json_writer.h"
> #include "libnetlink.h"
> #include "utils.h"
> #include "SNAPSHOT.h"

Thanks, I'll fix it.


Re: [PATCH iproute2 v2 1/3] ifstat: Add extended statistics to ifstat

2016-12-15 Thread Stephen Hemminger
On Thu, 15 Dec 2016 15:00:43 +0200
Nogah Frankel  wrote:

> Extended stats are part of the RTM_GETSTATS method. This patch adds them
> to ifstat.
> While extended stats can come in many forms, we support only the
> rtnl_link_stats64 struct for them (which is the 64 bits version of struct
> rtnl_link_stats).
> We support stats in the main nesting level, or one lower.
> The extension can be called by its name or any shorten of it. If there is
> more than one matched, the first one will be picked.
> 
> To get the extended stats the flag -x  is used.
> 
> Signed-off-by: Nogah Frankel 
> Reviewed-by: Jiri Pirko 
> ---
>  misc/ifstat.c | 161 
> --
>  1 file changed, 146 insertions(+), 15 deletions(-)
> 
> diff --git a/misc/ifstat.c b/misc/ifstat.c
> index 92d67b0..d17ae21 100644
> --- a/misc/ifstat.c
> +++ b/misc/ifstat.c
> @@ -35,6 +35,7 @@
>  
>  #include 
>  
> +#include "utils.h"
>  int dump_zeros;
>  int reset_history;
>  int ignore_history;

Minor nit, please cleanup include order here (original code was wrong).

Standard practice is:
 #include system headers (like stdio.h etc)
 #include "xxx.h" local headers.

Should be:
#include 

#include 
#include 

#include "json_writer.h"
#include "libnetlink.h"
#include "utils.h"
#include "SNAPSHOT.h"



[PATCH iproute2 v2 1/3] ifstat: Add extended statistics to ifstat

2016-12-15 Thread Nogah Frankel
Extended stats are part of the RTM_GETSTATS method. This patch adds them
to ifstat.
While extended stats can come in many forms, we support only the
rtnl_link_stats64 struct for them (which is the 64 bits version of struct
rtnl_link_stats).
We support stats in the main nesting level, or one lower.
The extension can be called by its name or any shorten of it. If there is
more than one matched, the first one will be picked.

To get the extended stats the flag -x  is used.

Signed-off-by: Nogah Frankel 
Reviewed-by: Jiri Pirko 
---
 misc/ifstat.c | 161 --
 1 file changed, 146 insertions(+), 15 deletions(-)

diff --git a/misc/ifstat.c b/misc/ifstat.c
index 92d67b0..d17ae21 100644
--- a/misc/ifstat.c
+++ b/misc/ifstat.c
@@ -35,6 +35,7 @@
 
 #include 
 
+#include "utils.h"
 int dump_zeros;
 int reset_history;
 int ignore_history;
@@ -48,17 +49,21 @@ int pretty;
 double W;
 char **patterns;
 int npatterns;
+bool is_extanded;
+int filter_type;
+int sub_type;
 
 char info_source[128];
 int source_mismatch;
 
 #define MAXS (sizeof(struct rtnl_link_stats)/sizeof(__u32))
+#define NO_SUB_TYPE 0x
 
 struct ifstat_ent {
struct ifstat_ent   *next;
char*name;
int ifindex;
-   unsigned long long  val[MAXS];
+   __u64   val[MAXS];
double  rate[MAXS];
__u32   ival[MAXS];
 };
@@ -106,6 +111,48 @@ static int match(const char *id)
return 0;
 }
 
+static int get_nlmsg_extanded(const struct sockaddr_nl *who,
+ struct nlmsghdr *m, void *arg)
+{
+   struct if_stats_msg *ifsm = NLMSG_DATA(m);
+   struct rtattr *tb[IFLA_STATS_MAX+1];
+   int len = m->nlmsg_len;
+   struct ifstat_ent *n;
+
+   if (m->nlmsg_type != RTM_NEWSTATS)
+   return 0;
+
+   len -= NLMSG_LENGTH(sizeof(*ifsm));
+   if (len < 0)
+   return -1;
+
+   parse_rtattr(tb, IFLA_STATS_MAX, IFLA_STATS_RTA(ifsm), len);
+   if (tb[filter_type] == NULL)
+   return 0;
+
+   n = malloc(sizeof(*n));
+   if (!n)
+   abort();
+
+   n->ifindex = ifsm->ifindex;
+   n->name = strdup(ll_index_to_name(ifsm->ifindex));
+
+   if (sub_type == NO_SUB_TYPE) {
+   memcpy(&n->val, RTA_DATA(tb[filter_type]), sizeof(n->val));
+   } else {
+   struct rtattr *attr;
+
+   attr = parse_rtattr_one_nested(sub_type, tb[filter_type]);
+   if (attr == NULL)
+   return 0;
+   memcpy(&n->val, RTA_DATA(attr), sizeof(n->val));
+   }
+   memset(&n->rate, 0, sizeof(n->rate));
+   n->next = kern_db;
+   kern_db = n;
+   return 0;
+}
+
 static int get_nlmsg(const struct sockaddr_nl *who,
 struct nlmsghdr *m, void *arg)
 {
@@ -147,18 +194,34 @@ static void load_info(void)
 {
struct ifstat_ent *db, *n;
struct rtnl_handle rth;
+   __u32 filter_mask;
 
if (rtnl_open(&rth, 0) < 0)
exit(1);
 
-   if (rtnl_wilddump_request(&rth, AF_INET, RTM_GETLINK) < 0) {
-   perror("Cannot send dump request");
-   exit(1);
-   }
+   if (is_extanded) {
+   ll_init_map(&rth);
+   filter_mask = IFLA_STATS_FILTER_BIT(filter_type);
+   if (rtnl_wilddump_stats_req_filter(&rth, AF_UNSPEC, 
RTM_GETSTATS,
+  filter_mask) < 0) {
+   perror("Cannot send dump request");
+   exit(1);
+   }
 
-   if (rtnl_dump_filter(&rth, get_nlmsg, NULL) < 0) {
-   fprintf(stderr, "Dump terminated\n");
-   exit(1);
+   if (rtnl_dump_filter(&rth, get_nlmsg_extanded, NULL) < 0) {
+   fprintf(stderr, "Dump terminated\n");
+   exit(1);
+   }
+   } else {
+   if (rtnl_wilddump_request(&rth, AF_INET, RTM_GETLINK) < 0) {
+   perror("Cannot send dump request");
+   exit(1);
+   }
+
+   if (rtnl_dump_filter(&rth, get_nlmsg, NULL) < 0) {
+   fprintf(stderr, "Dump terminated\n");
+   exit(1);
+   }
}
 
rtnl_close(&rth);
@@ -553,10 +616,17 @@ static void update_db(int interval)
}
for (i = 0; i < MAXS; i++) {
double sample;
-   unsigned long incr = h1->ival[i] - 
n->ival[i];
+   __u64 incr;
+
+   if (is_extanded) {
+   incr = h1->val[i] - n->val[i];
+   n->val[i] = h1->val[