[pmg-devel] [PATCH pmg-api v2 1/5] fix rendering of ipv(4|6) literal lmtp transports

[Stoiko Ivanov]

On Wed, Mar 18, 2020 at 02:13:09PM +0100, Dominik Csapak wrote:
> Looks mostly good to me, a few questions/nits inline
> 
> > On March 18, 2020 11:23 AM Stoiko Ivanov <s.ivanov at proxmox.com> wrote:
> > 
> >  
> > While reviewing support for lmtp as transport one thing I forgot to test was
> > adding a lmtp-transport pointing to an IPv6 address.
> > 
> > Using the use_mx flag (which only makes sense for domain-names) to provide the
> > information of whether the next-hop/hostname should be written out in square
> > brackets or not is a bit confusing, and leads to ambiguous results when
> > providing ipv6 literal addresses:
> > host: 2001:db8:25::25
> > port: 24
> > gets rendered as
> > lmtp:inet:2001:db8:25::25:24
> > 
> > (which postfix oddly enough parses 'correctly')
> 
> how? or does there always have to be a port?
> (if yes, ok this makes sense)
sorry - the message was a bit unclear - probably better phrased as:
postfix splits on ':' and uses the last part as port.
for ip4 literals and dns-names this works
for ip6 literals it only works if you provide a port
so it makes more sense to always write ip(4|6) literals in brackets

> 
> > 
> > By introducing a explicit flag "$bracket_host" and reordering the conditions
> > lmtp and smtp entries get rendered correctly (see `man smtp`).
> > 
> > Additionally fixes an indentation glitch in read_transport_map.
> > 
> > Signed-off-by: Stoiko Ivanov <s.ivanov at proxmox.com>
> > ---
> >  src/PMG/Config.pm | 14 ++++++++------
> >  1 file changed, 8 insertions(+), 6 deletions(-)
> > 
> > diff --git a/src/PMG/Config.pm b/src/PMG/Config.pm
> > index 5b5f03f..3c83944 100755
> > --- a/src/PMG/Config.pm
> > +++ b/src/PMG/Config.pm
> > @@ -1138,7 +1138,7 @@ sub read_transport_map {
> >  		$host = $1;
> >  		$use_mx = 0;
> >  	    }
> > -		$use_mx = 0 if ($protocol eq "lmtp");
> > +	    $use_mx = 0 if ($protocol eq "lmtp");
> >  
> >  	    eval { PVE::JSONSchema::pve_verify_address($host); };
> >  	    if (my $err = $@) {
> > @@ -1177,19 +1177,21 @@ sub write_transport_map {
> >  	    if defined($comment) && $comment !~ m/^\s*$/;
> >  
> >  	my $use_mx = $data->{use_mx};
> > -	$use_mx = 0 if $data->{host} =~ m/^(?:$IPV4RE|$IPV6RE)$/;
> > +	my $bracket_host = ! $use_mx;
> >  
> 
> we can drop the $use_mx completely here since we do not use it after, by writing:
> 
> my $bracket_host = ! $data->{use_mx};
> 
> we can do that in a followup
agreed!

> 
> although the same effect would have been achieved if only the $use_mx = 0.. line
> would have been moved below the lmtp check
> 
> (the diff would have been smaller, but it would not be clearer, so the renaming is OK)
> 
would have had the same effect - but since the naming of the flag got me
confused multiple times I would prefer the (also clumsily phrased) - not to
mix dns-lookups with what gets written.


> >  	if ($data->{protocol} eq 'lmtp') {
> > -	    $use_mx = 1;
> > +	    $bracket_host = 0;
> >  	    $data->{protocol} .= ":inet";
> >  	}
> >  
> > -	if ($use_mx) {
> > +	$bracket_host = 1 if $data->{host} =~ m/^(?:$IPV4RE|$IPV6RE)$/i;
> > +
> > +	if ($bracket_host) {
> >  	    PVE::Tools::safe_print(
> > -		$filename, $fh, "$data->{domain} $data->{protocol}:$data->{host}:$data->{port}\n");
> > +		$filename, $fh, "$data->{domain} $data->{protocol}:[$data->{host}]:$data->{port}\n");
> >  	} else {
> >  	    PVE::Tools::safe_print(
> > -		$filename, $fh, "$data->{domain} $data->{protocol}:[$data->{host}]:$data->{port}\n");
> > +		$filename, $fh, "$data->{domain} $data->{protocol}:$data->{host}:$data->{port}\n");
> >  	}
> >      }
> >  }
> > -- 
> > 2.20.1
> > 
> > 
> > _______________________________________________
> > pmg-devel mailing list
> > pmg-devel at pve.proxmox.com
> > https://pve.proxmox.com/cgi-bin/mailman/listinfo/pmg-devel