Re: [net-next PATCH iproute2 v2 1/1] tc: introduce IFE action

[Stephen Hemminger <shemming@brocade.com>]

On Wed,  9 Mar 2016 07:04:36 -0500
Jamal Hadi Salim <jhs@mojatatu.com> wrote:

> +		fprintf(f, "\t Metadata: ");
> +
> +		if (metalist[IFE_META_SKBMARK]) {
> +			len = RTA_PAYLOAD(metalist[IFE_META_SKBMARK]);
> +			if (len) {
> +				__u32 *mmark = RTA_DATA(metalist[IFE_META_SKBMARK]);
> +				fprintf(f, "use mark %d ", *mmark);
> +			} else
> +				fprintf(f, "allow mark ");

This code has diverged way from the general rule that ip utilities display
format should match the command format. For example the properties shown
on "ip route show" match those of "ip route add".

Also over the last several years, the code in iproute2 has switched from casting
RTA_DATA() everywhere to a cleaner interface rte_getattr_u32() more like what
is used in mnl library.

The code has also gotten deeply intended creating lots of lines that are too long.

WARNING: 'doesnt' may be misspelled - perhaps 'doesn't'?
#21: 
then provide a default so the user doesnt have to specify it.

WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#25: 
            "Distributing Linux Traffic Control Classifier-Action Subsystem"

WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#143: 
new file mode 100644

ERROR: "foo * bar" should be "foo *bar"
#378: FILE: tc/m_ife.c:231:
+static int print_ife(struct action_util *au, FILE * f, struct rtattr *arg)

WARNING: Missing a blank line after declarations
#384: FILE: tc/m_ife.c:237:
+	int has_optional = 0;
+	SPRINT_BUF(b1);

WARNING: Missing a blank line after declarations
#406: FILE: tc/m_ife.c:259:
+			struct tcf_t *tm = RTA_DATA(tb[TCA_IFE_TM]);
+			print_tm(f, tm);

WARNING: line over 80 characters
#449: FILE: tc/m_ife.c:302:
+				__u32 *mmark = RTA_DATA(metalist[IFE_META_SKBMARK]);

WARNING: Missing a blank line after declarations
#450: FILE: tc/m_ife.c:303:
+				__u32 *mmark = RTA_DATA(metalist[IFE_META_SKBMARK]);
+				fprintf(f, "use mark %d ", *mmark);

WARNING: line over 80 characters
#458: FILE: tc/m_ife.c:311:
+				__u32 *mhash = RTA_DATA(metalist[IFE_META_HASHID]);

WARNING: Missing a blank line after declarations
#459: FILE: tc/m_ife.c:312:
+				__u32 *mhash = RTA_DATA(metalist[IFE_META_HASHID]);
+				fprintf(f, "use hash %d ", *mhash);

WARNING: line over 80 characters
#467: FILE: tc/m_ife.c:320:
+				__u32 *mprio = RTA_DATA(metalist[IFE_META_PRIO]);

WARNING: Missing a blank line after declarations
#468: FILE: tc/m_ife.c:321:
+				__u32 *mprio = RTA_DATA(metalist[IFE_META_PRIO]);
+				fprintf(f, "use prio %d ", *mprio);

total: 1 errors, 11 warnings, 343 lines checked