On Tue, Oct 09, 2012 at 07:05:46PM +0200, Alexander Hall wrote:
> >The following diff is what I intend to commit tonight with an ok from
> >eric@. It applies on -current, but beware as it kills the "enable"
> >keyword:
> >
> > listen on bnx0 [...] auth # enable auth
> > listen on bnx0 [...] auth-require # require auth
>
> I thought 'enable auth' and 'require auth' were more readable, but I
> could live with this too (now, should we end up having the
> non-enforcing variant).
>
enable makes the lines very long when you want to enable both ssl
we discussed this with eric@ and this was the concensus. it's not
written in stone, if we get convinced of a nicer syntax, we can
change it again ... it's not like a one word change in a 4 lines
config is going to bug people that much :-)
> > flags = $5;
> >
> > if ($7)
> >- flags |= F_AUTH;
> >+ flags |= $7;
>
> The if statement is pretty pointless. Together with the prior line:
>
> flags = $5 | $7;
>
right, will change that
> >+ if (s->s_l->flags & F_STARTTLS_REQUIRE)
> >+ if (!(s->s_flags & F_SECURE)) {
> >+ session_respond(s, "530 5.7.0 Must issue a STARTTLS
> >command first");
>
> long line
>
> >+ return 1;
> >+ }
> >+
> >+ if (s->s_l->flags & F_AUTH_REQUIRE)
> >+ if (!(s->s_flags & F_AUTHENTICATED)) {
> >+ session_respond(s, "530 5.7.0 Must issue a AUTH command
> >first");
>
> long line
>
> maybe "_an_ AUTH command"
>
> also, is the nested if(), in both cases above, for style reasons?
>
will fix long lines and _an_
the nested if are here because it makes it more readable to me for one
and it allowed me to easily put log_debug() to ensure it was doing what
it was supposed to during my testing :-)
> Other than that, reads fine.
>
okie dokie !
--
Gilles Chehade
https://www.poolp.org @poolpOrg
