On 2024/03/21 09:16:36 +0100, Stefan Sperling <stsp@stsp.name> wrote:
> On Wed, Mar 20, 2024 at 07:49:54PM +0100, Omar Polo wrote:
> > On 2024/03/20 17:56:14 +0100, Stefan Sperling <stsp@stsp.name> wrote:
> > > > +	/*
> > > > +	 * Normally we would get just one reply, but the regress doesn't
> > > > +	 * use a real SMTP server and queues all the replies upfront.
> > > > +	 */
> > > > +	linelen += 2;
> > > > +	if (smtp_buflen == linelen)
> > > 
> > > Maybe write >= here just to make sure we don't drop into the else
> > > clause and do the memmove with bogus length values? I understand
> > > this cannot happen if the buffer smtp response with "\r\n" (and
> > > we wouldn't get here otherwise), but still.
> > 
> > In the normal case, you'd be right, but we break the regress like this.
> > A real smtp server will send just one reply, so at each call we only
> > read one line and process it.  However, the regress queues up all the
> > replies, so on the first read() we fill the buffer with all the replies,
> > then we just consume the buffer.
> > 
> > The if is actually a micro-optimization, I could just remove it and do
> > the memmove unconditionally.  at worst, it'll be done with a length of
> > zero.
> 
> I guess calling memmove with zero length would be fine.
> But it doesn't really matter.

saves a few lines and makes the comment more easibly understandable, so
I went with this change as well.

I've re-tested both against smtpd and the regress before committing.

> > Also, if we'll switch to using EHLO instead of HELO (which is actually
> > deprecated AFAICS) being able to process multiple lines of reply will
> > come in handy.
> 
> I don't really understand SMTP.
> I looked at SMTP page on wikipedia when I implemented this and went
> by the example there, and it worked :)
> If EHLO offers some advantages then I suppose we should use it.

Honestly, at least for now, it would just be more code.  EHLO (extended
helo) is to support service extensions which we don't need.  HELO is
currently supported only for legacy clients, and that's why I mentioned
it, but could be "good enough" for us.  It is definitely for now.

> By the way, I was wondering whether we should use smtpd.conf-style
> relay host URLs instead of 'hostname X port Y" to keep existing
> configs backwards compatible in case we add support for things like
> smtp+tls?

While I personally would prefer a url-style syntax, we could just add a
`tls' (or `starttls') keyword to keep the config backward compatible.

> In the long run we might end up adding TLS support and
> authentication because some people will want to avoid running a local MTA.
> In which case this tool will have to become smarter about SMTP anyway.

Sure.  TLS shouldn't bee too hard, and neither authentication.  We might
want to check that the gotd.conf file is not world-readable when we have
sensitive information, or allow to provide those via a separate file.

hum, we need the config to be word-readable for gitwrapper though.

We'll figure out something.