Decouple fizz::Aead and quic::Aead . This makes a large chunk of the codebase fizz agnostic. #16
Conversation
facebook-github-bot
added
the
CLA Signed
deadalnix
changed the title
There was a problem hiding this comment.
@mjoras has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
@mjoras has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
@mjoras has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
quic/handshake/FizzBridge.h
Outdated
| } | ||
|
|
||
| class FizzAead final : public Aead { | ||
| private: |
There was a problem hiding this comment.
let's move the private section to be after public
quic/handshake/FizzBridge.h
Outdated
| class FizzAead final : public Aead { | ||
| private: | ||
| std::unique_ptr<fizz::Aead> fizzAead; | ||
| FizzAead(std::unique_ptr<fizz::Aead> _fizzAead) |
There was a problem hiding this comment.
nit: rename this to fizzAeadIn if you want to avoid collided name. We don't use _someName anywhere else in mvfst.
| std::unique_ptr<folly::IOBuf> | ||
| encrypt(std::unique_ptr<folly::IOBuf> &&plaintext, | ||
| const folly::IOBuf *associatedData, uint64_t seqNum) const override { | ||
| return fizzAead->encrypt( |
There was a problem hiding this comment.
since one can wrap a null fizz::aead in, fizzAead can possibly be nullptr. Should you check against that before deref it?
There was a problem hiding this comment.
The wrap function ensures that this isn't the case.
There was a problem hiding this comment.
Why? is you pass nullptr to wrap, you get a FizzAead object with the fizzAead member being nullptr, right?
There was a problem hiding this comment.
The wrap funtion check the pointer that is passed in and return a null unique pointer when that's the case. The constructor is private, so by construct, the inner unique pointer is non null.
| encrypt(std::unique_ptr<folly::IOBuf> &&plaintext, | ||
| const folly::IOBuf *associatedData, uint64_t seqNum) const override { | ||
| return fizzAead->encrypt( | ||
| std::forward<std::unique_ptr<folly::IOBuf>>(plaintext), associatedData, |
There was a problem hiding this comment.
should this std::forward() simply be std::move() ?
Provide FizzAead as an implementation of quic::Aead that forward calls. Provide MockAead as a replacement for fizz::test::MockAead. Most of the codebase now do not rely on fizz::Aead and use the mvfst specific interface. The part remaining aware of fizz:Aead are the various handshake part of the code base, tests and FizzAead which provides a bridge between fizz and mvfst.
There was a problem hiding this comment.
@mjoras has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
This is based on top of #15 .
Now that the codebase have been refactored to use Aead/fizz:Aead and MockAead/fizz::test::MockAead in proper places, it is time to ensures the aren't aliases of each others anymore.
This introduces FizzAead as a wrapper for fizz::Aead that implements quic::Aead and forward all calls. Most of the codebase now uses quic::Aead, which a significant step toward being able to swap it for another implementation.