On 2023/01/03 00:16:40 +0100, Stefan Sperling <stsp@stsp.name> wrote:
> On Mon, Jan 02, 2023 at 08:36:39PM +0100, Omar Polo wrote:
> > On 2023/01/02 16:52:15 +0100, Stefan Sperling <stsp@stsp.name> wrote:
> > > --- gotd/gotd.conf.5
> > > +++ gotd/gotd.conf.5
> > > @@ -31,6 +31,56 @@ The available global configuration directives are as f
> > >  .Sh GLOBAL CONFIGURATION
> > >  The available global configuration directives are as follows:
> > >  .Bl -tag -width Ds
> > > +.It Ic connection Ar option
> > > +Set the specified options and limits for connections to the
> > > +.Xr gotd 8
> > > +unix socket.
> > > +.Pp
> > > +The
> > > +.Ic connection
> > > +directive may be specified multiple times, and multiple
> > > +.Ar option
> > > +arguments may be specified within curly braces:
> > > +.Pp
> > > +.Ic connection Brq Ar option1 Ar option2 ...
> > 
> > i'd drop "option1 option2".  Other manpages (e.g. httpd.conf(5))
> > usually just get away with
> > 
> > 	.Ic connection Brq ...
> 
> I felt the ... was a bit unclear, but sure, we can keep things consistent.

no strong opinion on it.  "option1 option2" read strange to me, but
that's highly subjective.

> > > +.Bl -tag -width Ds
> > > +.It Ic authentication timeout Ar seconds
> > > +Specify the timeout for client authentication.
> > > +If this timeout is exceeded due to an unexpected failure in the authentication
> > > +process, such as a crash, the connection will be terminated.
> > > +.Pp
> > > +The default timeout is 5 seconds.
> > > +This should only be changed if legitimate connections are exceeding the
> > > +default timeout for some reason.
> > 
> > To be fair I'm not convinced about this knob.
> > 
> > The auth process is just a getpeereuid() + a loop over the ACLs, it
> > shouldn't never hit the limit and if it does it's a bug.  users should
> > report it rather than trying to crank up the limit.
> > 
> > I'm not against the internal timeout for the auth process, I just
> > don't think it's worth exposing it.
> 
> You're right, I added this for symmetry to request timeout but there
> is probably no good reason to expose this to users. I've dropped the
> user-facing parts in the new patch below. We can always put them back
> in if they ever turn out to be needed (e.g. because someone runs yellow
> pages and passwd lookups take forever for some reason).

have to admit I haven't thought about YP at all.  I don't have any
experience with it.  Since some functions are now routed thru the
internet the timeout makes even more sense.

still no idea if the knob would be useful, i'll defer the decision to
people actually using it :)

> > > [...]
> > > +
> > > +# Use a larger request timeout value:
> > > +connection request timeout 7200  # 2 hours
> > > +
> > > +# Some users are granted a higher concurrent connection limit:
> > > +connection {
> > > +	user flan_hacker max 16
> > 
> > just wondering, what about adding a `limit' keyword there too, e.g.:
> > 
> > 	connection limit user flan_hacker max 16
> > 	connection {
> > 		limit user foo max 32
> > 		limit user bar max 64
> > 	}
> > 
> > It doesn't add anything, but it'd almost read as plain english then.
> 
> And then "limit" is a bit redundant with "max" isn't it?

ah, yeah

> So I have dropped "max" in the new patch below and syntax now reads:
> 
>  	connection limit user flan_hacker 16

reads fine to me!

> [...]
> New patch with above tweaks applied:

ok op@

> @@ -135,6 +139,16 @@ main		: UNIX_SOCKET STRING {
>  		| NUMBER { $$ = $1; }
>  		;
>  
> +timeout		: NUMBER {
> +			if ($1 < 0) {

should zero be disallowed too?

> +				yyerror("invalid timeout: %lld", $1);
> +				YYERROR;
> +			}
> +			$$.tv_sec = $1;
> +			$$.tv_usec = 0;
> +		}
> +		;
> +