conn.Read return value is ambiguous about disco HOT 3 CLOSED

Hey @Fasjeit. Thanks for pointing that out

return 2 + length, errors.New("disco: cannot decrypt the payload")

I tried to mirror crypto/tls code but noticed that it often returned an information-less 0 when there was an error.

return 0, err

I thought we could be more useful and use that return value even when there was an error. But indeed I do not know exactly how it could be useful.

If you think this is adding ambiguity, perhaps it is a good argument for constantly replacing that value with 0. Notice that in one point crypto/tls does return a more accurate length of byte read with an error.

from disco.

Looks like in tls non-zero return value is obtained here

n, err = c.input.Read(b)

so output value n in this case is always related to output value b. Basically it means "we managed to read something from input connection before, here is your data, here is it's length, but looks like there is an error when we are trying to read more."

In case of disco we return

return 2 + length, errors.New("disco: cannot decrypt the payload")

which not only does no good to user, but also preventing him from getting correct length of data, stored in buffer b after function call. It means "there is an error, we are not going to tell you how to truncate the buffer b to get at least some data we cached before, but instead here is the number of raw bytes from connection we managed to read."

If you want to copy tls it will probably be better to return readSoFar value in case of errors, so user can get correct data length, if it was read from inputBuffer.

// read whatever there is to read in the buffer
	readSoFar := 0
	if len(c.inputBuffer) > 0 {
		copy(b, c.inputBuffer)
		if len(c.inputBuffer) >= len(b) {
			c.inputBuffer = c.inputBuffer[len(b):]
			return len(b), nil
		}
		readSoFar += len(c.inputBuffer)
		c.inputBuffer = c.inputBuffer[:0]
        }

	// read header from socket
	bufHeader, err := readFromUntil(c.conn, 2)
	if err != nil {
                // already read readSoFar bytes to b
		return readSoFar, err
        }

        // -> same thing for errors below

from disco.

Ah I see what you mean, there is a difference between:

• we read n bytes (which is what we're doing here)
• we filled n bytes in the b buffer passed as argument to the function.

func (c *Conn) Read(b []byte) (n int, err error) {

In this case I think it makes sense to apply the change you're proposing (do you want to make a PR? Otherwise I can do it)

Or are you thinking of something else?

from disco.