Date:	Tue, 27 Oct 2015 14:04:56 +0100
From:	Phil Sutter <phil@....cc>
To:	Matthias Tafelmeier <matthias.tafelmeier@....net>
Cc:	netdev@...r.kernel.org, hagen@...u.net, shemminger@...l.org,
	fw@...len.de, edumazet@...gle.com, daniel@...earbox.net
Subject: Re: [PATCH v7 05/10] ss: replaced old output with new generic output
 mechanisms

On Thu, Sep 10, 2015 at 09:35:03PM +0200, Matthias Tafelmeier wrote:
> This patch just adds the -j and --json flag to ss. Also it ensures proper
> stats components bracketization – that goes for ex. TCP, UDP, NETLINK etc.
> 
> Moreover, this patch prevents human readable headers to be printed.
> 
> Most importantly, with this patch, the new prepared interface to the
> generic output handlers is used to replace the accustomed output
> logic.
> 
> Signed-off-by: Matthias Tafelmeier <matthias.tafelmeier@....net>
> Suggested-by: Hagen Paul Pfeifer <hagen@...u.net>
> ---
>  misc/ss.c | 487 ++++++++++++++++++++++++++++++++++----------------------------
>  1 file changed, 266 insertions(+), 221 deletions(-)
> 
> diff --git a/misc/ss.c b/misc/ss.c
> index e7ea041..7a1b6eb 100644
> --- a/misc/ss.c
> +++ b/misc/ss.c
> @@ -26,6 +26,7 @@
>  #include <fnmatch.h>
>  #include <getopt.h>
>  #include <stdbool.h>
> +#include <json_writer.h>
>  
>  #include "ss_types.h"
>  #include "utils.h"
> @@ -34,6 +35,9 @@
>  #include "libnetlink.h"
>  #include "namespace.h"
>  #include "SNAPSHOT.h"
> +#include "ss_out_fmt.h"
> +#include "ss_json_fmt.h"
> +#include "ss_types.h"
>  
>  #include <linux/tcp.h>
>  #include <linux/sock_diag.h>
> @@ -101,6 +105,7 @@ int show_sock_ctx = 0;
>  /* If show_users & show_proc_ctx only do user_ent_hash_build() once */
>  int user_ent_hash_build_init = 0;
>  int follow_events = 0;
> +int json_output = 0;
>  
>  int netid_width;
>  int state_width;
> @@ -109,11 +114,14 @@ int addr_width;
>  int serv_width;
>  int screen_width;
>  
> +enum out_fmt_type fmt_type = FMT_HR;
> +
>  static const char *TCP_PROTO = "tcp";
>  static const char *UDP_PROTO = "udp";
>  static const char *RAW_PROTO = "raw";
>  static const char *dg_proto = NULL;
>  
> +json_writer_t *json_wr;

Not sure if doable, but IMHO this should be hidden in ss_json_fmt.c.

>  #define PACKET_DBM ((1<<PACKET_DG_DB)|(1<<PACKET_R_DB))
>  #define UNIX_DBM ((1<<UNIX_DG_DB)|(1<<UNIX_ST_DB)|(1<<UNIX_SQ_DB))
> @@ -716,7 +724,6 @@ static int is_ephemeral(int port)
>  	return (port >= ip_local_port_min && port<= ip_local_port_max);
>  }
>  
> -

Export changes like this one into a dedicated patch at the start of the
series?

>  static const char *__resolve_service(int port)
>  {
>  	struct scache *c;
> @@ -790,7 +797,8 @@ do_numeric:
>  	return buf;
>  }
>  
> -static void inet_addr_print(const inet_prefix *a, int port, unsigned int ifindex)
> +static void inet_addr_print(const inet_prefix *a, int port,
> +			unsigned int ifindex, char *peer_kind)
>  {
>  	char buf[1024];
>  	const char *ap = buf;
> @@ -818,8 +826,8 @@ static void inet_addr_print(const inet_prefix *a, int port, unsigned int ifindex
>  		est_len -= strlen(ifname) + 1;  /* +1 for percent char */
>  	}
>  
> -	sock_addr_print_width(est_len, ap, ":", serv_width, resolve_service(port),
> -			ifname);
> +	sock_addr_fmt(ap, est_len, ":", serv_width, resolve_service(port),
> +			ifname, peer_kind);
>  }
>  
>  static int inet2_addr_match(const inet_prefix *a, const inet_prefix *p,
> @@ -1351,21 +1359,29 @@ static void inet_stats_print(struct sockstat *s, int protocol)
>  {
>  	char *buf = NULL;
>  
> -	sock_state_print(s, proto_name(protocol));
> +	sock_state_fmt(s, sstate_name, proto_name(protocol),
> +				netid_width, state_width);
>  
> -	inet_addr_print(&s->local, s->lport, s->iface);
> -	inet_addr_print(&s->remote, s->rport, 0);
> +	if (json_output) {
> +		jsonw_name(json_wr, "peers");
> +		jsonw_start_object(json_wr);
> +	}

How about an additional callback:

	fmt_new_object("peers");

The hr variant will be a no op, the json variant uses it's private (see
above) json_wr object.

> +	inet_addr_print(&s->local, s->lport, s->iface, "local");
> +	inet_addr_print(&s->remote, s->rport, 0, "remote");
> +	if (json_output)
> +		jsonw_end_object(json_wr);
>  
>  	if (show_proc_ctx || show_sock_ctx) {
>  		if (find_entry(s->ino, &buf,
> -				(show_proc_ctx & show_sock_ctx) ?
> -				PROC_SOCK_CTX : PROC_CTX) > 0) {
> -			printf(" users:(%s)", buf);
> +			       (show_proc_ctx & show_sock_ctx) ?
> +			       PROC_SOCK_CTX : PROC_CTX) > 0) {
> +			sock_users_fmt(buf);
>  			free(buf);
>  		}
>  	} else if (show_users) {
>  		if (find_entry(s->ino, &buf, USERS) > 0) {
> -			printf(" users:(%s)", buf);
> +			sock_users_fmt(buf);
>  			free(buf);
>  		}
>  	}
> @@ -1469,16 +1485,16 @@ static int tcp_show_line(char *line, const struct filter *f, int family)
>  	inet_stats_print(&s.ss, IPPROTO_TCP);
>  
>  	if (show_options)
> -		tcp_timer_print(&s);
> +		tcp_timer_fmt(&s);
>  
>  	if (show_details) {
> -		sock_details_print(&s.ss);
> +		sock_details_fmt(&s.ss, GENERIC_DETAIL, 0, 0);
>  		if (opt[0])
> -			printf(" opt:\"%s\"", opt);
> +			opt_fmt(opt);
>  	}
>  
>  	if (show_tcpinfo)
> -		tcp_stats_print(&s);
> +		tcp_stats_fmt(&s);
>  
>  	printf("\n");
>  	return 0;
> @@ -1522,31 +1538,14 @@ static void print_skmeminfo(struct rtattr *tb[], int attrtype)
>  			const struct inet_diag_meminfo *minfo =
>  				RTA_DATA(tb[INET_DIAG_MEMINFO]);
>  
> -			printf(" mem:(r%u,w%u,f%u,t%u)",
> -					minfo->idiag_rmem,
> -					minfo->idiag_wmem,
> -					minfo->idiag_fmem,
> -					minfo->idiag_tmem);
> +			mem_fmt(minfo);
>  		}
>  		return;
>  	}
>  
>  	skmeminfo = RTA_DATA(tb[attrtype]);
>  
> -	printf(" skmem:(r%u,rb%u,t%u,tb%u,f%u,w%u,o%u",
> -	       skmeminfo[SK_MEMINFO_RMEM_ALLOC],
> -	       skmeminfo[SK_MEMINFO_RCVBUF],
> -	       skmeminfo[SK_MEMINFO_WMEM_ALLOC],
> -	       skmeminfo[SK_MEMINFO_SNDBUF],
> -	       skmeminfo[SK_MEMINFO_FWD_ALLOC],
> -	       skmeminfo[SK_MEMINFO_WMEM_QUEUED],
> -	       skmeminfo[SK_MEMINFO_OPTMEM]);
> -
> -	if (RTA_PAYLOAD(tb[attrtype]) >=
> -		(SK_MEMINFO_BACKLOG + 1) * sizeof(__u32))
> -		printf(",bl%u", skmeminfo[SK_MEMINFO_BACKLOG]);
> -
> -	printf(")");
> +	skmem_fmt(skmeminfo, tb, attrtype);
>  }
>  
>  #define TCPI_HAS_OPT(info, opt) !!(info->tcpi_options & (opt))
> @@ -1659,8 +1658,11 @@ static void tcp_show_info(const struct nlmsghdr *nlh, struct inet_diag_msg *r,
>  		s.bytes_received = info->tcpi_bytes_received;
>  		s.segs_out = info->tcpi_segs_out;
>  		s.segs_in = info->tcpi_segs_in;
> -		tcp_stats_print(&s);
> -		free(s.dctcp);
> +		tcp_stats_fmt(&s);
> +		if (s.dctcp)
> +			free(s.dctcp);
> +		if (s.cong_alg)
> +			free(s.cong_alg);
>  	}
>  }
>  
> @@ -1684,6 +1686,8 @@ static int inet_show_sock(struct nlmsghdr *nlh, struct filter *f, int protocol)
>  	s.iface		= r->id.idiag_if;
>  	s.sk		= cookie_sk_get(&r->id.idiag_cookie[0]);
>  
> +	jsonw_start_object(json_wr);
> +
>  	if (s.local.family == AF_INET) {
>  		s.local.bytelen = s.remote.bytelen = 4;
>  	} else {
> @@ -1707,29 +1711,27 @@ static int inet_show_sock(struct nlmsghdr *nlh, struct filter *f, int protocol)
>  		t.timer = r->idiag_timer;
>  		t.timeout = r->idiag_expires;
>  		t.retrans = r->idiag_retrans;
> -		tcp_timer_print(&t);
> +		tcp_timer_fmt(&t);
>  	}
>  
>  	if (show_details) {
> -		sock_details_print(&s);
> -		if (s.local.family == AF_INET6 && tb[INET_DIAG_SKV6ONLY]) {
> -			unsigned char v6only;
> -			v6only = *(__u8 *)RTA_DATA(tb[INET_DIAG_SKV6ONLY]);
> -			printf(" v6only:%u", v6only);
> -		}
> +		sock_details_fmt(&s, GENERIC_DETAIL, 0, 0);

Looks like v6only state got optimized out. At least I don't see it being
exported in the sock_details_fmt callbacks.

>  		if (tb[INET_DIAG_SHUTDOWN]) {
>  			unsigned char mask;
>  			mask = *(__u8 *)RTA_DATA(tb[INET_DIAG_SHUTDOWN]);
> -			printf(" %c-%c", mask & 1 ? '-' : '<', mask & 2 ? '-' : '>');
> +			sock_conn_fmt(mask);

While it is questionable in general whether json output should follow
this kind of symbolism, it appears that both callbacks do the same
INET_DIAG_SHUTDOWN value encoding. Unnecessary and error-prone code
duplication IMO. As a side-note, one could also implement
sock_conn_fmt() to accept a format-string, although that's maybe not
worth the effort.

>  		}
>  	}
>  
>  	if (show_mem || show_tcpinfo) {
> -		printf("\n\t");

Hmm. Doesn't this change hr output? Not that it's not worth improving,
but I don't think it should be part of this series.

>  		tcp_show_info(nlh, r, tb);
>  	}
>  
> -	printf("\n");
> +	if (json_output)
> +		jsonw_end_object(json_wr);
> +	else
> +		printf("\n");
> +
>  	return 0;
>  }
>  
> @@ -2080,7 +2082,10 @@ static int dgram_show_line(char *line, const struct filter *f, int family)
>  	inet_stats_print(&s, dg_proto == UDP_PROTO ? IPPROTO_UDP : 0);
>  
>  	if (show_details && opt[0])
> -		printf(" opt:\"%s\"", opt);
> +		opt_fmt(opt);
> +
> +	if (json_output)
> +		jsonw_end_object(json_wr);
>  
>  	printf("\n");
>  	return 0;
> @@ -2257,27 +2262,39 @@ static void unix_stats_print(struct sockstat *list, struct filter *f)
>  				continue;
>  		}
>  
> -		sock_state_print(s, unix_netid_name(s->type));
> +		sock_state_fmt(s, sstate_name,
> +			unix_netid_name(s->type), netid_width, state_width);
>  
> -		sock_addr_print(s->name ?: "*", " ",
> -				int_to_str(s->lport, port_name), NULL);
> -		sock_addr_print(peer, " ", int_to_str(s->rport, port_name),
> -				NULL);
> +		if (json_output) {
> +			jsonw_name(json_wr, "peers");
> +			jsonw_start_object(json_wr);
> +		}
> +
> +		sock_addr_fmt(s->name ?: "*", addr_width,
> +			" ", serv_width,
> +			int_to_str(s->lport, port_name),
> +				NULL, "local");
> +		sock_addr_fmt(peer, addr_width, " ", serv_width,
> +				int_to_str(s->rport, port_name),
> +				NULL, "remote");
> +		if (json_output)
> +			jsonw_end_object(json_wr);
>  
>  		if (show_proc_ctx || show_sock_ctx) {
>  			if (find_entry(s->ino, &ctx_buf,
> -					(show_proc_ctx & show_sock_ctx) ?
> -					PROC_SOCK_CTX : PROC_CTX) > 0) {
> -				printf(" users:(%s)", ctx_buf);
> +				       (show_proc_ctx & show_sock_ctx) ?
> +				       PROC_SOCK_CTX : PROC_CTX) > 0) {
> +				sock_users_fmt(ctx_buf);
>  				free(ctx_buf);
>  			}
>  		} else if (show_users) {
>  			if (find_entry(s->ino, &ctx_buf, USERS) > 0) {
> -				printf(" users:(%s)", ctx_buf);
> +				sock_users_fmt(ctx_buf);
>  				free(ctx_buf);
>  			}
>  		}
> -		printf("\n");
> +		if (!json_output)
> +			printf("\n");
>  	}
>  }
>  
> @@ -2290,7 +2307,9 @@ static int unix_show_sock(const struct sockaddr_nl *addr, struct nlmsghdr *nlh,
>  	char name[128];
>  	struct sockstat stat = { .name = "*", .peer_name = "*" };
>  
> -	parse_rtattr(tb, UNIX_DIAG_MAX, (struct rtattr*)(r+1),
> +	jsonw_start_object(json_wr);
> +
> +	parse_rtattr(tb, UNIX_DIAG_MAX, (struct rtattr *)(r + 1),

Yet another unrelated change. Although I surely repeat myself: Fixing up
coding style is good, but complicating this patch with it is not. Might
go well along with empty line cleanup and other "meta" changes into an
initial patch.

>  		     nlh->nlmsg_len - NLMSG_LENGTH(sizeof(*r)));
>  
>  	stat.type  = r->udiag_type;
> @@ -2323,21 +2342,27 @@ static int unix_show_sock(const struct sockaddr_nl *addr, struct nlmsghdr *nlh,
>  		return 0;
>  
>  	unix_stats_print(&stat, f);
> -
>  	if (show_mem) {
> -		printf("\t");
> +		if (!json_output)
> +			printf("\t");
>  		print_skmeminfo(tb, UNIX_DIAG_MEMINFO);

Can't you just move the tab printing into mem_hr_fmt()? (Maybe I miss
something here.)

>  	}
>  	if (show_details) {
>  		if (tb[UNIX_DIAG_SHUTDOWN]) {
>  			unsigned char mask;
>  			mask = *(__u8 *)RTA_DATA(tb[UNIX_DIAG_SHUTDOWN]);
> -			printf(" %c-%c", mask & 1 ? '-' : '<', mask & 2 ? '-' : '>');
> +			sock_conn_fmt(mask);
>  		}
>  	}
>  	if (show_mem || show_details)
> -		printf("\n");
> +		if (!json_output)
> +			printf("\n");
> +
> +	if (json_output)
> +		jsonw_end_object(json_wr);
>  
> +	if (name)
> +		free(name);
>  	return 0;
>  }
>  
> @@ -2479,7 +2504,8 @@ static int packet_stats_print(struct sockstat *s, const struct filter *f)
>  			return 1;
>  	}
>  
> -	sock_state_print(s, s->type == SOCK_RAW ? "p_raw" : "p_dgr");
> +	sock_state_fmt(s, sstate_name, s->type == SOCK_RAW ? "p_raw" : "p_dgr",
> +					netid_width, state_width);
>  
>  	if (s->prot == 3)
>  		addr = "*";
> @@ -2491,25 +2517,37 @@ static int packet_stats_print(struct sockstat *s, const struct filter *f)
>  	else
>  		port = xll_index_to_name(s->iface);
>  
> -	sock_addr_print(addr, ":", port, NULL);
> -	sock_addr_print("", "*", "", NULL);
> +	if (json_output) {
> +		jsonw_name(json_wr, "peers");
> +		jsonw_start_object(json_wr);
> +	}
> +
> +	sock_addr_fmt(addr, addr_width, ":",
> +			serv_width, port,
> +			NULL, "local");
> +	sock_addr_fmt("", addr_width, "*",
> +			serv_width, "",
> +			NULL, "remote");

addr_width and serv_width belong to hr formatting only, json ignores
them altogether. Therefore hide them in hr output code.

> +	if (json_output)
> +		jsonw_end_object(json_wr);
>  
>  	if (show_proc_ctx || show_sock_ctx) {
>  		if (find_entry(s->ino, &buf,
> -					(show_proc_ctx & show_sock_ctx) ?
> -					PROC_SOCK_CTX : PROC_CTX) > 0) {
> -			printf(" users:(%s)", buf);
> +			       (show_proc_ctx & show_sock_ctx) ?
> +			       PROC_SOCK_CTX : PROC_CTX) > 0) {
> +			sock_users_fmt(buf);

Yet another case of unrelated changes (indenting fixup), this time
combined with real changes. Hard to spot what's really going on here.

>  			free(buf);
>  		}
>  	} else if (show_users) {
>  		if (find_entry(s->ino, &buf, USERS) > 0) {
> -			printf(" users:(%s)", buf);
> +			sock_users_fmt(buf);
>  			free(buf);
>  		}
>  	}
>  
>  	if (show_details)
> -		sock_details_print(s);
> +		sock_details_fmt(s, GENERIC_DETAIL, 0, 0);
>  
>  	return 0;
>  }
> @@ -2526,7 +2564,9 @@ static int packet_show_sock(const struct sockaddr_nl *addr,
>  	uint32_t fanout = 0;
>  	bool has_fanout = false;
>  
> -	parse_rtattr(tb, PACKET_DIAG_MAX, (struct rtattr*)(r+1),
> +	jsonw_start_object(json_wr);
> +
> +	parse_rtattr(tb, PACKET_DIAG_MAX, (struct rtattr *)(r + 1),
>  		     nlh->nlmsg_len - NLMSG_LENGTH(sizeof(*r)));
>  
>  	/* use /proc/net/packet if all info are not available */
> @@ -2566,60 +2606,9 @@ static int packet_show_sock(const struct sockaddr_nl *addr,
>  	if (packet_stats_print(&stat, f))
>  		return 0;
>  
> -	if (show_details) {
> -		if (pinfo) {
> -			printf("\n\tver:%d", pinfo->pdi_version);
> -			printf(" cpy_thresh:%d", pinfo->pdi_copy_thresh);
> -			printf(" flags( ");
> -			if (pinfo->pdi_flags & PDI_RUNNING)
> -				printf("running");
> -			if (pinfo->pdi_flags & PDI_AUXDATA)
> -				printf(" auxdata");
> -			if (pinfo->pdi_flags & PDI_ORIGDEV)
> -				printf(" origdev");
> -			if (pinfo->pdi_flags & PDI_VNETHDR)
> -				printf(" vnethdr");
> -			if (pinfo->pdi_flags & PDI_LOSS)
> -				printf(" loss");
> -			if (!pinfo->pdi_flags)
> -				printf("0");
> -			printf(" )");
> -		}
> -		if (ring_rx) {
> -			printf("\n\tring_rx(");
> -			packet_show_ring(ring_rx);
> -			printf(")");
> -		}
> -		if (ring_tx) {
> -			printf("\n\tring_tx(");
> -			packet_show_ring(ring_tx);
> -			printf(")");
> -		}
> -		if (has_fanout) {
> -			uint16_t type = (fanout >> 16) & 0xffff;
> -
> -			printf("\n\tfanout(");
> -			printf("id:%d,", fanout & 0xffff);
> -			printf("type:");
> -
> -			if (type == 0)
> -				printf("hash");
> -			else if (type == 1)
> -				printf("lb");
> -			else if (type == 2)
> -				printf("cpu");
> -			else if (type == 3)
> -				printf("roll");
> -			else if (type == 4)
> -				printf("random");
> -			else if (type == 5)
> -				printf("qm");
> -			else
> -				printf("0x%x", type);
> -
> -			printf(")");
> -		}
> -	}
> +	if (show_details)
> +		packet_details_fmt(pinfo,
> +				ring_rx, ring_tx, fanout, has_fanout);
>  
>  	if (show_bpf && tb[PACKET_DIAG_FILTER]) {
>  		struct sock_filter *fil =
> @@ -2627,15 +2616,15 @@ static int packet_show_sock(const struct sockaddr_nl *addr,
>  		int num = RTA_PAYLOAD(tb[PACKET_DIAG_FILTER]) /
>  			  sizeof(struct sock_filter);
>  
> -		printf("\n\tbpf filter (%d): ", num);
> -		while (num) {
> -			printf(" 0x%02x %u %u %u,",
> -			      fil->code, fil->jt, fil->jf, fil->k);
> -			num--;
> -			fil++;
> -		}
> +		bpf_filter_fmt(fil, num);
>  	}
> -	printf("\n");
> +
> +	if (json_output)
> +		jsonw_end_object(json_wr);
> +	else
> +		printf("\n");
> +
> +
>  	return 0;
>  }
>  
> @@ -2713,6 +2702,7 @@ static int netlink_show_one(struct filter *f,
>  	SPRINT_BUF(prot_buf) = {};
>  	const char *prot_name;
>  	char procname[64] = {};
> +	char *rem = "remote";
>  
>  	st.state = SS_CLOSE;
>  	st.rq	 = rq;
> @@ -2728,7 +2718,7 @@ static int netlink_show_one(struct filter *f,
>  			return 1;
>  	}
>  
> -	sock_state_print(&st, "nl");
> +	sock_state_fmt(&st, sstate_name, "nl", netid_width, state_width);
>  
>  	if (resolve_services)
>  		prot_name = nl_proto_n2a(prot, prot_buf, sizeof(prot_buf));
> @@ -2760,17 +2750,27 @@ static int netlink_show_one(struct filter *f,
>  		int_to_str(pid, procname);
>  	}
>  
> -	sock_addr_print(prot_name, ":", procname, NULL);
> +	if (json_output) {
> +		jsonw_name(json_wr, "peers");
> +		jsonw_start_object(json_wr);
> +	}
> +
> +	sock_addr_fmt(prot_name, addr_width, ":", serv_width,
> +			procname, NULL, "local");
>  
>  	if (state == NETLINK_CONNECTED) {
>  		char dst_group_buf[30];
>  		char dst_pid_buf[30];
> -		sock_addr_print(int_to_str(dst_group, dst_group_buf), ":",
> -				int_to_str(dst_pid, dst_pid_buf), NULL);
> +		sock_addr_fmt(int_to_str(dst_group, dst_group_buf), addr_width,
> +				":", serv_width, int_to_str(dst_pid, dst_pid_buf),
> +				NULL, rem);
>  	} else {
> -		sock_addr_print("", "*", "", NULL);
> +		sock_addr_fmt("", addr_width, "*", serv_width, "", NULL, rem);
>  	}
>  
> +	if (json_output)
> +		jsonw_end_object(json_wr);
> +
>  	char *pid_context = NULL;
>  	if (show_proc_ctx) {
>  		/* The pid value will either be:
> @@ -2784,16 +2784,11 @@ static int netlink_show_one(struct filter *f,
>  		else if (pid > 0)
>  			getpidcon(pid, &pid_context);
>  
> -		if (pid_context != NULL) {
> -			printf("proc_ctx=%-*s ", serv_width, pid_context);
> -			free(pid_context);
> -		} else {
> -			printf("proc_ctx=%-*s ", serv_width, "unavailable");
> -		}
> +		proc_fmt(serv_width, pid_context);
>  	}
>  
>  	if (show_details) {
> -		printf(" sk=%llx cb=%llx groups=0x%08x", sk, cb, groups);
> +		sock_details_fmt(&st, NETLINK_DETAIL, groups, cb);
>  	}
>  	printf("\n");
>  
> @@ -2809,7 +2804,9 @@ static int netlink_show_sock(const struct sockaddr_nl *addr,
>  	int rq = 0, wq = 0;
>  	unsigned long groups = 0;
>  
> -	parse_rtattr(tb, NETLINK_DIAG_MAX, (struct rtattr*)(r+1),
> +	jsonw_start_object(json_wr);
> +
> +	parse_rtattr(tb, NETLINK_DIAG_MAX, (struct rtattr *)(r + 1),
>  		     nlh->nlmsg_len - NLMSG_LENGTH(sizeof(*r)));
>  
>  	if (tb[NETLINK_DIAG_GROUPS] && RTA_PAYLOAD(tb[NETLINK_DIAG_GROUPS]))
> @@ -2835,6 +2832,9 @@ static int netlink_show_sock(const struct sockaddr_nl *addr,
>  		printf("\n");
>  	}
>  
> +	if (json_output)
> +		jsonw_end_object(json_wr);
> +
>  	return 0;
>  }
>  
> @@ -3042,32 +3042,7 @@ static int print_summary(void)
>  
>  	get_slabstat(&slabstat);
>  
> -	printf("Total: %d (kernel %d)\n", s.socks, slabstat.socks);
> -
> -	printf("TCP:   %d (estab %d, closed %d, orphaned %d, synrecv %d, timewait %d/%d), ports %d\n",
> -	       s.tcp_total + slabstat.tcp_syns + s.tcp_tws,
> -	       sn.tcp_estab,
> -	       s.tcp_total - (s.tcp4_hashed+s.tcp6_hashed-s.tcp_tws),
> -	       s.tcp_orphans,
> -	       slabstat.tcp_syns,
> -	       s.tcp_tws, slabstat.tcp_tws,
> -	       slabstat.tcp_ports
> -	       );
> -
> -	printf("\n");
> -	printf("Transport Total     IP        IPv6\n");
> -	printf("*	  %-9d %-9s %-9s\n", slabstat.socks, "-", "-");
> -	printf("RAW	  %-9d %-9d %-9d\n", s.raw4+s.raw6, s.raw4, s.raw6);
> -	printf("UDP	  %-9d %-9d %-9d\n", s.udp4+s.udp6, s.udp4, s.udp6);
> -	printf("TCP	  %-9d %-9d %-9d\n", s.tcp4_hashed+s.tcp6_hashed, s.tcp4_hashed, s.tcp6_hashed);
> -	printf("INET	  %-9d %-9d %-9d\n",
> -	       s.raw4+s.udp4+s.tcp4_hashed+
> -	       s.raw6+s.udp6+s.tcp6_hashed,
> -	       s.raw4+s.udp4+s.tcp4_hashed,
> -	       s.raw6+s.udp6+s.tcp6_hashed);
> -	printf("FRAG	  %-9d %-9d %-9d\n", s.frag4+s.frag6, s.frag4, s.frag6);
> -
> -	printf("\n");
> +	sock_summary_fmt(&s, &sn, &slabstat);
>  
>  	return 0;
>  }
> @@ -3095,6 +3070,7 @@ static void _usage(FILE *dest)
>  "   -z, --contexts      display process and socket SELinux security contexts\n"
>  "   -N, --net           switch to the specified network namespace name\n"
>  "\n"
> +"   -j, --json          format output in JSON\n"
>  "   -4, --ipv4          display only IP version 4 sockets\n"
>  "   -6, --ipv6          display only IP version 6 sockets\n"
>  "   -0, --packet        display PACKET sockets\n"
> @@ -3194,6 +3170,7 @@ static const struct option long_opts[] = {
>  	{ "help", 0, 0, 'h' },
>  	{ "context", 0, 0, 'Z' },
>  	{ "contexts", 0, 0, 'z' },
> +	{ "json", 0, 0, 'j' },
>  	{ "net", 1, 0, 'N' },
>  	{ 0 }
>  
> @@ -3209,7 +3186,7 @@ int main(int argc, char *argv[])
>  	int ch;
>  	int state_filter = 0;
>  
> -	while ((ch = getopt_long(argc, argv, "dhaletuwxnro460spbEf:miA:D:F:vVzZN:",
> +	while ((ch = getopt_long(argc, argv, "dhaletuwxnro460spbEf:miA:D:F:vVzZN:j",
>  				 long_opts, NULL)) != EOF) {
>  		switch(ch) {
>  		case 'n':
> @@ -3390,6 +3367,12 @@ int main(int argc, char *argv[])
>  			if (netns_switch(optarg))
>  				exit(1);
>  			break;
> +		case 'j':
> +			json_wr = jsonw_new(stdout);
> +			jsonw_pretty(json_wr, true);
> +			fmt_type = FMT_JSON;
> +			json_output = 1;
> +			break;

Careful here: Users may very well call 'ss -j -j'. Unless it's
impossible for this code to leak or get screwed up by invoking it
multiple times, rather just set 'json_output = 1' or maybe better just
'fmt_type = FMT_JSON' and have a switch(fmt_type) afterwards which does
the necessary init (hint: init callback ;).

>  		case 'h':
>  		case '?':
>  			help();
> @@ -3401,12 +3384,6 @@ int main(int argc, char *argv[])
>  	argc -= optind;
>  	argv += optind;
>  
> -	if (do_summary) {
> -		print_summary();
> -		if (do_default && argc == 0)
> -			exit(0);
> -	}
> -
>  	/* Now parse filter... */
>  	if (argc == 0 && filter_fp) {
>  		if (ssfilter_parse(&current_filter.f, 0, NULL, filter_fp))
> @@ -3471,11 +3448,24 @@ int main(int argc, char *argv[])
>  				exit(-1);
>  			}
>  		}
> +		jsonw_name(json_wr, "TCP");
> +		jsonw_start_array(json_wr);
>  		inet_show_netlink(&current_filter, dump_fp, IPPROTO_TCP);
> +		jsonw_end_array(json_wr);
> +		jsonw_destroy(&json_wr);
>  		fflush(dump_fp);
>  		exit(0);
>  	}
>  
> +	if (do_summary) {
> +		print_summary();
> +		if (do_default && argc == 0) {
> +			if (json_output)
> +				jsonw_destroy(&json_wr);
> +			exit(0);
> +		}
> +	}

Uhm, why move that do_summary conditional down here? Can't this lead
to extra output before the summary?

>  	if (ssfilter_parse(&current_filter.f, argc, argv, filter_fp))
>  		usage();
>  
> @@ -3497,62 +3487,117 @@ int main(int argc, char *argv[])
>  		}
>  	}
>  
> -	addrp_width = screen_width;
> -	addrp_width -= netid_width+1;
> -	addrp_width -= state_width+1;
> -	addrp_width -= 14;
> +	if (!json_output) {

Perfect case for init callbacks.

> -	if (addrp_width&1) {
> -		if (netid_width)
> -			netid_width++;
> -		else if (state_width)
> -			state_width++;
> -	}
> +		addrp_width = screen_width;
> +		addrp_width -= netid_width + 1;
> +		addrp_width -= state_width + 1;
> +		addrp_width -= 14;
>  
> -	addrp_width /= 2;
> -	addrp_width--;
> +		if (addrp_width & 1) {
> +			if (netid_width)
> +				netid_width++;
> +			else if (state_width)
> +				state_width++;
> +		}
>  
> -	serv_width = resolve_services ? 7 : 5;
> +		addrp_width /= 2;
> +		addrp_width--;
>  
> -	if (addrp_width < 15+serv_width+1)
> -		addrp_width = 15+serv_width+1;
> +		serv_width = resolve_services ? 7 : 5;
>  
> -	addr_width = addrp_width - serv_width - 1;
> +		if (addrp_width < 15 + serv_width + 1)
> +			addrp_width = 15 + serv_width + 1;
>  
> -	if (netid_width)
> -		printf("%-*s ", netid_width, "Netid");
> -	if (state_width)
> -		printf("%-*s ", state_width, "State");
> -	printf("%-6s %-6s ", "Recv-Q", "Send-Q");
> +		addr_width = addrp_width - serv_width - 1;
> +		if (netid_width)
> +			printf("%-*s ", netid_width, "Netid");
> +		if (state_width)
> +			printf("%-*s ", state_width, "State");
> +		printf("%-6s %-6s ", "Recv-Q", "Send-Q");
>  
> -	/* Make enough space for the local/remote port field */
> -	addr_width -= 13;
> -	serv_width += 13;
> +		/* Make enough space for the local/remote port field */
> +		addr_width -= 13;
> +		serv_width += 13;
>  
> -	printf("%*s:%-*s %*s:%-*s\n",
> -	       addr_width, "Local Address", serv_width, "Port",
> -	       addr_width, "Peer Address", serv_width, "Port");
> +		printf("%*s:%-*s %*s:%-*s\n",
> +				addr_width, "Local Address", serv_width, "Port",
> +				addr_width, "Peer Address", serv_width, "Port");
> +	}
> +
> +	fflush(stdout);
> +
> +	if (current_filter.dbs & (1<<NETLINK_DB)) {
> +		if (json_output) {
> +			jsonw_name(json_wr, "NETLINK");
> +			jsonw_start_array(json_wr);
> +			netlink_show(&current_filter);

Missing call to jsonw_end_array() here?

> +		} else
> +			netlink_show(&current_filter);
> +	}
> +	if (current_filter.dbs & PACKET_DBM) {
> +		if (json_output) {
> +			jsonw_name(json_wr, "PACKET");
> +			jsonw_start_array(json_wr);
> +			packet_show(&current_filter);
> +			jsonw_end_array(json_wr);
> +		} else
> +			packet_show(&current_filter);
> +	}
> +	if (current_filter.dbs & UNIX_DBM) {
> +		if (json_output) {
> +			jsonw_name(json_wr, "UNIX");
> +			jsonw_start_array(json_wr);
> +			unix_show(&current_filter);
> +			jsonw_end_array(json_wr);
> +		} else
> +			unix_show(&current_filter);
> +	}
> +	if (current_filter.dbs & (1<<RAW_DB)) {
> +		if (json_output) {
> +			jsonw_name(json_wr, "RAW");
> +			jsonw_start_array(json_wr);
> +			raw_show(&current_filter);
> +			jsonw_end_array(json_wr);
> +		} else
> +			raw_show(&current_filter);
> +	}
> +	if (current_filter.dbs & (1<<UDP_DB)) {
> +		if (json_output) {
> +			jsonw_name(json_wr, "UDP");
> +			jsonw_start_array(json_wr);
> +			udp_show(&current_filter);
> +			jsonw_end_array(json_wr);
> +		} else
> +			udp_show(&current_filter);
> +	}
> +	if (current_filter.dbs & (1<<TCP_DB)) {
> +		if (json_output) {
> +			jsonw_name(json_wr, "TCP");
> +			jsonw_start_array(json_wr);
> +			tcp_show(&current_filter, IPPROTO_TCP);
> +			jsonw_end_array(json_wr);
> +		} else
> +			tcp_show(&current_filter, IPPROTO_TCP);
> +	}
> +	if (current_filter.dbs & (1<<DCCP_DB)) {
> +		if (json_output) {
> +			jsonw_name(json_wr, "DCCP");
> +			jsonw_start_array(json_wr);
> +			tcp_show(&current_filter, IPPROTO_DCCP);
> +			jsonw_end_array(json_wr);
> +		} else
> +			tcp_show(&current_filter, IPPROTO_DCCP);
> +	}

Frankly, this whole construct is ugly. The question about whether
filters should affect json output at all aside, this should be cleaned
up. In the very basic form, move the json_output conditional into
jsonw_name(), jsonw_start_array() and jsonw_end_array(). You could also
move the json function calls into the *_show() functions.

> +
> +	if (json_output)
> +		jsonw_destroy(&json_wr);
>  
>  	fflush(stdout);
>  
>  	if (follow_events)
>  		exit(handle_follow_request(&current_filter));
>  
> -	if (current_filter.dbs & (1<<NETLINK_DB))
> -		netlink_show(&current_filter);
> -	if (current_filter.dbs & PACKET_DBM)
> -		packet_show(&current_filter);
> -	if (current_filter.dbs & UNIX_DBM)
> -		unix_show(&current_filter);
> -	if (current_filter.dbs & (1<<RAW_DB))
> -		raw_show(&current_filter);
> -	if (current_filter.dbs & (1<<UDP_DB))
> -		udp_show(&current_filter);
> -	if (current_filter.dbs & (1<<TCP_DB))
> -		tcp_show(&current_filter, IPPROTO_TCP);
> -	if (current_filter.dbs & (1<<DCCP_DB))
> -		tcp_show(&current_filter, IPPROTO_DCCP);
> -
>  	if (show_users || show_proc_ctx || show_sock_ctx)
>  		user_ent_destroy();
>  
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists