Skip to content

Conversation

@weissi
Copy link
Member

@weissi weissi commented Apr 21, 2022

Motivation:

NFS can be a very useful protocol.

Modifications:

Add the (read side, for now) of the NFS3 protocol.
This adds (partial) implementations of the following RFCs:

Essentially, NFS3 is a protocol which sends metadata & does framing via the (SUN) RPC protocol. The data is encoded using XDR. If you're familiar with gRPC, then XDR is NFS's protobuf, HTTP is NFS's RPC and NFS3 is the application protocol.

Result:

  • More supported protocols

Left to do before merging:

  • Look at the TODOs
  • Discuss public API changes if more of the NFS protocol is implemented
  • Rename the module to _NIONFS3Preview?
  • Decide if the dependency on https://github.com/apple/swift-distributed-tracing-baggage.git is acceptable

@weissi weissi marked this pull request as ready for review December 16, 2022 16:01
@weissi weissi requested a review from Lukasa December 16, 2022 16:01
@weissi weissi force-pushed the jw-nfs3 branch 3 times, most recently from e8c99a1 to ab005f1 Compare December 16, 2022 16:15
@weissi weissi requested a review from fabianfett December 16, 2022 17:41
@weissi weissi added the 🆕 semver/minor Adds new public API. label Dec 19, 2022
Copy link
Contributor

@glbrntt glbrntt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a bunch of comments inline. I have no doubt that I missed things too.

A few general notes:

  • There are a bunch of message types which are only Equatable -- these should probably be Hashable and Sendable too.
  • The ByteBuffer read helpers don't follow the normal pattern of returning nil if there aren't enough bytes and the write helpers don't return the number of bytes written.
  • The Mount* types seem to be duplicated as NFS* types. The latter don't appear to be used?

@weissi weissi requested a review from glbrntt December 19, 2022 15:22
.product(name: "NIOEmbedded", package: "swift-nio"),
])
]),
.target(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we intentionally not defining a product here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Lukasa for now, yes. In a follow-up I might either add a product (either NIONFS3 or _NIONFS3Preview) but we can discuss then. I want to first land the bulk of the code

@weissi weissi requested a review from Lukasa January 18, 2023 15:09
@Lukasa Lukasa merged commit 4569c69 into apple:main Mar 21, 2023
@weissi weissi mentioned this pull request Aug 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🆕 semver/minor Adds new public API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants