On 2024/04/17 11:50:07 +0200, Stefan Sperling <stsp@stsp.name> wrote:
> On Wed, Apr 17, 2024 at 11:15:59AM +0200, Omar Polo wrote:
> > On 2024/04/16 12:58:06 +0200, Stefan Sperling <stsp@stsp.name> wrote:
> > > On Tue, Apr 16, 2024 at 12:51:10PM +0200, Stefan Sperling wrote:
> > > > There is no need for bufio to be using errno as part of the API.
> > > > Since errno really belongs to libc I don't feel comfortable with
> > > > this, even though I understand it is supposed to emulate libc's
> > > > read/write interfaces.
> > > > 
> > > > This diff switches from errno to the -ERRCODE linux-ism simply
> > > > because this is the easiest way I found to get rid of errno at
> > > > the API level.
> > > > 
> > > > We could also switch to got_error but that would make it harder
> > > > to reuse bufio code elsewhere.
> > > > 
> > > > ok?
> > > 
> > > Had one little error where bufio_handshake() was returning
> > > EINVAL instead of -EINVAL. Not a big deal but it is probably
> > > better to stay consistent.
> > 
> > don't want to nitpick nor to veto the diff, but I really, really dislike
> > -ERRCODE.
> > 
> > If you really want to avoid using errno, what about defining
> > BUFIO_WANT_{READ,WRITE} to be negative like TLS_WANT_{READ,WRITE}?
> > 
> > Then you'd have a ssize_t return that can be 0 for EOF, -1 for an error,
> > BUFIO_WANT_{READ,WRITE} (still negative) for signaling the
> > retry/reschedule or a positive value being the number of bytes
> > read/wrote.
> > 
> > or even passing an extra optional argument that gets set to 1 to
> > indicate the retry, a-la
> > 
> > 	if (bufio_write(bio, &retry) == -1 && !retry)
> > 		/* handle error */
> > 
> > 
> > said this, I'm still missing what's so bad in using errno here.  The
> > functions are supposed to be called like this:
> > 
> > 	/* assuming the socket is ready to be written */
> > 	if (bufio_write(bio) == -1 && errno != EAGAIN)
> > 		fatalx("I/O error: %s", bufio_io_err(bio));
> > 
> > changing this to a complex -ERROR handling or a switch style handling
> > with several special return feels a step back to me.
> 
> Fine, I don't care strongly about it.
> 
> If we keep using errno then bufio should at least always set errno
> when -1 gets returned. For example, in bufio_close() the call to
> tls_close() will clear errno but as far as I understand will never
> set it. We could end up with return -1 and errno == 0, even if
> there was some error according to tls_error().

I haven't considered this case since all the times I'm comparing errno
only against EAGAIN, which is set by bufio, and relying on libtls to
clear errno.  Then, there's bufio_io_err() to get a error message after
an I/O failure, which will look at errno only in the non-TLS case.

> We could also convert our bufio API to struct got_error, maybe even
> wrapping the tls error stuff to convert tls error messages into a
> got_error with a special "TLS" error code. The bufio code would then
> become more tied to Got's internals, which may or may not be a bad
> thing depending on how reusable you want this code to be.

I'm not opposed at all!

I wrote this tiny buffered i/o layer in a different project where I
didn't want to add a dependency on libevent, then reused for the http
notifications because it has TLS support built-in easier to use than
libevent' bufferevents, and maybe for the smtp notifications too in the
future.

It's really a very simple buffering layer, so I'm open to every
improvements, even if they'll tie it more closely to the rest of the
codebase.

The number of bytes read/written returned by bufio_write() and
bufio_read() are usually not interesting*, so we could return a
got_error.


* because unlike write(), the data is all buffered.  The read and
  write buffer can be inspected to see how many bytes are available,
  and helper functions be added if we don't want to reach into the
  structs.