[pve-devel] [PATCH v2 common 1/4] fix #3893: network: make bridge vids configurable

[pve-devel] [PATCH v2 common 1/4] fix #3893: network: make bridge vids configurable

[Aaron Lauterer]

For that we need to add a new format option that checks against valid
VLAN tags and ranges, for example: 2 4 100-200

The check, if the default value should be used, needs to fail not just
when not defined, but also in case it is an empty string.

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
no changes since v1.

I think replacing the 'defined' check with 'length' should be fine. We
need to also handle the situation that the parameter is defined, but an
empty string. There should be no autovivification happening. If I missed
a side effect, let me know.

For the new format option I went with singular for the name as it only
checks a single VLAN ID/range from the list, 'pve-bridge-vid', but I am
not sure if it wouldn't be better to call it the actual parameter name
'pve-bridge-vids'.

 src/PVE/INotify.pm    |  2 +-
 src/PVE/JSONSchema.pm | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/src/PVE/INotify.pm b/src/PVE/INotify.pm
index bc33a8f..14f40ac 100644
--- a/src/PVE/INotify.pm
+++ b/src/PVE/INotify.pm
@@ -1270,7 +1270,7 @@ sub __interface_to_string {
 
 	if (defined($d->{bridge_vlan_aware})) {
 	    $raw .= "\tbridge-vlan-aware yes\n";
-	    my $vlans = defined($d->{bridge_vids}) ? $d->{bridge_vids} : "2-4094";
+	    my $vlans = length($d->{bridge_vids}) ? $d->{bridge_vids} : "2-4094";
 	    $raw .= "\tbridge-vids $vlans\n";
 	}
 	$done->{bridge_vlan_aware} = 1;
diff --git a/src/PVE/JSONSchema.pm b/src/PVE/JSONSchema.pm
index 85d47f2..1051a45 100644
--- a/src/PVE/JSONSchema.pm
+++ b/src/PVE/JSONSchema.pm
@@ -78,6 +78,12 @@ register_standard_option('pve-iface', {
     minLength => 2, maxLength => 20,
 });
 
+register_standard_option('pve-bridge-vid', {
+    description => "Bridge VLAN ID.",
+    type => 'string', format => 'pve-bridge-vid',
+    minLength => 1, maxLength => 9,
+});
+
 register_standard_option('pve-storage-id', {
     description => "The storage identifier.",
     type => 'string', format => 'pve-storage-id',
@@ -588,6 +594,32 @@ sub pve_verify_iface {
     return $id;
 }
 
+# bridge vlan id (vids)
+register_format('pve-bridge-vid', \&pve_verify_bridge_vid);
+sub pve_verify_bridge_vid {
+    my ($vlan, $noerr) = @_;
+
+    my $check_vid = sub {
+	my $id = shift;
+	if ( $id < 2 || $id > 4094) {
+	    return undef if $noerr;
+	    die "invalid VLAN tag '$id'\n";
+	}
+    };
+
+    if ($vlan !~ m/^(\d+)([-](\d+))?$/i) {
+	return undef if $noerr;
+	die "invalid VLAN configuration '$vlan'\n";
+    }
+    $check_vid->($1);
+    if ($3) {
+	$check_vid->($3);
+	die "VLAN range must go from lower to higher tag '$vlan'" if $1 > $3 && !$noerr;
+    }
+
+    return $vlan;
+}
+
 # general addresses by name or IP
 register_format('address', \&pve_verify_address);
 sub pve_verify_address {
-- 
2.39.2

[pve-devel] [PATCH widget-toolkit 2/4] fix #3892: Network: add bridge vids field for bridge_vids

[Aaron Lauterer]

The new optional bridge_vids field allows to set that property via the
GUI. Since the backend needs to support it, the field needs to be
explicitly enabled.

For now, Proxmox VE (PVE) is the use case.

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
changes since v2:

add a new option to the NodeNetworkEdit widget called 'bridge_set_vids'
which determines if the field will be added. We need to pass it through
the NetworkView widget as well.

 src/node/NetworkEdit.js | 23 +++++++++++++++++++++++
 src/node/NetworkView.js |  5 +++++
 2 files changed, 28 insertions(+)

diff --git a/src/node/NetworkEdit.js b/src/node/NetworkEdit.js
index bb9add3..799ca1e 100644
--- a/src/node/NetworkEdit.js
+++ b/src/node/NetworkEdit.js
@@ -2,6 +2,9 @@ Ext.define('Proxmox.node.NetworkEdit', {
     extend: 'Proxmox.window.Edit',
     alias: ['widget.proxmoxNodeNetworkEdit'],
 
+    // Enable to show the VLAN ID field
+    bridge_set_vids: false,
+
     initComponent: function() {
 	let me = this;
 
@@ -57,11 +60,28 @@ Ext.define('Proxmox.node.NetworkEdit', {
 	}
 
 	if (me.iftype === 'bridge') {
+	    let vids = Ext.create('Ext.form.field.Text', {
+		fieldLabel: gettext('Bridge VIDS'),
+		name: 'bridge_vids',
+		emptyText: '2-4094',
+		disabled: true,
+		autoEl: {
+		    tag: 'div',
+		    'data-qtip': gettext('Space-separated list of VLANs and ranges, for example: 2 4 100-200'),
+		},
+	    });
 	    column2.push({
 		xtype: 'proxmoxcheckbox',
 		fieldLabel: gettext('VLAN aware'),
 		name: 'bridge_vlan_aware',
 		deleteEmpty: !me.isCreate,
+		listeners: {
+		    change: function(f, newVal) {
+			if (me.bridge_set_vids) {
+			    vids.setDisabled(!newVal);
+			}
+		    },
+		},
 	    });
 	    column2.push({
 		xtype: 'textfield',
@@ -72,6 +92,9 @@ Ext.define('Proxmox.node.NetworkEdit', {
 		    'data-qtip': gettext('Space-separated list of interfaces, for example: enp0s0 enp1s0'),
 		},
 	    });
+	    if (me.bridge_set_vids) {
+		advancedColumn2.push(vids);
+	    }
 	} else if (me.iftype === 'OVSBridge') {
 	    column2.push({
 		xtype: 'textfield',
diff --git a/src/node/NetworkView.js b/src/node/NetworkView.js
index 1d67ac8..c08cbfa 100644
--- a/src/node/NetworkView.js
+++ b/src/node/NetworkView.js
@@ -33,6 +33,9 @@ Ext.define('Proxmox.node.NetworkView', {
 
     showApplyBtn: false,
 
+    // decides if VLAN IDs field for bridges is shown, depends on the product if needed
+    bridge_set_vids: false,
+
     initComponent: function() {
 	let me = this;
 
@@ -100,6 +103,7 @@ Ext.define('Proxmox.node.NetworkView', {
 		nodename: me.nodename,
 		iface: rec.data.iface,
 		iftype: rec.data.type,
+		bridge_set_vids: me.bridge_set_vids,
 		listeners: {
 		    destroy: () => reload(),
 		},
@@ -170,6 +174,7 @@ Ext.define('Proxmox.node.NetworkView', {
 		    nodename: me.nodename,
 		    iftype: iType,
 		    iface_default: findNextFreeInterfaceId(iDefault ?? iType),
+		    bridge_set_vids: me.bridge_set_vids,
 		    onlineHelp: 'sysadmin_network_configuration',
 		    listeners: {
 			destroy: () => reload(),
-- 
2.39.2

[pve-devel] [PATCH v2 manager 3/4] fix #3893: api: network: add bridge_vids parameter

[Aaron Lauterer]

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
no changes since v1

 PVE/API2/Network.pm | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/PVE/API2/Network.pm b/PVE/API2/Network.pm
index 00d964a7..6f4367cb 100644
--- a/PVE/API2/Network.pm
+++ b/PVE/API2/Network.pm
@@ -66,6 +66,11 @@ my $confdesc = {
 	type => 'boolean',
 	optional => 1,
     },
+    bridge_vids => {
+	description => "Specify the allowed vlans. For example: '2 4 100-200'. Only used if the bridge is VLAN aware.",
+	optional => 1,
+	type => 'string', format => 'pve-bridge-vid-list',
+    },
     bridge_ports => {
 	description => "Specify the interfaces you want to add to your bridge.",
 	optional => 1,
-- 
2.39.2

[pve-devel] [PATCH v2 manager 4/4] fix #3893: ui: network: enable bridge_vids field

[Aaron Lauterer]

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
 www/manager6/node/Config.js | 1 +
 1 file changed, 1 insertion(+)

diff --git a/www/manager6/node/Config.js b/www/manager6/node/Config.js
index 6ed2172a..77a6467c 100644
--- a/www/manager6/node/Config.js
+++ b/www/manager6/node/Config.js
@@ -179,6 +179,7 @@ Ext.define('PVE.node.Config', {
 		    showApplyBtn: true,
 		    groups: ['services'],
 		    nodename: nodename,
+		    bridge_set_vids: true,
 		    onlineHelp: 'sysadmin_network_configuration',
 		},
 		{
-- 
2.39.2

Re: [pve-devel] [PATCH widget-toolkit 2/4] fix #3892: Network: add bridge vids field for bridge_vids

[Aaron Lauterer]

oops.. typo in the commit title!

3893 and not 3892!

On 6/14/23 11:30, Aaron Lauterer wrote:
> The new optional bridge_vids field allows to set that property via the
> GUI. Since the backend needs to support it, the field needs to be
> explicitly enabled.
> 
> For now, Proxmox VE (PVE) is the use case.
> 
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
> changes since v2:
> 
> add a new option to the NodeNetworkEdit widget called 'bridge_set_vids'
> which determines if the field will be added. We need to pass it through
> the NetworkView widget as well.
> 
>   src/node/NetworkEdit.js | 23 +++++++++++++++++++++++
>   src/node/NetworkView.js |  5 +++++
>   2 files changed, 28 insertions(+)
> 
> diff --git a/src/node/NetworkEdit.js b/src/node/NetworkEdit.js
> index bb9add3..799ca1e 100644
> --- a/src/node/NetworkEdit.js
> +++ b/src/node/NetworkEdit.js
> @@ -2,6 +2,9 @@ Ext.define('Proxmox.node.NetworkEdit', {
>       extend: 'Proxmox.window.Edit',
>       alias: ['widget.proxmoxNodeNetworkEdit'],
>   
> +    // Enable to show the VLAN ID field
> +    bridge_set_vids: false,
> +
>       initComponent: function() {
>   	let me = this;
>   
> @@ -57,11 +60,28 @@ Ext.define('Proxmox.node.NetworkEdit', {
>   	}
>   
>   	if (me.iftype === 'bridge') {
> +	    let vids = Ext.create('Ext.form.field.Text', {
> +		fieldLabel: gettext('Bridge VIDS'),
> +		name: 'bridge_vids',
> +		emptyText: '2-4094',
> +		disabled: true,
> +		autoEl: {
> +		    tag: 'div',
> +		    'data-qtip': gettext('Space-separated list of VLANs and ranges, for example: 2 4 100-200'),
> +		},
> +	    });
>   	    column2.push({
>   		xtype: 'proxmoxcheckbox',
>   		fieldLabel: gettext('VLAN aware'),
>   		name: 'bridge_vlan_aware',
>   		deleteEmpty: !me.isCreate,
> +		listeners: {
> +		    change: function(f, newVal) {
> +			if (me.bridge_set_vids) {
> +			    vids.setDisabled(!newVal);
> +			}
> +		    },
> +		},
>   	    });
>   	    column2.push({
>   		xtype: 'textfield',
> @@ -72,6 +92,9 @@ Ext.define('Proxmox.node.NetworkEdit', {
>   		    'data-qtip': gettext('Space-separated list of interfaces, for example: enp0s0 enp1s0'),
>   		},
>   	    });
> +	    if (me.bridge_set_vids) {
> +		advancedColumn2.push(vids);
> +	    }
>   	} else if (me.iftype === 'OVSBridge') {
>   	    column2.push({
>   		xtype: 'textfield',
> diff --git a/src/node/NetworkView.js b/src/node/NetworkView.js
> index 1d67ac8..c08cbfa 100644
> --- a/src/node/NetworkView.js
> +++ b/src/node/NetworkView.js
> @@ -33,6 +33,9 @@ Ext.define('Proxmox.node.NetworkView', {
>   
>       showApplyBtn: false,
>   
> +    // decides if VLAN IDs field for bridges is shown, depends on the product if needed
> +    bridge_set_vids: false,
> +
>       initComponent: function() {
>   	let me = this;
>   
> @@ -100,6 +103,7 @@ Ext.define('Proxmox.node.NetworkView', {
>   		nodename: me.nodename,
>   		iface: rec.data.iface,
>   		iftype: rec.data.type,
> +		bridge_set_vids: me.bridge_set_vids,
>   		listeners: {
>   		    destroy: () => reload(),
>   		},
> @@ -170,6 +174,7 @@ Ext.define('Proxmox.node.NetworkView', {
>   		    nodename: me.nodename,
>   		    iftype: iType,
>   		    iface_default: findNextFreeInterfaceId(iDefault ?? iType),
> +		    bridge_set_vids: me.bridge_set_vids,
>   		    onlineHelp: 'sysadmin_network_configuration',
>   		    listeners: {
>   			destroy: () => reload(),

Re: [pve-devel] [PATCH v2 common 1/4] fix #3893: network: make bridge vids configurable

[Aaron Lauterer]

ping? patches still apply cleanly

On 6/14/23 11:30, Aaron Lauterer wrote:
> For that we need to add a new format option that checks against valid
> VLAN tags and ranges, for example: 2 4 100-200
> 
> The check, if the default value should be used, needs to fail not just
> when not defined, but also in case it is an empty string.
> 
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
> no changes since v1.
> 
> I think replacing the 'defined' check with 'length' should be fine. We
> need to also handle the situation that the parameter is defined, but an
> empty string. There should be no autovivification happening. If I missed
> a side effect, let me know.
> 
> For the new format option I went with singular for the name as it only
> checks a single VLAN ID/range from the list, 'pve-bridge-vid', but I am
> not sure if it wouldn't be better to call it the actual parameter name
> 'pve-bridge-vids'.
> 
>   src/PVE/INotify.pm    |  2 +-
>   src/PVE/JSONSchema.pm | 32 ++++++++++++++++++++++++++++++++
>   2 files changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/src/PVE/INotify.pm b/src/PVE/INotify.pm
> index bc33a8f..14f40ac 100644
> --- a/src/PVE/INotify.pm
> +++ b/src/PVE/INotify.pm
> @@ -1270,7 +1270,7 @@ sub __interface_to_string {
>   
>   	if (defined($d->{bridge_vlan_aware})) {
>   	    $raw .= "\tbridge-vlan-aware yes\n";
> -	    my $vlans = defined($d->{bridge_vids}) ? $d->{bridge_vids} : "2-4094";
> +	    my $vlans = length($d->{bridge_vids}) ? $d->{bridge_vids} : "2-4094";
>   	    $raw .= "\tbridge-vids $vlans\n";
>   	}
>   	$done->{bridge_vlan_aware} = 1;
> diff --git a/src/PVE/JSONSchema.pm b/src/PVE/JSONSchema.pm
> index 85d47f2..1051a45 100644
> --- a/src/PVE/JSONSchema.pm
> +++ b/src/PVE/JSONSchema.pm
> @@ -78,6 +78,12 @@ register_standard_option('pve-iface', {
>       minLength => 2, maxLength => 20,
>   });
>   
> +register_standard_option('pve-bridge-vid', {
> +    description => "Bridge VLAN ID.",
> +    type => 'string', format => 'pve-bridge-vid',
> +    minLength => 1, maxLength => 9,
> +});
> +
>   register_standard_option('pve-storage-id', {
>       description => "The storage identifier.",
>       type => 'string', format => 'pve-storage-id',
> @@ -588,6 +594,32 @@ sub pve_verify_iface {
>       return $id;
>   }
>   
> +# bridge vlan id (vids)
> +register_format('pve-bridge-vid', \&pve_verify_bridge_vid);
> +sub pve_verify_bridge_vid {
> +    my ($vlan, $noerr) = @_;
> +
> +    my $check_vid = sub {
> +	my $id = shift;
> +	if ( $id < 2 || $id > 4094) {
> +	    return undef if $noerr;
> +	    die "invalid VLAN tag '$id'\n";
> +	}
> +    };
> +
> +    if ($vlan !~ m/^(\d+)([-](\d+))?$/i) {
> +	return undef if $noerr;
> +	die "invalid VLAN configuration '$vlan'\n";
> +    }
> +    $check_vid->($1);
> +    if ($3) {
> +	$check_vid->($3);
> +	die "VLAN range must go from lower to higher tag '$vlan'" if $1 > $3 && !$noerr;
> +    }
> +
> +    return $vlan;
> +}
> +
>   # general addresses by name or IP
>   register_format('address', \&pve_verify_address);
>   sub pve_verify_address {

Re: [pve-devel] [PATCH v2 manager 3/4] fix #3893: api: network: add bridge_vids parameter

[Fabian Grünbichler]

On June 14, 2023 11:30 am, Aaron Lauterer wrote:
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
> no changes since v1
> 
>  PVE/API2/Network.pm | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/PVE/API2/Network.pm b/PVE/API2/Network.pm
> index 00d964a7..6f4367cb 100644
> --- a/PVE/API2/Network.pm
> +++ b/PVE/API2/Network.pm
> @@ -66,6 +66,11 @@ my $confdesc = {
>  	type => 'boolean',
>  	optional => 1,
>      },
> +    bridge_vids => {
> +	description => "Specify the allowed vlans. For example: '2 4 100-200'. Only used if the bridge is VLAN aware.",
> +	optional => 1,
> +	type => 'string', format => 'pve-bridge-vid-list',

unfortunately, pve-bridge-vid-list allows more than intended. we'd
either need to define a custom type for the lists, or improve the
handling. a `-list` format under the hood calls PVE::Tools::split_list,
which splits on zero bytes if the string contains one, or else on ',',
';' and whitespace.. and at least ';' leads to ifreload not applying the
VLAN settings (with a warning that is very easy to miss, since it
doesn't show up in the task log or task status at all, you only see it
if you run `ifreload -av`!).

two possible solutions:
- use a custom format here, split and call the verify helper from
  pve-common
- use -list here, but map the other delimiters to ' ' either here or in
  pve-common

  side-note: this is basically the "trunks" format, but with a different
  delimiter, couldn't we have a common option/format and just change the
  delimiter based on what we pass it along to?

> +    },
>      bridge_ports => {
>  	description => "Specify the interfaces you want to add to your bridge.",
>  	optional => 1,
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

Re: [pve-devel] [PATCH widget-toolkit 2/4] fix #3892: Network: add bridge vids field for bridge_vids

[Fabian Grünbichler]

On June 14, 2023 11:30 am, Aaron Lauterer wrote:
> The new optional bridge_vids field allows to set that property via the
> GUI. Since the backend needs to support it, the field needs to be
> explicitly enabled.
> 
> For now, Proxmox VE (PVE) is the use case.
> 
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
> changes since v2:
> 
> add a new option to the NodeNetworkEdit widget called 'bridge_set_vids'
> which determines if the field will be added. We need to pass it through
> the NetworkView widget as well.
> 
>  src/node/NetworkEdit.js | 23 +++++++++++++++++++++++
>  src/node/NetworkView.js |  5 +++++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/src/node/NetworkEdit.js b/src/node/NetworkEdit.js
> index bb9add3..799ca1e 100644
> --- a/src/node/NetworkEdit.js
> +++ b/src/node/NetworkEdit.js
> @@ -2,6 +2,9 @@ Ext.define('Proxmox.node.NetworkEdit', {
>      extend: 'Proxmox.window.Edit',
>      alias: ['widget.proxmoxNodeNetworkEdit'],
>  
> +    // Enable to show the VLAN ID field
> +    bridge_set_vids: false,
> +
>      initComponent: function() {
>  	let me = this;
>  
> @@ -57,11 +60,28 @@ Ext.define('Proxmox.node.NetworkEdit', {
>  	}
>  
>  	if (me.iftype === 'bridge') {
> +	    let vids = Ext.create('Ext.form.field.Text', {
> +		fieldLabel: gettext('Bridge VIDS'),
> +		name: 'bridge_vids',
> +		emptyText: '2-4094',
> +		disabled: true,
> +		autoEl: {
> +		    tag: 'div',
> +		    'data-qtip': gettext('Space-separated list of VLANs and ranges, for example: 2 4 100-200'),
> +		},
> +	    });

we might want to validate the format here as well to give feedback right
away?

>  	    column2.push({
>  		xtype: 'proxmoxcheckbox',
>  		fieldLabel: gettext('VLAN aware'),
>  		name: 'bridge_vlan_aware',
>  		deleteEmpty: !me.isCreate,
> +		listeners: {
> +		    change: function(f, newVal) {
> +			if (me.bridge_set_vids) {
> +			    vids.setDisabled(!newVal);
> +			}
> +		    },
> +		},
>  	    });
>  	    column2.push({
>  		xtype: 'textfield',
> @@ -72,6 +92,9 @@ Ext.define('Proxmox.node.NetworkEdit', {
>  		    'data-qtip': gettext('Space-separated list of interfaces, for example: enp0s0 enp1s0'),
>  		},
>  	    });
> +	    if (me.bridge_set_vids) {
> +		advancedColumn2.push(vids);
> +	    }
>  	} else if (me.iftype === 'OVSBridge') {
>  	    column2.push({
>  		xtype: 'textfield',
> diff --git a/src/node/NetworkView.js b/src/node/NetworkView.js
> index 1d67ac8..c08cbfa 100644
> --- a/src/node/NetworkView.js
> +++ b/src/node/NetworkView.js
> @@ -33,6 +33,9 @@ Ext.define('Proxmox.node.NetworkView', {
>  
>      showApplyBtn: false,
>  
> +    // decides if VLAN IDs field for bridges is shown, depends on the product if needed
> +    bridge_set_vids: false,
> +
>      initComponent: function() {
>  	let me = this;
>  
> @@ -100,6 +103,7 @@ Ext.define('Proxmox.node.NetworkView', {
>  		nodename: me.nodename,
>  		iface: rec.data.iface,
>  		iftype: rec.data.type,
> +		bridge_set_vids: me.bridge_set_vids,
>  		listeners: {
>  		    destroy: () => reload(),
>  		},
> @@ -170,6 +174,7 @@ Ext.define('Proxmox.node.NetworkView', {
>  		    nodename: me.nodename,
>  		    iftype: iType,
>  		    iface_default: findNextFreeInterfaceId(iDefault ?? iType),
> +		    bridge_set_vids: me.bridge_set_vids,
>  		    onlineHelp: 'sysadmin_network_configuration',
>  		    listeners: {
>  			destroy: () => reload(),
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

Re: [pve-devel] [PATCH v2 common 1/4] fix #3893: network: make bridge vids configurable

[Fabian Grünbichler]

On June 14, 2023 11:30 am, Aaron Lauterer wrote:
> For that we need to add a new format option that checks against valid
> VLAN tags and ranges, for example: 2 4 100-200
> 
> The check, if the default value should be used, needs to fail not just
> when not defined, but also in case it is an empty string.
> 
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
> no changes since v1.
> 
> I think replacing the 'defined' check with 'length' should be fine. We
> need to also handle the situation that the parameter is defined, but an
> empty string. There should be no autovivification happening. If I missed
> a side effect, let me know.
> 
> For the new format option I went with singular for the name as it only
> checks a single VLAN ID/range from the list, 'pve-bridge-vid', but I am
> not sure if it wouldn't be better to call it the actual parameter name
> 'pve-bridge-vids'.

I think the "count" of the name is fine, if at all, you might want to
add the "or range" part.

ifupdown2 calls the (full) format <number-comma-range-list> ;)

>  src/PVE/INotify.pm    |  2 +-
>  src/PVE/JSONSchema.pm | 32 ++++++++++++++++++++++++++++++++
>  2 files changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/src/PVE/INotify.pm b/src/PVE/INotify.pm
> index bc33a8f..14f40ac 100644
> --- a/src/PVE/INotify.pm
> +++ b/src/PVE/INotify.pm
> @@ -1270,7 +1270,7 @@ sub __interface_to_string {
>  
>  	if (defined($d->{bridge_vlan_aware})) {
>  	    $raw .= "\tbridge-vlan-aware yes\n";
> -	    my $vlans = defined($d->{bridge_vids}) ? $d->{bridge_vids} : "2-4094";
> +	    my $vlans = length($d->{bridge_vids}) ? $d->{bridge_vids} : "2-4094";
>  	    $raw .= "\tbridge-vids $vlans\n";
>  	}
>  	$done->{bridge_vlan_aware} = 1;
> diff --git a/src/PVE/JSONSchema.pm b/src/PVE/JSONSchema.pm
> index 85d47f2..1051a45 100644
> --- a/src/PVE/JSONSchema.pm
> +++ b/src/PVE/JSONSchema.pm
> @@ -78,6 +78,12 @@ register_standard_option('pve-iface', {
>      minLength => 2, maxLength => 20,
>  });
>  
> +register_standard_option('pve-bridge-vid', {
> +    description => "Bridge VLAN ID.",
> +    type => 'string', format => 'pve-bridge-vid',
> +    minLength => 1, maxLength => 9,
> +});
> +
>  register_standard_option('pve-storage-id', {
>      description => "The storage identifier.",
>      type => 'string', format => 'pve-storage-id',
> @@ -588,6 +594,32 @@ sub pve_verify_iface {
>      return $id;
>  }
>  
> +# bridge vlan id (vids)
> +register_format('pve-bridge-vid', \&pve_verify_bridge_vid);
> +sub pve_verify_bridge_vid {
> +    my ($vlan, $noerr) = @_;
> +
> +    my $check_vid = sub {
> +	my $id = shift;
> +	if ( $id < 2 || $id > 4094) {

nit style (spacing around ())

> +	    return undef if $noerr;

this does not work - the undef would not be propagated up since you
don't check the return value below..

> +	    die "invalid VLAN tag '$id'\n";

tag vs ID - it's a bit weird that both are used interchangably..

> +	}
> +    };
> +
> +    if ($vlan !~ m/^(\d+)([-](\d+))?$/i) {
> +	return undef if $noerr;
> +	die "invalid VLAN configuration '$vlan'\n";
> +    }
> +    $check_vid->($1);
> +    if ($3) {
> +	$check_vid->($3);
> +	die "VLAN range must go from lower to higher tag '$vlan'" if $1 > $3 && !$noerr;
> +    }

not a huge fan of all the $N here.. could we get named variables please?
$start, $end; or $first, $last ;)

> +
> +    return $vlan;
> +}
> +
>  # general addresses by name or IP
>  register_format('address', \&pve_verify_address);
>  sub pve_verify_address {
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

Re: [pve-devel] [PATCH v2 common 1/4] fix #3893: network: make bridge vids configurable

[Aaron Lauterer]

new patch series (v3) is available
https://lists.proxmox.com/pipermail/pve-devel/2024-July/064388.html

On  2023-06-14  11:30, Aaron Lauterer wrote:
> For that we need to add a new format option that checks against valid
> VLAN tags and ranges, for example: 2 4 100-200
> 
> The check, if the default value should be used, needs to fail not just
> when not defined, but also in case it is an empty string.
> 
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
> no changes since v1.
> 
> I think replacing the 'defined' check with 'length' should be fine. We
> need to also handle the situation that the parameter is defined, but an
> empty string. There should be no autovivification happening. If I missed
> a side effect, let me know.
> 
> For the new format option I went with singular for the name as it only
> checks a single VLAN ID/range from the list, 'pve-bridge-vid', but I am
> not sure if it wouldn't be better to call it the actual parameter name
> 'pve-bridge-vids'.
> 
>   src/PVE/INotify.pm    |  2 +-
>   src/PVE/JSONSchema.pm | 32 ++++++++++++++++++++++++++++++++
>   2 files changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/src/PVE/INotify.pm b/src/PVE/INotify.pm
> index bc33a8f..14f40ac 100644
> --- a/src/PVE/INotify.pm
> +++ b/src/PVE/INotify.pm
> @@ -1270,7 +1270,7 @@ sub __interface_to_string {
>   
>   	if (defined($d->{bridge_vlan_aware})) {
>   	    $raw .= "\tbridge-vlan-aware yes\n";
> -	    my $vlans = defined($d->{bridge_vids}) ? $d->{bridge_vids} : "2-4094";
> +	    my $vlans = length($d->{bridge_vids}) ? $d->{bridge_vids} : "2-4094";
>   	    $raw .= "\tbridge-vids $vlans\n";
>   	}
>   	$done->{bridge_vlan_aware} = 1;
> diff --git a/src/PVE/JSONSchema.pm b/src/PVE/JSONSchema.pm
> index 85d47f2..1051a45 100644
> --- a/src/PVE/JSONSchema.pm
> +++ b/src/PVE/JSONSchema.pm
> @@ -78,6 +78,12 @@ register_standard_option('pve-iface', {
>       minLength => 2, maxLength => 20,
>   });
>   
> +register_standard_option('pve-bridge-vid', {
> +    description => "Bridge VLAN ID.",
> +    type => 'string', format => 'pve-bridge-vid',
> +    minLength => 1, maxLength => 9,
> +});
> +
>   register_standard_option('pve-storage-id', {
>       description => "The storage identifier.",
>       type => 'string', format => 'pve-storage-id',
> @@ -588,6 +594,32 @@ sub pve_verify_iface {
>       return $id;
>   }
>   
> +# bridge vlan id (vids)
> +register_format('pve-bridge-vid', \&pve_verify_bridge_vid);
> +sub pve_verify_bridge_vid {
> +    my ($vlan, $noerr) = @_;
> +
> +    my $check_vid = sub {
> +	my $id = shift;
> +	if ( $id < 2 || $id > 4094) {
> +	    return undef if $noerr;
> +	    die "invalid VLAN tag '$id'\n";
> +	}
> +    };
> +
> +    if ($vlan !~ m/^(\d+)([-](\d+))?$/i) {
> +	return undef if $noerr;
> +	die "invalid VLAN configuration '$vlan'\n";
> +    }
> +    $check_vid->($1);
> +    if ($3) {
> +	$check_vid->($3);
> +	die "VLAN range must go from lower to higher tag '$vlan'" if $1 > $3 && !$noerr;
> +    }
> +
> +    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