Re: [PATCH V6 net-next iproute] ip: Add support for netdev events to monitor

2017-05-31 Thread Vlad Yasevich 
On 05/30/2017 02:26 PM, Vlad Yasevich wrote:
> On 05/30/2017 01:12 PM, Stephen Hemminger wrote:
>> On Sat, 27 May 2017 10:14:36 -0400
>> Vladislav Yasevich  wrote:
>>
>>>  
>>> +static const char *netdev_events[] = {"NONE",
>>> + "REBOOT",
>>> + "FEATURE CHANGE",
>>> + "BONDING FAILOVER",
>>> + "NOTIFY PEERS",
>>> + "RESEND IGMP",
>>> + "BONDING OPTION"};
>>
>> Overall this looks fine, I will pickup the if_link.h from net-next.
>>
>> One stylistic change.
>>
>> Please add simple line break, and initialize by value:
>>
>> static const char *netdev_events[] = {
>>  [IFLA_EVENT_NONE]   = "NONE",
>> ...
>>
>> Do you want some prefix or bounding around the event output?
> 
> Don't really care about output from my side.  If you think some prefix
> would be good, I can surely add it.
> 
>> Also a little concerned that the output format change may break some program
>> could the new output be at the end of the line?
>>
> 
> I can try moving it to the end.
>

Hi Stephen

So, I looked again at this patch and the output change I am proposing would look
something like this 'ip monitor':

1: lo:  mtu 6500 qdisc noqueue state UNKNOWN group 
default event
FEATURE CHANGE
link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00


It's already at the end of the like ant has an 'event' prefix similar to all 
the other
entries on the top line.  Does that look OK?  I don't think that would break 
anything.

Thanks
-vlad

Re: [PATCH V6 net-next iproute] ip: Add support for netdev events to monitor

2017-05-30 Thread Vlad Yasevich 
On 05/30/2017 01:12 PM, Stephen Hemminger wrote:
> On Sat, 27 May 2017 10:14:36 -0400
> Vladislav Yasevich  wrote:
> 
>>  
>> +static const char *netdev_events[] = {"NONE",
>> +  "REBOOT",
>> +  "FEATURE CHANGE",
>> +  "BONDING FAILOVER",
>> +  "NOTIFY PEERS",
>> +  "RESEND IGMP",
>> +  "BONDING OPTION"};
> 
> Overall this looks fine, I will pickup the if_link.h from net-next.
> 
> One stylistic change.
> 
> Please add simple line break, and initialize by value:
> 
> static const char *netdev_events[] = {
>   [IFLA_EVENT_NONE]   = "NONE",
> ...
> 
> Do you want some prefix or bounding around the event output?

Don't really care about output from my side.  If you think some prefix
would be good, I can surely add it.

> Also a little concerned that the output format change may break some program
> could the new output be at the end of the line?
> 

I can try moving it to the end.

Thanks
-vlad

Re: [PATCH V6 net-next iproute] ip: Add support for netdev events to monitor

2017-05-30 Thread Stephen Hemminger 
On Sat, 27 May 2017 10:14:36 -0400
Vladislav Yasevich  wrote:

>  
> +static const char *netdev_events[] = {"NONE",
> +   "REBOOT",
> +   "FEATURE CHANGE",
> +   "BONDING FAILOVER",
> +   "NOTIFY PEERS",
> +   "RESEND IGMP",
> +   "BONDING OPTION"};

Overall this looks fine, I will pickup the if_link.h from net-next.

One stylistic change.

Please add simple line break, and initialize by value:

static const char *netdev_events[] = {
[IFLA_EVENT_NONE]   = "NONE",
...

Do you want some prefix or bounding around the event output?
Also a little concerned that the output format change may break some program
could the new output be at the end of the line?

Re: [PATCH V6 net-next iproute] ip: Add support for netdev events to monitor

2017-05-30 Thread David Ahern 
On 5/27/17 8:14 AM, Vladislav Yasevich wrote:
> diff --git a/ip/ipaddress.c b/ip/ipaddress.c
> index b8d9c7d..c6e7413 100644
> --- a/ip/ipaddress.c
> +++ b/ip/ipaddress.c
> @@ -753,6 +753,24 @@ int print_linkinfo_brief(const struct sockaddr_nl *who,
>   return 0;
>  }
>  
> +static const char *netdev_events[] = {"NONE",

I would prefer ifla_events rather than netdev just to reinforce the
separation that an IFLA_EVENT does not necessarily correspond to a
NETDEV event.

[PATCH V6 net-next iproute] ip: Add support for netdev events to monitor

2017-05-27 Thread Vladislav Yasevich 
Add IFLA_EVENT handling so that event types can be viewed with
'monitor' command.  This gives a little more information for why
a given message was receivied.

Signed-off-by: Vladislav Yasevich 
---
 include/linux/if_link.h | 11 +++
 ip/ipaddress.c  | 21 +
 2 files changed, 32 insertions(+)

diff --git a/include/linux/if_link.h b/include/linux/if_link.h
index 5a3a048..c0a6769 100644
--- a/include/linux/if_link.h
+++ b/include/linux/if_link.h
@@ -157,6 +157,7 @@ enum {
IFLA_GSO_MAX_SIZE,
IFLA_PAD,
IFLA_XDP,
+   IFLA_EVENT,
__IFLA_MAX
 };
 
@@ -909,4 +910,14 @@ enum {
 
 #define IFLA_XDP_MAX (__IFLA_XDP_MAX - 1)
 
+enum {
+   IFLA_EVENT_NONE,
+   IFLA_EVENT_REBOOT,  /* internal reset / reboot */
+   IFLA_EVENT_FEATURES,/* change in offload features */
+   IFLA_EVENT_BONDING_FAILOVER,/* hange in active slave */
+   IFLA_EVENT_NOTIFY_PEERS,/* re-sent grat. arp/ndisc */
+   IFLA_EVENT_IGMP_RESEND, /* re-sent IGMP JOIN */
+   IFLA_EVENT_BONDING_OPTIONS, /* change in bonding options */
+};
+
 #endif /* _LINUX_IF_LINK_H */
diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index b8d9c7d..c6e7413 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -753,6 +753,24 @@ int print_linkinfo_brief(const struct sockaddr_nl *who,
return 0;
 }
 
+static const char *netdev_events[] = {"NONE",
+ "REBOOT",
+ "FEATURE CHANGE",
+ "BONDING FAILOVER",
+ "NOTIFY PEERS",
+ "RESEND IGMP",
+ "BONDING OPTION"};
+
+static void print_dev_event(FILE *f, __u32 event)
+{
+   if (event >= ARRAY_SIZE(netdev_events))
+   fprintf(f, "event %d ", event);
+   else {
+   if (event)
+   fprintf(f, "event %s ", netdev_events[event]);
+   }
+}
+
 int print_linkinfo(const struct sockaddr_nl *who,
   struct nlmsghdr *n, void *arg)
 {
@@ -858,6 +876,9 @@ int print_linkinfo(const struct sockaddr_nl *who,
if (filter.showqueue)
print_queuelen(fp, tb);
 
+   if (tb[IFLA_EVENT])
+   print_dev_event(fp, rta_getattr_u32(tb[IFLA_EVENT]));
+
if (!filter.family || filter.family == AF_PACKET || show_details) {
SPRINT_BUF(b1);
fprintf(fp, "%s", _SL_);
-- 
2.7.4