diff --git a/quic/api/QuicTransportFunctions.cpp b/quic/api/QuicTransportFunctions.cpp index f8197bad7..f890c389f 100644 --- a/quic/api/QuicTransportFunctions.cpp +++ b/quic/api/QuicTransportFunctions.cpp @@ -1745,11 +1745,11 @@ bool hasInitialOrHandshakeCiphers(QuicConnectionStateBase& conn) { } bool setCustomTransportParameter( - std::unique_ptr customParam, + const CustomTransportParameter& customParam, std::vector& customTransportParameters) { // Check that the parameter id is in the "private parameter" range, as // described by the spec. - if (static_cast(customParam->getParameterId()) < + if (static_cast(customParam.getParameterId()) < kCustomTransportParameterThreshold) { LOG(ERROR) << "invalid parameter id"; return false; @@ -1761,7 +1761,7 @@ bool setCustomTransportParameter( customTransportParameters.begin(), customTransportParameters.end(), [&customParam](const TransportParameter& param) { - return param.parameter == customParam->getParameterId(); + return param.parameter == customParam.getParameterId(); }); // if a match has been found, we return failure @@ -1770,7 +1770,7 @@ bool setCustomTransportParameter( return false; } - customTransportParameters.push_back(customParam->encode()); + customTransportParameters.push_back(customParam.encode()); return true; } diff --git a/quic/api/QuicTransportFunctions.h b/quic/api/QuicTransportFunctions.h index 1b5ba258d..d97cd2c99 100644 --- a/quic/api/QuicTransportFunctions.h +++ b/quic/api/QuicTransportFunctions.h @@ -332,7 +332,7 @@ bool writeLoopTimeLimit( * - checks that the param is not in the provided vector already */ bool setCustomTransportParameter( - std::unique_ptr customParam, + const CustomTransportParameter& customParam, std::vector& customTransportParameters); bool toWriteInitialAcks(const quic::QuicConnectionStateBase& conn); diff --git a/quic/api/test/QuicTransportFunctionsTest.cpp b/quic/api/test/QuicTransportFunctionsTest.cpp index 9247f271d..335c08376 100644 --- a/quic/api/test/QuicTransportFunctionsTest.cpp +++ b/quic/api/test/QuicTransportFunctionsTest.cpp @@ -4535,28 +4535,26 @@ TEST_F(QuicTransportFunctionsTest, CustomTransportParamTest) { // Add new param. EXPECT_TRUE(setCustomTransportParameter( - std::make_unique( - kCustomTransportParameterThreshold, 0), + CustomIntegralTransportParameter(kCustomTransportParameterThreshold, 0), customTransportParameters)); EXPECT_EQ(customTransportParameters.size(), 1); // Existing param not added. EXPECT_FALSE(setCustomTransportParameter( - std::make_unique( - kCustomTransportParameterThreshold, 1), + CustomIntegralTransportParameter(kCustomTransportParameterThreshold, 1), customTransportParameters)); EXPECT_EQ(customTransportParameters.size(), 1); // Bad param id is not added. EXPECT_FALSE(setCustomTransportParameter( - std::make_unique( + CustomIntegralTransportParameter( kCustomTransportParameterThreshold - 1, 2), customTransportParameters)); EXPECT_EQ(customTransportParameters.size(), 1); // Another valid param added. EXPECT_TRUE(setCustomTransportParameter( - std::make_unique( + CustomIntegralTransportParameter( kCustomTransportParameterThreshold + 1, 0), customTransportParameters)); EXPECT_EQ(customTransportParameters.size(), 2); diff --git a/quic/client/QuicClientTransport.cpp b/quic/client/QuicClientTransport.cpp index 1427ca1ce..175dc0757 100644 --- a/quic/client/QuicClientTransport.cpp +++ b/quic/client/QuicClientTransport.cpp @@ -1655,54 +1655,48 @@ void QuicClientTransport::setSelfOwning() { } void QuicClientTransport::setSupportedExtensionTransportParameters() { - if (conn_->transportSettings.minAckDelay.hasValue()) { - auto minAckDelayParam = std::make_unique( + const auto& ts = conn_->transportSettings; + + if (ts.minAckDelay.hasValue()) { + CustomIntegralTransportParameter minAckDelayParam( static_cast(TransportParameterId::min_ack_delay), - conn_->transportSettings.minAckDelay.value().count()); - customTransportParameters_.push_back(minAckDelayParam->encode()); - } - if (conn_->transportSettings.datagramConfig.enabled) { - auto maxDatagramFrameSize = - std::make_unique( - static_cast( - TransportParameterId::max_datagram_frame_size), - conn_->datagramState.maxReadFrameSize); - customTransportParameters_.push_back(maxDatagramFrameSize->encode()); + ts.minAckDelay.value().count()); + customTransportParameters_.push_back(minAckDelayParam.encode()); } - auto ackReceiveTimestampsEnabled = - std::make_unique( - static_cast( - TransportParameterId::ack_receive_timestamps_enabled), - conn_->transportSettings.maybeAckReceiveTimestampsConfigSentToPeer - .has_value() - ? 1 - : 0); - customTransportParameters_.push_back(ackReceiveTimestampsEnabled->encode()); - if (conn_->transportSettings.maybeAckReceiveTimestampsConfigSentToPeer - .has_value()) { - auto maxReceiveTimestampsPerAck = - std::make_unique( - static_cast( - TransportParameterId::max_receive_timestamps_per_ack), - conn_->transportSettings.maybeAckReceiveTimestampsConfigSentToPeer - .value() - .max_receive_timestamps_per_ack); - customTransportParameters_.push_back(maxReceiveTimestampsPerAck->encode()); - auto receiveTimestampsExponent = - std::make_unique( - static_cast( - TransportParameterId::receive_timestamps_exponent), - conn_->transportSettings.maybeAckReceiveTimestampsConfigSentToPeer - .value() - .receive_timestamps_exponent); - customTransportParameters_.push_back(receiveTimestampsExponent->encode()); + if (ts.datagramConfig.enabled) { + CustomIntegralTransportParameter maxDatagramFrameSize( + static_cast(TransportParameterId::max_datagram_frame_size), + conn_->datagramState.maxReadFrameSize); + customTransportParameters_.push_back(maxDatagramFrameSize.encode()); } - if (conn_->transportSettings.advertisedKnobFrameSupport) { - auto knobFrameSupport = std::make_unique( + CustomIntegralTransportParameter ackReceiveTimestampsEnabled( + static_cast( + TransportParameterId::ack_receive_timestamps_enabled), + ts.maybeAckReceiveTimestampsConfigSentToPeer.has_value() ? 1 : 0); + customTransportParameters_.push_back(ackReceiveTimestampsEnabled.encode()); + + if (ts.maybeAckReceiveTimestampsConfigSentToPeer.has_value()) { + CustomIntegralTransportParameter maxReceiveTimestampsPerAck( + static_cast( + TransportParameterId::max_receive_timestamps_per_ack), + ts.maybeAckReceiveTimestampsConfigSentToPeer.value() + .max_receive_timestamps_per_ack); + customTransportParameters_.push_back(maxReceiveTimestampsPerAck.encode()); + + CustomIntegralTransportParameter receiveTimestampsExponent( + static_cast( + TransportParameterId::receive_timestamps_exponent), + ts.maybeAckReceiveTimestampsConfigSentToPeer.value() + .receive_timestamps_exponent); + customTransportParameters_.push_back(receiveTimestampsExponent.encode()); + } + + if (ts.advertisedKnobFrameSupport) { + CustomIntegralTransportParameter knobFrameSupport( static_cast(TransportParameterId::knob_frames_supported), 1); - customTransportParameters_.push_back(knobFrameSupport->encode()); + customTransportParameters_.push_back(knobFrameSupport.encode()); } } @@ -1810,13 +1804,12 @@ void QuicClientTransport::maybeEnableStreamGroups() { return; } - auto streamGroupsEnabledParam = - std::make_unique( - static_cast(TransportParameterId::stream_groups_enabled), - conn_->transportSettings.advertisedMaxStreamGroups); + CustomIntegralTransportParameter streamGroupsEnabledParam( + static_cast(TransportParameterId::stream_groups_enabled), + conn_->transportSettings.advertisedMaxStreamGroups); if (!setCustomTransportParameter( - std::move(streamGroupsEnabledParam), customTransportParameters_)) { + streamGroupsEnabledParam, customTransportParameters_)) { LOG(ERROR) << "failed to set stream groups enabled transport parameter"; } } diff --git a/quic/client/test/ClientStateMachineTest.cpp b/quic/client/test/ClientStateMachineTest.cpp index 9e5b54c5e..a9637a8b2 100644 --- a/quic/client/test/ClientStateMachineTest.cpp +++ b/quic/client/test/ClientStateMachineTest.cpp @@ -236,12 +236,11 @@ TEST_P( std::vector transportParams; if (GetParam().peerMaxGroupsIn > 0) { - auto streamGroupsEnabledParam = - std::make_unique( - static_cast(TransportParameterId::stream_groups_enabled), - GetParam().peerMaxGroupsIn); - CHECK(setCustomTransportParameter( - std::move(streamGroupsEnabledParam), transportParams)); + CustomIntegralTransportParameter streamGroupsEnabledParam( + static_cast(TransportParameterId::stream_groups_enabled), + GetParam().peerMaxGroupsIn); + CHECK( + setCustomTransportParameter(streamGroupsEnabledParam, transportParams)); } ServerTransportParameters serverTransportParams = { std::move(transportParams)}; diff --git a/quic/handshake/TransportParameters.cpp b/quic/handshake/TransportParameters.cpp index 4837c741e..ac51b7f6e 100644 --- a/quic/handshake/TransportParameters.cpp +++ b/quic/handshake/TransportParameters.cpp @@ -80,7 +80,7 @@ TransportParameter encodeIntegerParameter( return {id, std::move(data)}; } -TransportParameterId CustomTransportParameter::getParameterId() { +TransportParameterId CustomTransportParameter::getParameterId() const { return static_cast(id_); } diff --git a/quic/handshake/TransportParameters.h b/quic/handshake/TransportParameters.h index ce513ecdb..b336941cc 100644 --- a/quic/handshake/TransportParameters.h +++ b/quic/handshake/TransportParameters.h @@ -56,7 +56,7 @@ struct TransportParameter { class CustomTransportParameter { public: - TransportParameterId getParameterId(); + TransportParameterId getParameterId() const; virtual TransportParameter encode() const = 0; diff --git a/quic/server/state/ServerStateMachine.cpp b/quic/server/state/ServerStateMachine.cpp index 70f1b11aa..64c29823d 100644 --- a/quic/server/state/ServerStateMachine.cpp +++ b/quic/server/state/ServerStateMachine.cpp @@ -1562,60 +1562,50 @@ QuicServerConnectionState::createAndAddNewSelfConnId() { std::vector setSupportedExtensionTransportParameters( QuicServerConnectionState& conn) { std::vector customTransportParams; - if (conn.transportSettings.datagramConfig.enabled) { - auto maxDatagramFrameSize = - std::make_unique( - static_cast( - TransportParameterId::max_datagram_frame_size), - conn.datagramState.maxReadFrameSize); - customTransportParams.push_back(maxDatagramFrameSize->encode()); + const auto& ts = conn.transportSettings; + if (ts.datagramConfig.enabled) { + CustomIntegralTransportParameter maxDatagramFrameSize( + static_cast(TransportParameterId::max_datagram_frame_size), + conn.datagramState.maxReadFrameSize); } - if (conn.transportSettings.advertisedMaxStreamGroups > 0) { - auto streamGroupsEnabledParam = - std::make_unique( - static_cast(TransportParameterId::stream_groups_enabled), - conn.transportSettings.advertisedMaxStreamGroups); + if (ts.advertisedMaxStreamGroups > 0) { + CustomIntegralTransportParameter streamGroupsEnabledParam( + static_cast(TransportParameterId::stream_groups_enabled), + ts.advertisedMaxStreamGroups); if (!setCustomTransportParameter( - std::move(streamGroupsEnabledParam), customTransportParams)) { + streamGroupsEnabledParam, customTransportParams)) { LOG(ERROR) << "failed to set stream groups enabled transport parameter"; } } - auto ackReceiveTimestampsEnabled = - std::make_unique( - static_cast( - TransportParameterId::ack_receive_timestamps_enabled), - conn.transportSettings.maybeAckReceiveTimestampsConfigSentToPeer - .has_value() - ? 1 - : 0); - customTransportParams.push_back(ackReceiveTimestampsEnabled->encode()); - if (conn.transportSettings.maybeAckReceiveTimestampsConfigSentToPeer - .has_value()) { - auto maxReceiveTimestampsPerAck = - std::make_unique( - static_cast( - TransportParameterId::max_receive_timestamps_per_ack), - conn.transportSettings.maybeAckReceiveTimestampsConfigSentToPeer - .value() - .max_receive_timestamps_per_ack); - customTransportParams.push_back(maxReceiveTimestampsPerAck->encode()); - auto receiveTimestampsExponent = - std::make_unique( - static_cast( - TransportParameterId::receive_timestamps_exponent), - conn.transportSettings.maybeAckReceiveTimestampsConfigSentToPeer - .value() - .receive_timestamps_exponent); - customTransportParams.push_back(receiveTimestampsExponent->encode()); + CustomIntegralTransportParameter ackReceiveTimestampsEnabled( + static_cast( + TransportParameterId::ack_receive_timestamps_enabled), + ts.maybeAckReceiveTimestampsConfigSentToPeer.has_value() ? 1 : 0); + customTransportParams.push_back(ackReceiveTimestampsEnabled.encode()); + + if (ts.maybeAckReceiveTimestampsConfigSentToPeer.has_value()) { + CustomIntegralTransportParameter maxReceiveTimestampsPerAck( + static_cast( + TransportParameterId::max_receive_timestamps_per_ack), + ts.maybeAckReceiveTimestampsConfigSentToPeer.value() + .max_receive_timestamps_per_ack); + customTransportParams.push_back(maxReceiveTimestampsPerAck.encode()); + + CustomIntegralTransportParameter receiveTimestampsExponent( + static_cast( + TransportParameterId::receive_timestamps_exponent), + ts.maybeAckReceiveTimestampsConfigSentToPeer.value() + .receive_timestamps_exponent); + customTransportParams.push_back(receiveTimestampsExponent.encode()); } - if (conn.transportSettings.advertisedKnobFrameSupport) { - auto knobFrameSupport = std::make_unique( + if (ts.advertisedKnobFrameSupport) { + CustomIntegralTransportParameter knobFrameSupport( static_cast(TransportParameterId::knob_frames_supported), 1); - customTransportParams.push_back(knobFrameSupport->encode()); + customTransportParams.push_back(knobFrameSupport.encode()); } return customTransportParams; diff --git a/quic/server/test/ServerStateMachineTest.cpp b/quic/server/test/ServerStateMachineTest.cpp index 9486c1ace..31143fbaf 100644 --- a/quic/server/test/ServerStateMachineTest.cpp +++ b/quic/server/test/ServerStateMachineTest.cpp @@ -296,12 +296,11 @@ TEST_P( std::vector transportParams; if (GetParam().peerMaxGroupsIn > 0) { - auto streamGroupsEnabledParam = - std::make_unique( - static_cast(TransportParameterId::stream_groups_enabled), - GetParam().peerMaxGroupsIn); - CHECK(setCustomTransportParameter( - std::move(streamGroupsEnabledParam), transportParams)); + CustomIntegralTransportParameter streamGroupsEnabledParam( + static_cast(TransportParameterId::stream_groups_enabled), + GetParam().peerMaxGroupsIn); + CHECK( + setCustomTransportParameter(streamGroupsEnabledParam, transportParams)); } ClientTransportParameters clientTransportParams = { std::move(transportParams)};