RE: [PATCH iproute2 v3 2/4] ifstat: Add extended statistics to ifstat

-----Original Message-----
From: Stephen Hemminger [mailto:stephen@networkplumber.org]
Sent: Thursday, December 22, 2016 8:59 PM
To: Nogah Frankel <redacted>
Cc: netdev@vger.kernel.org; roopa@cumulusnetworks.com; roszenrami@gmail.com; Or
Gerlitz [off-list ref]; Jiri Pirko [off-list ref]; Elad Raz
[off-list ref]; Yotam Gigi [off-list ref]; Ido Schimmel
[off-list ref]
Subject: Re: [PATCH iproute2 v3 2/4] ifstat: Add extended statistics to ifstat

On Thu, 22 Dec 2016 18:23:13 +0200
Nogah Frankel [off-list ref] wrote:
On Thu, 22 Dec 2016 18:23:13 +0200
Nogah Frankel [off-list ref] wrote:

quoted

 }

@@ -691,18 +804,22 @@ static const struct option longopts[] = {
 	{ "interval", 1, 0, 't' },
 	{ "version", 0, 0, 'V' },
 	{ "zeros", 0, 0, 'z' },
+	{ "extended", 1, 0, 'x'},
 	{ 0 }
 };

+
 int main(int argc, char *argv[])

You let extra whitespace changes creep in.

quoted

+		case 'x':
+			is_extended = true;
+			memset(stats_type, 0, 64);
+			strncpy(stats_type, optarg, 63);
+			break;

This seems like doing this either the paranoid or hard way.
Why not:
	const char *stats_type = NULL;
...

	case 'x':
		stats_type = optarg;
		break;
...
		if (stats_type)
			snprintf(hist_name, sizeof(hist_name),
				 "%s/.%s_ifstat.u%d", P_tmpdir, stats_type,
				 getuid());
		else
			snprintf(hist_name, sizeof(hist_name),
				 "%s/.ifstat.u%d", P_tmpdir, getuid());


Since:
	1) optarg points to area in argv that is persistent (avoid copy)
	2) don't need is_extended flag value then

Please cleanup and resubmit.

I will.
Thank you.