Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

1060: Catching up a few bug fixes #852

Merged
merged 3 commits into from
Mar 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 21 additions & 10 deletions http/http_client.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -380,6 +380,16 @@ class ConnectionInfo : public std::enable_shared_from_this<ConnectionInfo>
unsigned int respCode = parser->get().result_int();
BMCWEB_LOG_DEBUG << "recvMessage() Header Response Code: " << respCode;

// Handle the case of stream_truncated. Some servers close the ssl
Copy link
Contributor

Choose a reason for hiding this comment

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

can we take the latest code from upstream? It does not uses socket pointer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For 1060, we can't rebase with upstream but we try to catch up some potentially critical bug fixes if there are.
However, this particular lines are already synced with upstream - related to socket handling.

// connection uncleanly, so check to see if we got a full response
// before we handle this as an error.
if (!parser->is_done())
{
state = ConnState::recvFailed;
waitAndRetry();
return;
}

// Make sure the received response code is valid as defined by
// the associated retry policy
if (connPolicy->invalidResp(respCode))
Expand Down Expand Up @@ -499,12 +509,13 @@ class ConnectionInfo : public std::enable_shared_from_this<ConnectionInfo>
// Let's close the connection and restart from resolve.
shutdownConn(true);
}

void restartConnection()
{
BMCWEB_LOG_DEBUG << host << ":" << std::to_string(port)
<< ", id: " << std::to_string(connId)
<< " restartConnection ";
conn = makeConnection(ioc, sslConn.has_value());
initializeConnection(sslConn.has_value());
doResolve();
}
void shutdownConn(bool retry)
Expand Down Expand Up @@ -612,11 +623,11 @@ class ConnectionInfo : public std::enable_shared_from_this<ConnectionInfo>
return;
}
}
std::unique_ptr<boost::asio::ip::tcp::socket>
makeConnection(boost::asio::io_context& iocIn, bool useSsl)

void initializeConnection(bool ssl)
{
auto newconn = std::make_unique<boost::asio::ip::tcp::socket>(iocIn);
if (useSsl)
conn = std::make_unique<boost::asio::ip::tcp::socket>(ioc);
if (ssl)
{
/* std::optional<boost::asio::ssl::context> sslCtx =
ensuressl::getSSLClientContext();
Expand All @@ -636,10 +647,9 @@ class ConnectionInfo : public std::enable_shared_from_this<ConnectionInfo>
} */
boost::asio::ssl::context sslCtx{
boost::asio::ssl::context::tlsv13_client};
sslConn.emplace(*newconn.get(), sslCtx);
sslConn.emplace(*conn.get(), sslCtx);
setCipherSuiteTLSext();
}
return newconn;
}

public:
Expand All @@ -650,9 +660,10 @@ class ConnectionInfo : public std::enable_shared_from_this<ConnectionInfo>
unsigned int connIdIn) :
subId(idIn),
connPolicy(connPolicyIn), host(destIPIn), port(destPortIn),
connId(connIdIn), ioc(iocIn), conn(makeConnection(iocIn, useSSL)),
timer(iocIn)
{}
connId(connIdIn), ioc(iocIn), timer(iocIn)
{
initializeConnection(useSSL);
}
};

class ConnectionPool : public std::enable_shared_from_this<ConnectionPool>
Expand Down
4 changes: 2 additions & 2 deletions http/http_response.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ struct Response
{
BMCWEB_LOG_DEBUG << this << " Clearing response containers";
stringResponse.emplace(response_type{});
jsonValue.clear();
jsonValue = nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

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

jsonValue is an object not a pointer right? should't it be like jsonValue=nlohmann::json();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think both ways can be used - nlohmann/json#2156
Here, we're trying to sync up with what upstream does - https://github.com/openbmc/bmcweb/blob/9970e93f794d0110de436828775e333e81330ad5/http/http_response.hpp#L194

completed = false;
expectedHash = std::nullopt;
}
Expand Down Expand Up @@ -244,7 +244,7 @@ struct Response
addHeader(boost::beast::http::field::etag, hexVal);
if (expectedHash && hexVal == *expectedHash)
{
jsonValue.clear();
jsonValue = nullptr;
result(boost::beast::http::status::not_modified);
}
}
Expand Down
5 changes: 5 additions & 0 deletions http/websocket.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,11 @@ class ConnectionImpl : public Connection
[weak(weak_from_this()), onDone{std::move(onDone)}](
const boost::beast::error_code& ec, size_t) {
std::shared_ptr<Connection> self = weak.lock();
if (!self)
{
BMCWEB_LOG_ERROR << "Connection went away";
return;
}

// Call the done handler regardless of whether we
// errored, but before we close things out
Expand Down
1 change: 0 additions & 1 deletion redfish-core/lib/managers.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -349,7 +349,6 @@ inline void
if (ec)
{
BMCWEB_LOG_ERROR << ec;
asyncResp->res.jsonValue.clear();
messages::internalError(asyncResp->res);
return;
}
Expand Down