friendly ping, anyone has an opinion?

Omar Polo <op@omarpolo.com> wrote:
> On Tue Feb 20, 2024 at 10:59 PM CET, Gilles Chehade wrote:
> > On Tue, Feb 20, 2024 at 05:10:52PM +0100, Omar Polo wrote:
> > > [...]
> > > So, I'm proposing to require ipv6 addresses to be wrapped in braces in
> > > table(5) files, and to amend the parser to skip over to the matching ]
> > > if the line starts with [.  Is it enough or should it be more strict?
> > > 
> >
> > I dunno if it should be more strict but that would already be a good
> > start and an improvement over today.
> 
> One downside I realized only later is that it still allows gibberish
> after the ip addr, as in "[::1]foo".
> 
> > > [...]
> > > 
> > > thoughs?
> > > 
> >
> > Generally ok with this but also starting to wonder if makemap and static
> > tables parsing would not benefit from a regex(3)-based line parser to be
> > honest. It's not used anywhere an expensive regex would be an issue, and
> > it would be more maintainable than having to read it every ten year when
> > someone hits a case we didn't hit before.
> >
> > Dunno, just opening the discussion
> 
> Not sure if a regexp is needed here, but for sure it could be
> interesting to eventually settle on some other details of the parser.
> For example, I these confusing:
> 
> 	foo :: bar	becomes		"foo" -> ":: bar"
> 	foo::bar	becomes		"foo" -> ":bar"
> 	foo:: bar	becomes		"foo" -> "bar"
> 
> so, some details should be agreed upon regardless of the implementation
> of the parser.
> 
> my diff also introduces another ambiguity: what to do when there
> is no closing ']'?  does it makes sense for keys to include '['
> outside of ipv6 addresses?  (I guess no, but don't know for sure.)
> 
> Here's another try, where I may have gone too far, which considers
> a parse error if there is no closing ] and also improves IMHO the
> splitting of key-value.  Using the same example table as before,
> now it becomes:
> 
> 	suzaku% cat t
> 	[::1]: bar
> 	[::2]   bar
> 	foo1    bar
> 	foo2:   bar
> 	foo3:bar
> 	foo4 : bar
> 	foo5 :: bar
> 	foo6::bar
> 	foo7:: bar
> 	suzaku% ./makemap t && ./makemap -U t.db | sort
> 	[::1]   bar
> 	[::2]   bar
> 	foo1    bar
> 	foo2    bar
> 	foo3    bar
> 	foo4    : bar
> 	foo5    :: bar
> 	foo6    :bar
> 	foo7    : bar
> 
> i.e. it splits on the first space or colon, and then trims the
> spaces.  Multiple colons are not trimmed.  Does anybody have aliases
> lines with "foo : bar"?
> 
> 
> diffstat /home/op/w/smtpd
>  M  table.5  |  10+   7-
>  M  util.c   |  15+  23-
> 
> 2 files changed, 25 insertions(+), 30 deletions(-)
> 
> diff /home/op/w/smtpd
> commit - 0e1879aafc8d52781267651a6455236cadaf2b54
> path + /home/op/w/smtpd
> blob - 65dca359f38d257db5d8c948374a8433c15e1a9d
> file + table.5
> --- table.5
> +++ table.5
> @@ -173,10 +173,11 @@ of addresses in the table until a match is found.
>  A netaddr table can contain exact addresses or netmasks, and looks as follow:
>  .Bd -literal -offset indent
>  192.168.1.1
> -::1
> -ipv6:::1
> +[::1]
>  192.168.1.0/24
>  .Ed
> +.Pp
> +IPv6 addresses must be enclosed in square brackets.
>  .Ss Userinfo tables
>  Userinfo tables are used in rule context to specify an alternate userbase,
>  mapping virtual users to local system users by UID, GID and home directory.
> @@ -214,11 +215,11 @@ A source table looks as follow:
>  .Bd -literal -offset indent
>  192.168.1.2
>  192.168.1.3
> -::1
> -::2
> -ipv6:::3
> -ipv6:::4
> +[::1]
> +[::2]
>  .Ed
> +.Pp
> +IPv6 address must be enclosed in square brackets.
>  .Ss Mailaddr tables
>  Mailaddr tables are lists of email addresses.
>  They can be used in the following contexts:
> @@ -254,10 +255,12 @@ outgoing connection.
>  .Pp
>  The format is a mapping from inet4 or inet6 addresses to hostnames:
>  .Bd -literal -offset indent
> -::1		localhost
> +[::1]		localhost
>  127.0.0.1	localhost
>  88.190.23.165	www.opensmtpd.org
>  .Ed
> +.Pp
> +IPv6 addresses must be enclosed in square brackets.
>  .Sh SEE ALSO
>  .Xr smtpd.conf 5 ,
>  .Xr makemap 8 ,
> blob - d481162c8973a288e6470ba1ef1367ad487b8d2d
> file + util.c
> --- util.c
> +++ util.c
> @@ -855,7 +855,7 @@ int
>  parse_table_line(FILE *fp, char **line, size_t *linesize,
>      int *type, char **key, char **val, int *malformed)
>  {
> -	char	*keyp, *valp, *p;
> +	char	*keyp, *valp;
>  	ssize_t	 linelen;
>  
>  	*key = NULL;
> @@ -885,37 +885,29 @@ parse_table_line(FILE *fp, char **line, size_t *linesi
>  		return 0;
>  	}
>  
> -	if (*type == T_NONE) {
> -		for (p = keyp; *p; p++) {
> -			if (*p == ' ' || *p == '\t' || *p == ':') {
> -				*type = T_HASH;
> -				break;
> -			}
> +	if (*keyp == '[') {
> +		if ((valp = strchr(keyp, ']')) == NULL) {
> +			*malformed = 1;
> +			return (0);
>  		}
> -		if (*type == T_NONE)
> -			*type = T_LIST;
> -	}
> +		valp++;
> +	} else
> +		valp = keyp + strcspn(keyp, " \t:");
>  
> +	if (*type == T_NONE)
> +		*type = (*valp == '\0') ? T_LIST : T_HASH;
> +
>  	if (*type == T_LIST) {
>  		*key = keyp;
>  		return (0);
>  	}
>  
>  	/* T_HASH */
> -	valp = keyp;
> -	strsep(&valp, " \t:");
> -	if (valp) {
> -		while (*valp) {
> -			if (!isspace((unsigned char)*valp) &&
> -			    !(*valp == ':' &&
> -			    isspace((unsigned char)*(valp + 1))))
> -				break;
> -			++valp;
> -		}
> -		if (*valp == '\0')
> -			valp = NULL;
> +	if (*valp != '\0') {
> +		*valp++ = '\0';
> +		valp += strspn(valp, " \t");
>  	}
> -	if (valp == NULL)
> +	if (*valp == '\0')
>  		*malformed = 1;
>  
>  	*key = keyp;