[pve-devel] [PATCH manager 1/3] pvesubscription: add missing return statement

[pve-devel] [PATCH manager 1/3] pvesubscription: add missing return statement

[Alexander Zeidler]

Signed-off-by: Alexander Zeidler <a.zeidler@proxmox.com>
---
 PVE/CLI/pvesubscription.pm | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/PVE/CLI/pvesubscription.pm b/PVE/CLI/pvesubscription.pm
index 2eb26cb4..2fd641d0 100755
--- a/PVE/CLI/pvesubscription.pm
+++ b/PVE/CLI/pvesubscription.pm
@@ -47,6 +47,8 @@ __PACKAGE__->register_method({
 	PVE::API2::Subscription::check_key($info->{key}, PVE::API2::Subscription::get_sockets());
 
 	PVE::API2::Subscription::write_etc_subscription($info);
+
+	return undef;
 }});
 
 our $cmddef = {
-- 
2.39.2

[pve-devel] [PATCH manager 2/3] pvesubscription: update set-offline-key description

[Alexander Zeidler]

and point users to proxmox-offline-mirror-helper

Signed-off-by: Alexander Zeidler <a.zeidler@proxmox.com>
---
 PVE/CLI/pvesubscription.pm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/PVE/CLI/pvesubscription.pm b/PVE/CLI/pvesubscription.pm
index 2fd641d0..f1c11289 100755
--- a/PVE/CLI/pvesubscription.pm
+++ b/PVE/CLI/pvesubscription.pm
@@ -25,11 +25,12 @@ __PACKAGE__->register_method({
     name => 'set_offline_key',
     path => 'set_offline_key',
     method => 'POST',
-    description => "(Internal use only!) Set a signed subscription info blob as offline key",
+    description => "Internal use only! To set an offline key, use the package proxmox-offline-mirror-helper instead.",
     parameters => {
 	additionalProperties => 0,
 	properties => {
 	    data => {
+		description => "A signed subscription info blob",
 		type => "string",
 	    },
 	},
-- 
2.39.2

Re: [pve-devel] [PATCH manager 2/3] pvesubscription: update set-offline-key description

[Thomas Lamprecht]

Am 09/01/2024 um 15:23 schrieb Alexander Zeidler:
> and point users to proxmox-offline-mirror-helper

the change is fine, but having any background/reason/why here could be
still nice, I never saw a commit with to much references or reasons
state for why a change was done, and having those can definitively
help when looking at that in the future – as in contrast to the
"what/how it changed" (which basically is encoded in the diff itself)
this might not be recoverable in the future.

For any patches documenting the reason and background is most of
the time the important thing to do in the commit message.

Especially for small/docs patches the what/how can be nice for
when skimming the `git log` wihtout changes visible, but it's
definitively a bit less important in the commit message body.

Re: [pve-devel] [PATCH manager 2/3] pvesubscription: update set-offline-key description

[Alexander Zeidler]

On Wed, 2024-01-10 at 10:41 +0100, Thomas Lamprecht wrote:
> Am 09/01/2024 um 15:23 schrieb Alexander Zeidler:
> > and point users to proxmox-offline-mirror-helper
> 
> the change is fine, but having any background/reason/why here could be
> still nice,
Thanks! Since a customer was used to pvesubscription, he thought he had
to use it for POM as well (even when marked as internal only). He was
apparently not aware of proxmox-offline-mirror-helper.

[pve-devel] applied: [PATCH manager 2/3] pvesubscription: update set-offline-key description

[Thomas Lamprecht]

Am 09/01/2024 um 15:23 schrieb Alexander Zeidler:
> and point users to proxmox-offline-mirror-helper
> 
> Signed-off-by: Alexander Zeidler <a.zeidler@proxmox.com>
> ---
>  PVE/CLI/pvesubscription.pm | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
>

applied, thanks!

[pve-devel] [PATCH manager 3/3] report: format iptables output for readability

[Alexander Zeidler]

Signed-off-by: Alexander Zeidler <a.zeidler@proxmox.com>
---
 PVE/Report.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/PVE/Report.pm b/PVE/Report.pm
index 10b28c79..9c46aa9b 100644
--- a/PVE/Report.pm
+++ b/PVE/Report.pm
@@ -85,7 +85,7 @@ my $init_report_cmds = sub {
 	    cmds => [
 		sub { dir2text('/etc/pve/firewall/', '.*fw') },
 		'cat /etc/pve/local/host.fw',
-		'iptables-save -c',
+		'iptables-save -c | column -t -l4 -o" "',
 	    ],
 	},
 	cluster => {
-- 
2.39.2

[pve-devel] applied: [PATCH manager 3/3] report: format iptables output for readability

[Thomas Lamprecht]

Am 09/01/2024 um 15:23 schrieb Alexander Zeidler:
> Signed-off-by: Alexander Zeidler <a.zeidler@proxmox.com>
> ---
>  PVE/Report.pm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
>

applied this one, thanks!

It could have been a standalone patch though, as it's not really
related to the other ones.

Re: [pve-devel] [PATCH manager 1/3] pvesubscription: add missing return statement

[Thomas Lamprecht]

any reason this is relevant you might want to add to the commit
message here?

Am 09/01/2024 um 15:23 schrieb Alexander Zeidler:
> Signed-off-by: Alexander Zeidler <a.zeidler@proxmox.com>
> ---
>  PVE/CLI/pvesubscription.pm | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/PVE/CLI/pvesubscription.pm b/PVE/CLI/pvesubscription.pm
> index 2eb26cb4..2fd641d0 100755
> --- a/PVE/CLI/pvesubscription.pm
> +++ b/PVE/CLI/pvesubscription.pm
> @@ -47,6 +47,8 @@ __PACKAGE__->register_method({
>  	PVE::API2::Subscription::check_key($info->{key}, PVE::API2::Subscription::get_sockets());
>  
>  	PVE::API2::Subscription::write_etc_subscription($info);
> +
> +	return undef;

simple

return;

is slightly preferred for returning undef, as there is a very rare, but
 nasty bug one can hit with returning undef.
I.e., if the call site forces list-context and checks if something is
returned then the `return` works but return undef returns a single undef
element in a (defined) list, thus causing a false-positive fora check like
`if (my @foo = bar()) { ... }`.

As said, this is rather seldom and forced-list context is not that often
used so we do not need to clean up existing code, but for new one it makes
sense to default to a plain `return;` in such cases.

>  }});
>  
>  our $cmddef = {

Re: [pve-devel] [PATCH manager 1/3] pvesubscription: add missing return statement

[Alexander Zeidler]

On Wed, 2024-01-10 at 10:29 +0100, Thomas Lamprecht wrote:
> any reason this is relevant you might want to add to the commit
> message here?
to avoid a failing null check and its error message. This confused
users since the activation was successful anyway.


> simple
> 
> return;
> 
> is slightly preferred for returning undef
I considered using it, but then saw the established use of undef.
Thanks!

Re: [pve-devel] [PATCH manager 1/3] pvesubscription: add missing return statement

[Alexander Zeidler]

On Wed, 2024-01-10 at 11:59 +0100, Alexander Zeidler wrote:
> On Wed, 2024-01-10 at 10:29 +0100, Thomas Lamprecht wrote:
> > any reason this is relevant you might want to add to the commit
> > message here?
> to avoid a failing null check and its error message. This confused
> users since the activation was successful anyway.
> 
> 
> > simple
> > 
> > return;
> > 
> > is slightly preferred for returning undef
> I considered using it, but then saw the established use of undef.
> Thanks!
> 
v2:
https://lists.proxmox.com/pipermail/pve-devel/2024-January/061506.html