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

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On April 16, 2021 11:53 am, Thomas Lamprecht wrote:
> 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`?

mainly because of the pre-existing 'storagepair', but I am fine with 
either (and also with changing storagepair - this series already touches 
so many repos that change can be mixed in without extra churn I think).

same for the API parameter(s) (I recycled the existing 'targetstorage', 
although I'd find 'target-FOO' more readable and I am also fine with 
using that variant for the new API call for all parameters)

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