diff mbox series

[iptables,11/12] extensions: Do not print all-one's netmasks

Message ID 20221006002802.4917-12-phil@nwl.cc
State Changes Requested
Delegated to: Pablo Neira
Headers show
Series Speed up iptables-tests.py | expand

Commit Message

Phil Sutter Oct. 6, 2022, 12:28 a.m. UTC 
All one's netmasks are a trivial default, no point in printing them.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 extensions/libip6t_NETMAP.c  |  2 +-
 extensions/libipt_NETMAP.c   |  2 +-
 extensions/libxt_CONNMARK.c  | 32 +++++++++++++++++--------
 extensions/libxt_CONNMARK.t  |  4 ++--
 extensions/libxt_MARK.c      |  4 +++-
 extensions/libxt_connlimit.c |  8 +++++--
 extensions/libxt_connlimit.t |  8 +++----
 extensions/libxt_recent.c    | 45 ++++++++++++++++++++----------------
 extensions/libxt_recent.t    | 12 +++++-----
 9 files changed, 70 insertions(+), 47 deletions(-)

Comments

Jan Engelhardt Oct. 6, 2022, 6:27 a.m. UTC | #1 
On Thursday 2022-10-06 02:28, Phil Sutter wrote:

>All one's netmasks are a trivial default, no point in printing them.
>
>@@ -64,7 +64,7 @@ static void __NETMAP_print(const void *ip, const struct xt_entry_target *target,
> 	bits = xtables_ip6mask_to_cidr(&a);
> 	if (bits < 0)
> 		printf("/%s", xtables_ip6addr_to_numeric(&a));
>-	else
>+	else if (bits < sizeof(a) * 8)
> 		printf("/%d", bits);

I would rather see it stay.
- iproute2 also always prints the /128 suffix
- test parsers need not special case the absence of /128

>--- a/extensions/libxt_MARK.c
>@@ -242,7 +242,9 @@ static void mark_tg_save(const void *ip, const struct xt_entry_target *target)
> {
> 	const struct xt_mark_tginfo2 *info = (const void *)target->data;
> 
>-	printf(" --set-xmark 0x%x/0x%x", info->mark, info->mask);
>+	printf(" --set-xmark 0x%x", info->mark);
>+	if (info->mask != 0xffffffffU)
>+		printf("/0x%x", info->mask);

if (info->mask != UINT32_MAX)
Phil Sutter Oct. 6, 2022, 11:54 a.m. UTC | #2 
Hi,

On Thu, Oct 06, 2022 at 08:27:33AM +0200, Jan Engelhardt wrote:
> 
> On Thursday 2022-10-06 02:28, Phil Sutter wrote:
> 
> >All one's netmasks are a trivial default, no point in printing them.
> >
> >@@ -64,7 +64,7 @@ static void __NETMAP_print(const void *ip, const struct xt_entry_target *target,
> > 	bits = xtables_ip6mask_to_cidr(&a);
> > 	if (bits < 0)
> > 		printf("/%s", xtables_ip6addr_to_numeric(&a));
> >-	else
> >+	else if (bits < sizeof(a) * 8)
> > 		printf("/%d", bits);
> 
> I would rather see it stay.
> - iproute2 also always prints the /128 suffix
> - test parsers need not special case the absence of /128

I get your point. Screen-scraping is also not uncommon among iptables
users, so care has to be taken when "optimizing" output.

OTOH we're a bit inconsistent: xtables_ip(6)mask_to_numeric() explicitly
omits output if it would print "/32" or "/128".

Maybe I'll just leave the code as-is and adjust only the test cases
instead?

> >--- a/extensions/libxt_MARK.c
> >@@ -242,7 +242,9 @@ static void mark_tg_save(const void *ip, const struct xt_entry_target *target)
> > {
> > 	const struct xt_mark_tginfo2 *info = (const void *)target->data;
> > 
> >-	printf(" --set-xmark 0x%x/0x%x", info->mark, info->mask);
> >+	printf(" --set-xmark 0x%x", info->mark);
> >+	if (info->mask != 0xffffffffU)
> >+		printf("/0x%x", info->mask);
> 
> if (info->mask != UINT32_MAX)

ACK. I copied from mark_tg_print(), so that's a useful fix unrelated to
the discussion above.

Thanks, Phil
Jan Engelhardt Oct. 6, 2022, 7:01 p.m. UTC | #3 
On Thursday 2022-10-06 13:54, Phil Sutter wrote:

>> >-	else
>> >+	else if (bits < sizeof(a) * 8)
>> > 		printf("/%d", bits);
>> 
>> I would rather see it stay.
>> - iproute2 also always prints the /128 suffix
>> - test parsers need not special case the absence of /128
>>   [as in, when something reads e.g. iptables-save output]
>
>[...] OTOH we're a bit inconsistent: xtables_ip(6)mask_to_numeric() explicitly
>omits output if it would print "/32" or "/128".

We could consider just changing those two functions.
Any third-party program that interprets our output already has to deal
with a "/N" suffix in the same spot anyway.
diff mbox series

Patch

diff --git a/extensions/libip6t_NETMAP.c b/extensions/libip6t_NETMAP.c
index 579ed04ef0058..996c4059d3f11 100644
--- a/extensions/libip6t_NETMAP.c
+++ b/extensions/libip6t_NETMAP.c
@@ -64,7 +64,7 @@  static void __NETMAP_print(const void *ip, const struct xt_entry_target *target,
 	bits = xtables_ip6mask_to_cidr(&a);
 	if (bits < 0)
 		printf("/%s", xtables_ip6addr_to_numeric(&a));
-	else
+	else if (bits < sizeof(a) * 8)
 		printf("/%d", bits);
 }
 
diff --git a/extensions/libipt_NETMAP.c b/extensions/libipt_NETMAP.c
index f30615a357821..208d831009667 100644
--- a/extensions/libipt_NETMAP.c
+++ b/extensions/libipt_NETMAP.c
@@ -76,7 +76,7 @@  static void __NETMAP_print(const void *ip, const struct xt_entry_target *target,
 	bits = netmask2bits(a.s_addr);
 	if (bits < 0)
 		printf("/%s", xtables_ipaddr_to_numeric(&a));
-	else
+	else if (bits < sizeof(a) * 8)
 		printf("/%d", bits);
 }
 
diff --git a/extensions/libxt_CONNMARK.c b/extensions/libxt_CONNMARK.c
index 21e1091386294..4fa88854f7fd6 100644
--- a/extensions/libxt_CONNMARK.c
+++ b/extensions/libxt_CONNMARK.c
@@ -496,6 +496,12 @@  static void CONNMARK_init(struct xt_entry_target *t)
 	markinfo->mask = 0xffffffffUL;
 }
 
+static void print_mask_v1(const char *pfx, __u32 mask)
+{
+	if (mask != UINT32_MAX)
+		printf("%s0x%x", pfx, mask);
+}
+
 static void
 connmark_tg_save(const void *ip, const struct xt_entry_target *target)
 {
@@ -503,15 +509,18 @@  connmark_tg_save(const void *ip, const struct xt_entry_target *target)
 
 	switch (info->mode) {
 	case XT_CONNMARK_SET:
-		printf(" --set-xmark 0x%x/0x%x", info->ctmark, info->ctmask);
+		printf(" --set-xmark 0x%x", info->ctmark);
+		print_mask_v1("/", info->ctmask);
 		break;
 	case XT_CONNMARK_SAVE:
-		printf(" --save-mark --nfmask 0x%x --ctmask 0x%x",
-		       info->nfmask, info->ctmask);
+		printf(" --save-mark");
+		print_mask_v1(" --nfmask ", info->nfmask);
+		print_mask_v1(" --ctmask ", info->ctmask);
 		break;
 	case XT_CONNMARK_RESTORE:
-		printf(" --restore-mark --nfmask 0x%x --ctmask 0x%x",
-		       info->nfmask, info->ctmask);
+		printf(" --restore-mark");
+		print_mask_v1(" --nfmask ", info->nfmask);
+		print_mask_v1(" --ctmask ", info->ctmask);
 		break;
 	default:
 		printf(" ERROR: UNKNOWN CONNMARK MODE");
@@ -527,15 +536,18 @@  connmark_tg_save_v2(const void *ip, const struct xt_entry_target *target)
 
 	switch (info->mode) {
 	case XT_CONNMARK_SET:
-		printf(" --set-xmark 0x%x/0x%x", info->ctmark, info->ctmask);
+		printf(" --set-xmark 0x%x", info->ctmark);
+		print_mask_v1("/", info->ctmask);
 		break;
 	case XT_CONNMARK_SAVE:
-		printf(" --save-mark --nfmask 0x%x --ctmask 0x%x",
-		       info->nfmask, info->ctmask);
+		printf(" --save-mark");
+		print_mask_v1(" --nfmask ", info->nfmask);
+		print_mask_v1(" --ctmask ", info->ctmask);
 		break;
 	case XT_CONNMARK_RESTORE:
-		printf(" --restore-mark --nfmask 0x%x --ctmask 0x%x",
-		       info->nfmask, info->ctmask);
+		printf(" --restore-mark");
+		print_mask_v1(" --nfmask ", info->nfmask);
+		print_mask_v1(" --ctmask ", info->ctmask);
 		break;
 	default:
 		printf(" ERROR: UNKNOWN CONNMARK MODE");
diff --git a/extensions/libxt_CONNMARK.t b/extensions/libxt_CONNMARK.t
index 79a838fefaa14..1783f7a447195 100644
--- a/extensions/libxt_CONNMARK.t
+++ b/extensions/libxt_CONNMARK.t
@@ -2,6 +2,6 @@ 
 *mangle
 -j CONNMARK --restore-mark;=;OK
 -j CONNMARK --save-mark;=;OK
--j CONNMARK --save-mark --nfmask 0xfffffff --ctmask 0xffffffff;-j CONNMARK --save-mark;OK
--j CONNMARK --restore-mark --nfmask 0xfffffff --ctmask 0xffffffff;-j CONNMARK --restore-mark;OK
+-j CONNMARK --save-mark --nfmask 0xffffffff --ctmask 0xffffffff;-j CONNMARK --save-mark;OK
+-j CONNMARK --restore-mark --nfmask 0xffffffff --ctmask 0xffffffff;-j CONNMARK --restore-mark;OK
 -j CONNMARK;;FAIL
diff --git a/extensions/libxt_MARK.c b/extensions/libxt_MARK.c
index 1536563d0f4c7..36355f6645fbe 100644
--- a/extensions/libxt_MARK.c
+++ b/extensions/libxt_MARK.c
@@ -242,7 +242,9 @@  static void mark_tg_save(const void *ip, const struct xt_entry_target *target)
 {
 	const struct xt_mark_tginfo2 *info = (const void *)target->data;
 
-	printf(" --set-xmark 0x%x/0x%x", info->mark, info->mask);
+	printf(" --set-xmark 0x%x", info->mark);
+	if (info->mask != 0xffffffffU)
+		printf("/0x%x", info->mask);
 }
 
 static void mark_tg_arp_save(const void *ip, const struct xt_entry_target *target)
diff --git a/extensions/libxt_connlimit.c b/extensions/libxt_connlimit.c
index 118faea560f73..00cd0ba2f852c 100644
--- a/extensions/libxt_connlimit.c
+++ b/extensions/libxt_connlimit.c
@@ -152,13 +152,15 @@  static void connlimit_print6(const void *ip,
 static void connlimit_save4(const void *ip, const struct xt_entry_match *match)
 {
 	const struct xt_connlimit_info *info = (const void *)match->data;
+	unsigned int bits = count_bits4(info->v4_mask);
 	const int revision = match->u.user.revision;
 
 	if (info->flags & XT_CONNLIMIT_INVERT)
 		printf(" --connlimit-upto %u", info->limit);
 	else
 		printf(" --connlimit-above %u", info->limit);
-	printf(" --connlimit-mask %u", count_bits4(info->v4_mask));
+	if (bits != 32)
+		printf(" --connlimit-mask %u", bits);
 	if (revision >= 1) {
 		if (info->flags & XT_CONNLIMIT_DADDR)
 			printf(" --connlimit-daddr");
@@ -170,13 +172,15 @@  static void connlimit_save4(const void *ip, const struct xt_entry_match *match)
 static void connlimit_save6(const void *ip, const struct xt_entry_match *match)
 {
 	const struct xt_connlimit_info *info = (const void *)match->data;
+	unsigned int bits = count_bits6(info->v6_mask);
 	const int revision = match->u.user.revision;
 
 	if (info->flags & XT_CONNLIMIT_INVERT)
 		printf(" --connlimit-upto %u", info->limit);
 	else
 		printf(" --connlimit-above %u", info->limit);
-	printf(" --connlimit-mask %u", count_bits6(info->v6_mask));
+	if (bits != 128)
+		printf(" --connlimit-mask %u", bits);
 	if (revision >= 1) {
 		if (info->flags & XT_CONNLIMIT_DADDR)
 			printf(" --connlimit-daddr");
diff --git a/extensions/libxt_connlimit.t b/extensions/libxt_connlimit.t
index 23bba69474fed..8495b77fd2d2c 100644
--- a/extensions/libxt_connlimit.t
+++ b/extensions/libxt_connlimit.t
@@ -8,9 +8,9 @@ 
 -m connlimit --connlimit-above 4294967296 --connlimit-saddr;;FAIL
 -m connlimit --connlimit-above -1;;FAIL
 -m connlimit --connlimit-upto 1 --conlimit-above 1;;FAIL
--m connlimit --connlimit-above 10 --connlimit-saddr;-m connlimit --connlimit-above 10 --connlimit-mask 32 --connlimit-saddr;OK
--m connlimit --connlimit-above 10 --connlimit-daddr;-m connlimit --connlimit-above 10 --connlimit-mask 32 --connlimit-daddr;OK
+-m connlimit --connlimit-above 10 --connlimit-saddr;=;OK
+-m connlimit --connlimit-above 10 --connlimit-daddr;=;OK
 -m connlimit --connlimit-above 10 --connlimit-saddr --connlimit-daddr;;FAIL
--m connlimit --connlimit-above 10 --connlimit-mask 32 --connlimit-saddr;=;OK
--m connlimit --connlimit-above 10 --connlimit-mask 32 --connlimit-daddr;=;OK
+-m connlimit --connlimit-above 10 --connlimit-mask 32 --connlimit-saddr;-m connlimit --connlimit-above 10 --connlimit-saddr;OK
+-m connlimit --connlimit-above 10 --connlimit-mask 32 --connlimit-daddr;-m connlimit --connlimit-above 10 --connlimit-daddr;OK
 -m connlimit;;FAIL
diff --git a/extensions/libxt_recent.c b/extensions/libxt_recent.c
index 055ae35080346..659a91ba7b707 100644
--- a/extensions/libxt_recent.c
+++ b/extensions/libxt_recent.c
@@ -176,6 +176,29 @@  static void recent_check(struct xt_fcheck_call *cb)
 			"`--update' or `--remove'");
 }
 
+static void recent_print_mask(int family, const char *prefix,
+			      const union nf_inet_addr *mask)
+{
+	static const __u32 allones[4] = { -1, -1, -1, -1 };
+
+	switch(family) {
+	case NFPROTO_IPV4:
+		if (!memcmp(&mask->in, allones, sizeof(mask->in)))
+			return;
+
+		printf(" %s %s", prefix,
+		       xtables_ipaddr_to_numeric(&mask->in));
+		break;
+	case NFPROTO_IPV6:
+		if (!memcmp(&mask->in6, allones, sizeof(mask->in6)))
+			return;
+
+		printf(" %s %s", prefix,
+		       xtables_ip6addr_to_numeric(&mask->in6));
+		break;
+	}
+}
+
 static void recent_print(const void *ip, const struct xt_entry_match *match,
                          unsigned int family)
 {
@@ -205,16 +228,7 @@  static void recent_print(const void *ip, const struct xt_entry_match *match,
 	if (info->side == XT_RECENT_DEST)
 		printf(" side: dest");
 
-	switch(family) {
-	case NFPROTO_IPV4:
-		printf(" mask: %s",
-			xtables_ipaddr_to_numeric(&info->mask.in));
-		break;
-	case NFPROTO_IPV6:
-		printf(" mask: %s",
-			xtables_ip6addr_to_numeric(&info->mask.in6));
-		break;
-	}
+	recent_print_mask(family, "mask:", &info->mask);
 }
 
 static void recent_save(const void *ip, const struct xt_entry_match *match,
@@ -241,16 +255,7 @@  static void recent_save(const void *ip, const struct xt_entry_match *match,
 		printf(" --rttl");
 	printf(" --name %s",info->name);
 
-	switch(family) {
-	case NFPROTO_IPV4:
-		printf(" --mask %s",
-			xtables_ipaddr_to_numeric(&info->mask.in));
-		break;
-	case NFPROTO_IPV6:
-		printf(" --mask %s",
-			xtables_ip6addr_to_numeric(&info->mask.in6));
-		break;
-	}
+	recent_print_mask(family, "--mask", &info->mask);
 
 	if (info->side == XT_RECENT_SOURCE)
 		printf(" --rsource");
diff --git a/extensions/libxt_recent.t b/extensions/libxt_recent.t
index ce85b91bf9ac6..bccb8cecfd924 100644
--- a/extensions/libxt_recent.t
+++ b/extensions/libxt_recent.t
@@ -1,11 +1,11 @@ 
 :INPUT,FORWARD,OUTPUT
 -m recent --set;-m recent --set --name DEFAULT --rsource;OK
--m recent --rcheck --hitcount 8 --name foo --mask 255.255.255.255 --rsource;=;OK
--m recent --rcheck --hitcount 12 --name foo --mask 255.255.255.255 --rsource;=;OK
--m recent --update --rttl;-m recent --update --rttl --name DEFAULT --mask 255.255.255.255 --rsource;OK
+-m recent --rcheck --hitcount 8 --name foo --mask 255.255.255.255 --rsource;-m recent --rcheck --hitcount 8 --name foo --rsource;OK
+-m recent --rcheck --hitcount 12 --name foo --mask 255.255.255.255 --rsource;-m recent --rcheck --hitcount 12 --name foo --rsource;OK
+-m recent --update --rttl;-m recent --update --rttl --name DEFAULT --rsource;OK
 -m recent --set --rttl;;FAIL
 -m recent --rcheck --hitcount 999 --name foo --mask 255.255.255.255 --rsource;;FAIL
 # nonsensical, but all should load successfully:
--m recent --rcheck --hitcount 3 --name foo --mask 255.255.255.255 --rsource -m recent --rcheck --hitcount 4 --name foo --mask 255.255.255.255 --rsource;=;OK
--m recent --rcheck --hitcount 4 --name foo --mask 255.255.255.255 --rsource -m recent --rcheck --hitcount 4 --name foo --mask 255.255.255.255 --rsource;=;OK
--m recent --rcheck --hitcount 8 --name foo --mask 255.255.255.255 --rsource -m recent --rcheck --hitcount 12 --name foo --mask 255.255.255.255 --rsource;=;OK
+-m recent --rcheck --hitcount 3 --name foo --rsource -m recent --rcheck --hitcount 4 --name foo --rsource;=;OK
+-m recent --rcheck --hitcount 4 --name foo --rsource -m recent --rcheck --hitcount 4 --name foo --rsource;=;OK
+-m recent --rcheck --hitcount 8 --name foo --rsource -m recent --rcheck --hitcount 12 --name foo --rsource;=;OK