diff mbox

[libnftnl,1/2] src/rule: Removed mandatory attribute printing in rules

Message ID 20140309125959.23270.43422.stgit@Ph0enix
State Superseded
Headers show

Commit Message

Alvaro Neira March 9, 2014, 12:59 p.m. UTC 
From: Álvaro Neira Ayuso <alvaroneay@gmail.com>

Before this patch, the program tried to print some attribute
that maybe the user hasn't defined for printing. We can't
assume that the user want to print some attribute that we have put
mandatory in the rules. Example:

Before this patch, it's mandatory have a rule with family
and this is the output:

{"rule":{"family":"ip","handle":4...
<rule><family>ip</family><handle>4</handle>...

Now, we can print rule without some attribute:

{"rule":{"handle":4...
<rule><handle>4</handle>...

Signed-off-by: Alvaro Neira Ayuso <alvaroneay@gmail.com>
---
 src/rule.c |   92 ++++++++++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 79 insertions(+), 13 deletions(-)


--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Arturo Borrero March 9, 2014, 1:14 p.m. UTC | #1 
On 9 March 2014 13:59, Alvaro Neira Ayuso <alvaroneay@gmail.com> wrote:
> From: Álvaro Neira Ayuso <alvaroneay@gmail.com>
>
> Before this patch, the program tried to print some attribute
> that maybe the user hasn't defined for printing. We can't
> assume that the user want to print some attribute that we have put
> mandatory in the rules. Example:
>
> Before this patch, it's mandatory have a rule with family
> and this is the output:
>
> {"rule":{"family":"ip","handle":4...
> <rule><family>ip</family><handle>4</handle>...
>
> Now, we can print rule without some attribute:
>
> {"rule":{"handle":4...
> <rule><handle>4</handle>...
>
> Signed-off-by: Alvaro Neira Ayuso <alvaroneay@gmail.com>
> ---

Hi there!

These attributes are no longer mandatory? What is the idea behind this?

regards.
Arturo Borrero March 11, 2014, 7:59 p.m. UTC | #2 
Hi Alvaro,

I found a small issue in your patch, see below.

Except for this, the patch looks good to me.

On 9 March 2014 13:59, Alvaro Neira Ayuso <alvaroneay@gmail.com> wrote:
> @@ -865,15 +904,42 @@ static int nft_rule_snprintf_xml(char *buf, size_t size, struct nft_rule *r,
>         return offset;
>  }
>
> -static int nft_rule_snprintf_default(char *buf, size_t size, struct nft_rule *r,
> +static int nft_rule_snprintf_default(char *buf, size_t size, struct nft_rule *r,
>                                      uint32_t type, uint32_t flags)
>  {
>         struct nft_rule_expr *expr;
>         int ret, len = size, offset = 0, i;
>
> -       ret = snprintf(buf, len, "%s %s %s %"PRIu64" %"PRIu64"\n",
> -                       nft_family2str(r->family), r->table, r->chain,
> -                       r->handle, r->position);
> +       if (r->flags & (1 << NFT_RULE_ATTR_FAMILY)) {
> +               ret = snprintf(buf+offset, len, "%s ",
> +                              nft_family2str(r->family));
> +               SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
> +       }
> +
> +       if (r->flags & (1 << NFT_RULE_ATTR_TABLE)) {
> +               ret = snprintf(buf+offset, len, "%s ",
> +                              r->table);
> +               SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
> +       }
> +
> +       if (r->flags & (1 << NFT_RULE_ATTR_CHAIN)) {
> +               ret = snprintf(buf+offset, len, "%s ",
> +                              r->chain);
> +               SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
> +       }
> +       if (r->flags & (1 << NFT_RULE_ATTR_TABLE)) {
> +               ret = snprintf(buf+offset, len, "%llu ",
> +                              (unsigned long long)r->handle);
> +               SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
> +       }

Here you check the table attr but print handle.

Regards.
Pablo Neira Ayuso March 12, 2014, 12:50 p.m. UTC | #3 
On Sun, Mar 09, 2014 at 02:14:23PM +0100, Arturo Borrero Gonzalez wrote:
> On 9 March 2014 13:59, Alvaro Neira Ayuso <alvaroneay@gmail.com> wrote:
> > From: Álvaro Neira Ayuso <alvaroneay@gmail.com>
> >
> > Before this patch, the program tried to print some attribute
> > that maybe the user hasn't defined for printing. We can't
> > assume that the user want to print some attribute that we have put
> > mandatory in the rules. Example:
> >
> > Before this patch, it's mandatory have a rule with family
> > and this is the output:
> >
> > {"rule":{"family":"ip","handle":4...
> > <rule><family>ip</family><handle>4</handle>...
> >
> > Now, we can print rule without some attribute:
> >
> > {"rule":{"handle":4...
> > <rule><handle>4</handle>...
> >
> > Signed-off-by: Alvaro Neira Ayuso <alvaroneay@gmail.com>
> > ---
> 
> Hi there!
> 
> These attributes are no longer mandatory? What is the idea behind this?

I think we already discussed this time ago.

I'd like that the parsers don't enforce the occurrence of any
attribute, we should just let the kernel bail out if the configuration
that the user provides doesn't make sense.

This provides us more flexibility in case that we change any aspect
from the kernel side.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/src/rule.c b/src/rule.c
index 162a0a1..f0cafd3 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -775,12 +775,32 @@  static int nft_rule_snprintf_json(char *buf, size_t size, struct nft_rule *r,
 	int ret, len = size, offset = 0;
 	struct nft_rule_expr *expr;
 
-	ret = snprintf(buf, len, "{\"rule\":{\"family\":\"%s\",\"table\":\"%s\","
-				 "\"chain\":\"%s\",\"handle\":%llu,",
-		       nft_family2str(r->family), r->table, r->chain,
-		       (unsigned long long)r->handle);
+	ret = snprintf(buf, len, "{\"rule\":{");
 	SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
 
+	if (r->flags & (1 << NFT_RULE_ATTR_FAMILY)) {
+		ret = snprintf(buf+offset, len, "\"family\":\"%s\",",
+			       nft_family2str(r->family));
+		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+	}
+
+	if (r->flags & (1 << NFT_RULE_ATTR_TABLE)) {
+		ret = snprintf(buf+offset, len, "\"table\":\"%s\",",
+			       r->table);
+		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+	}
+
+	if (r->flags & (1 << NFT_RULE_ATTR_CHAIN)) {
+		ret = snprintf(buf+offset, len, "\"chain\":\"%s\",",
+			       r->chain);
+		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+	}
+	if (r->flags & (1 << NFT_RULE_ATTR_HANDLE)) {
+		ret = snprintf(buf+offset, len, "\"handle\":%llu,",
+			       (unsigned long long)r->handle);
+		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+	}
+
 	if (r->flags & (1 << NFT_RULE_ATTR_COMPAT_PROTO) ||
 	    r->flags & (1 << NFT_RULE_ATTR_COMPAT_FLAGS)) {
 		ret = snprintf(buf+offset, len, "\"compat_flags\":%u,"
@@ -824,13 +844,32 @@  static int nft_rule_snprintf_xml(char *buf, size_t size, struct nft_rule *r,
 	int ret, len = size, offset = 0;
 	struct nft_rule_expr *expr;
 
-	ret = snprintf(buf, len, "<rule><family>%s</family>"
-		       "<table>%s</table><chain>%s</chain>"
-		       "<handle>%llu</handle>",
-		       nft_family2str(r->family), r->table, r->chain,
-		       (unsigned long long)r->handle);
+	ret = snprintf(buf, len, "<rule>");
 	SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
 
+	if (r->flags & (1 << NFT_RULE_ATTR_FAMILY)) {
+		ret = snprintf(buf+offset, len, "<family>%s</family>",
+			       nft_family2str(r->family));
+		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+	}
+
+	if (r->flags & (1 << NFT_RULE_ATTR_TABLE)) {
+		ret = snprintf(buf+offset, len, "<table>%s</table>",
+			       r->table);
+		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+	}
+
+	if (r->flags & (1 << NFT_RULE_ATTR_CHAIN)) {
+		ret = snprintf(buf+offset, len, "<chain>%s</chain>",
+			       r->chain);
+		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+	}
+	if (r->flags & (1 << NFT_RULE_ATTR_HANDLE)) {
+		ret = snprintf(buf+offset, len, "<handle>%llu</handle>",
+			       (unsigned long long)r->handle);
+		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+	}
+
 	if (r->compat.flags != 0 || r->compat.proto != 0) {
 		ret = snprintf(buf+offset, len,
 			       "<compat_flags>%u</compat_flags>"
@@ -865,15 +904,42 @@  static int nft_rule_snprintf_xml(char *buf, size_t size, struct nft_rule *r,
 	return offset;
 }
 
-static int nft_rule_snprintf_default(char *buf, size_t size, struct nft_rule *r, 
+static int nft_rule_snprintf_default(char *buf, size_t size, struct nft_rule *r,
 				     uint32_t type, uint32_t flags)
 {
 	struct nft_rule_expr *expr;
 	int ret, len = size, offset = 0, i;
 
-	ret = snprintf(buf, len, "%s %s %s %"PRIu64" %"PRIu64"\n",
-			nft_family2str(r->family), r->table, r->chain,
-			r->handle, r->position);
+	if (r->flags & (1 << NFT_RULE_ATTR_FAMILY)) {
+		ret = snprintf(buf+offset, len, "%s ",
+			       nft_family2str(r->family));
+		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+	}
+
+	if (r->flags & (1 << NFT_RULE_ATTR_TABLE)) {
+		ret = snprintf(buf+offset, len, "%s ",
+			       r->table);
+		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+	}
+
+	if (r->flags & (1 << NFT_RULE_ATTR_CHAIN)) {
+		ret = snprintf(buf+offset, len, "%s ",
+			       r->chain);
+		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+	}
+	if (r->flags & (1 << NFT_RULE_ATTR_TABLE)) {
+		ret = snprintf(buf+offset, len, "%llu ",
+			       (unsigned long long)r->handle);
+		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+	}
+
+	if (r->flags & (1 << NFT_RULE_ATTR_POSITION)) {
+		ret = snprintf(buf+offset, len, "%llu ",
+			       (unsigned long long)r->position);
+		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+	}
+
+	ret = snprintf(buf+offset, len, "\n");
 	SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
 
 	list_for_each_entry(expr, &r->expr_list, head) {