[iproute2,net-next] ip: add a new parameter -Numeric
diff mbox series

Message ID 20190612092115.30043-1-liuhangbin@gmail.com
State Accepted
Delegated to: David Ahern
Headers show
Series
  • [iproute2,net-next] ip: add a new parameter -Numeric
Related show

Commit Message

Hangbin Liu June 12, 2019, 9:21 a.m. UTC
Add a new parameter '-Numeric' to show the number of protocol, scope,
dsfield, etc directly instead of converting it to human readable name.
Do the same on tc and ss.

This patch is based on David Ahern's previous patch.

Suggested-by: Phil Sutter <phil@nwl.cc>
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 include/utils.h  |  1 +
 ip/ip.c          |  6 +++++-
 ip/rtm_map.c     |  6 ++++++
 lib/inet_proto.c |  2 +-
 lib/ll_proto.c   |  2 +-
 lib/ll_types.c   |  3 ++-
 lib/rt_names.c   | 18 ++++++++++--------
 man/man8/ip.8    |  6 ++++++
 man/man8/tc.8    |  6 ++++++
 misc/ss.c        | 15 ++++++---------
 tc/tc.c          |  5 ++++-
 11 files changed, 48 insertions(+), 22 deletions(-)

Comments

Hangbin Liu June 12, 2019, 9:25 a.m. UTC | #1
On Wed, 12 Jun 2019 at 17:21, Hangbin Liu <liuhangbin@gmail.com> wrote:
>
> Add a new parameter '-Numeric' to show the number of protocol, scope,
> dsfield, etc directly instead of converting it to human readable name.
> Do the same on tc and ss.
>
> This patch is based on David Ahern's previous patch.
>
> Suggested-by: Phil Sutter <phil@nwl.cc>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---

Oh, forgot to fix the subject line and reply the the previous patch, this is
PATCH v2 of https://patchwork.ozlabs.org/patch/1101870/

Thanks
Hangbin
Roman Mashak June 12, 2019, 4:01 p.m. UTC | #2
Hangbin Liu <liuhangbin@gmail.com> writes:

> Add a new parameter '-Numeric' to show the number of protocol, scope,
> dsfield, etc directly instead of converting it to human readable name.
> Do the same on tc and ss.
>
> This patch is based on David Ahern's previous patch.
>

[...]

It would be good idea to specify the numerical format, e.g. hex or dec,
very often hex is more conveninet representation (for example, when
skbmark is encoded of IP address or such).

Could you think of extending it '-Numeric' to have an optional argument
hex, and use decimal as default.
Hangbin Liu June 13, 2019, 3:07 a.m. UTC | #3
Hi Roman,
On Wed, Jun 12, 2019 at 12:01:20PM -0400, Roman Mashak wrote:
> Hangbin Liu <liuhangbin@gmail.com> writes:
> 
> > Add a new parameter '-Numeric' to show the number of protocol, scope,
> > dsfield, etc directly instead of converting it to human readable name.
> > Do the same on tc and ss.
> >
> > This patch is based on David Ahern's previous patch.
> >
> 
> [...]
> 
> It would be good idea to specify the numerical format, e.g. hex or dec,
> very often hex is more conveninet representation (for example, when
> skbmark is encoded of IP address or such).

Some functions use hex and some functions use integer. It's looks hard to
unit all functions.

I tried to make all the functions use itself printf to keep the compatibility.
Only changed function nl_proto_n2a as it was only called by ss, and we
removed function nl_proto_n2a.

Thanks
Hangbin
> 
> Could you think of extending it '-Numeric' to have an optional argument
> hex, and use decimal as default.
David Ahern June 14, 2019, 2:04 p.m. UTC | #4
On 6/12/19 10:01 AM, Roman Mashak wrote:
> Hangbin Liu <liuhangbin@gmail.com> writes:
> 
>> Add a new parameter '-Numeric' to show the number of protocol, scope,
>> dsfield, etc directly instead of converting it to human readable name.
>> Do the same on tc and ss.
>>
>> This patch is based on David Ahern's previous patch.
>>
> 
> [...]
> 
> It would be good idea to specify the numerical format, e.g. hex or dec,
> very often hex is more conveninet representation (for example, when
> skbmark is encoded of IP address or such).
> 
> Could you think of extending it '-Numeric' to have an optional argument
> hex, and use decimal as default.
> 

I do not see how such an option could work. It would be best for
iproute2 commands to output fields as hex or decimal based on what makes
sense for each. That said, I do not see how this patch affects fields
such as skbmark. Can you give an example? The patch looks fine to me.
Roman Mashak June 14, 2019, 7 p.m. UTC | #5
David Ahern <dsahern@gmail.com> writes:

> On 6/12/19 10:01 AM, Roman Mashak wrote:
>> Hangbin Liu <liuhangbin@gmail.com> writes:
>> 
>>> Add a new parameter '-Numeric' to show the number of protocol, scope,
>>> dsfield, etc directly instead of converting it to human readable name.
>>> Do the same on tc and ss.
>>>
>>> This patch is based on David Ahern's previous patch.
>>>
>> 
>> [...]
>> 
>> It would be good idea to specify the numerical format, e.g. hex or dec,
>> very often hex is more conveninet representation (for example, when
>> skbmark is encoded of IP address or such).
>> 
>> Could you think of extending it '-Numeric' to have an optional argument
>> hex, and use decimal as default.
>> 
>
> I do not see how such an option could work.

'numeric' is extern object, so can be accessible from other modules. But
yes, it would require to add some wrappers around print_*int() APIs to
take into account this new parameter.

Yes, it is all-or-nothing approach, one can't have filters dumping their
integers values in hex but actions attached to the filters in decimal
(or the other way around).

> It would be best for iproute2 commands to output fields as hex
> or decimal based on what makes sense for each.

For example, m_police.c historically dumps its index value in hex,
although it's not very sensible IMHO :) Now it's risky to change the code,
because many scripts will likely break.

> That said, I do not see how this patch affects fields
> such as skbmark. Can you give an example? The patch looks fine to me.

The patch only exposes 'numeric' to tc, so yes -- it does not affect
anything. I only suggested to think of more generic approach.

On the 2nd thought: there already exists argument "-raw" for tc which
currently instructs printing handles in hex representation. Why not to
adopt this for ip and ss as well rather then adding new key?
David Ahern June 15, 2019, 1:05 a.m. UTC | #6
On 6/14/19 1:00 PM, Roman Mashak wrote:
> On the 2nd thought: there already exists argument "-raw" for tc which
> currently instructs printing handles in hex representation. Why not to
> adopt this for ip and ss as well rather then adding new key?

show_raw seems to mean dump extra data as opposed to "don't convert
numbers to names" which the Numeric option does.
David Ahern June 18, 2019, 3:41 p.m. UTC | #7
On 6/12/19 3:21 AM, Hangbin Liu wrote:
> Add a new parameter '-Numeric' to show the number of protocol, scope,
> dsfield, etc directly instead of converting it to human readable name.
> Do the same on tc and ss.
> 
> This patch is based on David Ahern's previous patch.
> 
> Suggested-by: Phil Sutter <phil@nwl.cc>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
>  include/utils.h  |  1 +
>  ip/ip.c          |  6 +++++-
>  ip/rtm_map.c     |  6 ++++++
>  lib/inet_proto.c |  2 +-
>  lib/ll_proto.c   |  2 +-
>  lib/ll_types.c   |  3 ++-
>  lib/rt_names.c   | 18 ++++++++++--------
>  man/man8/ip.8    |  6 ++++++
>  man/man8/tc.8    |  6 ++++++
>  misc/ss.c        | 15 ++++++---------
>  tc/tc.c          |  5 ++++-
>  11 files changed, 48 insertions(+), 22 deletions(-)
> 

applied to iproute2-next.Thanks

Patch
diff mbox series

diff --git a/include/utils.h b/include/utils.h
index 8a9c3020..0f57ee97 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -34,6 +34,7 @@  extern int timestamp_short;
 extern const char * _SL_;
 extern int max_flush_loops;
 extern int batch_mode;
+extern int numeric;
 extern bool do_all;
 
 #ifndef CONFDIR
diff --git a/ip/ip.c b/ip/ip.c
index b46fd8dd..fed26f8d 100644
--- a/ip/ip.c
+++ b/ip/ip.c
@@ -36,6 +36,7 @@  int timestamp;
 int force;
 int max_flush_loops = 10;
 int batch_mode;
+int numeric;
 bool do_all;
 
 struct rtnl_handle rth = { .fd = -1 };
@@ -57,7 +58,8 @@  static void usage(void)
 		"                    -4 | -6 | -I | -D | -M | -B | -0 |\n"
 		"                    -l[oops] { maximum-addr-flush-attempts } | -br[ief] |\n"
 		"                    -o[neline] | -t[imestamp] | -ts[hort] | -b[atch] [filename] |\n"
-		"                    -rc[vbuf] [size] | -n[etns] name | -a[ll] | -c[olor]}\n");
+		"                    -rc[vbuf] [size] | -n[etns] name | -N[umeric] | -a[ll] |\n"
+		"                    -c[olor]}\n");
 	exit(-1);
 }
 
@@ -288,6 +290,8 @@  int main(int argc, char **argv)
 			NEXT_ARG();
 			if (netns_switch(argv[1]))
 				exit(-1);
+		} else if (matches(opt, "-Numeric") == 0) {
+			++numeric;
 		} else if (matches(opt, "-all") == 0) {
 			do_all = true;
 		} else {
diff --git a/ip/rtm_map.c b/ip/rtm_map.c
index 76f93780..8d8eafe0 100644
--- a/ip/rtm_map.c
+++ b/ip/rtm_map.c
@@ -23,6 +23,12 @@ 
 
 char *rtnl_rtntype_n2a(int id, char *buf, int len)
 {
+
+	if (numeric) {
+		snprintf(buf, len, "%d", id);
+		return buf;
+	}
+
 	switch (id) {
 	case RTN_UNSPEC:
 		return "none";
diff --git a/lib/inet_proto.c b/lib/inet_proto.c
index 0836a4c9..41e2e8b8 100644
--- a/lib/inet_proto.c
+++ b/lib/inet_proto.c
@@ -32,7 +32,7 @@  const char *inet_proto_n2a(int proto, char *buf, int len)
 		return ncache;
 
 	pe = getprotobynumber(proto);
-	if (pe) {
+	if (pe && !numeric) {
 		if (icache != -1)
 			free(ncache);
 		icache = proto;
diff --git a/lib/ll_proto.c b/lib/ll_proto.c
index 8316a755..78c39616 100644
--- a/lib/ll_proto.c
+++ b/lib/ll_proto.c
@@ -92,7 +92,7 @@  const char * ll_proto_n2a(unsigned short id, char *buf, int len)
 
 	id = ntohs(id);
 
-        for (i=0; i<sizeof(llproto_names)/sizeof(llproto_names[0]); i++) {
+        for (i=0; !numeric && i<sizeof(llproto_names)/sizeof(llproto_names[0]); i++) {
                  if (llproto_names[i].id == id)
 			return llproto_names[i].name;
 	}
diff --git a/lib/ll_types.c b/lib/ll_types.c
index 32d04b5a..49da15df 100644
--- a/lib/ll_types.c
+++ b/lib/ll_types.c
@@ -24,6 +24,7 @@ 
 #include <linux/sockios.h>
 
 #include "rt_names.h"
+#include "utils.h"
 
 const char * ll_type_n2a(int type, char *buf, int len)
 {
@@ -112,7 +113,7 @@  __PF(VOID,void)
 #undef __PF
 
         int i;
-        for (i=0; i<sizeof(arphrd_names)/sizeof(arphrd_names[0]); i++) {
+        for (i=0; !numeric && i<sizeof(arphrd_names)/sizeof(arphrd_names[0]); i++) {
                  if (arphrd_names[i].type == type)
 			return arphrd_names[i].name;
 	}
diff --git a/lib/rt_names.c b/lib/rt_names.c
index 66d5f2f0..41cccfb8 100644
--- a/lib/rt_names.c
+++ b/lib/rt_names.c
@@ -27,6 +27,8 @@ 
 
 #define NAME_MAX_LEN 512
 
+int numeric;
+
 struct rtnl_hash_entry {
 	struct rtnl_hash_entry	*next;
 	const char		*name;
@@ -180,7 +182,7 @@  static void rtnl_rtprot_initialize(void)
 
 const char *rtnl_rtprot_n2a(int id, char *buf, int len)
 {
-	if (id < 0 || id >= 256) {
+	if (id < 0 || id >= 256 || numeric) {
 		snprintf(buf, len, "%u", id);
 		return buf;
 	}
@@ -246,7 +248,7 @@  static void rtnl_rtscope_initialize(void)
 
 const char *rtnl_rtscope_n2a(int id, char *buf, int len)
 {
-	if (id < 0 || id >= 256) {
+	if (id < 0 || id >= 256 || numeric) {
 		snprintf(buf, len, "%d", id);
 		return buf;
 	}
@@ -311,7 +313,7 @@  static void rtnl_rtrealm_initialize(void)
 
 const char *rtnl_rtrealm_n2a(int id, char *buf, int len)
 {
-	if (id < 0 || id >= 256) {
+	if (id < 0 || id >= 256 || numeric) {
 		snprintf(buf, len, "%d", id);
 		return buf;
 	}
@@ -419,7 +421,7 @@  const char *rtnl_rttable_n2a(__u32 id, char *buf, int len)
 	entry = rtnl_rttable_hash[id & 255];
 	while (entry && entry->id != id)
 		entry = entry->next;
-	if (entry)
+	if (!numeric && entry)
 		return entry->name;
 	snprintf(buf, len, "%u", id);
 	return buf;
@@ -484,7 +486,7 @@  const char *rtnl_dsfield_n2a(int id, char *buf, int len)
 		if (!rtnl_rtdsfield_init)
 			rtnl_rtdsfield_initialize();
 	}
-	if (rtnl_rtdsfield_tab[id])
+	if (!numeric && rtnl_rtdsfield_tab[id])
 		return rtnl_rtdsfield_tab[id];
 	snprintf(buf, len, "0x%02x", id);
 	return buf;
@@ -584,7 +586,7 @@  const char *rtnl_group_n2a(int id, char *buf, int len)
 	if (!rtnl_group_init)
 		rtnl_group_initialize();
 
-	for (i = 0; i < 256; i++) {
+	for (i = 0; !numeric && i < 256; i++) {
 		entry = rtnl_group_hash[i];
 
 		while (entry) {
@@ -633,8 +635,8 @@  static void nl_proto_initialize(void)
 
 const char *nl_proto_n2a(int id, char *buf, int len)
 {
-	if (id < 0 || id >= 256) {
-		snprintf(buf, len, "%u", id);
+	if (id < 0 || id >= 256 || numeric) {
+		snprintf(buf, len, "%d", id);
 		return buf;
 	}
 
diff --git a/man/man8/ip.8 b/man/man8/ip.8
index f4cbfc03..e2bda2a2 100644
--- a/man/man8/ip.8
+++ b/man/man8/ip.8
@@ -47,6 +47,7 @@  ip \- show / manipulate routing, network devices, interfaces and tunnels
 \fB\-t\fR[\fIimestamp\fR] |
 \fB\-ts\fR[\fIhort\fR] |
 \fB\-n\fR[\fIetns\fR] name |
+\fB\-N\fR[\fIumeric\fR] |
 \fB\-a\fR[\fIll\fR] |
 \fB\-c\fR[\fIolor\fR] |
 \fB\-br\fR[\fIief\fR] |
@@ -174,6 +175,11 @@  to
 .RI "-n[etns] " NETNS " [ " OPTIONS " ] " OBJECT " { " COMMAND " | "
 .BR help " }"
 
+.TP
+.BR "\-N" , " \-Numeric"
+Print the number of protocol, scope, dsfield, etc directly instead of
+converting it to human readable name.
+
 .TP
 .BR "\-a" , " \-all"
 executes specified command over all objects, it depends if command
diff --git a/man/man8/tc.8 b/man/man8/tc.8
index ab0bad8a..b81a396f 100644
--- a/man/man8/tc.8
+++ b/man/man8/tc.8
@@ -119,6 +119,7 @@  tc \- show / manipulate traffic control settings
 .IR OPTIONS " := {"
 \fB[ -force ] -b\fR[\fIatch\fR] \fB[ filename ] \fR|
 \fB[ \fB-n\fR[\fIetns\fR] name \fB] \fR|
+\fB[ \fB-N\fR[\fIumeric\fR] \fB] \fR|
 \fB[ \fB-nm \fR| \fB-nam\fR[\fIes\fR] \fB] \fR|
 \fB[ \fR{ \fB-cf \fR| \fB-c\fR[\fIonf\fR] \fR} \fB[ filename ] \fB] \fR
 \fB[ -t\fR[imestamp\fR] \fB\] \fR| \fB[ -t\fR[short\fR] \fR| \fB[
@@ -707,6 +708,11 @@  to
 .RI "-n[etns] " NETNS " [ " OPTIONS " ] " OBJECT " { " COMMAND " | "
 .BR help " }"
 
+.TP
+.BR "\-N" , " \-Numeric"
+Print the number of protocol, scope, dsfield, etc directly instead of
+converting it to human readable name.
+
 .TP
 .BR "\-cf" , " \-conf " <FILENAME>
 specifies path to the config file. This option is used in conjunction with other options (e.g.
diff --git a/misc/ss.c b/misc/ss.c
index 99c06d31..e01ebf4d 100644
--- a/misc/ss.c
+++ b/misc/ss.c
@@ -106,7 +106,6 @@  static int security_get_initial_context(char *name,  char **context)
 }
 #endif
 
-static int resolve_services = 1;
 int preferred_family = AF_UNSPEC;
 static int show_options;
 int show_details;
@@ -121,6 +120,7 @@  static int follow_events;
 static int sctp_ino;
 static int show_tipcinfo;
 static int show_tos;
+int numeric;
 int oneline;
 
 enum col_id {
@@ -1553,7 +1553,7 @@  static const char *resolve_service(int port)
 		return buf;
 	}
 
-	if (!resolve_services)
+	if (numeric)
 		goto do_numeric;
 
 	if (dg_proto == RAW_PROTO)
@@ -4296,14 +4296,11 @@  static int netlink_show_one(struct filter *f,
 
 	sock_state_print(&st);
 
-	if (resolve_services)
-		prot_name = nl_proto_n2a(prot, prot_buf, sizeof(prot_buf));
-	else
-		prot_name = int_to_str(prot, prot_buf);
+	prot_name = nl_proto_n2a(prot, prot_buf, sizeof(prot_buf));
 
 	if (pid == -1) {
 		procname[0] = '*';
-	} else if (resolve_services) {
+	} else if (!numeric) {
 		int done = 0;
 
 		if (!pid) {
@@ -5050,7 +5047,7 @@  int main(int argc, char *argv[])
 				 long_opts, NULL)) != EOF) {
 		switch (ch) {
 		case 'n':
-			resolve_services = 0;
+			numeric = 1;
 			break;
 		case 'r':
 			resolve_hosts = 1;
@@ -5268,7 +5265,7 @@  int main(int argc, char *argv[])
 	filter_states_set(&current_filter, state_filter);
 	filter_merge_defaults(&current_filter);
 
-	if (resolve_services && resolve_hosts &&
+	if (!numeric && resolve_hosts &&
 	    (current_filter.dbs & (UNIX_DBM|INET_L4_DBM)))
 		init_service_resolver();
 
diff --git a/tc/tc.c b/tc/tc.c
index e08f322a..64e342dd 100644
--- a/tc/tc.c
+++ b/tc/tc.c
@@ -43,6 +43,7 @@  bool use_names;
 int json;
 int color;
 int oneline;
+int numeric;
 
 static char *conf_file;
 
@@ -200,7 +201,7 @@  static void usage(void)
 		"		    action | monitor | exec }\n"
 		"       OPTIONS := { -V[ersion] | -s[tatistics] | -d[etails] | -r[aw] |\n"
 		"		    -o[neline] | -j[son] | -p[retty] | -c[olor]\n"
-		"		    -b[atch] [filename] | -n[etns] name |\n"
+		"		    -b[atch] [filename] | -n[etns] name | -N[umeric] |\n"
 		"		     -nm | -nam[es] | { -cf | -conf } path }\n");
 }
 
@@ -486,6 +487,8 @@  int main(int argc, char **argv)
 			NEXT_ARG();
 			if (netns_switch(argv[1]))
 				return -1;
+		} else if (matches(argv[1], "-Numeric") == 0) {
+			++numeric;
 		} else if (matches(argv[1], "-names") == 0 ||
 				matches(argv[1], "-nm") == 0) {
 			use_names = true;