[pmg-devel] [PATCH pmg-api 0/3] small improvments to pmgdb dump output

[pmg-devel] [PATCH pmg-api 0/3] small improvments to pmgdb dump output

[Stoiko Ivanov]

inspired by the patch from Dominik [0], I prepared the following patchset,
which hopefully improves the readability of the pmgdb dump output when
debugging.
As these dump-tool output UX questions are quite subjective would be
grateful for feedback from someone, who had the opportunity to use it for
debugging.

[0] https://lists.proxmox.com/pipermail/pmg-devel/2024-February/002690.html

Stoiko Ivanov (3):
  pmgdb: highlight active rules
  pmgdb: drop "found" prefixes for each rule and group
  pmgdb: add active parameter to dump

 src/PMG/CLI/pmgdb.pm | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

-- 
2.39.2

[pmg-devel] [PATCH pmg-api 1/3] pmgdb: highlight active rules

[Stoiko Ivanov]

by printing the ACTIVE in caps. For me this improves quickly skimming
through a ruleset (in a few support cases I wasted quite a bit of time
on the wrong spot - only to see that the rule in question is inactive
anyways)

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
 src/PMG/CLI/pmgdb.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/PMG/CLI/pmgdb.pm b/src/PMG/CLI/pmgdb.pm
index bb22965..87c8804 100644
--- a/src/PMG/CLI/pmgdb.pm
+++ b/src/PMG/CLI/pmgdb.pm
@@ -48,7 +48,7 @@ sub print_rule {
 	1 => 'out',
 	2 => 'in+out',
     };
-    my $active = $rule->{active} ? 'active' : 'inactive';
+    my $active = $rule->{active} ? 'ACTIVE' : 'inactive';
     my $dir = $direction->{$rule->{direction}};
     my $rulename = encode('UTF-8', $rule->{name});
 
-- 
2.39.2

[pmg-devel] [PATCH pmg-api 2/3] pmgdb: drop "found" prefixes for each rule and group

[Stoiko Ivanov]

conveys little information, still clutters the output.

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
 src/PMG/CLI/pmgdb.pm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/PMG/CLI/pmgdb.pm b/src/PMG/CLI/pmgdb.pm
index 87c8804..28e9583 100644
--- a/src/PMG/CLI/pmgdb.pm
+++ b/src/PMG/CLI/pmgdb.pm
@@ -52,7 +52,7 @@ sub print_rule {
     my $dir = $direction->{$rule->{direction}};
     my $rulename = encode('UTF-8', $rule->{name});
 
-    print "Found RULE $rule->{id} (prio: $rule->{priority}, $dir, $active): $rulename\n";
+    print "RULE $rule->{id} (prio: $rule->{priority}, $dir, $active): $rulename\n";
 
     my $print_group = sub {
 	my ($type, $og, $print_mode) = @_;
@@ -63,7 +63,7 @@ sub print_rule {
 	    my $invert = $og->{invert} // 0;
 	    $mode = " (and=$and, invert=$invert)";
 	}
-	print "  FOUND $type GROUP $og->{id}${mode}: $oname\n";
+	print "  $type group $og->{id}${mode}: $oname\n";
 	print_objects($ruledb, $og);
     };
 
-- 
2.39.2

[pmg-devel] [PATCH pmg-api 3/3] pmgdb: add active parameter to dump

[Stoiko Ivanov]

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
 src/PMG/CLI/pmgdb.pm | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/src/PMG/CLI/pmgdb.pm b/src/PMG/CLI/pmgdb.pm
index 28e9583..0ac3f82 100644
--- a/src/PMG/CLI/pmgdb.pm
+++ b/src/PMG/CLI/pmgdb.pm
@@ -39,10 +39,11 @@ sub print_objects {
 }
 
 sub print_rule {
-    my ($ruledb, $rule) = @_;
+    my ($ruledb, $rule, $active_only) = @_;
 
     $ruledb->load_rule_attributes($rule);
 
+    return if !$rule->{active} && $active_only;
     my $direction = {
 	0 => 'in',
 	1 => 'out',
@@ -112,7 +113,14 @@ __PACKAGE__->register_method ({
     description => "Print the PMG rule database.",
     parameters => {
 	additionalProperties => 0,
-	properties => {},
+	properties => {
+	    active => {
+		type => 'boolean',
+		description => "Print only active rules",
+		optional => 1,
+		default => 0,
+	    },
+	},
     },
     returns => { type => 'null'},
     code => sub {
@@ -124,7 +132,7 @@ __PACKAGE__->register_method ({
 	my $rules = $ruledb->load_rules();
 
 	foreach my $rule (@$rules) {
-	    print_rule($ruledb, $rule);
+	    print_rule($ruledb, $rule, $param->{active});
 	}
 
 	$ruledb->close();
-- 
2.39.2

Re: [pmg-devel] [PATCH pmg-api 0/3] small improvments to pmgdb dump output

[Friedrich Weber]

All three patches look good to me!

* 1/3 is nice. I've also spent some time analyzing inactive rules, I'd
expect the all-caps active flag to help here.
* 2/3 is nice as well.
* 3/3 sounds potentially useful! Wouldn't pass this flag when generating
the report though, as I'd like to see active and inactive rules there.

Tested-by: Friedrich Weber <f.weber@proxmox.com>

On 22/02/2024 22:06, Stoiko Ivanov wrote:
> inspired by the patch from Dominik [0], I prepared the following patchset,
> which hopefully improves the readability of the pmgdb dump output when
> debugging.
> As these dump-tool output UX questions are quite subjective would be
> grateful for feedback from someone, who had the opportunity to use it for
> debugging.
> 
> [0] https://lists.proxmox.com/pipermail/pmg-devel/2024-February/002690.html
> 
> Stoiko Ivanov (3):
>   pmgdb: highlight active rules
>   pmgdb: drop "found" prefixes for each rule and group
>   pmgdb: add active parameter to dump
> 
>  src/PMG/CLI/pmgdb.pm | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 

Re: [pmg-devel] [PATCH pmg-api 0/3] small improvments to pmgdb dump output

[Stoiko Ivanov]

On Fri, 23 Feb 2024 11:40:35 +0100
Friedrich Weber <f.weber@proxmox.com> wrote:

> All three patches look good to me!
> 
> * 1/3 is nice. I've also spent some time analyzing inactive rules, I'd
> expect the all-caps active flag to help here.
> * 2/3 is nice as well.
> * 3/3 sounds potentially useful! Wouldn't pass this flag when generating
> the report though, as I'd like to see active and inactive rules there.
yes - I agree with that - it's something we can ask users to run
explicitly in case their ruleset warrants it


> 
> Tested-by: Friedrich Weber <f.weber@proxmox.com>
> 
> On 22/02/2024 22:06, Stoiko Ivanov wrote:
> > inspired by the patch from Dominik [0], I prepared the following patchset,
> > which hopefully improves the readability of the pmgdb dump output when
> > debugging.
> > As these dump-tool output UX questions are quite subjective would be
> > grateful for feedback from someone, who had the opportunity to use it for
> > debugging.
> > 
> > [0] https://lists.proxmox.com/pipermail/pmg-devel/2024-February/002690.html
> > 
> > Stoiko Ivanov (3):
> >   pmgdb: highlight active rules
> >   pmgdb: drop "found" prefixes for each rule and group
> >   pmgdb: add active parameter to dump
> > 
> >  src/PMG/CLI/pmgdb.pm | 20 ++++++++++++++------
> >  1 file changed, 14 insertions(+), 6 deletions(-)
> >   

Re: [pmg-devel] [PATCH pmg-api 0/3] small improvments to pmgdb dump output

[Alexander Zeidler]

On Thu, 2024-02-22 at 22:06 +0100, Stoiko Ivanov wrote:
> inspired by the patch from Dominik [0], I prepared the following patchset,
> which hopefully improves the readability of the pmgdb dump output when
> debugging.
> As these dump-tool output UX questions are quite subjective would be
> grateful for feedback from someone, who had the opportunity to use it for
> debugging.
Just tested, definitely an improvement in my opinion, thanks!
> 
> [0] https://lists.proxmox.com/pipermail/pmg-devel/2024-February/002690.html
> 
> Stoiko Ivanov (3):
>   pmgdb: highlight active rules
>   pmgdb: drop "found" prefixes for each rule and group
>   pmgdb: add active parameter to dump
> 
>  src/PMG/CLI/pmgdb.pm | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 

[pmg-devel] applied-series: [PATCH pmg-api 0/3] small improvments to pmgdb dump output

[Thomas Lamprecht]

Am 22/02/2024 um 22:06 schrieb Stoiko Ivanov:
> inspired by the patch from Dominik [0], I prepared the following patchset,
> which hopefully improves the readability of the pmgdb dump output when
> debugging.
> As these dump-tool output UX questions are quite subjective would be
> grateful for feedback from someone, who had the opportunity to use it for
> debugging.
> 
> [0] https://lists.proxmox.com/pipermail/pmg-devel/2024-February/002690.html
> 
> Stoiko Ivanov (3):
>   pmgdb: highlight active rules
>   pmgdb: drop "found" prefixes for each rule and group
>   pmgdb: add active parameter to dump
> 
>  src/PMG/CLI/pmgdb.pm | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 


applied series, with Friedrich's and Alexander's T-b, thanks!

Re: [pmg-devel] [PATCH pmg-api 3/3] pmgdb: add active parameter to dump

[Thomas Lamprecht]

Am 22/02/2024 um 22:06 schrieb Stoiko Ivanov:
> Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> ---
>  src/PMG/CLI/pmgdb.pm | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/src/PMG/CLI/pmgdb.pm b/src/PMG/CLI/pmgdb.pm
> index 28e9583..0ac3f82 100644
> --- a/src/PMG/CLI/pmgdb.pm
> +++ b/src/PMG/CLI/pmgdb.pm
> @@ -39,10 +39,11 @@ sub print_objects {
>  }
>  
>  sub print_rule {
> -    my ($ruledb, $rule) = @_;
> +    my ($ruledb, $rule, $active_only) = @_;
>  
>      $ruledb->load_rule_attributes($rule);
>  
> +    return if !$rule->{active} && $active_only;
>      my $direction = {
>  	0 => 'in',
>  	1 => 'out',
> @@ -112,7 +113,14 @@ __PACKAGE__->register_method ({
>      description => "Print the PMG rule database.",
>      parameters => {
>  	additionalProperties => 0,
> -	properties => {},
> +	properties => {
> +	    active => {
> +		type => 'boolean',
> +		description => "Print only active rules",
> +		optional => 1,
> +		default => 0,
> +	    },
> +	},
>      },
>      returns => { type => 'null'},
>      code => sub {
> @@ -124,7 +132,7 @@ __PACKAGE__->register_method ({
>  	my $rules = $ruledb->load_rules();
>  
>  	foreach my $rule (@$rules) {
> -	    print_rule($ruledb, $rule);
> +	    print_rule($ruledb, $rule, $param->{active});
>  	}
>  
>  	$ruledb->close();


If I call this with active set to disabled, i.e. as `pmgdb dump --active 0`, then
I still get the active rules, which feels rather a bit wrong.

IMO this could be interpreted as ternary:

1. undef (not passed) -> both
2. true (--active[=1]) -> active only
3. false (--active=0) -> incative only

Re: [pmg-devel] [PATCH pmg-api 3/3] pmgdb: add active parameter to dump

[Stoiko Ivanov]

Thanks for the suggestion - comment in-line:

On Mon, 26 Feb 2024 20:11:28 +0100
Thomas Lamprecht <t.lamprecht@proxmox.com> wrote:

>..snip..
> 
> If I call this with active set to disabled, i.e. as `pmgdb dump --active 0`, then
> I still get the active rules, which feels rather a bit wrong.
> 
> IMO this could be interpreted as ternary:
> 
> 1. undef (not passed) -> both
> 2. true (--active[=1]) -> active only
> 3. false (--active=0) -> incative only

this does break how booleans behave across most of our CLI tools (afaict).
can be fine, especially with this particular call, which is mostly for
debugging.
Alternatively - why not make this a regular enum (--rules
<all|active|inactive>, defaulting to all)?

Re: [pmg-devel] [PATCH pmg-api 3/3] pmgdb: add active parameter to dump

[Thomas Lamprecht]

Am 27/02/2024 um 13:53 schrieb Stoiko Ivanov:
> On Mon, 26 Feb 2024 20:11:28 +0100
> Thomas Lamprecht <t.lamprecht@proxmox.com> wrote:
>> If I call this with active set to disabled, i.e. as `pmgdb dump --active 0`, then
>> I still get the active rules, which feels rather a bit wrong.
>>
>> IMO this could be interpreted as ternary:
>>
>> 1. undef (not passed) -> both
>> 2. true (--active[=1]) -> active only
>> 3. false (--active=0) -> incative only
> 
> this does break how booleans behave across most of our CLI tools (afaict).

I did not check, but I think we hardly have a very strong coherent behavior
here, but yeah, a tri-state behavior for a boolean flag isn't fully ideal
either.

> can be fine, especially with this particular call, which is mostly for
> debugging.
> Alternatively - why not make this a regular enum (--rules
> <all|active|inactive>, defaulting to all)?

That would be more explicit and avoid all misinterpretation potential, i.e.,
more than fine with me.