Re: [PATCH pve-guest-common v5 3/7] add module to track previously used guest IDs

["Michael Köppl" <m.koeppl@proxmox.com>]

On Tue Sep 22, 2026 at 4:22 PM CEST, Fiona Ebner wrote:

[snip]

>> diff --git a/src/PVE/UsedGuestIDs.pm b/src/PVE/UsedGuestIDs.pm
>> new file mode 100644
>> index 0000000..31036ff
>> --- /dev/null
>> +++ b/src/PVE/UsedGuestIDs.pm
>> @@ -0,0 +1,93 @@
>> +package PVE::UsedGuestIDs;
>
> Nit: maybe GuestID as a more general name? That would suggest moving or
> adding other stuff related to guest IDs here in the future. Otherwise,
> the module is quite specific. We would need to rename add_id() to
> add_used_id(), read_list() -> list_used_ids() for clarity then.

I think I'd also prefer PVE::GuestID here to avoid possibly having
another module later on or having to refactor this one. Will adapt for
v6!

>
>> +
>> +use strict;
>> +use warnings;
>
> Nit: I now this got picked up from before, but it would be nice to use
> v5.36; and function signatures for new modules.

will adapt for v6, thanks!

>
>> +
>> +use PVE::Cluster qw(
>> +    cfs_lock_file
>> +    cfs_read_file
>> +    cfs_register_file
>> +    cfs_write_file
>> +);
>> +
>> +my $FILENAME = 'used-guest-ids';
>> +
>> +my $parse_id_list = sub {
>
> Nit: If writing with a signature, use 'my sub', and similarly for the
> other two private functions.

will also be updated for v6, thanks!

>
>> +    my ($filename, $raw) = @_;
>> +
>> +    my $used_ids = {};
>> +
>> +    return $used_ids if !defined($raw);
>> +
>> +    for my $line (split(/\n/, $raw)) {
>> +        next if $line =~ m/^\s*$/;
>> +
>> +        if ($line =~ m/^(\d+)$/) {
>> +            $used_ids->{$1} = 1;
>> +        } elsif ($line =~ m/^(\d+)-(\d+)$/) {
>> +            my ($start, $end) = ($1, $2);
>> +            if ($start > $end) {
>> +                warn "skipping reversed range in $filename: $line\n";
>> +                next;
>> +            }
>> +            $used_ids->{$_} = 1 for $start .. $end;
>
> Nit: we could be smarter and avoid adding every single ID to the hash
> (with a lot of (past) guests that can become a non-trivial cost), and
> instead track ranges. Instead of having a public read_list() function
> then, there could be a check_id_unused() function. When writing out the
> file we can still merge adjacent ranges.

Thanks for pointing this out! I think the range approach is a good idea!
Should we have a public function that returns the end of a range for a
given guest ID instead of the boolean check_id_unused()? We could then
use it to skip ranges when searching for a free guest ID at
/cluster/nextid instead of invoking cfs_read_file() for each check if a
candidate ID is available. Something like this:

my $i = $lower;
while ($i < $upper) {
    if (defined($idlist->{$i})) { $i++; next; }
    if ($check_unique) {
        if (defined(my $end =
		  PVE::UsedGuestIDs::get_used_range_end($i))) {
            $i = $end + 1;
            next;
        }
    }
    return $i;
}

>
>> +        } else {
>> +            warn "skipping invalid entry in $filename: $line\n";
>> +        }
>> +    }
>> +
>> +    return $used_ids;
>> +};
>> +
>> +my $format_entry = sub {
>> +    my ($start, $last) = @_;
>> +    return $start == $last ? "$start\n" : "$start-$last\n";
>> +};

[snip]