Re: [PATCH guest-common v6 04/18] add module to track previously used guest IDs

[Fiona Ebner <f.ebner@proxmox.com>]

Am 24.09.26 um 6:17 PM schrieb Michael Köppl:
> diff --git a/src/Makefile b/src/Makefile
> index 030e7f7..ceba738 100644
> --- a/src/Makefile
> +++ b/src/Makefile
> @@ -14,6 +14,7 @@ install: PVE
>  	install -m 0644 PVE/Replication.pm ${PERL5DIR}/PVE/
>  	install -m 0644 PVE/StorageTunnel.pm ${PERL5DIR}/PVE/
>  	install -m 0644 PVE/Tunnel.pm ${PERL5DIR}/PVE/
> +	install -m 0644 PVE/GuestID.pm ${PERL5DIR}/PVE/

Nit: could be ordered alphabetically

>  	install -d ${PERL5DIR}/PVE/Mapping
>  	install -m 0644 PVE/Mapping/Dir.pm ${PERL5DIR}/PVE/Mapping/
>  	install -m 0644 PVE/Mapping/PCI.pm ${PERL5DIR}/PVE/Mapping/
> diff --git a/src/PVE/GuestID.pm b/src/PVE/GuestID.pm

---snip 8<---

> +my sub insert_id($ranges, $id) {
> +    my $i = 0;
> +
> +    while ($i < @$ranges && $ranges->[$i]->[1] < $id) {

Style nit: I'd prefer having an explicit scalar()

> +        $i++;
> +    }
> +
> +    return 0 if $i < @$ranges && $ranges->[$i]->[0] <= $id;

Same here.

> +
> +    splice(@$ranges, $i, 0, [$id, $id]);
> +
> +    return 1;
> +}
> +
> +sub register_used_id($id) {
> +    cfs_lock_file(
> +        $FILENAME,
> +        10,
> +        sub {
> +            my $ranges = cfs_read_file($FILENAME);
> +
> +            return if !insert_id($ranges, $id);
> +
> +            cfs_write_file($FILENAME, $ranges);
> +        },
> +    );
> +

Style nit: I'd avoid the blank to make it clear that it's connected
(makes it harder for new code to accidentally sneak in in-between).

> +    if (my $err = $@) {
> +        my $dc_conf = cfs_read_file('datacenter.cfg');

Edge case, but we should error out if reading the datacenter config fails.

> +
> +        my $emsg = "unable to record guest ID $id as used";
> +        die "$emsg - $err" if $dc_conf->{'next-id'}->{unique};
> +        warn "$emsg - $err";
> +    }
> +}
> +
> +cfs_register_file($FILENAME, \&parse_id_list, \&write_id_list);
> +
> +1;