public inbox for pmg-devel@lists.proxmox.com
 help / color / mirror / Atom feed
From: Christoph Heiss <c.heiss@proxmox.com>
To: Stoiko Ivanov <s.ivanov@proxmox.com>
Cc: pmg-devel@lists.proxmox.com
Subject: Re: [pmg-devel] [PATCH pmg-api] fix #4410: Remove non-null host-bits from CIDR when reading `mynetworks`
Date: Tue, 27 Dec 2022 10:27:32 +0100	[thread overview]
Message-ID: <7d9764c5-59d7-ad7f-e5b1-d08ffc0a414a@proxmox.com> (raw)
In-Reply-To: <20221222162501.1cd288c5@rosa.proxmox.com>

On 12/22/22 16:25, Stoiko Ivanov wrote:
> Huge thanks for addressing this!
It was a nice opportunity to get into Perl and PMG too :^)

> 
> I like the approach in general - two comments inline:
> 
> On Thu, 22 Dec 2022 11:19:40 +0100
> Christoph Heiss <c.heiss@proxmox.com> wrote:
> 
[..]
>> @@ -1008,13 +1009,13 @@ sub read_pmg_mynetworks {
>>   	while (defined(my $line = <$fh>)) {
>>   	    chomp $line;
>>   	    next if $line =~ m/^\s*$/;
>> -	    if ($line =~ m!^((?:$IPV4RE|$IPV6RE))/(\d+)\s*(?:#(.*)\s*)?$!) {
>> -		my ($network, $prefix_size, $comment) = ($1, $2, $3);
>> -		my $cidr = "$network/${prefix_size}";
>> -		$mynetworks->{$cidr} = {
>> -		    cidr => $cidr,
>> -		    network_address => $network,
>> -		    prefix_size => $prefix_size,
>> +	    if ($line =~ m!^((?:$IPV4RE|$IPV6RE)/\d+)\s*(?:#(.*)\s*)?$!) {
>> +		my ($cidr, $comment) = ($1, $2);
>> +		my $ip = PVE::Network::IP_from_cidr($cidr);
> this call expands the prefix to full-length - which I wouldn't have
> noticed for ipv4 - but is quite visible with ipv6:
> entering `2001:db8::/32` results in
> `2001:0db8:0000:0000:0000:0000:0000:0000/32`
> IIUC - Net::IP::ip_compress_prefix($ip->prefix(), $ip->version()) might
> be an approach - but even that adds the last quad of zeros...
> 
> If at all possible it would be great to keep the data as the user entered it.
> (In this case - in all situations where it's actually a valid prefix w/o
> host-bits set)
Ack, I didn't really test it all that extensively with IPv6. I'll look 
into it again and send a v2.

> 
> 
> 
>> +		$mynetworks->{$ip->prefix()} = {
>> +		    cidr => $ip->prefix(),
>> +		    network_address => $ip->ip(),
>> +		    prefix_size => $ip->prefixlen(),
>>   		    comment => $comment // '',
>>   		};
>>   	    } else {
>> @@ -1336,11 +1337,11 @@ sub get_template_vars {
>>       }
>>
>>       my $netlist = PVE::INotify::read_file('mynetworks');
>> -    foreach my $cidr (keys %$netlist) {
>> -	if ($cidr =~ m/^($IPV6RE)\/(\d+)$/) {
>> +    foreach my $ip (values %$netlist) {
> why switch here to iterating over the values - and then accessing the cidr
> field twice, if it's by construction above the same as the key?
This was a left-over from when working on the code. I'll remove it.

> 
>> +	if ($ip->{cidr} =~ m/^($IPV6RE)\/(\d+)$/) {
>>   	    $mynetworks->{"[$1]/$2"} = 1;
>>   	} else {
>> -	    $mynetworks->{$cidr} = 1;
>> +	    $mynetworks->{$ip->{cidr}} = 1;
>>   	}
>>       }
>>
>> --
>> 2.30.2
>>
>>
>>
>> _______________________________________________
>> pmg-devel mailing list
>> pmg-devel@lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pmg-devel
>>
>>
> 




      reply	other threads:[~2022-12-27  9:28 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-12-22 10:19 Christoph Heiss
2022-12-22 15:25 ` Stoiko Ivanov
2022-12-27  9:27   ` Christoph Heiss [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=7d9764c5-59d7-ad7f-e5b1-d08ffc0a414a@proxmox.com \
    --to=c.heiss@proxmox.com \
    --cc=pmg-devel@lists.proxmox.com \
    --cc=s.ivanov@proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal