all lists on lists.proxmox.com
 help / color / mirror / Atom feed
From: "Christoph Heiss" <c.heiss@proxmox.com>
To: "Stoiko Ivanov" <s.ivanov@proxmox.com>
Cc: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH installer v2 15/15] gui: add support for pinning network interface names
Date: Thu, 06 Nov 2025 13:06:28 +0100	[thread overview]
Message-ID: <DE1LKMPZF235.K1M3NSF1M8EU@proxmox.com> (raw)
In-Reply-To: <20251106101521.56191e73@rosa.proxmox.com>

On Thu Nov 6, 2025 at 10:15 AM CET, Stoiko Ivanov wrote:
> Thanks for tackling this! the first experience with this in GUI/TUI was
> good and I'm looking forward to having this available in our installers!

Thanks for taking a look at it!

>
> I ran into an issue and difference of behavior between GUI/TUI (and from a
> quick grep in our codebase between TUI and our API types) - my test was
> using single character iface names ('a', '_').
>
> Comment inline:
> On Thu, 30 Oct 2025 12:06:17 +0100
> Christoph Heiss <c.heiss@proxmox.com> wrote:
[..]
>> +    $dialog->signal_connect(
>> +        response => sub {
>> +            my $new_mapping = {};
>> +            my $reverse_mapping = {};
>> +            foreach my $mac (keys %$inputs) {
>> +                my $name = $inputs->{$mac}->get_text();
>> +
>> +                if (!defined($name) || $name eq '') {
>> +                    Proxmox::UI::message(
>> +                        "interface name mapping for '$mac' cannot be empty", $dialog,
>> +                    );
>> +                    return;
>> +                }
>> +
>> +                if (length($name) > MAX_IFNAME_LEN) {
>> +                    Proxmox::UI::message(
>> +                        "interface name mapping '$name' for '$mac' cannot be longer than "
>> +                            . MAX_IFNAME_LEN
>> +                            . " characters",
>> +                        $dialog,
>> +                    );
>> +                    return;
>> +                }
>> +
>> +                if ($name =~ m/^[0-9]/) {
>> +                    Proxmox::UI::message(
>> +                        "interface name mapping '$name' for '$mac' is invalid: must not start with a number",
>> +                        $dialog,
>> +                    );
>> +                    return;
>> +                }
>> +
>> +                if ($name !~ m/^[a-zA-Z][a-zA-Z0-9_]+$/) {
>> +                    Proxmox::UI::message(
>> +                        "interface name mapping '$name' for '$mac' is invalid: "
>> +                            . "must only consist alphanumeric characters and underscores",
> I received this as error-message when using 'a' as nic-name - so the error
> message does not cover that case - but looking through your other patches
> and also a bit later through json-schema:
> pve-common/src/PVE/JSONSchema.pm - this regex here seems correct - and the

Yeah, it's _mostly_ modeled after that regex, minus the VLAN & co parts.

> checks in the TUI are allowing too much (another nit there is the
> error-message when using '_' as interface name ('must not start with a
> number').

Thanks for testing that pretty thoroughly! I will go through all the
cases again.

>
> I did not check if we have similar inconsistencies in our rust-based
> pinning vs. the perl-based one - but if possible unifying what is allowed
> in all our installer interfaces would be good (most likely by disallowing
> the single-character ifaces)

Disallowing single-character names would be indeed correct, since that
is what the rest of our tooling enforces AFAICS.

I actually thought about (finally) introducing perlmod into the
installer, so that we don't have to keep duplicating all checks between
Perl and Rust, for the GUI and TUI/auto-installer, respectively.

> I'll give the autoinstaller a spin on one of our test-boxes with many
> interfaces next.


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


  reply	other threads:[~2025-11-06 12:06 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-10-30 11:06 [pve-devel] [PATCH installer v2 00/15] support network interface name pinning Christoph Heiss
2025-10-30 11:06 ` [pve-devel] [PATCH installer v2 01/15] test: parse-kernel-cmdline: fix module import statement Christoph Heiss
2025-10-30 11:06 ` [pve-devel] [PATCH installer v2 02/15] install: add support for network interface name pinning Christoph Heiss
2025-10-30 11:06 ` [pve-devel] [PATCH installer v2 03/15] run env: network: add kernel driver name to network interface info Christoph Heiss
2025-10-30 11:06 ` [pve-devel] [PATCH installer v2 04/15] common: utils: fix clippy warnings Christoph Heiss
2025-10-30 11:06 ` [pve-devel] [PATCH installer v2 05/15] common: setup: simplify network address list serialization Christoph Heiss
2025-10-30 11:06 ` [pve-devel] [PATCH installer v2 06/15] common: implement support for `network_interface_pin_map` config Christoph Heiss
2025-11-04 13:55   ` Michael Köppl
2025-10-30 11:06 ` [pve-devel] [PATCH installer v2 07/15] auto: add support for pinning network interface names Christoph Heiss
2025-10-30 11:06 ` [pve-devel] [PATCH installer v2 08/15] assistant: verify network settings in `validate-answer` subcommand Christoph Heiss
2025-10-30 11:06 ` [pve-devel] [PATCH installer v2 09/15] post-hook: avoid redundant Option<bool> for (de-)serialization Christoph Heiss
2025-10-30 11:06 ` [pve-devel] [PATCH installer v2 10/15] post-hook: add network interface name and pinning status Christoph Heiss
2025-10-30 11:06 ` [pve-devel] [PATCH installer v2 11/15] tui: views: move network options view to own module Christoph Heiss
2025-10-30 11:06 ` [pve-devel] [PATCH installer v2 12/15] tui: views: form: allow attaching user-defined data to children Christoph Heiss
2025-10-30 11:06 ` [pve-devel] [PATCH installer v2 13/15] tui: add support for pinning network interface names Christoph Heiss
2025-10-30 11:06 ` [pve-devel] [PATCH installer v2 14/15] ui: gtk3: allow passing of dialog parent window Christoph Heiss
2025-10-30 11:06 ` [pve-devel] [PATCH installer v2 15/15] gui: add support for pinning network interface names Christoph Heiss
2025-11-06  9:15   ` Stoiko Ivanov
2025-11-06 12:06     ` Christoph Heiss [this message]
2025-11-04 15:38 ` [pve-devel] [PATCH installer v2 00/15] support network interface name pinning Michael Köppl

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=DE1LKMPZF235.K1M3NSF1M8EU@proxmox.com \
    --to=c.heiss@proxmox.com \
    --cc=pve-devel@lists.proxmox.com \
    --cc=s.ivanov@proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal