[pve-devel] [PATCH installer 0/6] switch low-level installer protocol to json

[pve-devel] [PATCH installer 0/6] switch low-level installer protocol to json

[Christoph Heiss]

This switches the stdio-protocol for the low-level installer from
simple, line-based messages to JSON.

This solves a number of problems, most prominently that messages can now
contain multiline text (or for that matter, any kind of data), as JSON
handles that transparently.

The actual "meaty" changes are in #3, the others are either some simple
cleanups or test infra.

I have also included a testsuite for both the Perl side of things, as
well as the TUI, to ensure that it works and cannot be accidently broken
easily in the future.

Tested this by installing of PVE and PBS.

Christoph Heiss (6):
  low-level: align wording of finish message
  ui: stdio: log error if display_html() is called on stdio backend
  tui, ui: switch over to JSON-based protocol
  test: add tests for UI^2 stdio protocol
  buildsys: setup proper test environment for testsuite
  tui: install progress: add tests for UI^2 stdio protocol

 Makefile                                      |   9 +-
 Proxmox/UI/StdIO.pm                           |  43 ++--
 proxmox-low-level-installer                   |   2 +-
 .../src/views/install_progress.rs             | 195 +++++++++++++-----
 test/Makefile                                 |   6 +-
 test/ui2-stdio.pl                             |  96 +++++++++
 6 files changed, 280 insertions(+), 71 deletions(-)
 create mode 100755 test/ui2-stdio.pl

--
2.42.0

[pve-devel] [PATCH installer 1/6] low-level: align wording of finish message

[Christoph Heiss]

The other case uses "Installation finished [..]", thus use the same
wording here too.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 proxmox-low-level-installer | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/proxmox-low-level-installer b/proxmox-low-level-installer
index 9b4b773..d127a40 100755
--- a/proxmox-low-level-installer
+++ b/proxmox-low-level-installer
@@ -78,7 +78,7 @@ sub send_reboot_ui_message {
 	    $secs -= 1;
 	}
     } else {
-	Proxmox::UI::finished(1, "Installation complete - reboot now?");
+	Proxmox::UI::finished(1, "Installation finished - reboot now?");
     }
 }
 
-- 
2.42.0

[pve-devel] [PATCH installer 2/6] ui: stdio: log error if display_html() is called on stdio backend

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 Proxmox/UI/StdIO.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Proxmox/UI/StdIO.pm b/Proxmox/UI/StdIO.pm
index 75ddbeb..a97245c 100644
--- a/Proxmox/UI/StdIO.pm
+++ b/Proxmox/UI/StdIO.pm
@@ -49,7 +49,7 @@ sub prompt {
 sub display_html {
     my ($raw_html, $html_dir) = @_;
 
-    # ignore for now
+    log_error("display_html() not available for stdio backend!");
 }
 
 sub progress {
-- 
2.42.0

[pve-devel] [PATCH installer 3/6] tui, ui: switch over to JSON-based protocol

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 Proxmox/UI/StdIO.pm                           |  41 ++++--
 .../src/views/install_progress.rs             | 117 ++++++++----------
 2 files changed, 83 insertions(+), 75 deletions(-)

diff --git a/Proxmox/UI/StdIO.pm b/Proxmox/UI/StdIO.pm
index a97245c..25d1c82 100644
--- a/Proxmox/UI/StdIO.pm
+++ b/Proxmox/UI/StdIO.pm
@@ -3,10 +3,26 @@ package Proxmox::UI::StdIO;
 use strict;
 use warnings;
 
+use JSON qw(from_json to_json);
+
 use base qw(Proxmox::UI::Base);
 
 use Proxmox::Log;
 
+my sub send_msg : prototype($$) {
+    my ($type, %values) = @_;
+
+    my $json = to_json({ type => $type, %values }, { utf8 => 1, canonical => 1 });
+    print STDOUT "$json\n";
+}
+
+my sub recv_msg : prototype() {
+    my $response = <STDIN> // ''; # FIXME: error handling?
+    chomp($response);
+
+    return eval { from_json($response, { utf8 => 1 }) };
+}
+
 sub init {
     my ($self) = @_;
 
@@ -16,34 +32,33 @@ sub init {
 sub message {
     my ($self, $msg) = @_;
 
-    print STDOUT "message: $msg\n";
+    &send_msg('message', message => $msg);
 }
 
 sub error {
     my ($self, $msg) = @_;
-    log_err("error: $msg\n");
-    print STDOUT "error: $msg\n";
+
+    log_error("error: $msg");
+    &send_msg('error', message => $msg);
 }
 
 sub finished {
     my ($self, $success, $msg) = @_;
 
     my $state = $success ? 'ok' : 'err';
-    log_info("finished: $state, $msg\n");
-    print STDOUT "finished: $state, $msg\n";
+    log_info("finished: $state, $msg");
+    &send_msg('finished', state => $state, message => $msg);
 }
 
 sub prompt {
     my ($self, $query) = @_;
 
-    $query =~ s/\n/ /g; # FIXME: use a better serialisation (e.g., JSON)
-    print STDOUT "prompt: $query\n";
-
-    my $response = <STDIN> // ''; # FIXME: error handling?
-
-    chomp($response);
+    &send_msg('prompt', query => $query);
+    my $response = &recv_msg();
 
-    return lc($response) eq 'ok';
+    if (defined($response) && $response->{type} eq 'prompt-answer') {
+	return lc($response->{answer}) eq 'ok';
+    }
 }
 
 sub display_html {
@@ -57,7 +72,7 @@ sub progress {
 
     $text = '' if !defined($text);
 
-    print STDOUT "progress: $ratio $text\n";
+    &send_msg('progress', ratio => $ratio, text => $text);
 }
 
 sub process_events {
diff --git a/proxmox-tui-installer/src/views/install_progress.rs b/proxmox-tui-installer/src/views/install_progress.rs
index 01c9941..741529f 100644
--- a/proxmox-tui-installer/src/views/install_progress.rs
+++ b/proxmox-tui-installer/src/views/install_progress.rs
@@ -1,17 +1,16 @@
-use std::{
-    io::{BufRead, BufReader, Write},
-    str::FromStr,
-    sync::{Arc, Mutex},
-    thread,
-    time::Duration,
-};
-
 use cursive::{
     utils::Counter,
     view::{Nameable, Resizable, ViewWrapper},
     views::{Dialog, DummyView, LinearLayout, PaddedView, ProgressBar, TextContent, TextView},
     CbSink, Cursive,
 };
+use serde::Deserialize;
+use std::{
+    io::{BufRead, BufReader, Write},
+    sync::{Arc, Mutex},
+    thread,
+    time::Duration,
+};
 
 use crate::{abort_install_button, prompt_dialog, setup::InstallConfig, InstallerState};
 use proxmox_installer_common::setup::spawn_low_level_installer;
@@ -95,7 +94,7 @@ impl InstallProgressView {
                     Err(err) => return Err(format!("low-level installer exited early: {err}")),
                 };
 
-                let msg = match line.parse::<UiMessage>() {
+                let msg = match serde_json::from_str::<UiMessage>(&line) {
                     Ok(msg) => msg,
                     Err(stray) => {
                         // Not a fatal error, so don't abort the installation by returning
@@ -105,26 +104,26 @@ impl InstallProgressView {
                 };
 
                 let result = match msg.clone() {
-                    UiMessage::Info(s) => cb_sink.send(Box::new(|siv| {
-                        siv.add_layer(Dialog::info(s).title("Information"));
+                    UiMessage::Info { message } => cb_sink.send(Box::new(|siv| {
+                        siv.add_layer(Dialog::info(message).title("Information"));
                     })),
-                    UiMessage::Error(s) => cb_sink.send(Box::new(|siv| {
-                        siv.add_layer(Dialog::info(s).title("Error"));
+                    UiMessage::Error { message } => cb_sink.send(Box::new(|siv| {
+                        siv.add_layer(Dialog::info(message).title("Error"));
                     })),
-                    UiMessage::Prompt(s) => cb_sink.send({
+                    UiMessage::Prompt { query } => cb_sink.send({
                         let writer = writer.clone();
-                        Box::new(move |siv| Self::show_prompt(siv, &s, writer))
+                        Box::new(move |siv| Self::show_prompt(siv, &query, writer))
                     }),
-                    UiMessage::Progress(ratio, s) => {
-                        counter.set(ratio);
-                        progress_text.set_content(s);
+                    UiMessage::Progress { ratio, text } => {
+                        counter.set((ratio * 100.).floor() as usize);
+                        progress_text.set_content(text);
                         Ok(())
                     }
-                    UiMessage::Finished(success, msg) => {
+                    UiMessage::Finished { state, message } => {
                         counter.set(100);
-                        progress_text.set_content(msg.to_owned());
+                        progress_text.set_content(message.to_owned());
                         cb_sink.send(Box::new(move |siv| {
-                            Self::prepare_for_reboot(siv, success, &msg)
+                            Self::prepare_for_reboot(siv, state == "ok", &message);
                         }))
                     }
                 };
@@ -189,6 +188,19 @@ impl InstallProgressView {
     }
 
     fn show_prompt<W: Write + 'static>(siv: &mut Cursive, text: &str, writer: Arc<Mutex<W>>) {
+        let send_answer = |writer: Arc<Mutex<W>>, answer| {
+            if let Ok(mut writer) = writer.lock() {
+                let _ = writeln!(
+                    writer,
+                    "{}",
+                    serde_json::json!({
+                        "type" : "prompt-answer",
+                        "answer" : answer,
+                    })
+                );
+            }
+        };
+
         prompt_dialog(
             siv,
             "Prompt",
@@ -197,16 +209,12 @@ impl InstallProgressView {
             Box::new({
                 let writer = writer.clone();
                 move |_| {
-                    if let Ok(mut writer) = writer.lock() {
-                        let _ = writeln!(writer, "ok");
-                    }
+                    send_answer(writer.clone(), "ok");
                 }
             }),
             "Cancel",
             Box::new(move |_| {
-                if let Ok(mut writer) = writer.lock() {
-                    let _ = writeln!(writer);
-                }
+                send_answer(writer.clone(), "cancel");
             }),
         );
     }
@@ -216,40 +224,25 @@ impl ViewWrapper for InstallProgressView {
     cursive::wrap_impl!(self.view: PaddedView<LinearLayout>);
 }
 
-#[derive(Clone, Debug)]
+#[derive(Clone, Debug, Deserialize, PartialEq)]
+#[serde(tag = "type", rename_all = "lowercase")]
 enum UiMessage {
-    Info(String),
-    Error(String),
-    Prompt(String),
-    Finished(bool, String),
-    Progress(usize, String),
-}
-
-impl FromStr for UiMessage {
-    type Err = String;
-
-    fn from_str(s: &str) -> Result<Self, Self::Err> {
-        let (ty, rest) = s.split_once(": ").ok_or("invalid message: no type")?;
-
-        match ty {
-            "message" => Ok(UiMessage::Info(rest.to_owned())),
-            "error" => Ok(UiMessage::Error(rest.to_owned())),
-            "prompt" => Ok(UiMessage::Prompt(rest.to_owned())),
-            "finished" => {
-                let (state, rest) = rest.split_once(", ").ok_or("invalid message: no state")?;
-                Ok(UiMessage::Finished(state == "ok", rest.to_owned()))
-            }
-            "progress" => {
-                let (percent, rest) = rest.split_once(' ').ok_or("invalid progress message")?;
-                Ok(UiMessage::Progress(
-                    percent
-                        .parse::<f64>()
-                        .map(|v| (v * 100.).floor() as usize)
-                        .map_err(|err| err.to_string())?,
-                    rest.to_owned(),
-                ))
-            }
-            unknown => Err(format!("invalid message type {unknown}, rest: {rest}")),
-        }
-    }
+    #[serde(rename = "message")]
+    Info {
+        message: String,
+    },
+    Error {
+        message: String,
+    },
+    Prompt {
+        query: String,
+    },
+    Finished {
+        state: String,
+        message: String,
+    },
+    Progress {
+        ratio: f32,
+        text: String,
+    },
 }
-- 
2.42.0

[pve-devel] [PATCH installer 4/6] test: add tests for UI^2 stdio protocol

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 test/Makefile     |  6 ++-
 test/ui2-stdio.pl | 96 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 101 insertions(+), 1 deletion(-)
 create mode 100755 test/ui2-stdio.pl

diff --git a/test/Makefile b/test/Makefile
index fb80fc4..eef8830 100644
--- a/test/Makefile
+++ b/test/Makefile
@@ -3,8 +3,12 @@ all:
 export PERLLIB=..
 
 .PHONY: check
-check: test-zfs-arc-max
+check: test-zfs-arc-max test-ui2-stdio
 
 .PHONY: test-zfs-arc-max
 test-zfs-arc-max:
 	./zfs-arc-max.pl
+
+.PHONY: test-ui2-stdio
+test-ui2-stdio:
+	./ui2-stdio.pl
diff --git a/test/ui2-stdio.pl b/test/ui2-stdio.pl
new file mode 100755
index 0000000..f1c38e8
--- /dev/null
+++ b/test/ui2-stdio.pl
@@ -0,0 +1,96 @@
+#!/usr/bin/perl
+
+use strict;
+use warnings;
+
+use JSON qw(from_json);
+use Test::More;
+
+use Proxmox::Log;
+use Proxmox::UI;
+
+pipe(my $parent_reader, my $child_writer) || die;
+pipe(my $child_reader, my $parent_writer) || die;
+
+$parent_writer->autoflush(1);
+$child_writer->autoflush(1);
+$child_reader->autoflush(1);
+
+
+if (my $child_pid = fork()) {
+    # parent, the hypothetical low-level installer
+    close($parent_reader);
+    close($parent_writer);
+
+    # "mock" stdin and stdout for Proxmox::UI::StdIO
+    *STDIN = $child_reader;
+    *STDOUT = $child_writer;
+
+    Proxmox::Log::init('&STDERR'); # log to stderr
+    Proxmox::UI::init_stdio({}, {});
+
+    Proxmox::UI::message('foo');
+    Proxmox::UI::error('bar');
+    is(Proxmox::UI::prompt('baz?'), 1, 'prompt should get positive answer');
+    is(Proxmox::UI::prompt('not baz? :('), '', 'prompt should get negative answer');
+
+    Proxmox::UI::finished(1, 'install successful');
+    Proxmox::UI::finished(0, 'install failed');
+
+    Proxmox::UI::progress(0.2, 0, 1, '20% done');
+    Proxmox::UI::progress(0.2, 0, 1);
+    Proxmox::UI::progress(0.99, 0, 1, '99% done');
+    Proxmox::UI::progress(1, 0, 1, 'done');
+
+    waitpid($child_pid, 0);
+    done_testing();
+} else {
+    # child, e.g. the TUI
+    die 'failed to fork?' if !defined($child_pid);
+    close($child_reader);
+    close($child_writer);
+
+    use Test::More;
+
+    my $next_msg = sub {
+	chomp(my $msg = <$parent_reader>);
+	return from_json($msg, { utf8 => 1 });
+    };
+
+    is_deeply(&$next_msg(), { type => 'message', message => 'foo' }, 'should receive message');
+    is_deeply(&$next_msg(), { type => 'error', message => 'bar' }, 'should receive error');
+
+    is_deeply(&$next_msg(), { type => 'prompt', query => 'baz?' }, 'prompt works');
+    print $parent_writer "{\"type\":\"prompt-answer\",\"answer\":\"ok\"}\n";
+
+    is_deeply(&$next_msg(), { type => 'prompt', query => 'not baz? :(' }, 'prompt works');
+    print $parent_writer "{\"type\":\"prompt-answer\",\"answer\":\"cancel\"}\n";
+
+    is_deeply(
+	&$next_msg(), { type => 'finished', state => 'ok', message => 'install successful'},
+	'should receive successful finished message');
+
+    is_deeply(
+	&$next_msg(), { type => 'finished', state => 'err', message => 'install failed'},
+	'should receive failed finished message');
+
+    is_deeply(
+	&$next_msg(), { type => 'progress', ratio => 0.2, text => '20% done' },
+	'should get 20% done progress message');
+
+    is_deeply(
+	&$next_msg(), { type => 'progress', ratio => 0.2, text => '' },
+	'should get progress continuation message');
+
+    is_deeply(
+	&$next_msg(), { type => 'progress', ratio => 0.99, text => '99% done' },
+	'should get 99% done progress message');
+
+    is_deeply(
+	&$next_msg(), { type => 'progress', ratio => 1, text => 'done' },
+	'should get 100% done progress message');
+
+    done_testing();
+    exit(0);
+}
+
-- 
2.42.0

[pve-devel] [PATCH installer 5/6] buildsys: setup proper test environment for testsuite

[Christoph Heiss]

Some test to come will need a proper environment.

`prepare-test-env` can also be generally useful while developing to
quickly set up a new test environment as needed.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 Makefile | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 6f64c4e..601c836 100644
--- a/Makefile
+++ b/Makefile
@@ -77,8 +77,15 @@ $(DSC): $(BUILDDIR)
 sbuild: $(DSC)
 	sbuild $(DSC)
 
+.PHONY: prepare-test-env
+prepare-test-env: cd-info.test country.dat test.img
+	rm -rf testdir
+	mkdir -p testdir/var/lib/proxmox-installer/
+	cp -v country.dat testdir/var/lib/proxmox-installer/
+	./proxmox-low-level-installer -t test.img dump-env
+
 .PHONY: test
-test:
+test: prepare-test-env
 	$(MAKE) -C test check
 	$(CARGO) test --workspace $(CARGO_BUILD_ARGS)
 
-- 
2.42.0

[pve-devel] [PATCH installer 6/6] tui: install progress: add tests for UI^2 stdio protocol

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 .../src/views/install_progress.rs             | 94 +++++++++++++++++++
 1 file changed, 94 insertions(+)

diff --git a/proxmox-tui-installer/src/views/install_progress.rs b/proxmox-tui-installer/src/views/install_progress.rs
index 741529f..4626fe4 100644
--- a/proxmox-tui-installer/src/views/install_progress.rs
+++ b/proxmox-tui-installer/src/views/install_progress.rs
@@ -246,3 +246,97 @@ enum UiMessage {
         text: String,
     },
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use std::env;
+
+    #[test]
+    fn run_low_level_installer_test_session() {
+        env::set_current_dir("..").expect("failed to change working directory");
+        let mut child = spawn_low_level_installer(true)
+            .expect("failed to run low-level installer test session");
+
+        let mut reader = child
+            .stdout
+            .take()
+            .map(BufReader::new)
+            .expect("failed to get stdin reader");
+
+        let mut writer = child.stdin.take().expect("failed to get stdin writer");
+
+        serde_json::to_writer(&mut writer, &serde_json::json!({ "autoreboot": false }))
+            .expect("failed to serialize install config");
+
+        writeln!(writer).expect("failed to write install config: {err}");
+
+        let mut next_msg = || {
+            let mut line = String::new();
+            reader.read_line(&mut line).expect("a line");
+
+            match serde_json::from_str::<UiMessage>(&line) {
+                Ok(msg) => Some(msg),
+                Err(err) => panic!("unexpected error: '{err}'"),
+            }
+        };
+
+        assert_eq!(
+            next_msg(),
+            Some(UiMessage::Prompt {
+                query: "Reply anything?".to_owned()
+            }),
+        );
+
+        serde_json::to_writer(
+            &mut writer,
+            &serde_json::json!({"type": "prompt-answer", "answer": "ok"}),
+        )
+        .expect("failed to write prompt answer");
+        writeln!(writer).expect("failed to write prompt answer");
+
+        assert_eq!(
+            next_msg(),
+            Some(UiMessage::Info {
+                message: "Test Message - got ok".to_owned()
+            }),
+        );
+
+        for i in (1..=1000).step_by(3) {
+            assert_eq!(
+                next_msg(),
+                Some(UiMessage::Progress {
+                    ratio: (i as f32) / 1000.,
+                    text: format!("foo {i}"),
+                }),
+            );
+        }
+
+        assert_eq!(
+            next_msg(),
+            Some(UiMessage::Finished {
+                state: "ok".to_owned(),
+                message: "Installation finished - reboot now?".to_owned(),
+            }),
+        );
+
+        // Should be nothing left to read now
+        let mut line = String::new();
+        assert_eq!(reader.read_line(&mut line).expect("success"), 0);
+
+        // Give the low-level installer some time to exit properly
+        std::thread::sleep(Duration::new(1, 0));
+
+        match child.try_wait() {
+            Ok(Some(status)) => assert!(
+                status.success(),
+                "low-level installer did not exit successfully"
+            ),
+            Ok(None) => {
+                child.kill().expect("could not kill low-level installer");
+                panic!("low-level install was not successful");
+            }
+            Err(err) => panic!("failed to wait for low-level installer: {err}"),
+        }
+    }
+}
-- 
2.42.0

Re: [pve-devel] [PATCH installer 0/6] switch low-level installer protocol to json

[Christoph Heiss]

Ping.

As this is a prerequisite for the auto-installer [0], it would be nice
to get this is sooner than later.

[0] https://lists.proxmox.com/pipermail/pve-devel/2024-January/061431.html

On Wed, Dec 06, 2023 at 12:34:49PM +0100, Christoph Heiss wrote:
>
> This switches the stdio-protocol for the low-level installer from
> simple, line-based messages to JSON.
>
> This solves a number of problems, most prominently that messages can now
> contain multiline text (or for that matter, any kind of data), as JSON
> handles that transparently.
>
> The actual "meaty" changes are in #3, the others are either some simple
> cleanups or test infra.
>
> I have also included a testsuite for both the Perl side of things, as
> well as the TUI, to ensure that it works and cannot be accidently broken
> easily in the future.
>
> Tested this by installing of PVE and PBS.
>
> Christoph Heiss (6):
>   low-level: align wording of finish message
>   ui: stdio: log error if display_html() is called on stdio backend
>   tui, ui: switch over to JSON-based protocol
>   test: add tests for UI^2 stdio protocol
>   buildsys: setup proper test environment for testsuite
>   tui: install progress: add tests for UI^2 stdio protocol
>
>  Makefile                                      |   9 +-
>  Proxmox/UI/StdIO.pm                           |  43 ++--
>  proxmox-low-level-installer                   |   2 +-
>  .../src/views/install_progress.rs             | 195 +++++++++++++-----
>  test/Makefile                                 |   6 +-
>  test/ui2-stdio.pl                             |  96 +++++++++
>  6 files changed, 280 insertions(+), 71 deletions(-)
>  create mode 100755 test/ui2-stdio.pl
>
> --
> 2.42.0
>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>

Re: [pve-devel] [PATCH installer 3/6] tui, ui: switch over to JSON-based protocol

[Thomas Lamprecht]

Am 06/12/2023 um 12:34 schrieb Christoph Heiss:
> Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
> ---
>  Proxmox/UI/StdIO.pm                           |  41 ++++--
>  .../src/views/install_progress.rs             | 117 ++++++++----------
>  2 files changed, 83 insertions(+), 75 deletions(-)
> 
> diff --git a/Proxmox/UI/StdIO.pm b/Proxmox/UI/StdIO.pm
> index a97245c..25d1c82 100644
> --- a/Proxmox/UI/StdIO.pm
> +++ b/Proxmox/UI/StdIO.pm
> @@ -3,10 +3,26 @@ package Proxmox::UI::StdIO;
>  use strict;
>  use warnings;
>  
> +use JSON qw(from_json to_json);
> +
>  use base qw(Proxmox::UI::Base);
>  
>  use Proxmox::Log;
>  
> +my sub send_msg : prototype($$) {

wrong prototype, the second argument isn't a scalar but a list interpreted as
hash.

That's why you had to use &send_msg on the call sites, which calls it via
reference and so does not checks prototypes at all.

Correct would be prototype($%), but that's not really catching any issues,
so it could be just left out.

In general I'd recommend to try to not overuse prototypes, they are best
for cases where one interacts with other functions or bindings that have
to map to an exact argument style, like the syscall wrapper we have in
pve-common, and even there the "is correctly called" is mostly a side-effect
(that can be circumvented as you've done here).

Prototypes basically allow you to create functions that behave like Perl
built-in functions, and e.g., take an array-ref even if a list is passed,
but for us that's seldom of use, or rather said, a bit to magic for my
taste and can lead to bugs on its own (list based to scalar prototype
results in the list-size being actually passed, way better for long term
maintenance to just do such things explicitly...

That doesn't mean that they should not be used at all, they sometimes can
have benefits, but if you need to result to use &method() to call a sub then
you're just doing something strange (and most of the time also wrong).

[pve-devel] applied-series: [PATCH installer 0/6] switch low-level installer protocol to json

[Thomas Lamprecht]

Am 06/12/2023 um 12:34 schrieb Christoph Heiss:
> This switches the stdio-protocol for the low-level installer from
> simple, line-based messages to JSON.
> 
> This solves a number of problems, most prominently that messages can now
> contain multiline text (or for that matter, any kind of data), as JSON
> handles that transparently.
> 
> The actual "meaty" changes are in #3, the others are either some simple
> cleanups or test infra.
> 
> I have also included a testsuite for both the Perl side of things, as
> well as the TUI, to ensure that it works and cannot be accidently broken
> easily in the future.
> 
> Tested this by installing of PVE and PBS.
> 
> Christoph Heiss (6):
>   low-level: align wording of finish message
>   ui: stdio: log error if display_html() is called on stdio backend
>   tui, ui: switch over to JSON-based protocol
>   test: add tests for UI^2 stdio protocol
>   buildsys: setup proper test environment for testsuite
>   tui: install progress: add tests for UI^2 stdio protocol
> 
>  Makefile                                      |   9 +-
>  Proxmox/UI/StdIO.pm                           |  43 ++--
>  proxmox-low-level-installer                   |   2 +-
>  .../src/views/install_progress.rs             | 195 +++++++++++++-----
>  test/Makefile                                 |   6 +-
>  test/ui2-stdio.pl                             |  96 +++++++++
>  6 files changed, 280 insertions(+), 71 deletions(-)
>  create mode 100755 test/ui2-stdio.pl
> 

forgot to write:

applied series, with fix up for bogus prototype definition, thanks!