[PATCH nft 0/4] Output Flag Fixes

[PATCH nft 0/4] Output Flag Fixes

[Jeremy Sowden]

A handful of output-flag fixes: some missing documentation, a missing long
option, some missing Python methods and a superfluous assigment.

Jeremy Sowden (4):
  doc: add missing output flag documentation.
  py: add missing output flags.
  main: add missing `OPT_NUMERIC_PROTO` long option.
  main: remove duplicate output flag assignment.

 doc/libnftables.adoc |  6 ++++++
 py/nftables.py       | 35 +++++++++++++++++++++++++++++++++++
 src/main.c           |  5 ++++-
 3 files changed, 45 insertions(+), 1 deletion(-)

-- 
2.23.0

[PATCH nft 1/4] doc: add missing output flag documentation.

[Jeremy Sowden]

The documentation for NFT_CTX_OUTPUT_FLAG_NUMERIC_TIME and
NFT_CTX_OUTPUT_FLAG_NUMERIC_ALL is incomplete.  Add the missing bits.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 doc/libnftables.adoc | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/doc/libnftables.adoc b/doc/libnftables.adoc
index 8ce1196fd47e..ce4a361b84cc 100644
--- a/doc/libnftables.adoc
+++ b/doc/libnftables.adoc
@@ -90,6 +90,10 @@ enum {
         NFT_CTX_OUTPUT_NUMERIC_PROTO  = (1 << 7),
         NFT_CTX_OUTPUT_NUMERIC_PRIO   = (1 << 8),
         NFT_CTX_OUTPUT_NUMERIC_SYMBOL = (1 << 9),
+        NFT_CTX_OUTPUT_NUMERIC_TIME   = (1 << 10),
+        NFT_CTX_OUTPUT_NUMERIC_ALL    = (NFT_CTX_OUTPUT_NUMERIC_PROTO |
+                                         NFT_CTX_OUTPUT_NUMERIC_PRIO  |
+                                         NFT_CTX_OUTPUT_NUMERIC_TIME),
         NFT_CTX_OUTPUT_TERSE          = (1 << 11),
 };
 ----
@@ -122,6 +126,8 @@ NFT_CTX_OUTPUT_NUMERIC_PRIO::
 	Display base chain priority numerically.
 NFT_CTX_OUTPUT_NUMERIC_SYMBOL::
 	Display expression datatype as numeric value.
+NFT_CTX_OUTPUT_NUMERIC_TIME::
+	Display time, day and hour values in numeric format.
 NFT_CTX_OUTPUT_NUMERIC_ALL::
 	Display all numerically.
 NFT_CTX_OUTPUT_TERSE::
-- 
2.23.0

Re: [PATCH nft 1/4] doc: add missing output flag documentation.

[Pablo Neira Ayuso]

On Tue, Oct 22, 2019 at 09:58:52PM +0100, Jeremy Sowden wrote:
> The documentation for NFT_CTX_OUTPUT_FLAG_NUMERIC_TIME and
> NFT_CTX_OUTPUT_FLAG_NUMERIC_ALL is incomplete.  Add the missing bits.

Applied, thanks.

[PATCH nft 2/4] py: add missing output flags.

[Jeremy Sowden]

`terse` and `numeric_time` are missing from the `output_flags` dict.
Add them and getters and setters for them.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 py/nftables.py | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/py/nftables.py b/py/nftables.py
index 81e57567c802..48eb54fe232d 100644
--- a/py/nftables.py
+++ b/py/nftables.py
@@ -58,6 +58,8 @@ class Nftables:
         "numeric_proto":  (1 << 7),
         "numeric_prio":   (1 << 8),
         "numeric_symbol": (1 << 9),
+        "numeric_time":   (1 << 10),
+        "terse":          (1 << 11),
     }
 
     validator = None
@@ -305,6 +307,39 @@ class Nftables:
         """
         return self.__set_output_flag("numeric_symbol", val)
 
+    def get_numeric_time_output(self):
+        """Get current status of numeric times output flag.
+
+        Returns a boolean value indicating the status.
+        """
+        return self.__get_output_flag("numeric_time")
+
+    def set_numeric_time_output(self, val):
+        """Set numeric times output flag.
+
+        Accepts a boolean turning numeric representation of time values
+        in output either on or off.
+
+        Returns the previous value.
+        """
+        return self.__set_output_flag("numeric_time", val)
+
+    def get_terse_output(self):
+        """Get the current state of terse output.
+
+        Returns a boolean indicating whether terse output is active or not.
+        """
+        return self.__get_output_flag("terse")
+
+    def set_terse_output(self, val):
+        """Enable or disable terse output.
+
+        Accepts a boolean turning terse output either on or off.
+
+        Returns the previous value.
+        """
+        return self.__set_output_flag("terse", val)
+
     def get_debug(self):
         """Get currently active debug flags.
 
-- 
2.23.0

Re: [PATCH nft 2/4] py: add missing output flags.

[Pablo Neira Ayuso]

On Tue, Oct 22, 2019 at 09:58:53PM +0100, Jeremy Sowden wrote:
> `terse` and `numeric_time` are missing from the `output_flags` dict.
> Add them and getters and setters for them.

LGTM.

@Phil, is this fine with you? I let you decide on this.

BTW, would it make sense at some point to remove all the getter/setter
per option and use the setter/getter flags approach as in libnftables?

Thanks.

> Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
> ---
>  py/nftables.py | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/py/nftables.py b/py/nftables.py
> index 81e57567c802..48eb54fe232d 100644
> --- a/py/nftables.py
> +++ b/py/nftables.py
> @@ -58,6 +58,8 @@ class Nftables:
>          "numeric_proto":  (1 << 7),
>          "numeric_prio":   (1 << 8),
>          "numeric_symbol": (1 << 9),
> +        "numeric_time":   (1 << 10),
> +        "terse":          (1 << 11),
>      }
>  
>      validator = None
> @@ -305,6 +307,39 @@ class Nftables:
>          """
>          return self.__set_output_flag("numeric_symbol", val)
>  
> +    def get_numeric_time_output(self):
> +        """Get current status of numeric times output flag.
> +
> +        Returns a boolean value indicating the status.
> +        """
> +        return self.__get_output_flag("numeric_time")
> +
> +    def set_numeric_time_output(self, val):
> +        """Set numeric times output flag.
> +
> +        Accepts a boolean turning numeric representation of time values
> +        in output either on or off.
> +
> +        Returns the previous value.
> +        """
> +        return self.__set_output_flag("numeric_time", val)
> +
> +    def get_terse_output(self):
> +        """Get the current state of terse output.
> +
> +        Returns a boolean indicating whether terse output is active or not.
> +        """
> +        return self.__get_output_flag("terse")
> +
> +    def set_terse_output(self, val):
> +        """Enable or disable terse output.
> +
> +        Accepts a boolean turning terse output either on or off.
> +
> +        Returns the previous value.
> +        """
> +        return self.__set_output_flag("terse", val)
> +
>      def get_debug(self):
>          """Get currently active debug flags.
>  
> -- 
> 2.23.0
> 

Re: [PATCH nft 2/4] py: add missing output flags.

[Phil Sutter]

Hi,

On Wed, Oct 23, 2019 at 10:38:33PM +0200, Pablo Neira Ayuso wrote:
> On Tue, Oct 22, 2019 at 09:58:53PM +0100, Jeremy Sowden wrote:
> > `terse` and `numeric_time` are missing from the `output_flags` dict.
> > Add them and getters and setters for them.
> 
> LGTM.
> 
> @Phil, is this fine with you? I let you decide on this.

I just pushed it. Could you please update Patchwork? I'm not allowed to.

> BTW, would it make sense at some point to remove all the getter/setter
> per option and use the setter/getter flags approach as in libnftables?

Well, from a compat standpoint we can't remove them. The benefit of
those setter/getter methods is the clean interface (user's don't have to
memorize flag names) and the semantics of returning the old value. The
latter comes in handy when changing flags temporarily.

One could change the private __{g,s}et_output_flag() methods though and
make them similar to {g,s}et_debug() methods which probably resemble the
syntax you're looking for.

Cheers, Phil

Re: [PATCH nft 2/4] py: add missing output flags.

[Pablo Neira Ayuso]

On Thu, Oct 24, 2019 at 11:20:52AM +0200, Phil Sutter wrote:
> Hi,
> 
> On Wed, Oct 23, 2019 at 10:38:33PM +0200, Pablo Neira Ayuso wrote:
> > On Tue, Oct 22, 2019 at 09:58:53PM +0100, Jeremy Sowden wrote:
> > > `terse` and `numeric_time` are missing from the `output_flags` dict.
> > > Add them and getters and setters for them.
> > 
> > LGTM.
> > 
> > @Phil, is this fine with you? I let you decide on this.
> 
> I just pushed it. Could you please update Patchwork? I'm not allowed to.
> 
> > BTW, would it make sense at some point to remove all the getter/setter
> > per option and use the setter/getter flags approach as in libnftables?
> 
> Well, from a compat standpoint we can't remove them. The benefit of
> those setter/getter methods is the clean interface (user's don't have to
> memorize flag names) and the semantics of returning the old value. The
> latter comes in handy when changing flags temporarily.

Probably some transitioning? ie. add the generic set/get flag
interface. Update clients of this (Eric's code) to use. Leave the old
interfaces for a while there to make sure people have time to migrate.
Then remove them.

Anyway, I'm fine if you prefer this more verbose interface for python,
no issue.

> One could change the private __{g,s}et_output_flag() methods though and
> make them similar to {g,s}et_debug() methods which probably resemble the
> syntax you're looking for.

Hm, not sure what you mean.

Re: [PATCH nft 2/4] py: add missing output flags.

[Phil Sutter]

Hi,

On Thu, Oct 24, 2019 at 11:35:05AM +0200, Pablo Neira Ayuso wrote:
> On Thu, Oct 24, 2019 at 11:20:52AM +0200, Phil Sutter wrote:
> > Hi,
> > 
> > On Wed, Oct 23, 2019 at 10:38:33PM +0200, Pablo Neira Ayuso wrote:
> > > On Tue, Oct 22, 2019 at 09:58:53PM +0100, Jeremy Sowden wrote:
> > > > `terse` and `numeric_time` are missing from the `output_flags` dict.
> > > > Add them and getters and setters for them.
> > > 
> > > LGTM.
> > > 
> > > @Phil, is this fine with you? I let you decide on this.
> > 
> > I just pushed it. Could you please update Patchwork? I'm not allowed to.
> > 
> > > BTW, would it make sense at some point to remove all the getter/setter
> > > per option and use the setter/getter flags approach as in libnftables?
> > 
> > Well, from a compat standpoint we can't remove them. The benefit of
> > those setter/getter methods is the clean interface (user's don't have to
> > memorize flag names) and the semantics of returning the old value. The
> > latter comes in handy when changing flags temporarily.
> 
> Probably some transitioning? ie. add the generic set/get flag
> interface. Update clients of this (Eric's code) to use. Leave the old
> interfaces for a while there to make sure people have time to migrate.
> Then remove them.

Sounds good!

> Anyway, I'm fine if you prefer this more verbose interface for python,
> no issue.

Main goal was to keep the bit-fiddling away from users, but a method
which accepts an array of flag names (or flag values) and does the
binary OR'ing should serve fine, too.

> > One could change the private __{g,s}et_output_flag() methods though and
> > make them similar to {g,s}et_debug() methods which probably resemble the
> > syntax you're looking for.
> 
> Hm, not sure what you mean.

I was referring to get_debug() and set_debug() methods of class
Nftables. The getter returns an array of debug flag names, the setter
accepts either a single flag name/value or an array of those.

Cheers, Phil

Re: [PATCH nft 2/4] py: add missing output flags.

[Pablo Neira Ayuso]

On Thu, Oct 24, 2019 at 05:56:39PM +0200, Phil Sutter wrote:
> Hi,
> 
> On Thu, Oct 24, 2019 at 11:35:05AM +0200, Pablo Neira Ayuso wrote:
> > On Thu, Oct 24, 2019 at 11:20:52AM +0200, Phil Sutter wrote:
> > > Hi,
> > > 
> > > On Wed, Oct 23, 2019 at 10:38:33PM +0200, Pablo Neira Ayuso wrote:
> > > > On Tue, Oct 22, 2019 at 09:58:53PM +0100, Jeremy Sowden wrote:
> > > > > `terse` and `numeric_time` are missing from the `output_flags` dict.
> > > > > Add them and getters and setters for them.
> > > > 
> > > > LGTM.
> > > > 
> > > > @Phil, is this fine with you? I let you decide on this.
> > > 
> > > I just pushed it. Could you please update Patchwork? I'm not allowed to.
> > > 
> > > > BTW, would it make sense at some point to remove all the getter/setter
> > > > per option and use the setter/getter flags approach as in libnftables?
> > > 
> > > Well, from a compat standpoint we can't remove them. The benefit of
> > > those setter/getter methods is the clean interface (user's don't have to
> > > memorize flag names) and the semantics of returning the old value. The
> > > latter comes in handy when changing flags temporarily.
> > 
> > Probably some transitioning? ie. add the generic set/get flag
> > interface. Update clients of this (Eric's code) to use. Leave the old
> > interfaces for a while there to make sure people have time to migrate.
> > Then remove them.
> 
> Sounds good!
> 
> > Anyway, I'm fine if you prefer this more verbose interface for python,
> > no issue.
> 
> Main goal was to keep the bit-fiddling away from users, but a method
> which accepts an array of flag names (or flag values) and does the
> binary OR'ing should serve fine, too.

Good.

> > > One could change the private __{g,s}et_output_flag() methods though and
> > > make them similar to {g,s}et_debug() methods which probably resemble the
> > > syntax you're looking for.
> > 
> > Hm, not sure what you mean.
> 
> I was referring to get_debug() and set_debug() methods of class
> Nftables. The getter returns an array of debug flag names, the setter
> accepts either a single flag name/value or an array of those.

If that's the preferred way to do things in python, rather that using
flags like in C, that's fine with me.

Re: [PATCH nft 2/4] py: add missing output flags.

[Phil Sutter]

On Tue, Oct 22, 2019 at 09:58:53PM +0100, Jeremy Sowden wrote:
> `terse` and `numeric_time` are missing from the `output_flags` dict.
> Add them and getters and setters for them.
> 
> Signed-off-by: Jeremy Sowden <jeremy@azazel.net>

Applied, thanks!

[PATCH nft 3/4] main: add missing `OPT_NUMERIC_PROTO` long option.

[Jeremy Sowden]

The `options` array is missing an entry for `OPT_NUMERIC_PROTO`.  Add a
new option, `--numeric-protocol`, consistent with the documentation.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 src/main.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/main.c b/src/main.c
index ebd6d7c322d7..c1c404e14eeb 100644
--- a/src/main.c
+++ b/src/main.c
@@ -116,6 +116,10 @@ static const struct option options[] = {
 		.name		= "numeric-priority",
 		.val		= OPT_NUMERIC_PRIO,
 	},
+	{
+		.name		= "numeric-protocol",
+		.val		= OPT_NUMERIC_PROTO,
+	},
 	{
 		.name		= "numeric-time",
 		.val		= OPT_NUMERIC_TIME,
-- 
2.23.0

Re: [PATCH nft 3/4] main: add missing `OPT_NUMERIC_PROTO` long option.

[Pablo Neira Ayuso]

On Tue, Oct 22, 2019 at 09:58:54PM +0100, Jeremy Sowden wrote:
> The `options` array is missing an entry for `OPT_NUMERIC_PROTO`.  Add a
> new option, `--numeric-protocol`, consistent with the documentation.

Applied, thanks.

[PATCH nft 4/4] main: remove duplicate output flag assignment.

[Jeremy Sowden]

`NFT_CTX_OUTPUT_NUMERIC_TIME` is implicit in
`NFT_CTX_OUTPUT_NUMERIC_ALL`: there are is no need explicitly to OR it
into output_flags when `--numeric` is passed.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 src/main.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/main.c b/src/main.c
index c1c404e14eeb..a3b752384996 100644
--- a/src/main.c
+++ b/src/main.c
@@ -245,7 +245,6 @@ int main(int argc, char * const *argv)
 			break;
 		case OPT_NUMERIC:
 			output_flags |= NFT_CTX_OUTPUT_NUMERIC_ALL;
-			output_flags |= NFT_CTX_OUTPUT_NUMERIC_TIME;
 			break;
 		case OPT_STATELESS:
 			output_flags |= NFT_CTX_OUTPUT_STATELESS;
-- 
2.23.0

Re: [PATCH nft 4/4] main: remove duplicate output flag assignment.

[Pablo Neira Ayuso]

On Tue, Oct 22, 2019 at 09:58:55PM +0100, Jeremy Sowden wrote:
> `NFT_CTX_OUTPUT_NUMERIC_TIME` is implicit in
> `NFT_CTX_OUTPUT_NUMERIC_ALL`: there are is no need explicitly to OR it
> into output_flags when `--numeric` is passed.

Also applied, thanks.