Audit bad uses of Read

[Stebalien]

Read can:

1. Fail to fully fill the target buffer even if there's more data to be read.
2. Read data and return an EOF at the same time.

We haven't been handling these cases well leading to #4781. We need to audit all uses of Read.

Progress:

• ipfs org
  • go-ipfs: tar: fix Go 1.10 breakage #4790, fix incorrect Read calls #4792
  • go-ipfs-cmds: none
  • go-ipfs-cmdkit: none
  • fs-repo-migrations
• multiformats org: fix the a Read assumption multiformats/go-multihash#72, fix Read assumptions multiformats/go-multistream#24
• libp2p org
  • go-libp2p-pnet: correctly handle Read errors/EOF libp2p/go-libp2p-pnet#18
• whyrusleeping

(I'm ignoring test cases for now)

[kevina]

Not very common but, read can also:

3. Read nothing, even when not at EOF, that is both data and err will be nil.

[Stebalien]

Yes, but that case is also seriously discouraged. We should handle it but handling it may often involve returning a io.ErrNoProgress error (looping would likely be a bad idea in many cases).

[kevina]

Yes, but that case is also seriously discouraged. We should handle it but handling it may often involve returning a io.ErrNoProgress error (looping would likely be a bad idea in many cases).

Maybe, it is rare enough that it unlikely we need to write special code for it. This may mean looping until we get an EOF. The worst thing this will do is waste CPU time unless the Reader is broken in which case the function may never return. For example the fix #4710 (as of the time I wrote this comment) will simply loop until it gets an EOF.

Returning io.ErrNoProgress might be better, but it will also mean more code to handle a rare edge case.

The only thing we should not do is break out of a loop and not return any indication of an error when no data is read.

[Stebalien]

The only thing we should not do is break out of a loop and not return any indication of an error when no data is read.

Yes. Absolutely.