The prototypes where completely circumvented by calling those two
methods by reference via &, and that probably happened as the send_msg
one was just wrong, it forced scalar context for the second parameter,
while that was a list (or well hash, but the difference there can be
blurry).
Anyhow, prototypes are not always of help, and can be a PITA with
side-effects too, and especially for such small modules it has not
that much use to declare them for privately-scoped methods, so just
drop them and fix the calling style.
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Compare lazily to always avoid to vector collections and if one of the
first parts mismatch some lower_case calls.
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Add some negative tests to ensure a `return true` (exaggerated)
refactoring won't pass the suite, and add one test where a and b is
the same, just to be sure.
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Due to interpolation, the \. sequence must be double-escaped.
Previously, this would result in a non-escaped dot, thus matching much
more liberally than it should.
Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
[ TL: fix bug # reference in code comments ]
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
.. by moving it into its own subroutine. Makes the whole thing quite a
bit neater and easier to maintain.
No functional changes.
Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
Multiple DNS-related RFCs (notably RFC 952, RFC 1035 and RFC 4343)
reinforce that FQDN must not be case-sensitive.
Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
Debian limits labels to 63 characters each and the total length to 253
characters [0].
While at it, reference all the RFCs that apply when parsing FQDNs.
[0] https://manpages.debian.org/stable/manpages/hostname.7.en.html
Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
Graceful process termination can need a bit of time, so wait one
second between sending the (catchable) TERM signal and the
(uncatchable) KILL one.
Makes the code shorter as a side benefit.
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
This fixes a rather longstanding issue [0][1] with the country
detection, in that it might get completely stuck and thus hangs the
installation.
This is due how Perl, signals and line reading interacts.
A minimal reproducer, how the installer currently works, looks like
this:
```
#!/usr/bin/env perl
use strict;
use warnings;
open (my $fh, '-|', 'sleep', '1000') or die;
my $prev = alarm(2);
eval {
local $SIG{ALRM} = sub { die "timed out!\n" };
my $line;
while (defined ($line = <$fh>)) {
print "line: $line";
}
};
alarm($prev);
close($fh);
```
One might expect that this times out after 2 seconds, as specified in
`alarm(2)`. The thruth is that `$line = <$fh>` apparently prevents the
signal to go through. This then causes the installer to hang there
indefinitely, if `traceroute` never progresses - which seems to happen
on lots of (weird) networks, as evidently can be seen in the forum [1].
Proxmox::Sys::Command::run_command() handles of these weird cases, takes
care of the nitty-gritty details and - most importantly - interacts
properly with SIGALRM, so just use that instead.
This _should_ really fix that issue, but reproducing it 1:1 as part of
the installation process is _very_ hard, basically pure luck. But
rewriting the reproducer using run_command (in the exact same way that
this patch rewrites detect_country_tracing_to()) fixes the issue there,
so it's the best we can probably do.
NB: This causes that the traceroute command is now printed to the log
(as run_command() logs that by default), which we could also hide e.g.
through another parameter if wanted.
[0] https://bugzilla.proxmox.com/show_bug.cgi?id=4872
[1] https://forum.proxmox.com/threads/proxmox-installation-trying-to-detect-country.134301/
Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
If $noprint is set, the output of the command won't be printed to stdout
of the parent process.
Fully backwards-compatible again, only takes effect if the new argument
is actually specified.
Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
If the logging subroutine $func returns CMD_FINISHED after processing a
line, the running subprocess is killed early.
This mechanism can be used when e.g. only a certain part of the output
of a (long-running) command is needed, avoiding the extra time it would
take the command to finish properly.
This is done in a entirely backwards-compatible way, i.e. existing
usages don't need any modification.
Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
Previously, the I/O loop would continue endlessly until the subprocess
exited.
This explicit handling allows run_command() to be used with e.g.
alarm().
Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
This moves the kill() + waitpid() combo into a separate subroutine,
avoiding open-coding that sequence. wait_for_process() also handles
properly unkillable process (e.g. in D-state) and avoids completely
locking up the installer in such cases. See [0].
For the latter case, a timeout exists (with a default of 5 seconds) in
which to wait for the process to exit after sending an optional
TERM/KILL signal.
Also while at it, add a few basic tests for run_command().
[0] https://lists.proxmox.com/pipermail/pve-devel/2024-February/061697.html
Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
Some detection routines might try to log things and call some
Proxmox::Ui functions all the way down, so just initialize it with the
stdio backend to avoid errors.
Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
For consistency sake, all colons and trailing spaces in labels that were
followed with an entry were removed, this matches other panels such as
the password and country/timezone panels.
Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
Reviewed-by: Christoph Heiss <c.heiss@proxmox.com>
Tested-by: Christoph Heiss <c.heiss@proxmox.com>
Tested-by: Tested-by: Lukas Wagner <l.wagner@proxmox.com>
This accounts for the different layout set in the previous commit
9102da7 ("gui: use basic grid in the network panel")
Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
Reviewed-by: Christoph Heiss <c.heiss@proxmox.com>
Tested-by: Christoph Heiss <c.heiss@proxmox.com>
Tested-by: Tested-by: Lukas Wagner <l.wagner@proxmox.com>
Using boxes causes the labels to not align correctly in certain
circumstances. In the following commits we replace the use of boxes with
grids and set the margins and spacing directly on the respective grid.
Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
Reviewed-by: Christoph Heiss <c.heiss@proxmox.com>
Tested-by: Christoph Heiss <c.heiss@proxmox.com>
Tested-by: Tested-by: Lukas Wagner <l.wagner@proxmox.com>
Previously the grids were inserted in a succession of boxes each with
its own set of margins and spacing. We define the margins now
exclusively in the grid and account for previous values.
Note that we match the top and bottom margins of the 'Target Harddisk'
panel which does not need to use a grid.
Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
Reviewed-by: Christoph Heiss <c.heiss@proxmox.com>
Tested-by: Christoph Heiss <c.heiss@proxmox.com>
Tested-by: Tested-by: Lukas Wagner <l.wagner@proxmox.com>
The extra 10px margin on the email row was added to account for the
removed line:
$vbox->pack_start($hbox3, 0, 0, 15);
Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
Reviewed-by: Christoph Heiss <c.heiss@proxmox.com>
Tested-by: Christoph Heiss <c.heiss@proxmox.com>
Tested-by: Tested-by: Lukas Wagner <l.wagner@proxmox.com>
This will be used in future commits to create grids so we need it to be defined.
Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
Reviewed-by: Christoph Heiss <c.heiss@proxmox.com>
Tested-by: Christoph Heiss <c.heiss@proxmox.com>
Tested-by: Tested-by: Lukas Wagner <l.wagner@proxmox.com>
See also the thread at [0] for the initial discussion/idea.
Disabling checksums is considered an "extraordinarily bad idea" [1] (for
pretty obvious reason) and nobody should really ever use it.
Thus remove the option completely; just so that users cannot simply
disable checksum "for performance reasons" without knowing about the
implications of this.
As pointed out by Thomas, it can still be set to "off" after the
installation using the `zfs` tool, if really wanted.
[0] https://lists.proxmox.com/pipermail/pve-devel/2023-December/061188.html
[1] https://openzfs.github.io/openzfs-docs/Basic%20Concepts/Checksums.html#disabling-checksums
Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
Creating rpool/var/lib/vz and all intermediate datasets causes a
service-failure of `var.mount` upon shutdown.
creating the dataset for /var/lib/vz directly at the rpool and setting
its mountpoint property seems the most robust way to address this.
The alternative approach of setting `canmount=off` on the `var`
dataset seems a bit dangerous (users setting a zfs property and
suddenly hiding their /var contents).
The only small downside to this approach is that the setting of the
mountpoint happens quite a bit after extracting the data - but this
would probably be better addressed with a refactoring of the
lowlevel-installer code (setting the zfs-pool up under /target and
getting rid of a few special cases)
Fixes: dd19d40cea
Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
Matching if a serial will be needed for grub is based on the target
commandline - the speed is also read from there. The unit is based
on the ttyS device - although I'd assume that this might not always
match up.
Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
An uneven number of disks otherwise causes a panic due to an
out-of-bounds array access in the loop below.
Reported-by: Fiona Ebner <f.ebner@proxmox.com>
Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
This broke running the TUI installer in debug mode, does not effect
release builds in any way.
Fixes: 4b4dfa1 ("low level: testmode: take path to disk image instead of using /dev/null")
Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
Instead of reading the checkbox when continuing to the next screen, save
its toggle status to the installer state instead on change.
Reported-by: Fiona Ebner <f.ebner@proxmox.com>
Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
Currently, when multiple NICs are present in a system the TUI
sometimes selects the wrong interface (not the one that has the
default gateway/dhcp lease)
I assume this is due to HashMap's values yielding an iterator in
arbitrary order
Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
Co-authored-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
[ TL: avoid intermediate vector, reuse the SelectView's iter()]
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
according to `man zfsprops` the copies option can only be 1, 2, or 3.
limit the field to 3 just like we do for the GTK based UI, as setting
higher options can't work anyway.
Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
[ TL: fleece in note that we already limit this in the GTK UI ]
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
The GTK installer/UI module in the low-level installer does the same.
Messages used with this are worded for this, using yes/no instead can be
quite confusing (e.g.
Proxmox::Install::ask_existing_vg_rename_or_abort())
Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
The line-based protocol currently used cannot handle this properly, so
introduce this as a stop-gap measure - otherwise messages might be cut
off.
This makes it work for now, and the text is wrapped correctely for the
screen width in the TUI anyway - which is the only user of this so far.
Will be reworked properly later on.
Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
[ TL: add fix-me comment ]
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Happens due to a force-unwrap() under the false assumption that the
disk for LVM configurations always exists when switching to a LVM
filesystem.
This fails spectacularly with a panic when switching from e.g. Btrfs to
ext4 in the filesystem chooser.
Fixes: eda9fa0 ("fix #4856: tui: bootdisk: use correct defaults in advanced dialog")
Reported-by: Christian Ebner <c.ebner@proxmox.com>
Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
