[pve-devel] [PATCH pve-common] cli: fix #6762: only copy properties if defined

[pve-devel] [PATCH pve-common] cli: fix #6762: only copy properties if defined

[Nicolas Frey]

Adds a defined check to the copy, as to not result in the bugfixes
reported error when double tabbing on `pveceph status`.

Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=6762
Signed-off-by: Nicolas Frey <n.frey@proxmox.com>
---
 src/PVE/CLIHandler.pm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/PVE/CLIHandler.pm b/src/PVE/CLIHandler.pm
index 89cb7b7..cdd71c7 100644
--- a/src/PVE/CLIHandler.pm
+++ b/src/PVE/CLIHandler.pm
@@ -455,7 +455,8 @@ my $print_bash_completion = sub {
 
     my $info = $class->map_method_by_name($name);
 
-    my $prop = { %{ $info->{parameters}->{properties} } }; # copy
+    my $prop = { %{ $info->{parameters}->{properties} } }
+        if defined $info->{parameters}->{properties}; # copy
     $prop = { %$prop, %$formatter_properties } if $formatter_properties;
 
     my $print_parameter_completion = sub {
-- 
2.47.3


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH pve-common] cli: fix #6762: only copy properties if defined

[Thomas Lamprecht]

Am 07.10.25 um 15:56 schrieb Nicolas Frey:
> Adds a defined check to the copy, as to not result in the bugfixes
> reported error when double tabbing on `pveceph status`.
Feel encouraged to include errors in verbatim in commit message,
while we own bugzilla.proxmox.com and thus it's unlikely that it
will be gone without notice or migration it's still always nice to
keep git commits self-sufficient w.r.t core information. Makes it
also easier to find a commit if one just searches for the error
message.

> 
> Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=6762
> Signed-off-by: Nicolas Frey <n.frey@proxmox.com>
> ---
>  src/PVE/CLIHandler.pm | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/PVE/CLIHandler.pm b/src/PVE/CLIHandler.pm
> index 89cb7b7..cdd71c7 100644
> --- a/src/PVE/CLIHandler.pm
> +++ b/src/PVE/CLIHandler.pm
> @@ -455,7 +455,8 @@ my $print_bash_completion = sub {
>  
>      my $info = $class->map_method_by_name($name);
>  
> -    my $prop = { %{ $info->{parameters}->{properties} } }; # copy
> +    my $prop = { %{ $info->{parameters}->{properties} } }
> +        if defined $info->{parameters}->{properties}; # copy

You cannot know without having lots of perl experience, or having had
the bad luck of debugging this yourself early on, but a conditional
declaration of a variable is Real Bad™ perl, as it will retain the
value of the last time the condition held true for the case it's false
now.

See the NOTE at the end of the "Statement Modifiers" [0] section for
details.

[0]: https://perldoc.perl.org/perlsyn#Statement-Modifiers

Safe alternatives are:

my $prop;
$prop = { $info->{parameters}->{properties}->%* } if defined($info->{parameters}->{properties});

Similar, but with normal if block, which can be combined with adding a
intermediate variable.

my $prop;
if (defined(my $properties = $info->{parameters}->{properties})) {
    $prop = { $properties->%* }; # clone
}

Alternatively, one can use a intermediate variable + a fallback value

my $parameter_properties = $info->{parameters}->{properties} // {};
my $prop = { $parameter_properties->%* }; # copy

Alternatively, one can use a ternary statement:

$prop = defined($info->{parameters}->{properties})
 ? { $info->{parameters}->{properties}->%* }
 : {};

The last one is IMO a bit long here.


>      $prop = { %$prop, %$formatter_properties } if $formatter_properties;
>  
>      my $print_parameter_completion = sub {



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH pve-common] cli: fix #6762: only copy properties if defined

[Nicolas Frey]

On 10/7/25 4:46 PM, Thomas Lamprecht wrote:
> Am 07.10.25 um 15:56 schrieb Nicolas Frey:
>> Adds a defined check to the copy, as to not result in the bugfixes
>> reported error when double tabbing on `pveceph status`.
> Feel encouraged to include errors in verbatim in commit message,
> while we own bugzilla.proxmox.com and thus it's unlikely that it
> will be gone without notice or migration it's still always nice to
> keep git commits self-sufficient w.r.t core information. Makes it
> also easier to find a commit if one just searches for the error
> message.
> 

OK, will do in the future. Thanks for letting me know!

>>
>> Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=6762
>> Signed-off-by: Nicolas Frey <n.frey@proxmox.com>
>> ---
>>  src/PVE/CLIHandler.pm | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/PVE/CLIHandler.pm b/src/PVE/CLIHandler.pm
>> index 89cb7b7..cdd71c7 100644
>> --- a/src/PVE/CLIHandler.pm
>> +++ b/src/PVE/CLIHandler.pm
>> @@ -455,7 +455,8 @@ my $print_bash_completion = sub {
>>  
>>      my $info = $class->map_method_by_name($name);
>>  
>> -    my $prop = { %{ $info->{parameters}->{properties} } }; # copy
>> +    my $prop = { %{ $info->{parameters}->{properties} } }
>> +        if defined $info->{parameters}->{properties}; # copy
> 
> You cannot know without having lots of perl experience, or having had
> the bad luck of debugging this yourself early on, but a conditional
> declaration of a variable is Real Bad™ perl, as it will retain the
> value of the last time the condition held true for the case it's false
> now.
> 

TIL another weird quirk about perl. Thanks for pointing it out and
saving me from needing to debug this!

Will send a v2 momentarily.

> See the NOTE at the end of the "Statement Modifiers" [0] section for
> details.
> 
> [0]: https://perldoc.perl.org/perlsyn#Statement-Modifiers
> 
> Safe alternatives are:
> 
> my $prop;
> $prop = { $info->{parameters}->{properties}->%* } if defined($info->{parameters}->{properties});
> 
> Similar, but with normal if block, which can be combined with adding a
> intermediate variable.
> 
> my $prop;
> if (defined(my $properties = $info->{parameters}->{properties})) {
>     $prop = { $properties->%* }; # clone
> }
> 
> Alternatively, one can use a intermediate variable + a fallback value
> 
> my $parameter_properties = $info->{parameters}->{properties} // {};
> my $prop = { $parameter_properties->%* }; # copy
> 
> Alternatively, one can use a ternary statement:
> 
> $prop = defined($info->{parameters}->{properties})
>  ? { $info->{parameters}->{properties}->%* }
>  : {};
> 
> The last one is IMO a bit long here.
> 
> 
>>      $prop = { %$prop, %$formatter_properties } if $formatter_properties;
>>  
>>      my $print_parameter_completion = sub {
> 



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH pve-common] cli: fix #6762: only copy properties if defined

[Nicolas Frey]

Superseded-by: https://lore.proxmox.com/pve-devel/20251007151649.163255-1-n.frey@proxmox.com/T/#u

On 10/7/25 3:56 PM, Nicolas Frey wrote:
> Adds a defined check to the copy, as to not result in the bugfixes
> reported error when double tabbing on `pveceph status`.
> 
> Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=6762
> Signed-off-by: Nicolas Frey <n.frey@proxmox.com>
> ---
>  src/PVE/CLIHandler.pm | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/PVE/CLIHandler.pm b/src/PVE/CLIHandler.pm
> index 89cb7b7..cdd71c7 100644
> --- a/src/PVE/CLIHandler.pm
> +++ b/src/PVE/CLIHandler.pm
> @@ -455,7 +455,8 @@ my $print_bash_completion = sub {
>  
>      my $info = $class->map_method_by_name($name);
>  
> -    my $prop = { %{ $info->{parameters}->{properties} } }; # copy
> +    my $prop = { %{ $info->{parameters}->{properties} } }
> +        if defined $info->{parameters}->{properties}; # copy
>      $prop = { %$prop, %$formatter_properties } if $formatter_properties;
>  
>      my $print_parameter_completion = sub {



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel