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().

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.