Re: [pve-devel] [PATCH common v4 2/7] fix #3893: network: add vlan id and range parameter definitions

[Fiona Ebner <f.ebner@proxmox.com>]

Am 29.07.24 um 13:55 schrieb Aaron Lauterer:
> @@ -595,6 +601,34 @@ sub pve_verify_iface {
>      return $id;
>  }
>  
> +# vlan id (vids), single or range
> +register_format('pve-vlan-id-or-range', \&pve_verify_vlan_id_or_range);
> +sub pve_verify_vlan_id_or_range {
> +    my ($vlan, $noerr) = @_;
> +
> +    my $check_vid = sub {
> +	my $tag = shift;
> +	if ( $tag < 2 || $tag > 4094) {

Style nit: extra space after parenthesis

> +	    return undef if $noerr;
> +	    die "invalid VLAN tag '$tag'\n";
> +	}
> +    };
> +
> +    if ($vlan !~ m/^(\d+)(?:-(\d+))?$/) {
> +	return undef if $noerr;
> +	die "invalid VLAN configuration '$vlan'\n";
> +    }
> +    my $start = $1;
> +    my $end = $2;
> +    return undef if (!defined $check_vid->($start));

check_vid does not explicitly return anything in the success case, so
this check seems very brittle and I guess relies on implicit return?

Style nits:
parentheses for post-if
missing parentheses for defined()
perlcritic prefers 'return' over 'return undef'

> +    if ($end) {
> +	return undef if (!defined $check_vid->($end));
> +	die "VLAN range must go from lower to higher tag '$vlan'" if $start >= $end && !$noerr;

What if $noerr is set, but $start >= $end? You want to have a 'return'
then, right?

> +    }
> +
> +    return $vlan;
> +}
> +
>  # general addresses by name or IP
>  register_format('address', \&pve_verify_address);
>  sub pve_verify_address {


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