Re: [pbs-devel] [PATCH proxmox-backup 1/6] feature #3690: add regex, format & schema for partition names to pbs-api-types

[Lukas Wagner <l.wagner@proxmox.com>]

Hello, first some general remarks about this series:

  - the prefix for the commits should be 'fix #num:' or 'close #num:' [1]
  - patches 1, 2 and 6 are missing subsystem tags (e.g. for 1 this could 
be 'api-types')
  - A cover letter with some brief summary of the changes would be nice, 
so that I don't have to pull up the bugzilla entry first to see what 
this series is actually about.


The features seems to work as intended, I tested it on the latest 
master. A few things should be changed though before this goes in,
see the inline comments for the individual patches.

[1] 
https://pve.proxmox.com/wiki/Developer_Documentation#Commits_and_Commit_Messages


On 11/10/23 14:50, Markus Frank wrote:
> Signed-off-by: Markus Frank <m.frank@proxmox.com>
> ---
>   pbs-api-types/src/lib.rs | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/pbs-api-types/src/lib.rs b/pbs-api-types/src/lib.rs
> index 4306eca3..af8ad9cc 100644
> --- a/pbs-api-types/src/lib.rs
> +++ b/pbs-api-types/src/lib.rs
> @@ -191,6 +191,7 @@ const_regex! {
>       );
>   
>       pub BLOCKDEVICE_NAME_REGEX = r"^(?:(?:h|s|x?v)d[a-z]+)|(?:nvme\d+n\d+)$";
> +    pub BLOCKDEVICE_PARTITION_NAME_REGEX = r"^(?:(?:h|s|x?v)d[a-z]+\d*)|(?:nvme\d+n\d+(p\d+)?)$";

It appears that this regex also matches disks, not only partitions (e.g 
sda AND sda1 both match). From the rest of the code this seems intended, 
since you are also able to wipe disks AND partitions, but maybe the name 
for this constant should reflect that.

>       pub SUBSCRIPTION_KEY_REGEX = concat!(r"^pbs(?:[cbsp])-[0-9a-f]{10}$");
>   }
>   
> @@ -205,6 +206,8 @@ pub const PASSWORD_FORMAT: ApiStringFormat = ApiStringFormat::Pattern(&PASSWORD_
>   pub const UUID_FORMAT: ApiStringFormat = ApiStringFormat::Pattern(&UUID_REGEX);
>   pub const BLOCKDEVICE_NAME_FORMAT: ApiStringFormat =
>       ApiStringFormat::Pattern(&BLOCKDEVICE_NAME_REGEX);
> +pub const BLOCKDEVICE_PARTITION_NAME_FORMAT: ApiStringFormat =
> +    ApiStringFormat::Pattern(&BLOCKDEVICE_PARTITION_NAME_REGEX);
>   pub const SUBSCRIPTION_KEY_FORMAT: ApiStringFormat =
>       ApiStringFormat::Pattern(&SUBSCRIPTION_KEY_REGEX);
>   pub const SYSTEMD_DATETIME_FORMAT: ApiStringFormat =
> @@ -285,6 +288,13 @@ pub const BLOCKDEVICE_NAME_SCHEMA: Schema =
>           .max_length(64)
>           .schema();
>   
> +pub const BLOCKDEVICE_PARTITION_NAME_SCHEMA: Schema =
> +    StringSchema::new("(Partition) block device name (/sys/class/block/<name>).")
> +        .format(&BLOCKDEVICE_PARTITION_NAME_FORMAT)
> +        .min_length(3)
> +        .max_length(64)
> +        .schema();
> +
>   pub const DISK_ARRAY_SCHEMA: Schema =
>       ArraySchema::new("Disk name list.", &BLOCKDEVICE_NAME_SCHEMA).schema();
>   

-- 
- Lukas