Re: [pve-devel] [PATCH common 2/2] schema: add pve-bridge-id option/format/pair

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

On 13.04.21 14:16, Fabian Grünbichler wrote:
> for re-use in qemu-server/pve-container, which already have this option
> duplicated. the '-pair' is needed for remote migration, but can also be
> a nice addition to regular intra-cluster migration to lift the
> restriction of having identically named bridges.
> 

looks OK, one naming issue inline

> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
>  src/PVE/JSONSchema.pm | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/src/PVE/JSONSchema.pm b/src/PVE/JSONSchema.pm
> index f2ddb50..bf30b33 100644
> --- a/src/PVE/JSONSchema.pm
> +++ b/src/PVE/JSONSchema.pm
> @@ -82,6 +82,12 @@ register_standard_option('pve-storage-id', {
>      type => 'string', format => 'pve-storage-id',
>  });
>  
> +register_standard_option('pve-bridge-id', {
> +    description => "Bridge to attach guest network devices to.",
> +    type => 'string', format => 'pve-bridge-id',
> +    format_description => 'bridge',
> +});
> +
>  register_standard_option('pve-config-digest', {
>      description => 'Prevent changes if current configuration file has different SHA1 digest. This can be used to prevent concurrent modifications.',
>      type => 'string',
> @@ -193,6 +199,17 @@ sub parse_storage_id {
>      return parse_id($storeid, 'storage', $noerr);
>  }
>  
> +PVE::JSONSchema::register_format('pve-bridge-id', \&parse_bridge_id);
> +sub parse_bridge_id {
> +    my ($id, $noerr) = @_;
> +
> +    if ($id !~ m/^[-_.\w\d]+$/) {
> +	return undef if $noerr;
> +	die "invalid bridge ID '$id'\n";
> +    }
> +    return $id;
> +}
> +
>  PVE::JSONSchema::register_format('acme-plugin-id', \&parse_acme_plugin_id);
>  sub parse_acme_plugin_id {
>      my ($pluginid, $noerr) = @_;
> @@ -293,6 +310,14 @@ sub verify_storagepair {
>      my ($storagepair, $noerr) = @_;
>      return $verify_idpair->($storagepair, $noerr, 'pve-storage-id');
>  }
> +
> +# note: this only checks a single list entry
> +# when using a bridgepair-list map, you need to pass the full parameter to
> +# parse_idmap
> +register_format('bridgepair', \&verify_bridgepair);

pve-bridge-id vs. bridgepair seems slightly odd as syntax choice?

Why not `bridge-pair` or even `pve-bridge-pair`?

> +sub verify_bridgepair {
> +    my ($bridgepair, $noerr) = @_;
> +    return $verify_idpair->($bridgepair, $noerr, 'pve-bridge-id');
>  }
>  
>  register_format('mac-addr', \&pve_verify_mac_addr);
>