[pve-devel] applied: [PATCH 1/2] run command: avoid using 1 as special value

[pve-devel] applied: [PATCH 1/2] run command: avoid using 1 as special value

[Thomas Lamprecht]

In Perl, the last expression of a block (e.g. of a method, eval) gets
returned if there's no explicit return statement. Quite often that is
truthy, i.e., 1.

As that was chosen as the special value for the CMD_FINISHED flag it
had quite a few false positives, causing weird effects and
installation failure.

Reserve that overly problematic value and chose 2 as new CMD_FINISHED
value, albeit it could be better to signal this even more explicitly,
like with a structured hash reference, but for now this is a good stop
gap.

Fixes: 23c5fbe ("sys: command: allow terminating the process early from log subroutine")
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---
 Proxmox/Sys/Command.pm | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/Proxmox/Sys/Command.pm b/Proxmox/Sys/Command.pm
index cb5fe76..ac505c4 100644
--- a/Proxmox/Sys/Command.pm
+++ b/Proxmox/Sys/Command.pm
@@ -17,7 +17,10 @@ use Proxmox::UI;
 use base qw(Exporter);
 our @EXPORT_OK = qw(run_command syscmd CMD_FINISHED);
 
-use constant CMD_FINISHED => 1;
+use constant {
+    CMD_RESERVED => 1<<0, # reserve 1 as it's often the default return value of closures
+    CMD_FINISHED => 1<<1,
+};
 
 my sub shellquote {
     my $str = shift;
@@ -78,8 +81,9 @@ sub syscmd {
 #
 # Arguments:
 # * $cmd - The command to run, either a single string or array with individual arguments
-# * $func - Logging subroutine to call, receives both stdout and stderr. Might return CMD_FINISHED
-#           to exit early and ignore the rest of the process output.
+# * $func - Logging subroutine to call, receives both stdout and stderr.
+#           Can return CMD_FINISHED to exit early and ignore the rest of the process output.
+#           Should use an explicit  `return;` to avoid misinterpretation of return value.
 # * $input - Stdin contents for the spawned subprocess
 # * $noout - Whether to append any process output to the return value
 # * $noprint - Whether to print any process output to the parents stdout
-- 
2.39.2

[pve-devel] applied: [PATCH 2/2] run command: use explicit return undef in closures on call sites

[Thomas Lamprecht]

To avoid a misinterpretation of the auto-return value:

> In the absence of an explicit return, a subroutine, eval, or do FILE
> automatically returns the value of the last expression evaluated.
-- https://perldoc.perl.org/functions/return

Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---
 Proxmox/Install.pm        | 5 +++++
 Proxmox/Install/RunEnv.pm | 1 +
 2 files changed, 6 insertions(+)

diff --git a/Proxmox/Install.pm b/Proxmox/Install.pm
index a0e1f5d..67093a1 100644
--- a/Proxmox/Install.pm
+++ b/Proxmox/Install.pm
@@ -121,6 +121,7 @@ sub create_filesystem {
 	} elsif ($line =~ m/Writing superblocks and filesystem.*done/) {
 	    update_progress(1, $rs, $re);
 	}
+	return;
     });
 }
 
@@ -358,6 +359,7 @@ sub get_pv_list_from_vgname {
 	} else {
 	    $res->{$vg_uuid}->{pvs} .= ", $pv";
 	}
+	return;
     };
     run_command("pvs --noheadings -o pv_name,vg_uuid -S vg_name='$vgname'", $parser, undef, 1);
 
@@ -903,6 +905,7 @@ sub extract_data {
 		my $frac = $per > 100 ? 1 : $per/100;
 		update_progress($frac, $maxper, 0.5);
 	    }
+	    return;
 	});
 
 	syscmd("mount -n -t tmpfs tmpfs $targetdir/tmp") == 0 || die "unable to mount tmpfs on $targetdir/tmp\n";
@@ -1004,6 +1007,7 @@ sub extract_data {
 		if ($line =~ m/^UUID=([A-Fa-f0-9\-]+)$/) {
 		    $fsuuid = $1;
 		}
+		return;
 	    });
 
 	    die "unable to detect FS UUID" if !defined($fsuuid);
@@ -1114,6 +1118,7 @@ _EOD
 	    if ($line =~ m/Setting up\s+(\S+)/) {
 		update_progress((++$count)/$pkg_count, 0.75, 0.95, "configuring $1");
 	    }
+	    return;
 	});
 
 	unlink "$targetdir/etc/mailname";
diff --git a/Proxmox/Install/RunEnv.pm b/Proxmox/Install/RunEnv.pm
index d7ccea6..25b6bb3 100644
--- a/Proxmox/Install/RunEnv.pm
+++ b/Proxmox/Install/RunEnv.pm
@@ -212,6 +212,7 @@ my sub detect_country_tracing_to : prototype($$) {
 		    log_info("DC FOUND: $country\n");
 		    return CMD_FINISHED;
 		}
+		return;
 	    }
 	}, undef, undef, 1);
 
-- 
2.39.2