search

google / gvisor

Application Kernel for Containers
https://gvisor.dev
Apache License 2.0
15.85k stars 1.3k forks source link

tcpip.Payloader is a reimplementation of io.Reader #1509

Closed tamird closed 3 years ago

tamird commented 4 years ago

https://godoc.org/gvisor.dev/gvisor/pkg/tcpip#Payloader is roughly the same as io.Reader. We should remove that type in favor of io.Reader, which is more general and better composes with the standard library.

cc @stijlist @iangudger

iangudger commented 4 years ago

The big difference is that we are using our own error types. This was an international change to constrain the error space to eliminate difficult to debug non-convertible error issues.

tamird commented 4 years ago

Understood - and yet there exists a Payloader implementation that wraps an io.Reader, thus fully negating this benefit.

https://github.com/google/gvisor/blob/51f3ab85e024fcd74c49d273ce5202a207577d31/pkg/sentry/socket/netstack/netstack.go#L544-L583

iangudger commented 4 years ago

We haven't converted the whole sentry to use its own error types yet. That is still a work in progress.

tamird commented 4 years ago

The readerPayload was added only 4 months ago. The trend seems to be going in the opposite direction.

https://github.com/google/gvisor/commit/7c6ab6a219f37a1d4c18ced4a602458fcf363f85#diff-82479556143e8dc38884a4daa6bc79e1R508

tamird commented 4 years ago

We may need a deeper rethink of the tcpip.Endpoint API to address longstanding integration issues with Fuchsia. In particular, Fuchsia currently uses a pair of loops to shuttle bytes and signals between a zircon socket and the tcpip.Endpoint. This is necessary because networking is implemented in userspace in Fuchisa, meaning the networking implementation cannot read applications' memory:

+-------------+     +-----------------------+     +----------+
| Application | --- | Kernel-managed socket | --- | Netstack |
+-------------+     +-----------------------+     +----------+

This results in odd behaviour such as certain errors being first observed and consumed by these loops (e.g. ECONNRESET), preventing retrieval by getsockopt(..., SO_ERROR) without additional work to cache those errors in the integrator. It would be better to teach tcpip.Endpoint (stream endpoints in particular) about externally furnished buffers, which would address this problem.

cc @ghanan94